feat(cognito): add mutable property in cognito user pool custom attribute #7190

tom139 · 2020-04-06T10:05:07Z

This PR adds missing property mutable to custom attributes in Cognito User Pool.

The properties are add to all data types (String, Number, Boolean and DateTime) as per CloudFormation docs.

Required property must be false for custom attributes, so is not added.

closes: #7011

Pull Request Checklist

Testing
- Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change) to existing user-pool-attr.test.ts file
- CLI change?: no
- cdk-init template change?: no
Docs
- jsdocs: All public APIs documented
- README: README and/or documentation topic updated
- Design: For significant features, design document added to design folder N.A.
Title and Description
- Change type: title prefixed with fix, feat and module name in parens, which will appear in changelog
- Title: use lower-case and doesn't end with a period
- Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
- Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
[n/a] Sensitive Modules (requires 2 PR approvers)
- IAM Policy Document (in @aws-cdk/aws-iam)
- EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
- Grant APIs (only if not based on official documentation with a reference)

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

Add properties `mutable` and `developerOnly` to all CustomAttribute types. fixes: aws#7011

aws-cdk-automation · 2020-04-06T10:36:25Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: e53f7a8
Result: SUCCEEDED
Build Logs (available for 30 days)

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

packages/@aws-cdk/aws-cognito/README.md

nija-at · 2020-04-08T08:32:16Z

packages/@aws-cdk/aws-cognito/README.md

-Custom attributes cannot be marked as required.
+All custom attributes share the common properties `developerOnly` and `mutable`.
+
+ - `developerOnly` means that this attribute can only be modified by an administrator. The use of this property is discouraged


The use of this property is discouraged

Is this Cognito's recommendation or something you are suggesting?
If it's the former, can you point to where they are saying so?

OTOH, if it's the latter, could you explain why you are suggesting this?

It is Cognito's recommendation

We recommend that you use WriteAttributes in the user pool client to control how attributes can be mutated for new use cases instead of using DeveloperOnlyAttribute.

from AWS::Cognito::UserPool SchemaAttribute in Cloudformation

In such a case, should the CDK even implement this? Can we instead implement WriteAttributes on the client, so users use the right thing?

For users who really want to use this attribute or if they're migrating an existing user pool to the CDK, they can always access this via CDK's escape hatches.

I like your point.

I think the implementation might involve changing the UserPoolClient class to allow set the read/write attributes for that specific client.

Do you think this should be implemented in its own issue/PR or is it fine to do it here?

Do you think this should be implemented in its own issue/PR or is it fine to do it here?

A separate PR would be preferable and cleaner.

Ok, I will remove the support for the developerOnly attribute altogether and propose a new PR.

nija-at · 2020-04-08T09:40:54Z

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

+/**
+ * Constraints that can be applied to a custom attribute of string type.
+ */
+export interface BaseCustomAttributeProps {


Drop the prefix Base from here.

nija-at · 2020-04-08T09:41:09Z

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

+ * All common properties are set here and the method `baseAttributeConfig`
+ * should be used by subclasses to create base CustomAttributeConfig object inside the `bind()` method.
+ */
+abstract class BaseCustomAttribute implements ICustomAttribute {


Same here. Let's drop 'Base'.

Unfortunately, if an exported class inherits this, this will also need to be exported.

Suggested change

abstract class BaseCustomAttribute implements ICustomAttribute {

export abstract class CustomAttribute implements ICustomAttribute {

nija-at · 2020-04-08T09:48:42Z

packages/@aws-cdk/aws-cognito/README.md

+ - `developerOnly` means that this attribute can only be modified by an administrator. The use of this property is discouraged
+ in favour of the use of write permissions for attributes in the user pool client (see [AppClients](https://docs.aws.amazon.com/cognito/latest/developerguide/user-pool-settings-client-apps.html)),
+
+ - `mutable` allows the value to be changed after the value is set by the user.


Does setting mutable to false mean that even administrators can't modify it?

In general, is there a combination of mutable and developerOnly that is invalid or does not make sense?

Great question.
Yes, setting mutable to false means that even administrators can't modify it but it still make sense to keep both properties in case the users are allowed to sign up by themselves.

If only admins (==developers) can create new users than mutable && developerOnly has the same meaning of mutable because the attribute must be set on creation and only developers can do it.

If sign up is enabled, then mutable && developerOnly on an attribute means that only users created via the AdminCreateUser api can have that attribute set while users created via the SignUp API will never be able to set it.

fixes aws#7190

Pull request has been modified.

fixes aws#7190

aws-cdk-automation · 2020-04-08T11:52:42Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 5b29492
Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-04-08T11:56:40Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 4cb27f8
Result: FAILED
Build Logs (available for 30 days)

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

fixes aws#7011

tom139 · 2020-04-08T17:09:42Z

The default behavior for CloudFormation is for the attributes to be immutable.
I'm not sure this is the expected behavior: do you think it might be worth changing the default to true?

Changing the default would lead to a breaking change.

aws-cdk-automation · 2020-04-08T17:21:29Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 70db8d7
Result: FAILED
Build Logs (available for 30 days)

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

…-attributes

aws-cdk-automation · 2020-04-09T06:55:19Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 7a449b9
Result: SUCCEEDED
Build Logs (available for 30 days)

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

nija-at · 2020-04-15T10:57:19Z

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

+ * All common properties are set here and the method `baseAttributeConfig`
+ * should be used by subclasses to create base CustomAttributeConfig object inside the `bind()` method.
+ */
+export abstract class CustomAttribute implements ICustomAttribute {


I don't see much value in this base class anymore, now that we've dropped the adminOnly property.

It would be simpler to just store the mutable value as an instance property in each class and return it as part of bind(). And to revert the data type to how it was modeled previously.

What do you think?

nija-at · 2020-04-16T10:10:22Z

The default behavior for CloudFormation is for the attributes to be immutable.
I'm not sure this is the expected behavior: do you think it might be worth changing the default to true?

Changing the default would lead to a breaking change.

Let's keep the same default as CloudFormation.

nija-at

Moving this to request changes. Let's remove the base class here.

closes aws#7011

Pull request has been modified.

aws-cdk-automation · 2020-04-16T18:13:18Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 7b772f5
Result: FAILED
Build Logs (available for 30 days)

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

…-attributes

aws-cdk-automation · 2020-04-17T08:01:59Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 05975a6
Result: SUCCEEDED
Build Logs (available for 30 days)

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

nija-at

I've made slight adjustments to the test statements. Hope that's ok.

LGTM otherwise.

…-attributes

Pull request has been modified.

tom139 · 2020-04-17T18:02:44Z

Looks great for me!

aws-cdk-automation · 2020-04-17T18:06:13Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: fb8dfa3
Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-04-17T18:31:29Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 9ace0a2
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2020-04-17T19:59:04Z

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

…-attributes

aws-cdk-automation · 2020-04-17T20:31:35Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 5100fa1
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2020-04-17T20:31:54Z

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

tom139 added 2 commits April 6, 2020 11:11

feat(cognito): add properties to custom attribute

512e588

Add properties `mutable` and `developerOnly` to all CustomAttribute types. fixes: aws#7011

docs(cognito): remove reference to required property

e53f7a8

SomayaB assigned nija-at Apr 6, 2020

nija-at previously requested changes Apr 8, 2020

View reviewed changes

refactor(cognito): remove Base prefix in CustomAttribute class

5b29492

fixes aws#7190

fix(cognito): export CustomAttribute abstract class

4cb27f8

fixes aws#7190

tom139 added 2 commits April 8, 2020 18:24

docs(cognito): restore README line about required fields

c7f064f

fixes aws#7011

fix(cognito): remove developerOnly property in CustomAttribute

70db8d7

fixes aws#7011

tom139 changed the title ~~feat(cognito): add mutable and developerOnly properties in cognito user pool custom attribute~~ feat(cognito): add mutable property in cognito user pool custom attribute Apr 8, 2020

Merge branch 'master' into tom139/cognito-userpool-support-mutable-in…

7a449b9

…-attributes

nija-at reviewed Apr 15, 2020

View reviewed changes

nija-at previously requested changes Apr 16, 2020

View reviewed changes

refactor(cognito): remove BaseClass for custom attributes

7b772f5

closes aws#7011

Merge branch 'master' into tom139/cognito-userpool-support-mutable-in…

05975a6

…-attributes

adjustments to test statements

fb8dfa3

nija-at previously approved these changes Apr 17, 2020

View reviewed changes

Merge branch 'master' into tom139/cognito-userpool-support-mutable-in…

9ace0a2

…-attributes

nija-at approved these changes Apr 17, 2020

View reviewed changes

Merge branch 'master' into tom139/cognito-userpool-support-mutable-in…

5100fa1

…-attributes

mergify bot merged commit 16e85df into aws:master Apr 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cognito): add mutable property in cognito user pool custom attribute #7190

feat(cognito): add mutable property in cognito user pool custom attribute #7190

tom139 commented Apr 6, 2020 •

edited

aws-cdk-automation commented Apr 6, 2020

nija-at Apr 8, 2020

tom139 Apr 8, 2020

nija-at Apr 8, 2020

tom139 Apr 8, 2020

nija-at Apr 8, 2020

tom139 Apr 8, 2020

nija-at Apr 8, 2020

tom139 Apr 8, 2020

nija-at Apr 8, 2020 •

edited

tom139 Apr 8, 2020

nija-at Apr 8, 2020

tom139 Apr 8, 2020

aws-cdk-automation commented Apr 8, 2020

aws-cdk-automation commented Apr 8, 2020

tom139 commented Apr 8, 2020

aws-cdk-automation commented Apr 8, 2020

aws-cdk-automation commented Apr 9, 2020

nija-at Apr 15, 2020

nija-at commented Apr 16, 2020

nija-at left a comment

aws-cdk-automation commented Apr 16, 2020

aws-cdk-automation commented Apr 17, 2020

nija-at left a comment

tom139 commented Apr 17, 2020

aws-cdk-automation commented Apr 17, 2020

aws-cdk-automation commented Apr 17, 2020

mergify bot commented Apr 17, 2020

aws-cdk-automation commented Apr 17, 2020

mergify bot commented Apr 17, 2020

	abstract class BaseCustomAttribute implements ICustomAttribute {
	export abstract class CustomAttribute implements ICustomAttribute {

feat(cognito): add mutable property in cognito user pool custom attribute #7190

feat(cognito): add mutable property in cognito user pool custom attribute #7190

Conversation

tom139 commented Apr 6, 2020 • edited

Pull Request Checklist

aws-cdk-automation commented Apr 6, 2020

AWS CodeBuild CI Report

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

nija-at Apr 8, 2020 • edited

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

aws-cdk-automation commented Apr 8, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Apr 8, 2020

AWS CodeBuild CI Report

tom139 commented Apr 8, 2020

aws-cdk-automation commented Apr 8, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Apr 9, 2020

AWS CodeBuild CI Report

Choose a reason for hiding this comment

nija-at commented Apr 16, 2020

nija-at left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Apr 16, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Apr 17, 2020

AWS CodeBuild CI Report

nija-at left a comment

Choose a reason for hiding this comment

tom139 commented Apr 17, 2020

aws-cdk-automation commented Apr 17, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Apr 17, 2020

AWS CodeBuild CI Report

mergify bot commented Apr 17, 2020

aws-cdk-automation commented Apr 17, 2020

AWS CodeBuild CI Report

mergify bot commented Apr 17, 2020

tom139 commented Apr 6, 2020 •

edited

nija-at Apr 8, 2020 •

edited