-
Notifications
You must be signed in to change notification settings - Fork 221
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?
Changes from 3 commits
f386a64
9f81731
d7415a4
c164905
6b35ed5
30058c8
56378fd
b9fd855
2739eee
3fe008e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -28,32 +28,34 @@ struct RefreshUserPoolTokens: Action { | |||||||||||||||||||||||||||||||||||||||
return | ||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
let authEnv = try environment.authEnvironment() | ||||||||||||||||||||||||||||||||||||||||
let config = environment.userPoolConfiguration | ||||||||||||||||||||||||||||||||||||||||
let client = try? environment.cognitoUserPoolFactory() | ||||||||||||||||||||||||||||||||||||||||
let existingTokens = existingSignedIndata.cognitoUserPoolTokens | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
let deviceMetadata = await DeviceMetadataHelper.getDeviceMetadata( | ||||||||||||||||||||||||||||||||||||||||
for: existingSignedIndata.username, | ||||||||||||||||||||||||||||||||||||||||
with: environment) | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
let asfDeviceId = try await CognitoUserPoolASF.asfDeviceID( | ||||||||||||||||||||||||||||||||||||||||
for: existingSignedIndata.username, | ||||||||||||||||||||||||||||||||||||||||
credentialStoreClient: authEnv.credentialsClient) | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
let input = await InitiateAuthInput.refreshAuthInput( | ||||||||||||||||||||||||||||||||||||||||
username: existingSignedIndata.username, | ||||||||||||||||||||||||||||||||||||||||
refreshToken: existingTokens.refreshToken, | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
let deviceKey: String? = { | ||||||||||||||||||||||||||||||||||||||||
if case .metadata(let data) = deviceMetadata { | ||||||||||||||||||||||||||||||||||||||||
return data.deviceKey | ||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
return nil | ||||||||||||||||||||||||||||||||||||||||
}() | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
let input = GetTokensFromRefreshTokenInput( | ||||||||||||||||||||||||||||||||||||||||
clientId: config.clientId, | ||||||||||||||||||||||||||||||||||||||||
clientMetadata: [:], | ||||||||||||||||||||||||||||||||||||||||
asfDeviceId: asfDeviceId, | ||||||||||||||||||||||||||||||||||||||||
deviceMetadata: deviceMetadata, | ||||||||||||||||||||||||||||||||||||||||
environment: environment) | ||||||||||||||||||||||||||||||||||||||||
clientSecret: config.clientSecret, | ||||||||||||||||||||||||||||||||||||||||
deviceKey: deviceKey, | ||||||||||||||||||||||||||||||||||||||||
refreshToken: existingTokens.refreshToken | ||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
logVerbose("\(#fileID) Starting initiate auth refresh token", environment: environment) | ||||||||||||||||||||||||||||||||||||||||
logVerbose("\(#fileID) Starting get tokens from refresh token", environment: environment) | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
let response = try await client?.initiateAuth(input: input) | ||||||||||||||||||||||||||||||||||||||||
let response = try await client?.getTokensFromRefreshToken(input: input) | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
logVerbose("\(#fileID) Initiate auth response received", environment: environment) | ||||||||||||||||||||||||||||||||||||||||
logVerbose("\(#fileID) Get tokens from refresh token response received", environment: environment) | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
guard let authenticationResult = response?.authenticationResult, | ||||||||||||||||||||||||||||||||||||||||
let idToken = authenticationResult.idToken, | ||||||||||||||||||||||||||||||||||||||||
|
@@ -69,8 +71,7 @@ struct RefreshUserPoolTokens: Action { | |||||||||||||||||||||||||||||||||||||||
let userPoolTokens = AWSCognitoUserPoolTokens( | ||||||||||||||||||||||||||||||||||||||||
idToken: idToken, | ||||||||||||||||||||||||||||||||||||||||
accessToken: accessToken, | ||||||||||||||||||||||||||||||||||||||||
refreshToken: existingTokens.refreshToken, | ||||||||||||||||||||||||||||||||||||||||
expiresIn: authenticationResult.expiresIn | ||||||||||||||||||||||||||||||||||||||||
refreshToken: authenticationResult.refreshToken ?? existingTokens.refreshToken | ||||||||||||||||||||||||||||||||||||||||
|
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,8 +45,8 @@ class RefreshUserPoolTokensTests: XCTestCase { | |
let expectation = expectation(description: "refreshUserPoolTokens") | ||
let identityProviderFactory: BasicSRPAuthEnvironment.CognitoUserPoolFactory = { | ||
MockIdentityProvider( | ||
mockInitiateAuthResponse: { _ in | ||
return InitiateAuthOutput() | ||
mockGetTokensFromRefreshTokenResponse: { _ in | ||
return GetTokensFromRefreshTokenOutput() | ||
} | ||
) | ||
} | ||
|
@@ -77,8 +77,8 @@ class RefreshUserPoolTokensTests: XCTestCase { | |
let expectation = expectation(description: "refreshUserPoolTokens") | ||
let identityProviderFactory: BasicSRPAuthEnvironment.CognitoUserPoolFactory = { | ||
MockIdentityProvider( | ||
mockInitiateAuthResponse: { _ in | ||
return InitiateAuthOutput( | ||
mockGetTokensFromRefreshTokenResponse: { _ in | ||
return GetTokensFromRefreshTokenOutput( | ||
authenticationResult: .init( | ||
accessToken: "accessTokenNew", | ||
expiresIn: 100, | ||
|
@@ -114,7 +114,7 @@ class RefreshUserPoolTokensTests: XCTestCase { | |
|
||
let identityProviderFactory: BasicSRPAuthEnvironment.CognitoUserPoolFactory = { | ||
MockIdentityProvider( | ||
mockInitiateAuthResponse: { _ in | ||
mockGetTokensFromRefreshTokenResponse: { _ in | ||
throw testError | ||
} | ||
) | ||
|
@@ -144,4 +144,74 @@ class RefreshUserPoolTokensTests: XCTestCase { | |
) | ||
} | ||
|
||
func testRefreshTokenRotation() async { | ||
|
||
let expectation = expectation(description: "refreshTokenRotation") | ||
let identityProviderFactory: BasicSRPAuthEnvironment.CognitoUserPoolFactory = { | ||
MockIdentityProvider( | ||
mockGetTokensFromRefreshTokenResponse: { _ in | ||
return GetTokensFromRefreshTokenOutput( | ||
authenticationResult: .init( | ||
accessToken: "accessTokenNew", | ||
expiresIn: 100, | ||
idToken: "idTokenNew", | ||
refreshToken: "refreshTokenRotated")) | ||
} | ||
) | ||
} | ||
|
||
let action = RefreshUserPoolTokens(existingSignedIndata: .testData) | ||
|
||
await action.execute(withDispatcher: MockDispatcher { event in | ||
|
||
if let userPoolEvent = event as? RefreshSessionEvent, | ||
case let .refreshIdentityInfo(signedInData, _) = userPoolEvent.eventType { | ||
XCTAssertEqual(signedInData.cognitoUserPoolTokens.refreshToken, "refreshTokenRotated") | ||
expectation.fulfill() | ||
} | ||
}, environment: Defaults.makeDefaultAuthEnvironment( | ||
userPoolFactory: identityProviderFactory) | ||
) | ||
|
||
await fulfillment( | ||
of: [expectation], | ||
timeout: 0.1 | ||
) | ||
} | ||
|
||
func testRefreshTokenNoRotation() async { | ||
|
||
let expectation = expectation(description: "refreshTokenNoRotation") | ||
let identityProviderFactory: BasicSRPAuthEnvironment.CognitoUserPoolFactory = { | ||
MockIdentityProvider( | ||
mockGetTokensFromRefreshTokenResponse: { _ in | ||
return GetTokensFromRefreshTokenOutput( | ||
authenticationResult: .init( | ||
accessToken: "accessTokenNew", | ||
expiresIn: 100, | ||
idToken: "idTokenNew", | ||
refreshToken: nil)) | ||
Comment on lines
+199
to
+204
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since the refresh token is There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 action = RefreshUserPoolTokens(existingSignedIndata: .testData) | ||
|
||
await action.execute(withDispatcher: MockDispatcher { event in | ||
|
||
if let userPoolEvent = event as? RefreshSessionEvent, | ||
case let .refreshIdentityInfo(signedInData, _) = userPoolEvent.eventType { | ||
XCTAssertEqual(signedInData.cognitoUserPoolTokens.refreshToken, "refreshToken") | ||
expectation.fulfill() | ||
} | ||
}, environment: Defaults.makeDefaultAuthEnvironment( | ||
userPoolFactory: identityProviderFactory) | ||
) | ||
|
||
await fulfillment( | ||
of: [expectation], | ||
timeout: 0.1 | ||
) | ||
} | ||
|
||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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