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.55k stars 3.87k forks source link

aws-cognito: identity provider attribute mapping mishandles custom attributes #26820

Open diego-santacruz opened 1 year ago

diego-santacruz commented 1 year ago

Describe the bug

I have added a custom attribute to my Cognito user pool and I wanted for an identity provider to map a claim to this custom attribute.

Following the documentation at https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_cognito.AttributeMapping.html I defined the mapping like this

{
  email: cognito.ProviderAttribute.other('email'),
  familyName: cognito.ProviderAttribute.other('family_name'),
  givenName: cognito.ProviderAttribute.other('given_name'),
  fullname: cognito.ProviderAttribute.other('name'),
  custom: {
      'idp_userid': cognito.ProviderAttribute.other('sub')
  }
}

And then pass that to the UserPoolIdentityProviderOidcProps's attributeMapping property.

However, this generates the wrong CloudFormation template, as the custom attribute is not prefixed with custom:. Although it deploys, when editing the mapping from the console, the mapping is not recognized, but if modified manually the console adds the custom: prefix.

The generated template looks like this

   "Type": "AWS::Cognito::UserPoolIdentityProvider",
   "Properties": {
    "AttributeMapping": {
     "email": "email",
     "family_name": "family_name",
     "given_name": "given_name",
     "name": "name",
     "custom:idp_userid": "sub"
    },

When it should look like

Expected Behavior

The generated template should looks like this

   "Type": "AWS::Cognito::UserPoolIdentityProvider",
   "Properties": {
    "AttributeMapping": {
     "email": "email",
     "family_name": "family_name",
     "given_name": "given_name",
     "name": "name",
     "custom:idp_userid": "sub"
    },

Current Behavior

The generated template should looks like this

   "Type": "AWS::Cognito::UserPoolIdentityProvider",
   "Properties": {
    "AttributeMapping": {
     "email": "email",
     "family_name": "family_name",
     "given_name": "given_name",
     "name": "name",
     "idp_userid": "sub"
    },

Reproduction Steps

const userPool = new cognito.UserPool(this, 'CloudUser', {
    accountRecovery: cognito.AccountRecovery.EMAIL_ONLY,
    customAttributes: {
        idp_userid: new cognito.StringAttribute({
            minLen: 0,
            maxLen: 128,
            mutable: true
        })
    },
    standardAttributes: {
        email: {
          required: true
        }
    },
    selfSignUpEnabled: true,
    signInAliases: { email: true },
    signInCaseSensitive: false,
    userPoolName: `spx-cloud-${props.stage}-mock`,
    removalPolicy: RemovalPolicy.DESTROY
});

new cognito.UserPoolIdentityProviderOidc(this, 'SsoIdP',
    {
        name: 'test',
        userPool: userPool,
        attributeMapping: {
            email: cognito.ProviderAttribute.other('email'),
            familyName: cognito.ProviderAttribute.other('family_name'),
            givenName: cognito.ProviderAttribute.other('given_name'),
            fullname: cognito.ProviderAttribute.other('name'),
            custom: {
                'idp_userid': cognito.ProviderAttribute.other('sub')
            }
        },
        clientId: 'abcde',
        clientSecret: 'password',
        issuerUrl: 'http://....',
        cognito.OidcAttributeRequestMethod.GET,
        scopes: ['openid', 'email', 'profile']
    }
);

Possible Solution

A workaround is to define the mapping as follows

{
  email: cognito.ProviderAttribute.other('email'),
  familyName: cognito.ProviderAttribute.other('family_name'),
  givenName: cognito.ProviderAttribute.other('given_name'),
  fullname: cognito.ProviderAttribute.other('name'),
  custom: {
      'custom:idp_userid': cognito.ProviderAttribute.other('sub')
  }
}

I think the problem is in https://github.com/aws/aws-cdk/blame/00a7f033f6ad19160a7350784243ecf9c71c388b/packages/aws-cdk-lib/aws-cognito/lib/user-pool-idps/private/user-pool-idp-base.ts#L33C11-L33C11

Instead of

return { ...agg, [k]: v.attributeName };

The code should probably read

return { ...agg, [k]: `custom:${v.attributeName}` };

Additional Information/Context

No response

CDK CLI Version

2.92.0

Framework Version

2.92.0

Node.js Version

v16.18.1

OS

Linux

Language

Typescript

Language Version

No response

Other information

No response

peterwoodworth commented 1 year ago

Think this would be a docs issue, but we could probably implement this as a feature while keeping backwards compatibility in mind without a feature flag

diego-santacruz commented 1 year ago

Well the CDK interfaces and code treats custom attributes separately, so it seems a bit silly to name an attribute as "custom:my_attr" while it sits under a "custom" property, separate from the built-in properties.

But yes, a change in the docs would certainly work (note that the doc page I linked is not the only one to change), albeit the resulting code looks a bit awkward.

peterwoodworth commented 1 year ago

Yeah I agree that it would be better for us to handle this rather than just changing the docs

psistorm commented 3 months ago

We had this problem just today. This is still an issue and totally unexpected behavior. I have to strengthen the position of @diego-santacruz . It's totally unexpected behavior to need to add custom: prefix when it already sits under the custom property. When creating the property with the pool it works as expected but differently to here.