feat(cognito): add SAML user pool identity provider #21879

ktfleming · 2022-09-01T16:56:33Z

Adds a construct for a SAML user pool identity provider.

I based much of this off of #20241, as the OIDC and SAML identity pool providers share e.g. the length limitations on provider names.

For the integration test, you have to specify a valid SAML metadata URL or XML document, or the stack won't be created. I used a sample URL from the samling project, but this could be changed if anyone has a better suggestion.

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

gitpod-io · 2022-09-01T16:56:40Z

TheRealAmazonKendra

Thanks so much for the contribution! I have a few comments inline.

TheRealAmazonKendra · 2022-09-01T20:34:10Z

packages/@aws-cdk/aws-cognito/lib/user-pool-idps/saml.ts

+  private getProviderName(name?: string): string {
+    if (name) {
+      if (!Token.isUnresolved(name) && (name.length < 3 || name.length > 32)) {
+        throw new Error(`Expected provider name to be between 3 and 32 characters, received ${name} (${name.length} characters)`);
+      }
+      return name;
+    }
+
+    const uniqueId = Names.uniqueId(this);
+
+    if (uniqueId.length < 3) {
+      return `${uniqueId}saml`;
+    }
+
+    if (uniqueId.length > 32) {
+      return uniqueId.substring(0, 16) + uniqueId.substring(uniqueId.length - 16);
+    }
+    return uniqueId;


Instead of this, `Names.uniqueResourceName() should be used.

I've changed Names.uniqueId to Names.uniqueResourceName, and used the maxLength option to avoid having to truncate it down to 32 characters.

TheRealAmazonKendra · 2022-09-01T20:35:56Z

packages/@aws-cdk/aws-cognito/lib/user-pool-idps/saml.ts

+    if (props.name && !Token.isUnresolved(props.name) && (props.name.length < 3 || props.name.length > 32)) {
+      throw new Error(`Expected provider name to be between 3 and 32 characters, received ${props.name} (${props.name.length} characters)`);
+    }


You're doing this check multiple times. Let's remove the redundancy.

Why is 3-32 the correct length required here? Also, your validation doesn't allow for it to be 3 characters or 32 so the error message is slightly misleading.

Refactored into a validateName function.

Why is 3-32 the correct length required here?

That restriction appears in this page, under the "ProviderName" description.

your validation doesn't allow for it to be 3 characters or 32 so the error message is slightly misleading.

Hmm, I'm not sure about this, it only throws an error on props.name.length < 3 || props.name.length > 32, so I believe lengths of exactly 3 or 32 should still be allowed through.

Oof. My tired brain reversed the logic in that. I was definitely wrong.

TheRealAmazonKendra · 2022-09-01T20:41:17Z

packages/@aws-cdk/aws-cognito/lib/user-pool-idps/saml.ts

+  /**
+   * The name of the provider.
+   *
+   * @default - the unique ID of the construct
+   */


If the length needs to be restricted, it should be added to the docstring so the first place the user encounters the information isn't in an error message.

I added this information to the docstring.

TheRealAmazonKendra · 2022-09-02T03:42:33Z

packages/@aws-cdk/aws-cognito/lib/user-pool-idps/saml.ts

+   * The SAML metadata file content.
+   *
+   * @default - no file content specified
+   */
+  readonly metadataFile?: string;
+
+  /**
+   * The SAML metadata URL.
+   *
+   * @default - no URL specified
+   */
+  readonly metadataUrl?: string;


Instead of having these be mutually exclusive and optional, let's have just one field that is required that can be of either type that we translate into the right field.

Sounds good -- I replaced these with metadataType and metadataContent props, both required. The former is an enum that determines how the latter should be interpreted (as a URL, or as XML content). This removes the need for the mutually-exclusive validation. Is that what you had in mind?

What I meant here was more along the lines of how we handle the different types of input that we turn into Schedule. I think that same conceptual approach can be used here. My apologies for not providing further context and that link here in the first place.

I see! Trying to follow the example of Schedule, I've reworked it now to just have one field of type UserPoolIdentityProviderSamlMetadata, which can only be created by one of two methods, url and file. Using the appropriate one should ensure that the providerDetails in the CloudFormation resource are set correctly (translated to MetadataURL and MetadataFile respectively).

Pull request has been modified.

ktfleming · 2022-09-02T13:58:28Z

Thanks for the feedback @TheRealAmazonKendra! I've made changes and replied/explained in each of your comments.

TheRealAmazonKendra

In addition to the comment inline, please see my response to your comments from the last review. Besides those comments, however, I just want to say that the overall quality of this PR is excellent. Thank you for all your work on this!

TheRealAmazonKendra · 2022-09-03T02:50:14Z

packages/@aws-cdk/aws-cognito/lib/user-pool-idps/saml.ts

+
+    this.validateName(props.name);
+
+    const providerDetails: Record<string, string | boolean> = {


I'm pretty sure this will run afoul of jsii. The union operator doesn't translate well in most cases.

With the rework of how metadata is provided in 36965e7, this union operator has gone away.

Pull request has been modified.

ktfleming · 2022-09-03T11:08:10Z

In addition to the comment inline, please see my response to your comments from the last review. Besides those comments, however, I just want to say that the overall quality of this PR is excellent. Thank you for all your work on this!

Thank you! I tried a rework of the metadata part to bring it closer to the example that you showed me.

mergify · 2022-09-09T00:29:12Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2022-09-09T01:06:00Z

AWS CodeBuild CI Report

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

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

mergify · 2022-09-09T01:06:21Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Adds a construct for a SAML user pool identity provider. I based much of this off of aws#20241, as the OIDC and SAML identity pool providers share e.g. the length limitations on provider names. For the integration test, you have to specify a valid SAML metadata URL or XML document, or the stack won't be created. I used a sample URL from the [samling](https://fujifish.github.io/samling/samling.html) project, but this could be changed if anyone has a better suggestion. ---- ### All Submissions: * [X] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [X] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [X] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

ktfleming added 5 commits September 1, 2022 15:10

Add SAML user pool identity provider construct

bd2ab7f

Update Cognito README with SAML IdP support

ae1e2c5

Ignore props-physical-name awslint rule for SAML props

5ce936a

Add unit tests for SAML user pool identity provider

5126400

Add integ test for SAML user pool identity provider

8394f9f

github-actions bot added the p2 label Sep 1, 2022

aws-cdk-automation requested a review from a team September 1, 2022 16:57

ktfleming changed the title ~~feat(cognito): SAML user pool identity provider~~ feat(cognito): add SAML user pool identity provider Sep 1, 2022

TheRealAmazonKendra previously requested changes Sep 2, 2022

View reviewed changes

ktfleming added 5 commits September 2, 2022 14:26

Use Names.uniqueResourceName instead of Names.uniqueId

265c785

Extract validateName function

6946c0b

Add provider name length restrictions to docstring

b3f44e1

Rework how SAML metadata is specified

afda2ff

Update SAML IdP tests

62e0da1

TheRealAmazonKendra previously requested changes Sep 3, 2022

View reviewed changes

Rework how SAML metadata is provided

36965e7

TheRealAmazonKendra approved these changes Sep 9, 2022

View reviewed changes

Merge branch 'main' into cognito-saml-idp

af574c0

mergify bot merged commit 76d446b into aws:main Sep 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cognito): add SAML user pool identity provider #21879

feat(cognito): add SAML user pool identity provider #21879

ktfleming commented Sep 1, 2022

gitpod-io bot commented Sep 1, 2022

TheRealAmazonKendra left a comment

TheRealAmazonKendra Sep 1, 2022

ktfleming Sep 2, 2022

TheRealAmazonKendra Sep 1, 2022

TheRealAmazonKendra Sep 1, 2022

ktfleming Sep 2, 2022

TheRealAmazonKendra Sep 3, 2022

TheRealAmazonKendra Sep 1, 2022

ktfleming Sep 2, 2022

TheRealAmazonKendra Sep 2, 2022

ktfleming Sep 2, 2022

TheRealAmazonKendra Sep 3, 2022

ktfleming Sep 3, 2022

ktfleming commented Sep 2, 2022

TheRealAmazonKendra left a comment

TheRealAmazonKendra Sep 3, 2022

ktfleming Sep 3, 2022

ktfleming commented Sep 3, 2022

mergify bot commented Sep 9, 2022

aws-cdk-automation commented Sep 9, 2022

mergify bot commented Sep 9, 2022


		this.validateName(props.name);

		const providerDetails: Record<string, string \| boolean> = {

feat(cognito): add SAML user pool identity provider #21879

feat(cognito): add SAML user pool identity provider #21879

Conversation

ktfleming commented Sep 1, 2022

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Sep 1, 2022

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktfleming commented Sep 2, 2022

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktfleming commented Sep 3, 2022

mergify bot commented Sep 9, 2022

aws-cdk-automation commented Sep 9, 2022

AWS CodeBuild CI Report

mergify bot commented Sep 9, 2022