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

[aws-cdk-automation]

AWS CodeBuild CI Report

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

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