Skip to content
New issue

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

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

Already on GitHub? # to your account

aws-cognito: can't set custom attribute as non-writable #30608

Open
njeirath opened this issue Jun 21, 2024 · 6 comments
Open

aws-cognito: can't set custom attribute as non-writable #30608

njeirath opened this issue Jun 21, 2024 · 6 comments
Labels
@aws-cdk/aws-cognito Related to Amazon Cognito bug This issue is a bug. effort/medium Medium work item – several days of effort p3

Comments

@njeirath
Copy link

Describe the bug

I have a user pool with a single mutable custom attribute named custom:tier. I'd like to mark it as non writable by my client however specifying writeAttributes when adding the client to the pool with the code below leaves all attributes (both custom and standard) as writable other than email_verified and phone_number_verified.

      writeAttributes: new cognito.ClientAttributes()
        .withStandardAttributes({
          emailVerified: false,
          phoneNumberVerified: false,
        })
        .withCustomAttributes()

If I update this to include my custom attribute in the withCustomAttributes it marks all the standard attributes as non-writable. Similarly, if I include a standard attribute as writable like email: true it marks email writable as expected and marks everything else as non-writable. It appears the writeAttributes requires at least one attribute to be writable in order to mark the rest as non-writable.

Expected Behavior

I would expect for the attributes specified to writeAttributes to be the only ones that are writable and everything else to be non-writable.

Current Behavior

If I specify standard attributes as non-writable, it leaves my custom attributes writable and there doesn't appear to be any way to mark them non-writable.

Reproduction Steps

interface Props extends cdk.StackProps {}

export class TestStack extends cdk.Stack {
  constructor(scope: Construct, id: string, props: Props) {
    super(scope, id, props);

    const userPool = new cognito.UserPool(this, 'UserPool', {
      userPoolName: 'TestPool',
      signInCaseSensitive: false,
      self#Enabled: true,
      userVerification: {
        emailSubject: 'Verify your email',
        emailBody: 'Thanks for signing up! Your verification code is {####}',
        emailStyle: cognito.VerificationEmailStyle.CODE,
      },
      signInAliases: {
        email: true,
      },
      accountRecovery: cognito.AccountRecovery.EMAIL_ONLY,
      advancedSecurityMode: cognito.AdvancedSecurityMode.AUDIT,
      customAttributes: {
        tier: new cognito.NumberAttribute({ mutable: true }),
      },
    });

    const client = userPool.addClient('AppClient', {
      preventUserExistenceErrors: true,
      readAttributes: new cognito.ClientAttributes()
        .withStandardAttributes({
          email: true,
          emailVerified: true,
          phoneNumberVerified: true,
        })
        writeAttributes: new cognito.ClientAttributes()
        .withStandardAttributes({
          emailVerified: false,
          phoneNumberVerified: false,
        })
        .withCustomAttributes()
      }),
    });
  }
}

Deploying the above stack will result in the client's attributes settings looking like this:

image

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.146.0 (build b368c78)

Framework Version

2.147.0

Node.js Version

v22.3.0

OS

MacOS

Language

TypeScript

Language Version

~4.9.5

Other information

No response

@njeirath njeirath added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jun 21, 2024
@github-actions github-actions bot added the @aws-cdk/aws-cognito Related to Amazon Cognito label Jun 21, 2024
@ashishdhingra ashishdhingra self-assigned this Jun 21, 2024
@ashishdhingra ashishdhingra added needs-reproduction This issue needs reproduction. and removed needs-triage This issue or PR still needs to be triaged. labels Jun 21, 2024
@ashishdhingra
Copy link
Contributor

ashishdhingra commented Jun 24, 2024

@njeirath Good morning. Based on my testing, I think this is a limitation of CloudFormation (not CDK).

  • Per AWS::Cognito::UserPoolClient:

    • ReadAttributes is the list of user attributes that you want your app client to have read-only access to.
    • WriteAttributes is the list of user attributes that you want your app client to have write access to. It also mentions that When you don't specify the WriteAttributes for your app client, your app can write the values of the Standard attributes of your user pool..

    Both ReadAttributes and WriteAttributes are an array/list of strings.

  • In CDK, withStandardAttributes accepts parameter of type StandardAttributesMask, where the boolean value specifies whether to include the attribute in the list or not based on logic here. It is the convenience provided by Cognito L2 construct.

  • In your use case, since you use the below code:

          writeAttributes: new cognito.ClientAttributes()
                .withStandardAttributes({
                  emailVerified: false,
                  phoneNumberVerified: false,
                })
                .withCustomAttributes()

    the emitted WriteAttributes in the generated CloudFormation template is an empty list as shown below:

    ...
    UserPoolAppClientDD0407EC:
      Type: AWS::Cognito::UserPoolClient
      Properties:
        AllowedOAuthFlows:
          - implicit
          - code
        AllowedOAuthFlowsUserPoolClient: true
        AllowedOAuthScopes:
          - profile
          - phone
          - email
          - openid
          - aws.cognito.signin.user.admin
        CallbackURLs:
          - https://example.com
        PreventUserExistenceErrors: ENABLED
        ReadAttributes:
          - email
          - email_verified
          - phone_number_verified
        SupportedIdentityProviders:
          - COGNITO
        UserPoolId:
          Ref: UserPool6BA7E5F2
        WriteAttributes: []
      Metadata:
        aws:cdk:path: Issue30608Stack/UserPool/AppClient/Resource
    ...

    Hence CloudFormation (CDK is no longer in picture now), uses the default behavior where it marks all attributes as writable (except email_verified and phone_number_verified). These attributes cannot be specified as writable attributes since these are internally used by Cognito. If one tries to specify these attributes as writable, then even though CloudFormation template is synthesized, during deployment CloudFormation gives the below error:

    3:50:21 PM | CREATE_FAILED        | AWS::Cognito::UserPoolClient | UserPool/AppClient
    Resource handler returned message: "Invalid write attributes specified while creating a client (Service: 
    CognitoIdentityProvider, Status Code: 400, Request ID: bbf368d2-68e2-403a-ad
    70-4a6d6c126449)" (RequestToken: 220ab2cd-1b91-273e-632d-a51b7f8e3efd, HandlerErrorCode: InvalidRequest)
    3:50:22 PM | ROLLBACK_IN_PROGRESS | AWS::CloudFormation::Stack   | Issue30608Stack
    The following resource(s) failed to create: [UserPoolAppClientDD0407EC]. Rollback requested by user.
    3:50:22 PM | ROLLBACK_IN_PROGRESS | AWS::CloudFormation::Stack   | Issue30608Stack
    The following resource(s) failed to create: [UserPoolAppClientDD0407EC]. Rollback requested by user.
    

So, in order for your scenario to work, we would need to ensure non-empty WriteAttributes parameter in generated CloudFormation template. You would need to include at least one writable attribute. As an example below, I added email as writable attribute:

import * as cdk from 'aws-cdk-lib';
import { Construct } from 'constructs';
import * as cognito from 'aws-cdk-lib/aws-cognito';

export class Issue30608Stack extends cdk.Stack {
  constructor(scope: Construct, id: string, props?: cdk.StackProps) {
    super(scope, id, props);

    const userPool = new cognito.UserPool(this, 'UserPool', {
      userPoolName: 'TestPool',
      signInCaseSensitive: false,
      self#Enabled: true,
      userVerification: {
        emailSubject: 'Verify your email',
        emailBody: 'Thanks for signing up! Your verification code is {####}',
        emailStyle: cognito.VerificationEmailStyle.CODE
      },
      signInAliases: {
        email: true
      },
      accountRecovery: cognito.AccountRecovery.EMAIL_ONLY,
      advancedSecurityMode: cognito.AdvancedSecurityMode.AUDIT,
      customAttributes: {
        'tier': new cognito.NumberAttribute({mutable: true}) 
      }
    });

    const client = userPool.addClient('AppClient', {
      preventUserExistenceErrors: true,
      readAttributes: new cognito.ClientAttributes()
                            .withStandardAttributes({ 
                              email: true, 
                              emailVerified: true, 
                              phoneNumberVerified: true}),
      writeAttributes: new cognito.ClientAttributes()
                            .withStandardAttributes({
                              email: true
                            })
    });
  }
}

This generated the below CloudFormation template:

Resources:
  UserPool6BA7E5F2:
    Type: AWS::Cognito::UserPool
    Properties:
      AccountRecoverySetting:
        RecoveryMechanisms:
          - Name: verified_email
            Priority: 1
      AdminCreateUserConfig:
        AllowAdminCreateUserOnly: false
      AutoVerifiedAttributes:
        - email
      EmailVerificationMessage: Thanks for signing up! Your verification code is {####}
      EmailVerificationSubject: Verify your email
      Schema:
        - AttributeDataType: Number
          Mutable: true
          Name: tier
      SmsVerificationMessage: The verification code to your new account is {####}
      UserPoolAddOns:
        AdvancedSecurityMode: AUDIT
      UserPoolName: TestPool
      UsernameAttributes:
        - email
      UsernameConfiguration:
        CaseSensitive: false
      VerificationMessageTemplate:
        DefaultEmailOption: CONFIRM_WITH_CODE
        EmailMessage: Thanks for signing up! Your verification code is {####}
        EmailSubject: Verify your email
        SmsMessage: The verification code to your new account is {####}
    UpdateReplacePolicy: Retain
    DeletionPolicy: Retain
    Metadata:
      aws:cdk:path: Issue30608Stack/UserPool/Resource
  UserPoolAppClientDD0407EC:
    Type: AWS::Cognito::UserPoolClient
    Properties:
      AllowedOAuthFlows:
        - implicit
        - code
      AllowedOAuthFlowsUserPoolClient: true
      AllowedOAuthScopes:
        - profile
        - phone
        - email
        - openid
        - aws.cognito.signin.user.admin
      CallbackURLs:
        - https://example.com
      PreventUserExistenceErrors: ENABLED
      ReadAttributes:
        - email
        - email_verified
        - phone_number_verified
      SupportedIdentityProviders:
        - COGNITO
      UserPoolId:
        Ref: UserPool6BA7E5F2
      WriteAttributes:
        - email
    Metadata:
      aws:cdk:path: Issue30608Stack/UserPool/AppClient/Resource
...

After deployment, we get the results as you expected for your scenario:
Screenshot 2024-06-24 at 3 57 03 PM

Thanks,
Ashish

@ashishdhingra ashishdhingra added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed needs-reproduction This issue needs reproduction. labels Jun 24, 2024
@njeirath
Copy link
Author

@ashishdhingra Thanks for the response. That was the workaround I ended up using as well which worked for my use case.

What I'm wondering is given my original code:

      writeAttributes: new cognito.ClientAttributes()
            .withStandardAttributes({
              emailVerified: false,
              phoneNumberVerified: false,
            })
            .withCustomAttributes()

Would it be better for the generated WriteAttributes to be the full list of standard Cognito attributes with email_verified and phone_number_verified left out of the list.

UserPoolAppClientDD0407EC:
  Type: AWS::Cognito::UserPoolClient
  Properties:
    ...
    WriteAttributes: ['address', 'birthdate', ...] #All standard attributes except email_verified and phone_number_verified

That way any custom attributes would be set to not allow writing unless they were explicitly passed to withCustomAttributes.

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Jun 25, 2024
@ashishdhingra
Copy link
Contributor

Would it be better for the generated WriteAttributes to be the full list of standard Cognito attributes with email_verified and phone_number_verified left out of the list.

UserPoolAppClientDD0407EC:
  Type: AWS::Cognito::UserPoolClient
  Properties:
    ...
    WriteAttributes: ['address', 'birthdate', ...] #All standard attributes except email_verified and phone_number_verified

That way any custom attributes would be set to not allow writing unless they were explicitly passed to withCustomAttributes.

@njeirath Based on the implementation and CloudFormation spec, one needs to explicitly specify the list. That's the reason why standard attributes are supported in the form of mask, which could be turned on/off. This is a convenience so that one doesn't need to remember standard attribute names. Also, changing the default behavior to include all standard attributes by default is a breaking change and undermines the default Cognito behavior to app can write the values of the Standard attributes of your user pool if not explicitly set.

Thanks,
Ashish

@ashishdhingra ashishdhingra added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Jun 25, 2024
Copy link

This issue has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Jun 27, 2024
@njeirath
Copy link
Author

I think the issue is that an empty list does not only allow writing of standard attributes but also custom attributes.

As you mentioned, the CloudFormation docs say When you don't specify the WriteAttributes for your app client, your app can write the values of the Standard attributes of your user pool.

So an empty list passed to WriteAttributes in the template would be the equivalent of explicitly passing an array that includes all the standard attributes meaning new cognito.ClientAttributes().withStandardAttributes({}) should be able to return either an empty array (as it currently does) or return a list of all the standard attributes.

This would still allow masking out any of the standard attributes the user does not want to include by leaving it off the generated list but would have the added benefit of allowing one to opt in to including custom attributes.

I understand this could be considered a breaking change but wondering if it would be useful for this behavior to be added to a new class like cognito.ClientWriteAttributes and change writeAttributes to accept either class as input?

@github-actions github-actions bot removed closing-soon This issue will automatically close in 4 days unless further comments are made. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. labels Jun 28, 2024
@ashishdhingra
Copy link
Contributor

I think the issue is that an empty list does not only allow writing of standard attributes but also custom attributes.

As you mentioned, the CloudFormation docs say When you don't specify the WriteAttributes for your app client, your app can write the values of the Standard attributes of your user pool.

So an empty list passed to WriteAttributes in the template would be the equivalent of explicitly passing an array that includes all the standard attributes meaning new cognito.ClientAttributes().withStandardAttributes({}) should be able to return either an empty array (as it currently does) or return a list of all the standard attributes.

This would still allow masking out any of the standard attributes the user does not want to include by leaving it off the generated list but would have the added benefit of allowing one to opt in to including custom attributes.

I understand this could be considered a breaking change but wondering if it would be useful for this behavior to be added to a new class like cognito.ClientWriteAttributes and change writeAttributes to accept either class as input?

@njeirath The write behavior on custom attributes is controlled by the CloudFormation, not by the CDK. CDK is sending empty list in generated CloudFormation template as observed in #30608 (comment). In case you think the documentation needs to be updated you may share feedback using Provide Feedback link on the page AWS::Cognito::UserPoolClient.

As a convenience, it would be more desirable to add something like withAllStandardAttributes() to provide requested functionality in #30608 (comment). The implementation should take care of scenario if user also specified withStandardAttributes() to avoid any duplicates (basically the attributes should be contained in a hash set. Feel free to contribute PR if you think if it would be useful to add this helper method (also refer to Contributing to the AWS Cloud Development Kit).

Thanks,
Ashish

@ashishdhingra ashishdhingra added effort/medium Medium work item – several days of effort p3 labels Jun 28, 2024
@ashishdhingra ashishdhingra removed their assignment Jun 28, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
@aws-cdk/aws-cognito Related to Amazon Cognito bug This issue is a bug. effort/medium Medium work item – several days of effort p3
Projects
None yet
Development

No branches or pull requests

2 participants