Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

aws-cognito: not providing the authFlows property creates default authFlows for user pool client #26680

Closed
eli6 opened this issue Aug 9, 2023 · 4 comments · Fixed by #26700
Closed
Labels
@aws-cdk/aws-cognito Related to Amazon Cognito bug This issue is a bug. effort/small Small work item – less than a day of effort p1

Comments

@eli6
Copy link

eli6 commented Aug 9, 2023

Describe the bug

The documentation for the authFlows attribute of UserPoolClient says that the default behaviour is "all auth flows disabled", however this is not the case. If no authFlows attribute is provided, it defaults to ALLOW_REFRESH_TOKEN_AUTH , ALLOW_CUSTOM_AUTH and ALLOW_USER_SRP_AUTH.

Expected Behavior

I expected there to be no AuthFlows activated for the client if I don't include the authFlowsattribute.

Current Behavior

My Client with no specified authFlows property gets these flows per default:
ALLOW_REFRESH_TOKEN_AUTH , ALLOW_CUSTOM_AUTH and ALLOW_USER_SRP_AUTH.

Reproduction Steps

this creates a client with auth flows, even though authFlows was not provided.

 pool.addClient(clientName, {
    userPoolClientName: clientName,
    generateSecret: true,
    enableTokenRevocation: true,
  });

Possible Solution

In the below code, always pushing ALLOW_REFRESH_TOKEN_AUTH is a workaround so that CfnUserPoolClient always gets a value for explicitAuthFlows, otherwise it defaults to the values mentioned above. However, if the whole property is missing it returns undefined which creates the default flows.

My suggestion (to at least limit the impact) is to push the ALLOW_REFRESH_TOKEN_AUTH if the property is missing, just like we do if any of the authFlows have been set.

An even cleaner solution would be to update the behavior of CfnUserPoolClient so it doesn't create anything per default, but I am not sure where that function resides or how possible that is.

https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-cognito/lib/user-pool-client.ts

 private configureAuthFlows(props: UserPoolClientProps): string[] | undefined {
    if (!props.authFlows || Object.keys(props.authFlows).length === 0) return undefined;

    const authFlows: string[] = [];
    if (props.authFlows.userPassword) { authFlows.push('ALLOW_USER_PASSWORD_AUTH'); }
    if (props.authFlows.adminUserPassword) { authFlows.push('ALLOW_ADMIN_USER_PASSWORD_AUTH'); }
    if (props.authFlows.custom) { authFlows.push('ALLOW_CUSTOM_AUTH'); }
    if (props.authFlows.userSrp) { authFlows.push('ALLOW_USER_SRP_AUTH'); }

    // refreshToken should always be allowed if authFlows are present
    authFlows.push('ALLOW_REFRESH_TOKEN_AUTH');

    return authFlows;
  }

Additional Information/Context

No response

CDK CLI Version

2.87.0

Framework Version

No response

Node.js Version

18

OS

OSX

Language

Typescript

Language Version

No response

Other information

No response

@eli6 eli6 added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Aug 9, 2023
@github-actions github-actions bot added the @aws-cdk/aws-cognito Related to Amazon Cognito label Aug 9, 2023
@peterwoodworth
Copy link
Contributor

You're right that the docs we have are inaccurate, since providing nothing to this prop at the Cfn level will create authFlows by default. Thanks for the report

@peterwoodworth peterwoodworth added p1 effort/small Small work item – less than a day of effort and removed needs-triage This issue or PR still needs to be triaged. labels Aug 9, 2023
@lpizzinidev
Copy link
Contributor

To me, this is just a documentation issue.

@eli6

My suggestion (to at least limit the impact) is to push the ALLOW_REFRESH_TOKEN_AUTH if the property is missing, just like we do if any of the authFlows have been set.

An even cleaner solution would be to update the behavior of CfnUserPoolClient so it doesn't create anything per default, but I am not sure where that function resides or how possible that is.

You should be able to achieve the behavior by passing false to all types:

pool.addClient(clientName, {
    userPoolClientName: clientName,
    generateSecret: true,
    enableTokenRevocation: true,
    authFlows: {
        userPassword: false,
        adminUserPassword: false,
        custom: false,
        userSrp: false,
    }
});

@eli6
Copy link
Author

eli6 commented Aug 10, 2023

@lpizzinidev that is true, we could also make it transparent in the documentation that there are default values and leave this implementation. It depends on how we want the behaviour to be obviously. To me it feels clearer that nothing is created when nothing is specified.

Currently, the moment you set any single auth flow to false, nothing is created by default (except that the REFRESH flow is always added). But if you don't have any authFlows property or an empty object, you get the 3 defaults.

@mergify mergify bot closed this as completed in #26700 Aug 10, 2023
mergify bot pushed a commit that referenced this issue Aug 10, 2023
When `authFlows` is not defined `UserPoolClient` will support ALLOW_REFRESH_TOKEN_AUTH, ALLOW_USER_SRP_AUTH, and ALLOW_CUSTOM_AUTH.

[Link](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-cognito-userpoolclient.html#cfn-cognito-userpoolclient-explicitauthflows) to CFN docs.

Closes #26680.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-cognito Related to Amazon Cognito bug This issue is a bug. effort/small Small work item – less than a day of effort p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants