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.

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

SSO State Migration + Logger refactor #436

Merged
merged 29 commits into from Oct 31, 2018

Conversation

Projects
None yet
2 participants
@iambmelt
Copy link
Member

commented Oct 24, 2018

Summary of changes:

API Changes:

  • PublicClientApplication#getAccounts() is now asynchronous, performs ADAL token cache update if 0 == getAccounts().size() && !hasMigrated
  • Introduces new class TokenMigrationUtility, general purpose migration utility, leveraging IShareSingleSignOnState
  • IShareSingleSignOnState now returns a boolean result indicated success/failure of SSO set-state

Under the hood:
* Adds new SharedPreferences file to track state migration: com.microsoft.identity.client.migration_status
* Does not delete any ADAL state after migration

Corner cases + expected behavior:

  • If upgrading from 1.15.+, migration is skipped as the MsalOAuth2TokenCache is already synced by ADALOAuth2TokenCache
  • Before attempting migration, the cache is consulted. If it contains >0 records, migration is skipped and it is marked as complete (so that it will not happen in the future if records are removed)
  • Version rollbacks are supported because ADAL cache state is not deleted by migration
    • Please note: Any Accounts or Credentials added to an application after a rollback will not be synced once hasMigrated is set to true.

Corollary PR:
AzureAD/microsoft-authentication-library-common-for-android#296

@iambmelt iambmelt self-assigned this Oct 24, 2018

@@ -307,6 +309,37 @@ public static String getSdkVersion() {
return BuildConfig.VERSION_NAME;
}

public void restoreAdalCache(final Context context) {

This comment has been minimized.

Copy link
@iambmelt

iambmelt Oct 24, 2018

Author Member

This piece of API is going to go away

// THE SOFTWARE.
package com.microsoft.identity.client;

interface TokenMigrationCallback {

This comment was marked as resolved.

Copy link
@iambmelt

iambmelt Oct 24, 2018

Author Member

Javadoc this

import java.util.List;
import java.util.Map;

public interface IMigrationAdapter<T extends BaseAccount, U extends RefreshToken> {

This comment was marked as resolved.

Copy link
@iambmelt

iambmelt Oct 24, 2018

Author Member

Javadoc this class

import java.util.List;
import java.util.Map;

public class AdalMigrationAdapter implements IMigrationAdapter<MicrosoftAccount, MicrosoftRefreshToken> {

This comment was marked as resolved.

Copy link
@iambmelt

iambmelt Oct 24, 2018

Author Member

Add comments to this class - code is complex

iambmelt added some commits Oct 24, 2018

@iambmelt iambmelt changed the title [WIP] SSO State Migration SSO State Migration Oct 29, 2018

@iambmelt iambmelt requested review from shoatman and amishra-dev Oct 29, 2018

@iambmelt iambmelt referenced this pull request Oct 29, 2018

Merged

SSO State Migration #296

iambmelt added some commits Oct 30, 2018

Bugfix: do not mark migration as complete if the
InstanceDiscoveryMetadata fails to load

@iambmelt iambmelt changed the title SSO State Migration SSO State Migration + Logger refactor Oct 30, 2018

@@ -0,0 +1,531 @@
// Copyright (c) Microsoft Corporation.

This comment was marked as resolved.

Copy link
@iambmelt

iambmelt Oct 30, 2018

Author Member

Move to common

@@ -0,0 +1,49 @@
// Copyright (c) Microsoft Corporation.

This comment was marked as resolved.

Copy link
@iambmelt

iambmelt Oct 30, 2018

Author Member

Move to common

import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;

public class TokenMigrationUtility<T extends BaseAccount, U extends RefreshToken> {

This comment was marked as resolved.

Copy link
@iambmelt

iambmelt Oct 30, 2018

Author Member

Move to common

@amishra-dev
Copy link
Contributor

left a comment

2 nits

@Override
public void onAccountsLoaded(List<IAccount> accountsToRemove) {
for (final IAccount accountToRemove : accountsToRemove) {
if (TextUtils.isEmpty(username) || accountToRemove.getUsername().equals(username.trim().toLowerCase())) {

This comment has been minimized.

Copy link
@amishra-dev

amishra-dev Oct 31, 2018

Contributor

how about using equalsIgnoreCase and removing the toLower, that way we get independent of the casing of acccountToRemove

IAccount requestAccount = null;

for (final IAccount account : accounts) {
if (account.getUsername().equals(requestOptions.getLoginHint().trim().toLowerCase())) {

This comment has been minimized.

Copy link
@amishra-dev

amishra-dev Oct 31, 2018

Contributor

same here

@iambmelt iambmelt merged commit 2b2384b into dev Oct 31, 2018

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla All CLA requirements met.
Details

@iambmelt iambmelt deleted the iambmelt/adal-cache-migration branch Oct 31, 2018

@iambmelt iambmelt referenced this pull request Nov 19, 2018

Merged

Resolves #460 #461

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.