Skip to content

Commit

Permalink
Merge branch 'dev' into jinjia/apiEvents
Browse files Browse the repository at this point in the history
* dev:
  Handle the app installation link for browser flow (#611)
  Correcting javadoc (#610)
  Fix the back-compat of hello() (#606)
  Closes #607, #604 (#608)
  Adds implementation for OpenIdConfig (#605)
  Add PCA initialization error strings (#598)
  Change log update
  Do not load access tokens from foci cache fallback logic (#599)
  Update to 0.0.16 (#596)
  Add clearBrokerSecretKeys() (#595)
  Fix for foci lookups relative to migration (#594)
  Change telemetry type from 'session' to 'event' (#592)
  • Loading branch information
heidijinxujia committed Aug 29, 2019
2 parents e150971 + 607c345 commit 9cb168d
Show file tree
Hide file tree
Showing 20 changed files with 998 additions and 136 deletions.
5 changes: 5 additions & 0 deletions changelog.txt
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
Version 0.0.17
-------------
- BugFix : Fix for foci lookup issue relative to migration
- Add clearBrokerSecretKeys() to AuthenticationSettings.

Version 0.0.15
-------------
- Bug Fix : Adding null safety check to avoid crash on EmbeddedWebViewStrategy
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ public void testRetrieveFrt() throws ClientException {

final ICacheRecord familyCacheRecord = mOauth2TokenCache.loadByFamilyId(
null,
TARGET,
frtTestBundle.mGeneratedAccount
);

Expand All @@ -156,30 +157,33 @@ public void testRetrieveFrt() throws ClientException {

final ICacheRecord familyCacheRecordWithClientId = mOauth2TokenCache.loadByFamilyId(
CLIENT_ID,
TARGET,
frtTestBundle.mGeneratedAccount
);

assertNotNull(familyCacheRecordWithClientId);
assertNotNull(familyCacheRecordWithClientId.getAccount());
assertNotNull(familyCacheRecordWithClientId.getRefreshToken());
assertNull(familyCacheRecordWithClientId.getIdToken());
assertNull(familyCacheRecordWithClientId.getAccessToken());
assertNotNull(familyCacheRecordWithClientId.getIdToken());
assertNotNull(familyCacheRecordWithClientId.getAccessToken());

final ICacheRecord familyCacheRecordWithClientIdButNonMatchingTarget =
mOauth2TokenCache.loadByFamilyId(
CLIENT_ID,
TARGET,
frtTestBundle.mGeneratedAccount
);

assertNotNull(familyCacheRecordWithClientIdButNonMatchingTarget);
assertNotNull(familyCacheRecordWithClientIdButNonMatchingTarget.getAccount());
assertNotNull(familyCacheRecordWithClientIdButNonMatchingTarget.getRefreshToken());
assertNull(familyCacheRecordWithClientIdButNonMatchingTarget.getIdToken());
assertNull(familyCacheRecordWithClientIdButNonMatchingTarget.getAccessToken());
assertNotNull(familyCacheRecordWithClientIdButNonMatchingTarget.getIdToken());
assertNotNull(familyCacheRecordWithClientIdButNonMatchingTarget.getAccessToken());

final ICacheRecord wrongClientIdResult =
mOauth2TokenCache.loadByFamilyId(
"12345",
TARGET,
frtTestBundle.mGeneratedAccount
);

Expand Down Expand Up @@ -314,13 +318,14 @@ public void testOnlyOneFrtMayExistAcrossClientsForAccount() throws ClientExcepti
// Test only one FRT exists and it is the second one saved...
final ICacheRecord cacheRecord = mOauth2TokenCache.loadByFamilyId(
CLIENT_ID,
TARGET,
frtTestBundle2.mGeneratedAccount
);

assertNotNull(cacheRecord);
assertNotNull(cacheRecord.getRefreshToken());
assertNull(cacheRecord.getAccessToken());
assertNull(cacheRecord.getIdToken());
assertNotNull(cacheRecord.getAccessToken());
assertNotNull(cacheRecord.getIdToken());
assertEquals(
CLIENT_ID + "2",
cacheRecord.getRefreshToken().getClientId()
Expand All @@ -329,13 +334,14 @@ public void testOnlyOneFrtMayExistAcrossClientsForAccount() throws ClientExcepti
// Check querying for the FRT in the second app yields the same FRT
final ICacheRecord cacheRecord2 = mOauth2TokenCache.loadByFamilyId(
CLIENT_ID + "2",
TARGET,
frtTestBundle2.mGeneratedAccount
);

assertNotNull(cacheRecord2);
assertNotNull(cacheRecord2.getRefreshToken());
assertNull(cacheRecord2.getAccessToken());
assertNull(cacheRecord2.getIdToken());
assertNotNull(cacheRecord2.getAccessToken());
assertNotNull(cacheRecord2.getIdToken());
assertEquals(
CLIENT_ID + "2",
cacheRecord2.getRefreshToken().getClientId()
Expand All @@ -353,6 +359,7 @@ public void testOnlyOneFrtMayExistAcrossClientsForAccount() throws ClientExcepti

final ICacheRecord cacheRecord3 = mOauth2TokenCache.loadByFamilyId(
CLIENT_ID + "2",
TARGET,
randomAcct
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1262,13 +1262,21 @@ public static final class MediaType {
public static final class TelemetryEvents {
public static final String DECRYPTION_ERROR = "decryption_error_v2";

public static final String KEYCHAIN_WRITE = "keychain_write_v2";
public static final String KEYCHAIN_WRITE_START = "keychain_write_v2_start";

public static final String KEYCHAIN_READ = "keychain_read_v2";
public static final String KEYCHAIN_WRITE_END = "keychain_write_v2_end";

public static final String KEY_RETRIEVAL = "key_retrieval_v2";
public static final String KEYCHAIN_READ_START = "keychain_read_v2_start";

public static final String KEY_DISTRIBUTION = "key_distribution_v2";
public static final String KEYCHAIN_READ_END = "keychain_read_v2_end";

public static final String KEY_RETRIEVAL_START = "key_retrieval_v2_start";

public static final String KEY_RETRIEVAL_END = "key_retrieval_v2_end";

public static final String KEY_DISTRIBUTION_START = "key_distribution_v2_start";

public static final String KEY_DISTRIBUTION_END = "key_distribution_v2_end";

public static final String KEY_CREATED = "key_created_v2";
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,14 @@ public void setBrokerSecretKeys(final Map<String, byte[]> secretKeys) {
}
}

/**
* Clear broker secret keys.
* Introduced as a temporary workaround to make sure Broker code clears up Broker keys in common before it's used by ADAL/MSAL.
* */
public void clearBrokerSecretKeys(){
mBrokerSecretKeys.clear();
}

/**
* For test cases only.
* */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,4 @@
* */
public interface IWpjTelemetryCallback {
void logEvent(Context context, final String operation, final Boolean isFailed, final String reason);
void logSessionStart(Context context, final String operation);
void logSessionEnd(Context context, final String operation, final Boolean isFailed, final String reason);
}
Original file line number Diff line number Diff line change
Expand Up @@ -638,7 +638,7 @@ private synchronized KeyPair generateKeyPairFromAndroidKeyStore()
final String methodName = ":generateKeyPairFromAndroidKeyStore";

try {
logFlowStart(methodName, AuthenticationConstants.TelemetryEvents.KEYCHAIN_WRITE);
logFlowStart(methodName, AuthenticationConstants.TelemetryEvents.KEYCHAIN_WRITE_START);

final KeyStore keyStore = KeyStore.getInstance(ANDROID_KEY_STORE);
keyStore.load(null);
Expand All @@ -656,10 +656,10 @@ private synchronized KeyPair generateKeyPairFromAndroidKeyStore()
generator.initialize(getKeyPairGeneratorSpec(mContext, start.getTime(), end.getTime()));

final KeyPair keyPair = generator.generateKeyPair();
logFlowSuccess(methodName, AuthenticationConstants.TelemetryEvents.KEYCHAIN_WRITE, "");
logFlowSuccess(methodName, AuthenticationConstants.TelemetryEvents.KEYCHAIN_WRITE_END, "");
return keyPair;
} catch (final GeneralSecurityException | IOException e) {
logFlowError(methodName, AuthenticationConstants.TelemetryEvents.KEYCHAIN_WRITE, e.toString(), e);
logFlowError(methodName, AuthenticationConstants.TelemetryEvents.KEYCHAIN_WRITE_END, e.toString(), e);
throw e;
} catch (final IllegalStateException e) {
// There is an issue with AndroidKeyStore when attempting to generate keypair
Expand All @@ -671,7 +671,7 @@ private synchronized KeyPair generateKeyPairFromAndroidKeyStore()
// The thrown exception in this case is:
// java.lang.IllegalStateException: could not generate key in keystore
// To avoid app crashing, re-throw as checked exception
logFlowError(methodName, AuthenticationConstants.TelemetryEvents.KEYCHAIN_WRITE, e.toString(), e);
logFlowError(methodName, AuthenticationConstants.TelemetryEvents.KEYCHAIN_WRITE_END, e.toString(), e);
throw new KeyStoreException(e);
}
}
Expand All @@ -688,7 +688,7 @@ private synchronized KeyPair readKeyPair()
Logger.verbose(TAG + methodName, "Reading Key entry");

try {
logFlowStart(methodName, AuthenticationConstants.TelemetryEvents.KEYCHAIN_READ);
logFlowStart(methodName, AuthenticationConstants.TelemetryEvents.KEYCHAIN_READ_START);

final KeyStore keyStore = KeyStore.getInstance(ANDROID_KEY_STORE);
keyStore.load(null);
Expand All @@ -697,16 +697,16 @@ private synchronized KeyPair readKeyPair()
final Key privateKey = keyStore.getKey(KEY_STORE_CERT_ALIAS, null);

if (cert == null || privateKey == null) {
logFlowSuccess(methodName, AuthenticationConstants.TelemetryEvents.KEYCHAIN_READ, "KeyStore is empty.");
logFlowSuccess(methodName, AuthenticationConstants.TelemetryEvents.KEYCHAIN_READ_END, "KeyStore is empty.");
Logger.verbose(TAG + methodName, "Key entry doesn't exist.");
return null;
}

final KeyPair keyPair = new KeyPair(cert.getPublicKey(), (PrivateKey) privateKey);
logFlowSuccess(methodName, AuthenticationConstants.TelemetryEvents.KEYCHAIN_READ, "KeyStore KeyPair is loaded.");
logFlowSuccess(methodName, AuthenticationConstants.TelemetryEvents.KEYCHAIN_READ_END, "KeyStore KeyPair is loaded.");
return keyPair;
} catch (final GeneralSecurityException | IOException e) {
logFlowError(methodName, AuthenticationConstants.TelemetryEvents.KEYCHAIN_READ, e.toString(), e);
logFlowError(methodName, AuthenticationConstants.TelemetryEvents.KEYCHAIN_READ_END, e.toString(), e);
throw e;
} catch (final RuntimeException e) {
// There is an issue in android keystore that resets keystore
Expand All @@ -715,7 +715,7 @@ private synchronized KeyPair readKeyPair()
// in this case getEntry throws
// java.lang.RuntimeException: error:0D07207B:asn1 encoding routines:ASN1_get_object:header too long
// handle it as regular KeyStoreException
logFlowError(methodName, AuthenticationConstants.TelemetryEvents.KEYCHAIN_READ, e.toString(), e);
logFlowError(methodName, AuthenticationConstants.TelemetryEvents.KEYCHAIN_READ_END, e.toString(), e);
throw new KeyStoreException(e);
}
}
Expand Down Expand Up @@ -939,7 +939,7 @@ private void logFlowStart(@NonNull final String methodName,
@NonNull final String operationName) {
Logger.verbose(TAG + methodName, operationName + " started.");
if (mTelemetryCallback != null) {
mTelemetryCallback.logSessionStart(mContext, operationName);
mTelemetryCallback.logEvent(mContext, operationName, false, "");
}
}

Expand All @@ -948,7 +948,7 @@ private void logFlowSuccess(@NonNull final String methodName,
@NonNull final String reason) {
Logger.verbose(TAG + methodName, operationName + " successfully finished: " + reason);
if (mTelemetryCallback != null) {
mTelemetryCallback.logSessionEnd(mContext, operationName, false, reason);
mTelemetryCallback.logEvent(mContext, operationName, false, reason);
}
}

Expand All @@ -958,7 +958,7 @@ private void logFlowError(@NonNull final String methodName,
@Nullable Exception e) {
Logger.error(TAG + methodName, operationName + " failed: " + reason, e);
if (mTelemetryCallback != null) {
mTelemetryCallback.logSessionEnd(mContext, operationName, true, reason);
mTelemetryCallback.logEvent(mContext, operationName, true, reason);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -350,4 +350,34 @@ private ErrorStrings() {
*/
public static final String KEY_NOT_FOUND = "key_not_found";

/**
* AccountMode in configuration is set to multiple. However, the device is marked as shared (which requires single account mode).
*/
public static final String MULTIPLE_ACCOUNT_PCA_INIT_FAIL_ON_SHARED_DEVICE_ERROR_CODE = "multiple_account_pca_init_fail_on_shared_device";
public static final String MULTIPLE_ACCOUNT_PCA_INIT_FAIL_ON_SHARED_DEVICE_ERROR_MESSAGE = "AccountMode in configuration is set to multiple. However, the device is marked as shared (which requires single account mode).";

/**
* Multiple account PublicClientApplication could not be created for unknown reasons
*/
public static final String MULTIPLE_ACCOUNT_PCA_INIT_FAIL_UNKNOWN_REASON_ERROR_CODE = "multiple_account_pca_init_fail_unknown_reason";
public static final String MULTIPLE_ACCOUNT_PCA_INIT_FAIL_UNKNOWN_REASON_ERROR_MESSAGE = "Multiple account PublicClientApplication could not be created for unknown reasons";

/**
* AccountMode in configuration is not set to multiple. Cannot initialize multiple account PublicClientApplication.
*/
public static final String MULTIPLE_ACCOUNT_PCA_INIT_FAIL_ACCOUNT_MODE_ERROR_CODE = "multiple_account_pca_init_fail_account_mode";
public static final String MULTIPLE_ACCOUNT_PCA_INIT_FAIL_ACCOUNT_MODE_ERROR_MESSAGE = "AccountMode in configuration is not set to multiple. Cannot initialize multiple account PublicClientApplication.";

/**
* AccountMode in configuration is not set to single. Cannot initialize single account PublicClientApplication.
*/
public static final String SINGLE_ACCOUNT_PCA_INIT_FAIL_ACCOUNT_MODE_ERROR_CODE = "single_account_pca_init_fail_account_mode";
public static final String SINGLE_ACCOUNT_PCA_INIT_FAIL_ACCOUNT_MODE_ERROR_MESSAGE = "AccountMode in configuration is not set to single. Cannot initialize single account PublicClientApplication.";

/**
* A single account public client application could not be created for unknown reasons.
*/
public static final String SINGLE_ACCOUNT_PCA_INIT_FAIL_UNKNOWN_REASON_ERROR_CODE = "single_account_pca_init_fail_unknown_reason";
public static final String SINGLE_ACCOUNT_PCA_INIT_FAIL_UNKNOWN_REASON_ERROR_MESSAGE = "A single account public client application could not be created for unknown reasons.";

}
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@

public class ServiceException extends BaseException {

public static final String OPENID_PROVIDER_CONFIGURATION_FAILED_TO_LOAD =
"failed_to_load_openid_configuration";

/**
* This request is missing a required parameter, includes an invalid parameter, includes a
* parameter more than
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ private class SerializedNames {
static final String CACHED_AT = "cached_at";
static final String REFRESH_TOKEN_AGE = "refresh_token_age";
static final String SUCCESS = "success";
static final String NEGOTIATED_BROKER_PROTOCOL_VERSION = "negotiated.broker.protocol.version.name";

// Error constants
/**
Expand Down Expand Up @@ -85,6 +84,8 @@ private class SerializedNames {
static final String CLI_TELEM_SUB_ERROR_CODE = "cli_telem_suberror_code";
}

private static final long serialVersionUID = 8606631820514878489L;

// Success parameters
/**
* Access token from the response
Expand Down Expand Up @@ -221,13 +222,6 @@ private class SerializedNames {
@SerializedName(SerializedNames.REFRESH_TOKEN_AGE)
private String mRefreshTokenAge;

/**
* Negotiated broker protocol version between broker client and broker service.
*/
@Nullable
@SerializedName(SerializedNames.NEGOTIATED_BROKER_PROTOCOL_VERSION)
private String mNegotiatedBrokerProtocolVersion;

/**
* Boolean to indicate if the request succeeded without exceptions.
*/
Expand Down Expand Up @@ -328,7 +322,6 @@ private BrokerResult(@NonNull final Builder builder) {
mRefreshTokenAge = builder.mRefreshTokenAge;
mSuccess = builder.mSuccess;
mTenantProfileData = builder.mTenantProfileData;
mNegotiatedBrokerProtocolVersion = builder.mNegotiatedBrokerProtocolVersion;

mErrorCode = builder.mErrorCode;
mErrorMessage = builder.mErrorMessage;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -198,8 +198,8 @@ protected List<Credential> getCredentialsFilteredByInternal(@Nullable String hom
* @return True, if the credentialTarget contains all of the targets (scopes) declared by
* targetToMatch. False otherwise.
*/
private static boolean targetsIntersect(@NonNull final String targetToMatch,
@NonNull final String credentialTarget) {
static boolean targetsIntersect(@NonNull final String targetToMatch,
@NonNull final String credentialTarget) {
// The credentialTarget must contain all of the scopes in the targetToMatch
// It may contain more, but it must contain minimally those
// Matching is case-insensitive
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -451,20 +451,32 @@ public ICacheRecord load(@NonNull final String clientId,
"Performing lookup in app-specific cache."
);

final BrokerApplicationMetadata appMetadata = mApplicationMetadataCache.getMetadata(
clientId,
account.getEnvironment(),
mCallingProcessUid
);

boolean isKnownFoci = false;

if (null != appMetadata) {
isKnownFoci = null != appMetadata.getFoci();
}

final OAuth2TokenCache targetCache = getTokenCacheForClient(
clientId,
account.getEnvironment(),
mCallingProcessUid
);

final boolean shouldUseFociCache = null == targetCache;
final ICacheRecord resultRecord;
final boolean shouldUseFociCache = null == targetCache || isKnownFoci;

ICacheRecord resultRecord;

if (shouldUseFociCache) {
// We do not have a cache for this app or it is not yet known to be a member of the family
// use the foci cache....
resultRecord = mFociCache.loadByFamilyId(
clientId,
target,
account
);
} else {
Expand Down Expand Up @@ -537,6 +549,7 @@ public List<ICacheRecord> loadWithAggregatedAccountData(@NonNull final String cl
resultRecords.add(
mFociCache.loadByFamilyId(
clientId,
target,
account
)
);
Expand Down
Loading

0 comments on commit 9cb168d

Please sign in to comment.