Conversation
| val expiresAt = newCredentials.expiresAt.time | ||
| val willAccessTokenExpire = willExpire(expiresAt, minTtl.toLong()) | ||
| if (willAccessTokenExpire) { | ||
| val tokenLifetime = (expiresAt - currentTimeInMillis - minTtl * 1000) / -1000 |
Check warning
Code scanning / CodeQL
Result of multiplication cast to wider type Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 11 months ago
To fix the issue, we need to ensure that the multiplication minTtl * 1000 is performed in a long context to avoid integer overflow. This can be achieved by explicitly casting one of the operands (minTtl or 1000) to long before the multiplication. This ensures that the result of the multiplication is a long and can safely handle larger values.
The specific change will be made on line 610, where minTtl * 1000 is replaced with minTtl.toLong() * 1000. This change does not alter the logic of the program but ensures that the multiplication is performed in a long context.
| @@ -609,3 +609,3 @@ | ||
| if (willAccessTokenExpire) { | ||
| val tokenLifetime = (expiresAt - currentTimeInMillis - minTtl * 1000) / -1000 | ||
| val tokenLifetime = (expiresAt - currentTimeInMillis - minTtl.toLong() * 1000) / -1000 | ||
| val wrongTtlException = CredentialsManagerException( |
| val expiresAt = newCredentials.expiresAt.time | ||
| val willAccessTokenExpire = willExpire(expiresAt, minTtl.toLong()) | ||
| if (willAccessTokenExpire) { | ||
| val tokenLifetime = (expiresAt - currentTimeInMillis - minTtl * 1000) / -1000 |
Check warning
Code scanning / CodeQL
Result of multiplication cast to wider type Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 11 months ago
To fix the issue, we need to ensure that the multiplication minTtl * 1000 is performed safely without risking integer overflow. This can be achieved by casting one of the operands to Long before the multiplication. This ensures that the multiplication is performed in the long domain, which has a much larger range than int.
Specifically, we can cast minTtl to Long before multiplying it by 1000. This change is minimal and does not alter the logic of the code. The corrected line will look like this: (minTtl.toLong() * 1000).
| @@ -942,3 +942,3 @@ | ||
| if (willAccessTokenExpire) { | ||
| val tokenLifetime = (expiresAt - currentTimeInMillis - minTtl * 1000) / -1000 | ||
| val tokenLifetime = (expiresAt - currentTimeInMillis - minTtl.toLong() * 1000) / -1000 | ||
| val wrongTtlException = CredentialsManagerException( |
| * @param audience the audience for which the credentials are stored | ||
| */ | ||
| override fun saveApiCredentials(apiCredentials: APICredentials, audience: String) { | ||
| gson.toJson(apiCredentials).let { |
There was a problem hiding this comment.
Should this method throw an error if the serialization fails?
There was a problem hiding this comment.
This shouldn't cause any serialization issue and wouldn't require to throw an exception
|
|
||
| /** | ||
| * Retrieves API credentials from storage and automatically renews them using the refresh token if the access | ||
| * token is expired. Otherwise, the retrieved API credentials will be returned via the success case as they are still valid. |
There was a problem hiding this comment.
This method is an async wrapper, so there is no success case.
| * token is expired. Otherwise, the retrieved API credentials will be returned via the success case as they are still valid. | ||
| * | ||
| * If there are no stored API credentials, the refresh token will be exchanged for a new set of API credentials. | ||
| * New or renewed API credentials will be automatically stored in storage. |
There was a problem hiding this comment.
| * New or renewed API credentials will be automatically stored in storage. | |
| * New or renewed API credentials will be automatically persisted in storage. |
| * token is expired. Otherwise, the retrieved API credentials will be returned via the success case as they are still valid. | ||
| * | ||
| * If there are no stored API credentials, the refresh token will be exchanged for a new set of API credentials. | ||
| * New or renewed API credentials will be automatically stored in storage. |
There was a problem hiding this comment.
| * New or renewed API credentials will be automatically stored in storage. | |
| * New or renewed API credentials will be automatically persisted in storage. |
|
|
||
| /** | ||
| * Retrieves API credentials from storage and automatically renews them using the refresh token if the access | ||
| * token is expired. Otherwise, the retrieved API credentials will be returned via the success case as they are still valid. |
There was a problem hiding this comment.
| * token is expired. Otherwise, the retrieved API credentials will be returned via the success case as they are still valid. | |
| * token is expired. Otherwise, the retrieved API credentials will be returned via the success callback as they are still valid. |
|
|
||
| /** | ||
| * Retrieves API credentials from storage and automatically renews them using the refresh token if the access | ||
| * token is expired. Otherwise, the retrieved API credentials will be returned via the success case as they are still valid. |
There was a problem hiding this comment.
This method is an async wrapper, so there is no success case.
| * token is expired. Otherwise, the retrieved API credentials will be returned via the success case as they are still valid. | ||
| * | ||
| * If there are no stored API credentials, the refresh token will be exchanged for a new set of API credentials. | ||
| * New or renewed API credentials will be automatically stored in storage. |
There was a problem hiding this comment.
| * New or renewed API credentials will be automatically stored in storage. | |
| * New or renewed API credentials will be automatically persisted in storage. |
|
|
||
| /** | ||
| * Retrieves API credentials from storage and automatically renews them using the refresh token if the access | ||
| * token is expired. Otherwise, the retrieved API credentials will be returned via the success case as they are still valid. |
There was a problem hiding this comment.
| * token is expired. Otherwise, the retrieved API credentials will be returned via the success case as they are still valid. | |
| * token is expired. Otherwise, the retrieved API credentials will be returned via the success callback as they are still valid. |
| ) { | ||
| val localAuthenticationManager = localAuthenticationManagerFactory?.create( | ||
| activity = fragmentActivity, | ||
| authenticationOptions = localAuthenticationOptions!!, |
There was a problem hiding this comment.
Should this value be passed via argument, like fragmentActivity and biometricResultCallback?
There was a problem hiding this comment.
This is a global property that will be set when the instance of the SecureCredentialsManager is created. Hence no need to pass as an argument
There was a problem hiding this comment.
Yes. but fragment activity is a weak reference which we check if there is a valid reference present or not before invoking the methods. Hence they are passed as arguments
There was a problem hiding this comment.
Yes, but you also check that localAuthenticationOptions is not null before calling this method. Like you do for the fragment activity.
There was a problem hiding this comment.
The only difference between the two is that one is weak and the other is not, but both are nullable. Hence the comment.
There was a problem hiding this comment.
We do have the check of localAuthenticationOptions != null before calling . Hence we are accessing this globally. For fragment activity we have two null checks , one of the property is not null and they secondary check to ensure the reference inside the property is still non null or not. We pass this non null reference as local parameter ,to avoid any issues even if the main reference got destroyed.
if (fragmentActivity != null && localAuthenticationOptions != null && localAuthenticationManagerFactory != null) {
fragmentActivity.get()?.let { fragmentActivity ->
startBiometricAuthentication(
fragmentActivity,
biometricAuthenticationApiCredentialsCallback(
audience, scope, minTtl, parameters, headers, callback
)
)
} ?: run {
callback.onFailure(CredentialsManagerException.BIOMETRIC_ERROR_NO_ACTIVITY)
}
return
}
| * Removes the credentials for the given audience from the storage if present. | ||
| */ | ||
| override fun clearApiCredentials(audience: String) { | ||
| storage.remove(audience) |
There was a problem hiding this comment.
Should this be logged like in the SecureCredentialsManager? E.g.:
Log.d(TAG, "API Credentials for $audience were just removed from the storage")
| internal fun continueGetApiCredentials( | ||
| audience: String, | ||
| scope: String?, | ||
| minTtl: Int = 0, |
There was a problem hiding this comment.
Does this argument need a default value here?
There was a problem hiding this comment.
Not needed. Removed
| @field:SerializedName("expires_at") | ||
| val expiresAt: Date, | ||
| /** | ||
| * Getter for the access token's granted scope. Only available if the requested scope differs from the granted one. |
There was a problem hiding this comment.
Only available if the requested scope differs from the granted one. This doesn't seem to be the case anymore.
| * Converts a Credentials instance to an APICredentials instance. | ||
| */ | ||
| internal fun Credentials.toAPICredentials(): APICredentials { | ||
| val newScope = scope ?: "openid" |
There was a problem hiding this comment.
I'd suggest using an empty string instead. If the server response doesn't include a scope value, we cannot assume openid was the only requested scope.
| val auth0 = auth0 | ||
| val client = AuthenticationAPIClient(auth0) | ||
| mockAPI.willReturnSuccessfulLogin() | ||
| val credentials = client.renewAuth(refreshToken = "refreshToken", scope = "read:data") |
There was a problem hiding this comment.
To differentiate this case from shouldRenewAuthWithOAuthAudienceAndScopeEnforcingOpendId in the "enforcing openid" aspect, I'd suggest using openid read:data as the scope value here.
There was a problem hiding this comment.
Also, that would make shouldRenewAuthWithOAuthAudienceAndScopeNotEnforcingOpendId redundant, so it could be removed.
| val renewedCredentials = | ||
| Credentials("newId", "newAccess", "newType", newRefresh, newDate, "newScope") | ||
| Mockito.`when`(request.execute()).thenReturn(renewedCredentials) | ||
| manager.getApiCredentials("audience", "newScope", minTtl = 10, callback = apiCredentialsCallback) |
There was a problem hiding this comment.
I'd suggest using the same scope here to make it clear the renewal is not happening because of the different scopes, but because of the minTtl.
| val renewedCredentials = | ||
| Credentials("newId", "newAccess", "newType", newRefresh, newDate, "newScope") | ||
| Mockito.`when`(request.execute()).thenReturn(renewedCredentials) | ||
| manager.getApiCredentials("audience", "newScope", callback = apiCredentialsCallback) |
There was a problem hiding this comment.
I'd suggest using the same scope here to make it clear that the renewal is not happening because of the different scopes but because the access token has expired.
|
|
||
| // Verify the credentials are property stored | ||
| verify(storage).store("com.auth0.id_token", renewedCredentials.idToken) | ||
| // RefreshToken should not be replaced |
There was a problem hiding this comment.
| // RefreshToken should not be replaced | |
| // RefreshToken should be replaced |
|
|
||
| // Verify the credentials are property stored | ||
| verify(storage).store(eq("com.auth0.credentials"), stringCaptor.capture()) | ||
| MatcherAssert.assertThat(stringCaptor.firstValue, Is.`is`(Matchers.notNullValue())) |
There was a problem hiding this comment.
Should we assert here that the refresh token was not replaced?
| apiCredentialsCaptor.capture() | ||
| ) | ||
|
|
||
| // Verify the returned credentials are the latest |
| apiCredentialsCaptor.capture() | ||
| ) | ||
|
|
||
| // Verify the returned credentials are the latest |
| apiCredentialsCaptor.capture() | ||
| ) | ||
|
|
||
| // Verify the returned credentials are the latest |
|
|
||
| // Verify the returned credentials are the latest |
There was a problem hiding this comment.
| // Verify the returned credentials are the latest |
|
|
||
| // Verify the returned credentials are the latest | ||
| val exception = exceptionCaptor.firstValue | ||
| MatcherAssert.assertThat(exception, Is.`is`(Matchers.notNullValue())) |
There was a problem hiding this comment.
Should we assert on the exception message here?
There was a problem hiding this comment.
The timing shown in the message can vary dynamically so can't have the text
Changes
This PR moves the Credentials Manager from a single credentials model to a multiple credentials one, supporting:
1 set of app credentials (the existing functionality)
N sets of API-specific credentials
To this end, two new public methods were added to the Credentials Manager:
Testing
Please describe how this can be tested by reviewers. Be specific about anything not tested and reasons why. Since this library has unit testing, tests should be added for new functionality and existing tests should complete without errors.
This change adds unit test coverage
This change adds integration test coverage
This change has been tested on the latest version of the platform/language or why not
Checklist
I have read the Auth0 general contribution guidelines
I have read the Auth0 Code of Conduct
All existing and new tests complete without errors