Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: implemented biometrics authentication for SecureCredentialsManager using androidx.biometrics package #745

Merged
merged 31 commits into from
Aug 1, 2024
Merged

feat: implemented biometrics authentication for SecureCredentialsManager using androidx.biometrics package #745

merged 31 commits into from
Aug 1, 2024
+2,579 −970

Conversation

desusai7
Copy link
Contributor

@desusai7 desusai7 commented Jun 10, 2024 •
edited
Loading

Migrated to use BioMetricManager for performing Authentication before accessing credentials via SecureCredentialsManager

Breaking Changes:

  • Removed the requireAuthentication API which used to be a pre-requisite previously for performing authentication while retrieving Credentials as it is no longer needed.
  • Constructor of Auth0 class is made private, and now you will have to use Auth0.getInstance(clientId, domain) to get an instance of Auth0, it would check if the instance with the given configuration exists already, if yes returns it, if not would create one and returns it.
  • Added multiple overloads of getCredentials() & awaitCredentials() to the BaseCredentialsManager interface, now all the implementations of this interface would have to implemented these new methods using override as previously they used to define these methods directly in their implementations.
  • await function of Request interface has been made abstract now and all the implementations of the interface should implement the method.
  • Credentials class has been converted to a data class now this class can longer be extended and also the currentTimeInMillis property of Credentials class has been removed.

Other changes:

  • We've added a dependency on androidx.biometrics package to use the BiometricsManager for authentication purposes.
  • Updated the constructor of SecureCredentialsManager to accept fragmentActivity & authenticationOptions which are utilised while performing authentication using BiometricPrompt before retrieving credentials. It's no longer needed to call requireAuthentication before retrieving credentials.
  • Added class LocalAuthenticationManager which consolidates all the logic for interacting with the BiometricsManager
  • Added class LocalAuthenticationOptions which helps to configure the behaviour of the BiometricsManager like title, description displayed within in the biometric prompt and the level of authentication required.
  • Updated CredentialsManagerException to contain enum Code describing the reason for exception and allowing users to check all the cases of CredentialsManagerException and handle accordingly

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

@desusai7 desusai7 self-assigned this Jun 10, 2024
github-advanced-security[bot]
github-advanced-security bot found potential problems Jul 8, 2024
View reviewed changes
auth0/src/main/java/com/auth0/android/authentication/storage/LocalAuthenticationManager.kt
Comment on lines +84 to +87
override fun onAuthenticationSucceeded(result: BiometricPrompt.AuthenticationResult) {
super.onAuthenticationSucceeded(result)
resultCallback.onSuccess(true)
}

Check warning

Code scanning / CodeQL

Insecure local authentication Medium

This authentication callback does not use its result for a cryptographic operation.
@desusai7 desusai7 marked this pull request as ready for review July 8, 2024 14:43
@desusai7 desusai7 requested a review from a team as a code owner July 8, 2024 14:43
@desusai7 desusai7 enabled auto-merge (squash) July 8, 2024 14:43
poovamraj
poovamraj reviewed Jul 17, 2024
View reviewed changes
Copy link
Contributor

@poovamraj poovamraj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please take not of all the breaking changes in each PR so that it can be collected together for the major release. We don't want to lose track of them

auth0/src/main/java/com/auth0/android/authentication/storage/BaseCredentialsManager.kt Outdated
) : BaseCredentialsManager(
authenticationClient, storage, jwtDecoder
) {
public abstract fun getCredentials(fragmentActivity: FragmentActivity, authenticationOptions: LocalAuthenticationOptions, callback: Callback<Credentials, CredentialsManagerException>)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If FragmentActivity is required in both method, can't we have this as a class level dependency, that way you can have same abstract method for both cred managers.

We just have to ensure there is no context leak by having fragment activity at a class level

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, updated the code accordingly, please take a look again.

auth0/src/main/java/com/auth0/android/authentication/storage/CredentialsManagerException.kt Show resolved Hide resolved
@desusai7 desusai7 requested a review from poovamraj July 19, 2024 15:49
igorwojda and others added 14 commits August 1, 2024 20:51
@igorwojda @desusai7
docs: Clarify Readme around targets (#744)
19e8fcd 
Signed-off-by: Sai Venkat Desu <[email protected]>
@dependabot @desusai7
Bump codecov/codecov-action from 4.4.1 to 4.5.0 (#746)
08851a8 
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Sai Venkat Desu <[email protected]>
@desusai7
chore: updated SecureCredentialsManager to accept fragment activity a…
710561c 
…t constructor instead of at each api level

Signed-off-by: Sai Venkat Desu <[email protected]>
@desusai7
chore: updated SecureCredentialsManager to return credentials without…
d8415f6 
… authenticaton as well, in cases where customers just want to encode and encrypt the credentials

Signed-off-by: Sai Venkat Desu <[email protected]>
@desusai7
chore: minor changes
66ab119 
Signed-off-by: Sai Venkat Desu <[email protected]>
@desusai7
chore: minor updates to the setter methods in LocalAuthenticationOpti…
8d6c33e 
…ons.Builder class

Signed-off-by: Sai Venkat Desu <[email protected]>
@desusai7
feat: added synchronization to support secure credentials manager fro…
46ad75b 
…m multiple threads

Signed-off-by: Sai Venkat Desu <[email protected]>
@desusai7
test: added unit tests for synchronization of activity across multipl…
c8b7e11 
…e instances of SecureCredentialsManager

Signed-off-by: Sai Venkat Desu <[email protected]>
@desusai7
chore: minor refactoring
9ee0052 
Signed-off-by: Sai Venkat Desu <[email protected]>
@desusai7
BREAKING CHANGE: incorporated changes planned for v3 (#751)
1f3b7b3 
Signed-off-by: Sai Venkat Desu <[email protected]>
@desusai7
chore: minor changes
7356905 
Signed-off-by: Sai Venkat Desu <[email protected]>
@desusai7
docs: updated examples and added migration guide
9f6a1a6 
Signed-off-by: Sai Venkat Desu <[email protected]>
@desusai7
chore: updated snyk ignore because of issues with dokka
856cebe 
Signed-off-by: Sai Venkat Desu <[email protected]>
@desusai7
chore: overridden hashcode method of CredentialsManagerException
46d024b 
Signed-off-by: Sai Venkat Desu <[email protected]>
@desusai7
Copy link
Contributor Author

desusai7 commented Aug 1, 2024

Hi, we are an auth0 customer building for Android, and we are interested in seeing this ship. Is there any update?

@sgammon, we will be releasing it shortly.

@desusai7 desusai7 enabled auto-merge (squash) August 1, 2024 15:25
@desusai7 desusai7 merged commit beaabdf into main Aug 1, 2024
9 checks passed
@desusai7 desusai7 deleted the feat/biometric branch August 1, 2024 15:32
@desusai7 desusai7 mentioned this pull request Aug 1, 2024
Merged
@sgammon
Copy link

sgammon commented Aug 2, 2024

@desusai7 Awesome! Thank you so much, we're very excited! 😄

@desusai7
Copy link
Contributor Author

desusai7 commented Aug 5, 2024

@sgammon, we've released it as part of version 3.0.0-beta.0.

@sgammon
Copy link

sgammon commented Aug 6, 2024

We are already using it! I haven't figured out how to use the new biometrics yet, but that is next on the list. I plan to check samples, test suites, etc., but if there is already documentation, please let me know 😄

@desusai7
Copy link
Contributor Author

desusai7 commented Aug 6, 2024

We are already using it! I haven't figured out how to use the new biometrics yet, but that is next on the list. I plan to check samples, test suites, etc., but if there is already documentation, please let me know 😄

documentation exists for it here

@pmathew92 pmathew92 mentioned this pull request Oct 30, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@poovamraj poovamraj poovamraj approved these changes

Assignees

@desusai7 desusai7

Labels
CH: Breaking Change F3: in progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@desusai7 @sgammon @poovamraj @igorwojda