Skip to content
This repository was archived by the owner on Jan 21, 2022. It is now read-only.

Storing the code verifier using shared preference to make it persistent #390

Open
wants to merge 15 commits into
base: MAS-2.1.00
Choose a base branch
from
Open
Original file line number Diff line number Diff line change
Expand Up @@ -8,46 +8,51 @@

package com.ca.mas.core.oauth;

import com.ca.mas.core.storage.sharedstorage.SharedPreferencesUtil;

/**
* Temporary cache to store code verifier
* Shared preference implementation to store code verifier
*/
public class CodeVerifierCache {

private static CodeVerifierCache instance = new CodeVerifierCache();

private String state;
private String codeVerifier;
private SharedPreferencesUtil prefUtil = null;
private static final String DEFAULT_STATE = "##default-state##";

private CodeVerifierCache() {
prefUtil = new SharedPreferencesUtil("codeverifier");
}

public static CodeVerifierCache getInstance() {
return instance;
}

public void store(String state, String codeVerifier) {
this.state = state;
this.codeVerifier = codeVerifier;
prefUtil.save(DEFAULT_STATE, codeVerifier);
if (state != null) {
prefUtil.save(state, codeVerifier);
}
}

public String take(String state) {
if (this.state == null && state != null
|| this.state != null && !this.state.equals(state)) {
//Workaround for pre MAG 3.3, Defect reference DE256594
public String take() {
String cv = prefUtil.getString(DEFAULT_STATE);
if (cv == null) {
throw new IllegalStateException("OAuth State Mismatch");
}
String cv = this.codeVerifier;
this.state = null;
this.codeVerifier = null;
prefUtil.delete(DEFAULT_STATE);
return cv;
}

//Workaround for pre MAG 3.3, Defect reference DE256594
public String take() {
String cv = this.codeVerifier;
this.state = null;
this.codeVerifier = null;
public String take(String state) {
if (state == null) {
return take();
}
String cv = prefUtil.getString(state);
if (cv == null) {
throw new IllegalStateException("OAuth State Mismatch");
}
prefUtil.delete(state);
return cv;
}


}
66 changes: 43 additions & 23 deletions mas-foundation/src/main/java/com/ca/mas/core/storage/sharedstorage/AccountManagerUtil.java
100644 → 100755
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.logging.Logger;

import static com.ca.mas.foundation.MAS.TAG;
import static com.ca.mas.foundation.MAS.getContext;
Expand All @@ -38,40 +39,59 @@ public class AccountManagerUtil implements StorageActions {
public AccountManagerUtil(Context context, String accountName, boolean sharedStorage){

// Gets the account type from the manifest
String accountType = getAccountType(context);
final String accountType = getAccountType(context);
final StringBuilder messageBuilder = new StringBuilder();

if (accountType == null || accountType.isEmpty()) {
throw new IllegalArgumentException(MASFoundationStrings.SHARED_STORAGE_NULL_ACCOUNT_TYPE);
}

if (accountName == null) {
throw new IllegalArgumentException(MASFoundationStrings.SHARED_STORAGE_NULL_ACCOUNT_NAME);
}

shared = sharedStorage;

try {
SharedStorageIdentifier identifier = new SharedStorageIdentifier();

mAccountManager = AccountManager.get(MAS.getContext());
//Attempt to retrieve the account
Account[] accounts = mAccountManager.getAccountsByType(accountType);
for (Account account : accounts) {
if (accountName.equals(account.name)) {
String password = mAccountManager.getPassword(account);
String savedPassword = identifier.toString();
if (password.equals(savedPassword)) {
mAccount = account;
}else {
// - case migration from old AccountManagerStoreDataSource
mAccount = null;
identifier = new SharedStorageIdentifier();
synchronized (mutex) {

Choose a reason for hiding this comment

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

usually, I use synchronized first and then try-catch block.

Rule of thump: Keep the try-catch constructs as close to the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Whats wrong in synchronising entire constructor ?

SharedStorageIdentifier identifier = new SharedStorageIdentifier();

mAccountManager = AccountManager.get(MAS.getContext());

//Attempt to retrieve the account
Account[] accounts = mAccountManager.getAccountsByType(accountType);
messageBuilder.append(" existing accounts (" + accountType + ")=" + accounts.length);

Choose a reason for hiding this comment

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

I assume use of message builder is an instrumentation purpose for detailing the logs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These changes are as part of another PR. I will take these and update the other PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)) {

Choose a reason for hiding this comment

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

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.

String password = mAccountManager.getPassword(account);
String savedPassword = identifier.toString();
if (password != null && password.equals(savedPassword)) {
mAccount = account;
}else {
// - case migration from old AccountManagerStoreDataSource
mAccount = null;
messageBuilder.append(" password is null or password not equals to saved password:");
identifier = new SharedStorageIdentifier();
}
}
}
}

//Create the account if it wasn't retrieved,
if (mAccount == null) {
mAccount = new Account(accountName, accountType);
mAccountManager.addAccountExplicitly(mAccount, identifier.toString(), null);
//Create the account if it wasn't retrieved,
if (mAccount == null) {
messageBuilder.append(" account identifier when mAccount is null:" +identifier);
messageBuilder.append(" attempt to create an account explicitly name=" + accountName + ", accountType=" + accountType);
mAccount = new Account(accountName, accountType);
boolean accountCreated = mAccountManager.addAccountExplicitly(mAccount, identifier.toString(), null);
messageBuilder.append("created account status=" + accountCreated);
}
}
Log.e(TAG, "Retrieved account details name="+mAccount.name+
" type=" +mAccount.type+" hashcode=" + mAccount.hashCode());
} catch (Exception e) {
throw new MASSharedStorageException(e.getMessage(), e);
Log.e(TAG, "Failed to retrieve account, " + e.getMessage() + " - " + messageBuilder.toString());
throw new MASSharedStorageException(e.getMessage() +" - " + messageBuilder.toString(), e);
}
}

Expand Down Expand Up @@ -223,4 +243,4 @@ private String proxyKey(String key){

return retVal;
}
}
}