-
Notifications
You must be signed in to change notification settings - Fork 147
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
Accept a custom clock instance in both Credentials Managers [SDK-1973] #358
Conversation
* <p> | ||
* This class is thread-safe. | ||
*/ | ||
final class ClockImpl implements Clock { |
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.
no need to expose this implementation
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.
Is using the Impl
suffix on something that implements an interface a common thing in the Java world?
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.
Some people use it, others prefer the IClock
(interface) and Clock
(implementation) variation. In this case, this implementation is internal so the pre/suffix doesn't really matter. For reference, in java-jwt we are doing the same https://github.com/auth0/java-jwt/blob/master/lib/src/main/java/com/auth0/jwt/ClockImpl.java.
auth0/src/main/java/com/auth0/android/authentication/storage/SecureCredentialsManager.java
Show resolved
Hide resolved
* @see com.auth0.android.authentication.storage.SecureCredentialsManager | ||
* @see com.auth0.android.authentication.storage.CredentialsManager | ||
*/ | ||
public interface Clock { |
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 what the developers would implement
when(storage.retrieveLong("com.auth0.cache_expires_at")).thenReturn(expirationTime); | ||
when(storage.retrieveString("com.auth0.refresh_token")).thenReturn(null); | ||
when(storage.retrieveString("com.auth0.access_token")).thenReturn("accessToken"); | ||
assertThat(manager.hasValidCredentials(), is(false)); |
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.
these tests ensure first that the credentials were deemed expired (due to the date) and when the clock instance is swapped, they get fixed
Changes
This change adds support for passing a custom implementation of the Clock used for verifying the expiration of the credentials stored in the any of the two credentials manager implementations.
References
Testing
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