feat(cognito): support refresh token rotation #34360
Conversation
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
212351e to
0544a67
Compare
| * The state of refresh token rotation for the current app client. | ||
| * @default - undefined (CloudFormation defaults to DISABLED) | ||
| */ | ||
| readonly feature?: boolean; |
There was a problem hiding this comment.
Re-reading this after the change with the boolean: I don't think we need the user to set this field: if the user passes a RefreshTokenRotation property in the UserPoolClientProps then the feature should be considered enabled.
Note: I would still keep the RefreshTokenRotation interface with only retryGracePeriodSeconds in case the service adds new feature.
| * Grace period for the original refresh token (0-60 seconds). | ||
| * @default - undefined (CloudFormation defaults value) | ||
| */ | ||
| readonly retryGracePeriodSeconds?: Duration; |
There was a problem hiding this comment.
Because refreshTokenRotation is optional in the UserPoolClientOptions, this field should not be optional.
| props.refreshTokenRotation.retryGracePeriodSeconds.toSeconds() > 0 ? 'ENABLED' : 'DISABLED', | ||
| retryGracePeriodSeconds: props.refreshTokenRotation.retryGracePeriodSeconds ? | ||
| props.refreshTokenRotation.retryGracePeriodSeconds.toSeconds() : 0, | ||
| } : undefined, |
There was a problem hiding this comment.
I would move this logic outside of the CfnUserPoolClient creation for better readability (+ if the feature field is removed the logic can be simplified).
|
@leonmk-aws Thank you for your patience, I made the changes. |
There was a problem hiding this comment.
@iridescent99 Added comments, I think you added a bug when making the latest changes
| // refreshToken should always be allowed if authFlows are present | ||
| authFlows.push('ALLOW_REFRESH_TOKEN_AUTH'); | ||
| // refreshToken should only be allowed if authFlows are present and refreshTokenRotation is disabled | ||
| if (!props.refreshTokenRotation || props.refreshTokenRotation.retryGracePeriodSeconds.toSeconds() === 0) { |
There was a problem hiding this comment.
Your previous implementation was correct, this one is not: when the retryGracePeriodSeconds is set to 0, the feature is still enabled: you get a new refreshtoken and the old refresh token is invalidated immediately.
The condition here should simply be if (!props.refreshTokenRotation)
| } | ||
| resource.refreshTokenRotation = props.refreshTokenRotation | ||
| ? { | ||
| feature: props.refreshTokenRotation.retryGracePeriodSeconds.toSeconds() > 0 ? 'ENABLED' : 'DISABLED', |
There was a problem hiding this comment.
This is not correct, the feature is enabled even if the grace period is set to 0 (see my comment above)
| * Grace period for the original refresh token (0-60 seconds). | ||
| * @default - undefined (CloudFormation defaults value) | ||
| */ | ||
| readonly retryGracePeriodSeconds: Duration; |
There was a problem hiding this comment.
Now that we use a duration type, retryGracePeriod would be a better name.
| export interface RefreshTokenRotation { | ||
| /** | ||
| * Grace period for the original refresh token (0-60 seconds). | ||
| * @default - undefined (CloudFormation defaults value) |
There was a problem hiding this comment.
There is no more default anymore
|
@leonmk-aws ok another attempt |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
Issue # (if applicable)
Closes #34344
Reason for this change
Cognito added support for short-lived refresh tokens.
Description of changes
Added refreshTokenRotation property to UserPoolClient
Describe any new or updated permissions being added
NA
Description of how you validated changes
Unit + integrationt tests
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license