Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(cognito): add mutable property in cognito user pool custom attribute #7190
feat(cognito): add mutable property in cognito user pool custom attribute #7190
Changes from 2 commits
512e588
e53f7a8
5b29492
4cb27f8
c7f064f
70db8d7
7a449b9
7b772f5
05975a6
fb8dfa3
9ace0a2
5100fa1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is Cognito's recommendation
from AWS::Cognito::UserPool SchemaAttribute in Cloudformation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A separate PR would be preferable and cleaner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I will remove the support for the
developerOnly
attribute altogether and propose a new PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does setting
mutable
tofalse
mean that even administrators can't modify it?In general, is there a combination of
mutable
anddeveloperOnly
that is invalid or does not make sense?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great question.
Yes, setting
mutable
tofalse
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 ofmutable
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 theAdminCreateUser
api can have that attribute set while users created via theSignUp
API will never be able to set it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop the prefix
Base
from here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done ✅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. Let's drop 'Base'.
Unfortunately, if an exported class inherits this, this will also need to be exported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done ✅