aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.68k stars 3.92k forks source link

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

Open njeirath opened 4 months ago

njeirath commented 4 months ago

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,
      selfSignUpEnabled: 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

ashishdhingra commented 4 months ago

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

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,
      selfSignUpEnabled: 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

njeirath commented 4 months ago

@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.

ashishdhingra commented 4 months ago

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

github-actions[bot] commented 4 months ago

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.

njeirath commented 4 months ago

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?

ashishdhingra commented 4 months ago

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 https://github.com/aws/aws-cdk/issues/30608#issuecomment-2187568914. 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 https://github.com/aws/aws-cdk/issues/30608#issuecomment-2188013874. 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