aws_cdk.aws_cognito: 'AccountRecoverySetting' property gets removed from generated CFN template when enum 'AccountRecovery' is set to option 'PHONE_AND_EMAIL' and MFA is set to Off

a4arjuna commented 1 year ago

Describe the bug

AccountRecoverySetting property gets removed from generated CFN template when enum AccountRecovery is set to option PHONE_AND_EMAIL and MFA is set to Off.

Even though this is a invalid option (PHONE_AND_EMAIL) to use when MFA is set OFF, users should receive an error message during synthesis instead of just removing the property in the generated template.

Expected Behavior

When an invalid option is given i.e., setting AccountRecovery to PHONE_AND_EMAIL and MFA is set to Off, it should produce an error something like invalid option or cannot use this option when MFA is set to OFF.

Current Behavior

Instead of throwing an error for invalid option, after 'cdk synth' generated template just removing the property 'AccountRecoverySetting' without any information to user on why the property is removed.

Reproduction Steps

Try to define a cdk construct for UserPool as shown below:

self._cognito_user_pool=aws_cognito.UserPool(
            self,
            id=f"{application_name}-{stage}-pool",
            user_pool_name=f"{application_name}-{stage}-pool",
            auto_verify=aws_cognito.AutoVerifiedAttrs(email=True, phone=True),
            self_sign_up_enabled=False,
            account_recovery=aws_cognito.AccountRecovery.PHONE_AND_EMAIL,
            mfa=aws_cognito.Mfa.OFF,
            email= aws_cognito.UserPoolEmail.with_cognito(reply_to=None),
            sign_in_aliases=aws_cognito.SignInAliases(email=True, phone=True),
            device_tracking=None,
            lambda_triggers=None,
            standard_attributes= None,
            sign_in_case_sensitive= True
        )

Generated template with user pool resource after cdk synth where AccountRecoverySetting is missing even after defining in the construct:

firstpool4D5829AE:
    Type: AWS::Cognito::UserPool
    Properties:
      AdminCreateUserConfig:
        AllowAdminCreateUserOnly: true
      AutoVerifiedAttributes:
        - email
        - phone_number
      EmailConfiguration:
        EmailSendingAccount: COGNITO_DEFAULT
      EmailVerificationMessage: The verification code to your new account is {####}
      EmailVerificationSubject: Verify your new account
      MfaConfiguration: "OFF"
      SmsConfiguration:
        ExternalId: MyCdkProjectStackarjunfirstpool4EE49DC3
        SnsCallerArn:
          Fn::GetAtt:
            - arjunfirstpoolsmsRole66FD9C16
            - Arn
      SmsVerificationMessage: The verification code to your new account is {####}
      UsernameAttributes:
        - email
        - phone_number
      UsernameConfiguration:
        CaseSensitive: true
      UserPoolName: arjun-first-pool
      VerificationMessageTemplate:
        DefaultEmailOption: CONFIRM_WITH_CODE
        EmailMessage: The verification code to your new account is {####}
        EmailSubject: Verify your new account
        SmsMessage: The verification code to your new account is {####}
    UpdateReplacePolicy: Retain
    DeletionPolicy: Retain
    Metadata:
      aws:cdk:path: MyCdkProjectStack/first-pool/Resource

Expected is to return a error when invalid option is provided instead of just removing the property from the template...

Also if you can add below line for the enum AccountRecovery [1] documentation for option PHONE_AND_EMAIL which can also help users to not define this option when MFA is turned off.

"Not applicable when MFA is turned OFF"

[1] https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_cognito.AccountRecovery.html

Let me know if you have any questions..

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.58.1

Framework Version

No response

Node.js Version

v16.15.0

OS

macOS 12.6.2

Language

Python

Language Version

Python 3.10.5

Other information

No response

peterwoodworth commented 1 year ago

The property being removed in the final template is intentional - Not specifying this property is how we get the legacy default behavior

What occurs when you attempt to deploy with this configuration?

a4arjuna commented 1 year ago

I am able to deploy this configuration successfully. But even when I am defining this property in the CDK as 'account_recovery' with MFA turned OFF, this (account_recovery) is being removed. I understand that when mfa is turned off, having this property is not valid, so instead of removing it shouldn't it error out saying invalid property specification?

Mejjnor commented 1 year ago

Actually, the "account_recovery" section doesn't appear even if the MFA is required.

Code: self._cognito_user_pool=aws_cognito.UserPool( self, id=f"{application_name}-{stage}-pool", user_pool_name=f"{application_name}-{stage}-cog-pool", auto_verify=aws_cognito.AutoVerifiedAttrs(email=True, phone=True), self_sign_up_enabled=True, account_recovery=aws_cognito.AccountRecovery.PHONE_AND_EMAIL, mfa=aws_cognito.Mfa.REQUIRED, mfa_second_factor=aws_cognito.MfaSecondFactor(sms=True,otp=False), removal_policy=RemovalPolicy.DESTROY, )

Resulting template: { "Resources": { "testingttpool63CF389E": { "Type": "AWS::Cognito::UserPool", "Properties": { "AdminCreateUserConfig": { "AllowAdminCreateUserOnly": false }, "AutoVerifiedAttributes": [ "email", "phone_number" ], "EmailVerificationMessage": "The verification code to your new account is {####}", "EmailVerificationSubject": "Verify your new account", "EnabledMfas": [ "SMS_MFA" ], "MfaConfiguration": "ON", "SmsConfiguration": { "ExternalId": "cogstacktestingttpool736F6B9C", "SnsCallerArn": { "Fn::GetAtt": [ "testingttpoolsmsRole6F773802", "Arn" ] } }, "SmsVerificationMessage": "The verification code to your new account is {####}", "UserPoolName": "testing-tt-cog-pool", "VerificationMessageTemplate": { "DefaultEmailOption": "CONFIRM_WITH_CODE", "EmailMessage": "The verification code to your new account is {####}", "EmailSubject": "Verify your new account", "SmsMessage": "The verification code to your new account is {####}" } }, "UpdateReplacePolicy": "Delete", "DeletionPolicy": "Delete", "Metadata": { "aws:cdk:path": "cog-stack/testing-tt-pool/Resource" } },

aws / aws-cdk