feat(cognito): allow to set read and write attributes in Cognito UserPoolClient #7607

tom139 · 2020-04-25T16:26:36Z

Commit Message

feat(cognito): allow to set read and write attributes in Cognito UserPoolClient

Add ability to specify which attributes each App Client can read or write.

End Commit Message

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

When creating a userpool client, the user can now specify which attributes (custom or standard) the app client will be able to read or write. Closes aws#7407

Add section to explain hot to make specific attributes available for read and write to specific clients. Fixes aws#7407

nija-at

Thanks for submitting this PR.

I have some comments on this below.

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

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

Co-authored-by: Niranjan Jayakar <nija@amazon.com>

Pull request has been modified.

specifying how read/write attributes behave when new attributes get added to a user pool.

…te-attributes

Co-authored-by: Niranjan Jayakar <nija@amazon.com>

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

nija-at

Please address existing comments.

arnulfojr · 2020-06-10T19:50:41Z

any updates?

Initial work to add the notion of set of attributes (mixed standard and custom) fixes aws#7407

fixes aws#7407

in userpool client Allow to set read and write attributes in Cognito UserPoolClient fixes aws#7407

… tom139/cognito-userpool-client-set-readwrite-attributes

gitpod-io · 2020-12-14T22:50:15Z

Pull request has been modified.

tom139 · 2020-12-14T22:57:00Z

Sorry for making you wait so long. 🙏🏽

I started from scratch with the approach suggested by @nija-at inspired by AccessLogFormat. The main change is the addition of the AttributeSet class that represent a set of attributes (both standard and custom).

Naming of the methods may be misleading, but I couldn't think anything better and am open to suggestion.

Usage is:

const pool = new cognito.UserPool(this, 'Pool');
pool.addClient('app-client', {
  // ...
  readAttributes: AttributeSet.allStandard(['custom:my_attribute_1', 'custom:my_attribute_2']),
  writeAttributes: AttributeSet.from({name: true, locale: true}, ['custom:my_attribute_1']),
});

…te-attributes

nija-at · 2020-12-18T11:45:12Z

Hey @tom139 -

I'm going to be on vacation for the next couple of weeks for the holidays. I'll take a look at this once I'm back.
Happy holidays!

tom139 · 2020-12-20T11:45:27Z

Have a nice time and stay safe! :)

nija-at

Thanks for updating this PR. Some questions, comments and suggestions below.

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

nija-at · 2021-01-11T11:33:28Z

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

+/**
+ * This interface contains all standard attributes recognized by Cognito
+ * from https://docs.aws.amazon.com/cognito/latest/developerguide/user-pool-settings-attributes.html
+ * including `preferred_email` and `preferred_phone_number`


Curious: why are preferred_email and preferred_number_number special?
They don't feature as standard attributes in the cognito docs. why are they considered as standard?

My bad. It is not preferred_email and preferred_phone_number to be special, it is email_verified and phone_number_verified.

These are special because are not directly mentioned in the list at the linked page but are present in the cited OpenID Connect Specification and are in fact present among the other standard attributes.

These two attributes are the missing ones from the cited StandardAttributes interface.

are in fact present among the other standard attributes

What do you mean by this? Were you able to confirm that they are actually supported by Cognito, but just missing in the documentation?

If that's the case, we should just add these to StandardAttributes.

I have conducted some tests and I confirm Configuring User Pool is missing two attributes: email_verified and phone_number_verified.

Even though it states:

You get a set of default attributes, called "standard attributes," with all user pools. You can also add custom attributes to your user pool definition in the AWS Management Console. This topic describes those attributes in detail and gives you tips on how to set up your user pool.

and does not list them under standard attributes, the following call:

❯ aws cognito-idp create-user-pool --cli-input-json file://userpool.json ❯ cat userpool.json { "PoolName": "test", "Policies": { "PasswordPolicy": { "MinimumLength": 10, "RequireUppercase": true, "RequireLowercase": true, "RequireNumbers": true, "RequireSymbols": true, "TemporaryPasswordValidityDays": 10 } }, "AutoVerifiedAttributes": [ "email" ], "UsernameAttributes": [ "email" ], "Schema": [ { "Name": "email", "AttributeDataType": "String", "Mutable": true, "Required": true, "StringAttributeConstraints": { "MinLength": "5", "MaxLength": "2048" } }, { "Name": "email_verified", "AttributeDataType": "Boolean", "Mutable": true, "Required": true } ], "UsernameConfiguration": { "CaseSensitive": true } }

works as expected, setting both email and email_verified as required and mutable attributes.

The UI Console has the same problem as it does not show (phone_number|email)_verified in the attribute list:

I filed feedbacks for the documentation and the web ui pages.

That said, I will add those 2 attributes to the StandardAttributes interface, so StandardAttributes and StandardAttributesMask will have the same properties.

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

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

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

…te-attributes

Pull request has been modified.

Co-authored-by: Niranjan Jayakar <nija@amazon.com>

tom139

I think I addressed everything except setting up a test for detecting drifts between StandardAttributes and StandardAttributesMask.

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

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

…te-attributes

nija-at

Looks closer. A few more comments. Hopefully the last round of review before merging 😊

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

nija-at · 2021-01-18T11:22:27Z

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

+  /**
+   * Creates an empty ClientAttributes
+   */
+  public static empty(): ClientAttributes {


Do we still need the ClientAttributes.empty() method? Customers can just use the constructor new ClientAttributes(), no?

Much nicer 👍🏽 I did the refactor.

nija-at · 2021-01-18T11:44:13Z

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

+The default behaviour is to allow read and write permissions on all attributes.
+
+```ts
+const pool = new cognito.UserPool(this, 'Pool');
+// create a set of attributes that the client will be allowed to set
+const clientWriteAttributes = ClientAttributes.empty()
+  .withStandardAttributes({name: true, email: true})
+  .withCustomAttributes(['favouritePizza']);
+// read attributes are usually more than the writable ones
+const clientReadAttributes = clientWriteAttributes
+  .withStandardAttributes({emailVerified: true})
+  .withCustomAttributes(['pointsEarned']);
+pool.addClient('app-client', {
+  // ...
+  readAttributes: clientReadAttributes,
+  writeAttributes: clientWriteAttributes,
+});
+```


Slight tweaks to the documentation.

Suggested change

The default behaviour is to allow read and write permissions on all attributes.

```ts

const pool = new cognito.UserPool(this, 'Pool');

// create a set of attributes that the client will be allowed to set

const clientWriteAttributes = ClientAttributes.empty()

.withStandardAttributes({name: true, email: true})

.withCustomAttributes(['favouritePizza']);

// read attributes are usually more than the writable ones

const clientReadAttributes = clientWriteAttributes

.withStandardAttributes({emailVerified: true})

.withCustomAttributes(['pointsEarned']);

pool.addClient('app-client', {

// ...

readAttributes: clientReadAttributes,

writeAttributes: clientWriteAttributes,

});

```

The default behaviour is to allow read and write permissions on all attributes. The following code shows how this can be configured for a client.

```ts

const pool = new cognito.UserPool(this, 'Pool');

const clientWriteAttributes = ClientAttributes.empty()

.withStandardAttributes({name: true, email: true})

.withCustomAttributes(['favouritePizza']);

const clientReadAttributes = clientWriteAttributes

.withStandardAttributes({emailVerified: true})

.withCustomAttributes(['pointsEarned']);

pool.addClient('app-client', {

// ...

readAttributes: clientReadAttributes,

writeAttributes: clientWriteAttributes,

});

```

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

and replace it with (new ClientAttribute())

Pull request has been modified.

tom139

Thank you for the time spent reviewing this!

Just a question more: am I supposed to hit "Resolve Conversation" when I think I addressed the issue, or should I let you do it?

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

tom139 · 2021-01-18T21:53:17Z

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

+  /**
+   * Creates an empty ClientAttributes
+   */
+  public static empty(): ClientAttributes {


Much nicer 👍🏽 I did the refactor.

…te-attributes

@nija-at

as suggested by @nija-at closes aws#7607

nija-at · 2021-01-20T17:57:55Z

am I supposed to hit "Resolve Conversation" when I think I addressed the issue

Clicking 'resolve conversation' is fine once you've addressed it 😊

mergify · 2021-01-20T18:00:25Z

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).

…te-attributes

aws-cdk-automation · 2021-01-20T18:36:45Z

AWS CodeBuild CI Report

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

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

mergify · 2021-01-20T18:37:14Z

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).

…PoolClient (aws#7607) ### Commit Message feat(cognito): allow to set read and write attributes in Cognito UserPoolClient Add ability to specify which attributes each App Client can read or write. closes aws#7407 ### End Commit Message ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

russelldavis · 2021-01-25T04:50:45Z

@tom139 @nija-at this is great, thank you! One question: why does withStandardAttributes take in an object like {name: true, email: true}, instead of a simple list like ["name", "email"]? It's also inconsistent with withCustomAttributes, which takes in a list of names.

russelldavis · 2021-01-25T04:56:46Z

Also, regarding #7607 (comment), I'd like to express interest in the additional static method you discussed, which would prepopulate all of the standard attributes.

A common use case (I think) is to allow write access to everything except for a small number of custom attributes. To do this now, I'll have to manually list all of the standard attributes, and keep my manual list up to date if those ever change.

Thanks again.

russelldavis · 2021-01-25T05:47:15Z

To follow up on that last comment, here's the code I ended up writing:

const standardWriteAttributes = new ClientAttributes().withStandardAttributes({
  address: true,
  birthdate: true,
  email: true,
  familyName: true,
  gender: true,
  givenName: true,
  locale: true,
  middleName: true,
  fullname: true,
  nickname: true,
  phoneNumber: true,
  profilePicture: true,
  preferredUsername: true,
  profilePage: true,
  timezone: true,
  lastUpdateTime: true,
  website: true,
});

const standardReadAttributes = standardWriteAttributes.withStandardAttributes({
  emailVerified: true,
  phoneNumberVerified: true,
});

new UserPoolClient(this, "UserPoolClient", {
  // Allow everything except for writing custom attributes.
  readAttributes: standardReadAttributes.withCustomAttributes(...Object.keys(customAttributes)),
  writeAttributes: standardWriteAttributes,
});

Would be great if those lists were exposed, perhaps via withAllStandardReadAttributes() and withAllStandardWriteAttributes() functions.

Note that they need to be separate lists, because emailVerified and phoneNumberVerified cannot be set as writable. I learned this the hard way when cdk deploy gave me an error of "Invalid write attributes specified" without telling me which ones were invalid (another reason having these predefined would be helpful.)

tom139 · 2021-01-25T07:45:22Z

Hi, thank you for your feedback! I totally agree with you. What do you think should be the best way to implement this?

Having two all attributes methods

as you suggested: one for Writing and one for Reading attributes.
.withAllStandardReadAttributes() for all attributes,
.withAllStandardWriteAttributes() for all attributes except *Verified.

Usage:

const readAttr = (new ClientAttributes()).withAllStandardReadAttributes();
const writeAttr = (new ClientAttributes()).withAllStandardWriteAttributes();

Having only one method

withAllStandardAttributes() that adds all standard attributes and adding a check for removing *Verified attribute at UserPoolClient creation time

Usage:

const readAttr = (new ClientAttributes()).withAllStandardAttributes();
const writeAttr = (new ClientAttributes()).withAllStandardAttributes();

I think the latter would be is easier to use, what do you think?

tom139 · 2021-01-25T07:57:41Z

Sorry, I forgot to write about different design choices for withCustomAttributes() vs _withStandardAttributes()`.

Having an interface for the standard attributes allow the user to easily find the available values through code completion; and in my opinion it is more valuable than consistency among with*Attributes() methods.

russelldavis · 2021-01-25T09:18:33Z

What do you think should be the best way to implement this?

I like the idea of having as few methods as possible, but in this case I think keeping them separate is best. Automatically removing the *Verified attributes could cause some confusion — a developer would reasonably assume those attributes are writable if they put them in writeAttributes and everything succeeds, but they'd be wrong. "Explicit is better than implicit."

russelldavis · 2021-01-25T09:20:37Z

I'm with you about the value of code completion. Fortunately there's a way to have that without sacrificing consistency. For example:

type StandardAttribute = "name" | "email";
function withStandardAttributes(...attributes: StandardAttribute[]);

When you type withStandardAttributes(" in VSCode, it will give you code completion, and show an error if you type a value not in the list.

tom139 · 2021-01-25T15:07:26Z

Yeah, it is very cool! Unfortunately we must ensure that JSII can synthesise this information in other languages too… I don't think this will work outside of Typescript. 😔

@nija-at could you help us on that?

tom139 added 2 commits April 25, 2020 18:00

feat(cognito): allow set read write attributes in client

acc76d4

When creating a userpool client, the user can now specify which attributes (custom or standard) the app client will be able to read or write. Closes aws#7407

docs(cognito): add section for read/write attributes in clients

7ca180c

Add section to explain hot to make specific attributes available for read and write to specific clients. Fixes aws#7407

SomayaB assigned nija-at Apr 28, 2020

nija-at previously requested changes Apr 30, 2020

View reviewed changes

docs: improve read-write attributes section in README

562cc4d

Co-authored-by: Niranjan Jayakar <nija@amazon.com>

tom139 and others added 4 commits May 3, 2020 12:36

docs: improve read-write attribute section in user pool client

f57d268

docs(cognito): add clarification about user pool client

8fe041b

specifying how read/write attributes behave when new attributes get added to a user pool.

Merge branch 'master' into tom139/cognito-userpool-client-set-readwri…

401493f

…te-attributes

docs(cognito): improve docstring in userpool

9315f41

Co-authored-by: Niranjan Jayakar <nija@amazon.com>

tom139 commented May 3, 2020

View reviewed changes

packages/@aws-cdk/aws-cognito/README.md Outdated Show resolved Hide resolved

nija-at previously requested changes May 12, 2020

View reviewed changes

tom139 added 5 commits December 14, 2020 21:43

feat(cognito): add AttributeSet and StandardAttributeMask

117eb53

Initial work to add the notion of set of attributes (mixed standard and custom) fixes aws#7407

fix: wrong attribute name for profile and picture

c95781b

feat(cognito): add ability to set read and write attributes in client

a9bbdeb

fixes aws#7407

docs(cognito): for read and write attributes

36aa216

in userpool client Allow to set read and write attributes in Cognito UserPoolClient fixes aws#7407

Merge branch 'cognito-userpool-client-set-read-write-attributes' into…

3e45153

… tom139/cognito-userpool-client-set-readwrite-attributes

Merge branch 'master' into tom139/cognito-userpool-client-set-readwri…

9b5e866

…te-attributes

nija-at previously requested changes Jan 11, 2021

View reviewed changes

tom139 added 4 commits January 12, 2021 20:43

refactor: rename AttributeSet to ClientAttributes

41a2220

Merge branch 'master' into tom139/cognito-userpool-client-set-readwri…

56fe812

…te-attributes

refactor: ClientAttributes to more expressive API

9264458

docs: fix comment on email_verified and phone_number_verified

76be44d

docs: update default behaviour description for ClientAttributes

9c986b3

Co-authored-by: Niranjan Jayakar <nija@amazon.com>

tom139 commented Jan 13, 2021

View reviewed changes

packages/@aws-cdk/aws-cognito/lib/user-pool-client.ts Outdated Show resolved Hide resolved

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

packages/@aws-cdk/aws-cognito/lib/user-pool-attr.ts Outdated Show resolved Hide resolved

tom139 and others added 2 commits January 13, 2021 01:04

Merge branch 'master' into tom139/cognito-userpool-client-set-readwri…

7765318

…te-attributes

fix: add emailVerified and phoneNumberVerified to StandardAttributes

6b6630c

nija-at previously requested changes Jan 18, 2021

View reviewed changes

refactor: remove ClientAttributes.empty() method

2c2009d

and replace it with (new ClientAttribute())

tom139 commented Jan 18, 2021

View reviewed changes

tom139 and others added 2 commits January 18, 2021 22:57

Merge branch 'master' into tom139/cognito-userpool-client-set-readwri…

2af64b0

…te-attributes

docs: improve README on ClientAttributes

b9e0a2c

as suggested by @nija-at closes aws#7607

nija-at approved these changes Jan 20, 2021

View reviewed changes

Merge branch 'master' into tom139/cognito-userpool-client-set-readwri…

49f6eea

…te-attributes

mergify bot merged commit 552e1e9 into aws:master Jan 20, 2021

tom139 deleted the tom139/cognito-userpool-client-set-readwrite-attributes branch January 25, 2021 07:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cognito): allow to set read and write attributes in Cognito UserPoolClient #7607

feat(cognito): allow to set read and write attributes in Cognito UserPoolClient #7607

tom139 commented Apr 25, 2020

nija-at left a comment

nija-at left a comment •

edited

Loading

arnulfojr commented Jun 10, 2020

gitpod-io bot commented Dec 14, 2020 •

edited

Loading

tom139 commented Dec 14, 2020

nija-at commented Dec 18, 2020

tom139 commented Dec 20, 2020

nija-at left a comment

nija-at Jan 11, 2021

tom139 Jan 12, 2021 •

edited

Loading

nija-at Jan 14, 2021

tom139 Jan 14, 2021

tom139 left a comment

nija-at left a comment

nija-at Jan 18, 2021 •

edited

Loading

tom139 Jan 18, 2021

nija-at Jan 18, 2021

tom139 left a comment •

edited

Loading

tom139 Jan 18, 2021

nija-at commented Jan 20, 2021

mergify bot commented Jan 20, 2021

aws-cdk-automation commented Jan 20, 2021

mergify bot commented Jan 20, 2021

russelldavis commented Jan 25, 2021 •

edited

Loading

russelldavis commented Jan 25, 2021

russelldavis commented Jan 25, 2021

tom139 commented Jan 25, 2021

tom139 commented Jan 25, 2021

russelldavis commented Jan 25, 2021

russelldavis commented Jan 25, 2021

tom139 commented Jan 25, 2021

feat(cognito): allow to set read and write attributes in Cognito UserPoolClient #7607

feat(cognito): allow to set read and write attributes in Cognito UserPoolClient #7607

Conversation

tom139 commented Apr 25, 2020

Commit Message

End Commit Message

nija-at left a comment

Choose a reason for hiding this comment

nija-at left a comment • edited Loading

Choose a reason for hiding this comment

arnulfojr commented Jun 10, 2020

gitpod-io bot commented Dec 14, 2020 • edited Loading

tom139 commented Dec 14, 2020

nija-at commented Dec 18, 2020

tom139 commented Dec 20, 2020

nija-at left a comment

Choose a reason for hiding this comment

nija-at Jan 11, 2021

Choose a reason for hiding this comment

tom139 Jan 12, 2021 • edited Loading

Choose a reason for hiding this comment

nija-at Jan 14, 2021

Choose a reason for hiding this comment

tom139 Jan 14, 2021

Choose a reason for hiding this comment

tom139 left a comment

Choose a reason for hiding this comment

nija-at left a comment

Choose a reason for hiding this comment

nija-at Jan 18, 2021 • edited Loading

Choose a reason for hiding this comment

tom139 Jan 18, 2021

Choose a reason for hiding this comment

nija-at Jan 18, 2021

Choose a reason for hiding this comment

tom139 left a comment • edited Loading

Choose a reason for hiding this comment

tom139 Jan 18, 2021

Choose a reason for hiding this comment

nija-at commented Jan 20, 2021

mergify bot commented Jan 20, 2021

aws-cdk-automation commented Jan 20, 2021

AWS CodeBuild CI Report

mergify bot commented Jan 20, 2021

russelldavis commented Jan 25, 2021 • edited Loading

russelldavis commented Jan 25, 2021

russelldavis commented Jan 25, 2021

tom139 commented Jan 25, 2021

Having two all attributes methods

Having only one method

tom139 commented Jan 25, 2021

russelldavis commented Jan 25, 2021

russelldavis commented Jan 25, 2021

tom139 commented Jan 25, 2021

nija-at left a comment •

edited

Loading

gitpod-io bot commented Dec 14, 2020 •

edited

Loading

tom139 Jan 12, 2021 •

edited

Loading

nija-at Jan 18, 2021 •

edited

Loading

tom139 left a comment •

edited

Loading

russelldavis commented Jan 25, 2021 •

edited

Loading