Storing the code verifier using shared preference to make it persistent #390
Conversation
| } | ||
|
|
||
| public static CodeVerifierCache getInstance() { | ||
| return instance; | ||
| } | ||
|
|
||
| public void store(String state, String codeVerifier) { | ||
| this.state = state; | ||
| this.codeVerifier = codeVerifier; | ||
| prefUtil.save("state", state); |
There was a problem hiding this comment.
I think, there's a one-one relation between state and codeVerifier. You should be saving the codeVerifier using the state as key, Please check it out.
There was a problem hiding this comment.
@graju256 I also thought to store code verifier using state as key. But we have two overloading methods of take. That is the reason I implemented this way. Any other thoughts here, please let me know.
public class MASAuthCredentialsAuthorizationCode implements MASAuthCredentials {
private String code;
private String state;
private String codeVerifier;
public MASAuthCredentialsAuthorizationCode(String code, String state) {
this.code = code;
this.state = state;
if (ConfigurationManager.getInstance().isPKCEEnabled()) {
if (state != null && !state.isEmpty()) {
this.codeVerifier = CodeVerifierCache.getInstance().take(state);
} else {
this.codeVerifier = CodeVerifierCache.getInstance().take();
}
}
}
There was a problem hiding this comment.
Still, not convincing to me.
Method take with no arguments is simply returning what is there with cache without checking the state.
Whereas, method with argument is strictly checking the state match. Then only it is returning the code-verifier.
Hence, you must use state as key to store the code-verifier.
For backward compatibilty, you can store the latest code-verifier using "__system-default" key
| String cv = this.codeVerifier; | ||
| this.state = null; | ||
| this.codeVerifier = null; | ||
| String cv = prefUtil.getString("code"); |
There was a problem hiding this comment.
Having the one-one relation between state and codeverifier, you should think of retrieving the codeverifier using state.
And, what is the expectation of take method. Is it both retrieve and delete functionality?
There was a problem hiding this comment.
Yes, take method is used for both retrieve and delete. As code verifier will be generated while checking the AppAuth.
| // - case migration from old AccountManagerStoreDataSource | ||
| mAccount = null; | ||
| identifier = new SharedStorageIdentifier(); | ||
| synchronized (mutex) { |
There was a problem hiding this comment.
usually, I use synchronized first and then try-catch block.
Rule of thump: Keep the try-catch constructs as close to the code.
There was a problem hiding this comment.
Whats wrong in synchronising entire constructor ?
|
|
||
| //Attempt to retrieve the account | ||
| Account[] accounts = mAccountManager.getAccountsByType(accountType); | ||
| messageBuilder.append(" existing accounts (" + accountType + ")=" + accounts.length); |
There was a problem hiding this comment.
I assume use of message builder is an instrumentation purpose for detailing the logs.
There was a problem hiding this comment.
These changes are as part of another PR. I will take these and update the other PR.
There was a problem hiding this comment.
And yes, this is as part of the logging mechanism and is temporary. We will remove that as soon we get a solution of the other SecurityException issue.
| for (Account account : accounts) { | ||
| messageBuilder.append(" trying account:" + account.name); | ||
|
|
||
| if (accountName.equals(account.name)) { |
There was a problem hiding this comment.
I hope, we don't find duplicates with the same accountName. Hence, break the loop as soon as we encounter the account with the given name.
| this.state = state; | ||
| this.codeVerifier = codeVerifier; | ||
| prefUtil.save("state", state); | ||
| prefUtil.save("code", codeVerifier); |
There was a problem hiding this comment.
Is it a good idea to store code-verifier with state string as key ?
There was a problem hiding this comment.
Added the changes. state as key.
| private String state; | ||
| private String codeVerifier; | ||
| private SharedPreferencesUtil prefUtil = null; | ||
| private String mState = "##default-state##"; |
There was a problem hiding this comment.
can you make this static final;
private static final String DEFAULT_STATE = "##default-state##";
Issue:
Presently Code verifier is getting generated and stored in Memory cache. When mobile app which is built using SDK calls another external app for authentication, it goes to background. In some cases, it is getting killed by the OS. Due to this, code verifier, which is stored in memory cache is getting lost.
Fix:
SDK has a support for Shared Preference which is persistent local database for Android. Implemented the shared preference and storing the code verifier and the state. This will ensure that the values will be present although app is in background or getting killed by any chance.