-
Notifications
You must be signed in to change notification settings - Fork 149
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
Add an encrypted version of the CredentialsManager #115
Conversation
7d19726
to
1fb3f34
Compare
README.md
Outdated
|
||
## Credentials Manager | ||
|
||
This library ships with two additional classes that help you store and retrieve the `Credentials` received in authentication calls. Depending on the minimum API level that your application is targeting you'd like to use a different 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.
you store and retrieve the Credentials
received in authentication calls. ->
you manage the Credentials
received during authentication.
README.md
Outdated
1. **Instantiate the manager** | ||
You'll need an `AuthenticationAPIClient` instance used to renew the credentials when they expire and a `Storage`. The Storage implementation is up to you. We provide a `SharedPreferencesStorage` that uses `SharedPreferences` to create a file in the application's directory with Context.MODE_PRIVATE mode. This implementation is thread safe and can either be obtained through a Singleton like method or be created every time it's needed. | ||
1. **Instantiate the manager:** | ||
You'll need an `AuthenticationAPIClient` instance used to renew the credentials when they expire and a `Storage`. The Storage implementation is up to you. We provide a `SharedPreferencesStorage` that uses `SharedPreferences` to create a file in the application's directory with **Context.MODE_PRIVATE** mode. This implementation is thread safe and can either be obtained through a Singleton method or be created every time it's needed. |
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'll need an AuthenticationAPIClient
instance used to renew the credentials when they expire and a Storage
instance. The Storage implementation is up to you.
This implementation is thread safe and can either be obtained through a Singleton method or be created every time it's needed. ->
This implementation is thread safe and can either be obtained through a shared method or on demand.
README.md
Outdated
|
||
### Encryption enforced (Min API 21) | ||
|
||
The enhanced version contains the same methods as the _Basic_ manager but encrypts the data before storing it, and in those devices where a Secure LockScreen has been configured it can require the user authentication before letting them obtain the stored credentials. The class is called `CryptoManager`. |
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.
The enhanced version contains the same methods as the Basic manager but encrypts the data before storing it, and in ->
This version expands the minimum version and adds encryption to the data storage. In
README.md
Outdated
|
||
|
||
#### Usage | ||
The usage is the same as in the _Basic_ version. What changes is the way you instantiate the manager as it now requires a valid `Context`. |
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.
Change this, don't like calling them basic etc don't need to say it.
In this version the manager requires a valid `context as shown below:
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.
Few language changes to consider.
b55a333
to
c21a265
Compare
ad2df66
to
aad51ae
Compare
Modify your codecov.yml to allow for a little variance in overall and patch. For me -0.26% is acceptable so is 80% coverage in patch as sometimes it's just not possible. See https://github.com/auth0/Auth0.swift/blob/master/codecov.yml |
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.
Update your codecov to pass.
19c6238
to
ef3ebf9
Compare
This PR adds a new class, similar but separate to the existing
CredentialsManager
one, that encrypts the data before persisting it and can also request the user authentication before accessing the stored credentials. This authentication behavior is disabled by default. By using the@RequiresAPI
annotation I can make the compiler show an error if the developer is using this class on lower APIs. It requires minimum API 21 because that's when the "Confirm credentials" intent is first available.Security discussion
CryptoUtil
class.Pending stuff
CryptoUtil
tests. White-box tests using PowerMock and Mockito. Not many alternatives here... The coverage report will show "zero coverage" because of Jacoco lack of support for the PowerMock test runner.CryptoManager
tests. Including the "requires authentication" flow.CryptoManager
to something cooler, like "EnhancedCredentialsManager". What do you think?