- 
                Notifications
    You must be signed in to change notification settings 
- Fork 4.3k
feat(cognito): support refresh token rotation #34360
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
Conversation
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.
(This review is outdated)
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
| Thanks! This okay? | 
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.
Small comment, feel free to add your opinion on it
| * @see https://docs.aws.amazon.com/cognito/latest/developerguide/amazon-cognito-user-pools-using-the-refresh-token.html#using-the-refresh-token-rotation | ||
| * @default - undefined (refresh token rotation is disabled) | ||
| */ | ||
| readonly refreshTokenGracePeriod?: Duration; | 
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 think having the variable named refreshTokenRotationGracePeriod would bring visibility that this is only effective when using refresh token rotation because people could get confused that this sets a grace period when not using the refresh token rotation feature. Also I think updating the documentation to make it clear that this enables the refresh token rotation feature would be valuable (the README is very clear in that regard, but having it here as well would be important even if the link to the doc is provided).
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.
To be more clear: before the changes of yesterday, it was very clear for the user that this field only is only effective when enabling refresh token rotation and I feel that when flattening we slightly lost this clarity and we can improve 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.
That's fair, I was having doubts about the description here as well. Sorry about the name, I didnt read that correctly.
So I will change the name to refreshTokenRotationGracePeriod.
and for the description this ok?
Enables refresh token rotation when set.
Defines the grace period for the original refresh token (0-60 seconds).
Can I still push these changes after approval?
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.
Yes, you can make changes after approval
| 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). | 
| This pull request has been removed from the queue for the following reason:  The pull request can't be updated. You should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again. | 
Pull request has been modified.
| AWS CodeBuild CI Report
 Powered by github-codebuild-logs, available on the AWS Serverless Application Repository | 
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.
Could you resolve merge conflicts?
Pull request has been modified.
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.
LGTM
| 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). | 
| This pull request has been removed from the queue for the following reason:  The pull request can't be updated. You should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again. | 
Pull request has been modified.
| 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). | 
| Comments on closed issues and PRs are hard for our team to see. | 
Issue # (if applicable)
Closes #34344
Reason for this change
Cognito added support for short-lived refresh tokens.
Description of changes
Added refreshTokenRotationGracePeriod property to UserPoolClient
Describe any new or updated permissions being added
NA
Description of how you validated changes
Unit + integration tests
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license