fix(cognito): custom attributes in userpoolidentityprovider appear without the prefix (custom:)

[Balaji-JBC]

Issue #26820

Closes #26820

Reason for this change

See discussion in mentioned issue.

Description of changes

custom attributes in userpoolidentityproviderbase is modified to be prepended with custom: so that we don't have to explicitly add custom: which is the current workaround. Part of this solution was observed from @diego-santacruz who created this issue

Description of how you validated changes

The existing unit and integration tests has been modified to accommodate the change

Checklist

• My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[Leo10Gama]

Thanks for the contribution! It seems like this change won't be backwards compatible with other versions, since it replaces the attribute value, and thus will be a breaking change. Instead, can we put this new behaviour behind a feature flag and set the default to be the current behaviour?

It also looks like the integ tests were updated manually, since normally the entire snapshot folder changes with different updates. Can we also update each of those by running this in the framework-integ folder:

yarn integ test/aws-cognito/test/integ.<TEST_NAME>.js --update-on-failed

If you're having trouble updating them this way, I can also rerun the snapshots on my machine and push the updates here.

[Balaji-JBC]

Thanks for the contribution! It seems like this change won't be backwards compatible with other versions, since it replaces the attribute value, and thus will be a breaking change. Instead, can we put this new behaviour behind a feature flag and set the default to be the current behaviour?

It also looks like the integ tests were updated manually, since normally the entire snapshot folder changes with different updates. Can we also update each of those by running this in the framework-integ folder:

yarn integ test/aws-cognito/test/integ.<TEST_NAME>.js --update-on-failed

If you're having trouble updating them this way, I can also rerun the snapshots on my machine and push the updates here.

Hello @Leo10Gama , Thanks for taking time to review. The current behaviour is for the user to manually prepend custom: before the attribute. However, If we want to keep this behaviour for now, the docs have to be updated. Otherwise, can we modify the attribute such that if the custom: is already prepended, we can keep the attribute as it is, otherwise we can prepend the custom: to the attribute to maintain backward compatibility? Because as per the current behaviour, the cognito will never add this attribute without prefix and neither it'll throw an error unless we check in the aws console.

If you feel that my proposal may not be suitable. I'm ok with setting it as feature flag as well.

Also, I'm unable to update the snapshot as you mentioned. Will you please push the update for the same

[Leo10Gama]

Hi @Balaji-JBC, thanks for the quick response! I like that proposal, only touching custom mappings if they don't already have "custom:" appended, but I want to be sure that customers providing these values are truly broken (i.e. everyone currently providing in values cannot use them). That way, the only customers who need to redeploy their stacks are those whose stacks weren't working to begin with. Are we able to confirm that, even though it shows in the console, these values aren't being read and used correctly? If so, then let's go with that approach.

Doing a deep dive into the issue, it seems like it's not just a matter of appending "custom:" to the beginning of the string, but that the custom attribute itself has to exist in the UserPool. Adding that extra verification may be out of scope for this PR, but I think it means we should also update the integration test to include the extra customAttributes field for the matching custom attributeMapping. I'll refresh the integ test snapshots once that's included.

[diego-santacruz]

Hi,

As I reported in the description of #26820 a workaround is possible with the old broken code, which I am using and maybe other people are using too. The workaround would normally output a broken template with the fix, unless there is a special provision for removing a duplicate custom: from the resulting CloudFormation template in the code.

Pull request has been modified.

[Balaji-JBC]

Hello @Leo10Gama , I've included the backward compatibility and updated the test cases. Could you please update the snapshots for the same? Also, let me know if more custom attributes to be added for the test.

Doing a deep dive into the issue, it seems like it's not just a matter of appending "custom:" to the beginning of the string, but that the custom attribute itself has to exist in the UserPool.

You're right. In the console, you don't get these attributes listed unless it is available in the UserPool but in the aws-cdk you can create any attributes and deploy and aws ignores invalid attributes without any error. Shall I create a separate PR with a fix for that as well?

[Leo10Gama]

Hi @Balaji-JBC! I'm going through and updating the integ tests now, however I was only able to update one of them, as the others throw the error:

Error: integ.<test> is a new test. Please use the IntegTest construct to configure the test

Can you please go through each of the affected tests and update their outputs to use the IntegTest class? You should be able to simply provide the test stack under testCases similarly to how integ.user-pool-idp.saml.ts is.

[Balaji-JBC]

Hello @Leo10Gama , I've ran the update-on-failed on integ tests and pushed the updated snapshot

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.54%. Comparing base (744c2c1) to head (1d73b74).
Report is 5 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #32009   +/-   ##
=======================================
  Coverage   80.54%   80.54%           
=======================================
  Files         106      106           
  Lines        6954     6954           
  Branches     1287     1287           
=======================================
  Hits         5601     5601           
  Misses       1175     1175           
  Partials      178      178           

Flag	Coverage Δ
suite.unit	80.54% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	80.54% <ø> (ø)

[gracelu0]

While we've fixed the prefix issue, these integration tests don't properly set up the custom attributes because the UserPool does not define them (I verified this by deploying the integ.user-pool-idp.amazon.ts test template).

A couple of asks (once addressed I'm happy to approve):

1. Could we update the UserPool in the integration tests to create the corresponding custom attributes?

For example in this test:

    const userpool = new UserPool(this, 'pool', {
      removalPolicy: RemovalPolicy.DESTROY,
      customAttributes: {
        department: new StringAttribute({ minLen: 5, maxLen: 40, mutable: false }),
      }
    });

2. Could we update the README example here as well to create the uniqueId custom attribute in the UserPool?
3. Could we update the integ.user-pool-idp-*.ts tests to use the new IntegTest framework? Example:

import { IntegTest } from '@aws-cdk/integ-tests-alpha'; //add this to imports

const app = new App();
const stack = new Stack(app, 'integ-user-pool-idp-google');
.
.
.

new IntegTest(app, 'integ-user-pool-identity-provider-google-test', {
  testCases: [stack],
});

Thank you! Appreciate your work on this.

[aws-cdk-automation]

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

Pull request has been modified.

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: b98507c
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[Leo10Gama]

Thanks for updating the snapshots! I want to echo @gracelu0's sentiments here as well. I think we should include those suggested changes:

• Adding the extra customAttributes field to the UserPool in the integ.user-pool-idp.saml.ts test so that it deploys with the correct field visible in console.
• Updating the README.md example in the section related to custom user pool attributes (found here, since that file has been updated since the last review) to reflect that the UserPool needs to have the customAttributes field as well.
• Updating the integ.user-pool-idp-*.ts tests to use the new IntegTest framework as opposed to their current model. The saml test seems to be a good example of how to implement it, if it seems confusing.

In addition, it seems like some merge conflicts have come up since the last review. Could you please address those as well?

Feel free to reach out if you need any additional clarification. Thanks again for your contribution!

[aws-cdk-automation]

This PR has been in the MERGE CONFLICTS state for 3 weeks, and looks abandoned. Note that PRs with failing linting check or builds are not reviewed, please ensure your build is passing

To prevent automatic closure:

• Resume work on the PR
• OR request an exemption by adding a comment containing 'Exemption Request' with justification e.x "Exemption Request: "
• OR request clarification by adding a comment containing 'Clarification Request' with a question e.x "Clarification Request: "

This PR will automatically close in 14 days if no action is taken.

[aws-cdk-automation]

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

[github-actions]

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.