-
Notifications
You must be signed in to change notification settings - Fork 218
feat(auth): Refresh Token Rotation #4043
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
base: main
Are you sure you want to change the base?
Conversation
… enable refresh token rotation, also updated test mock clients and added unit tests
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #4043 +/- ##
===========================================
- Coverage 67.80% 48.03% -19.78%
===========================================
Files 1129 366 -763
Lines 52478 8917 -43561
===========================================
- Hits 35585 4283 -31302
+ Misses 16893 4634 -12259
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
refreshToken: existingTokens.refreshToken, | ||
expiresIn: authenticationResult.expiresIn | ||
refreshToken: authenticationResult.refreshToken ?? existingTokens.refreshToken |
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.
refreshToken: existingTokens.refreshToken, | |
expiresIn: authenticationResult.expiresIn | |
refreshToken: authenticationResult.refreshToken ?? existingTokens.refreshToken | |
guard let authenticationResult = response?.authenticationResult, | |
let idToken = authenticationResult.idToken, | |
let accessToken = authenticationResult.accessToken, | |
let refreshToken = authenticationResult.refreshToken. <------- change | |
else { | |
let event = RefreshSessionEvent(eventType: .throwError(.invalidTokens)) | |
await dispatcher.send(event) | |
logVerbose("\(#fileID) Sending event \(event.type)", environment: environment) | |
return | |
} | |
let userPoolTokens = AWSCognitoUserPoolTokens( | |
idToken: idToken, | |
accessToken: accessToken, | |
refreshToken: refreshToken. <------- change |
I don't think we should use the old refresh token at this point.. Should throw an error if the refresh token doesn't exist, as we have entered an unknown state because of missing tokens.
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.
You are right, there's no use for the old token, this was here because its possible the user doesn't have rotation enabled but in that case the returned token would just be their one token anyways
return GetTokensFromRefreshTokenOutput( | ||
authenticationResult: .init( | ||
accessToken: "accessTokenNew", | ||
expiresIn: 100, | ||
idToken: "idTokenNew", | ||
refreshToken: nil)) |
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.
Since the refresh token is nil
, can that ever happen.. I personally think it shouldn't and this test case should throw and error instead.
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.
You are right, while the API can return a null token we should just throw an error in that case
|
||
let input = await InitiateAuthInput.refreshAuthInput( | ||
username: existingSignedIndata.username, | ||
refreshToken: existingTokens.refreshToken, |
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.
We should remove the implementation of refreshAuthInput
method as its not longer required.
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 already removed in this PR unless I missed something
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 meant the implementation in InitiateAuthInput+Amplify.swift
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.
got it
@@ -9,7 +9,7 @@ let platforms: [SupportedPlatform] = [ | |||
.watchOS(.v9) | |||
] | |||
let dependencies: [Package.Dependency] = [ | |||
.package(url: "https://github.com/awslabs/aws-sdk-swift", exact: "1.5.14"), | |||
.package(url: "https://github.com/awslabs/aws-sdk-swift", exact: "1.5.18"), |
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 is the version required for the patch SDK change?
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 the minimum
…ck for invalidTokens error in case the API returns a null token
…4041) * chore(build): fix build warnings for swift 6 experimental features * chore: add @unchecked Sendable conformances and nonisolated modifiers * chore: fix swiftlint errors * chore: add @preconcurrency attribute for module imports * chore: add more preconcurrency attributes * chore: add preconcurrency to model generated files * chore: fix failing unit test * chore: revert ActivityTracker changes * chore: update EndpointResolving * chore: resolve swiftlint error
…n canary build (#4047) * chore(build): increase fastlane timeout for failing canary * update code * update code * update canaries project * update amplify dependency * revert gems to older version for dependency review failure * revert gem with incompatible license * update code * update code * update code
* chore: update Github action workflows chore: updating Github action workflows remove runtime check another update more updates update versions that are available for integration tests update versions add lowest as 18.4 test disabling parallel builds Revert "test disabling parallel builds" This reverts commit ce7eb6f. try to use the webauthn approach remove --set testing another update Update index.mjs another update try another fix another update another try final update try install simulators if needed flow run download only for xcode 16 * list available content list * try selecting xcode before * Update canary.yml
Changed InitiateAuth API for refreshing Cognito User Pool tokens to the new GetTokensFromRefreshToken API which enables a user to rotate their refresh token if they have the feature enabled
Additionally updated the AWS Swift SDK version which fixed a bug in this API
Issue #
#4027
General Checklist
Given When Then
inline code documentation and are named accordinglytestThing_condition_expectation()
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.