Skip to content

Commit

Permalink
Fix NPE in SingleAccountPublicClientApplication.getPersistedCurrentAc…
Browse files Browse the repository at this point in the history
…count (#1933)

IcM:
https://portal.microsofticm.com/imp/v3/incidents/incident/435962618/summary

The issue is that MSAL is seeing the following crash due to the missing
localAccountId in the cached "persistedCurrentAccount" in Shared Device
Mode

```
2023-10-20T08:00:54.2270000  ERR_  com.microsoft.windowsintune.companyportal.CompanyPortalApplication  16377      2  Unhandled exception which will shut down the OMADM process in Thread[main,5,main]
  java.lang.NullPointerException: Attempt to invoke interface method 'java.lang.String java.lang.CharSequence.toString()' on a null object reference
    at java.lang.String.contains(String.java:2662)
    at com.microsoft.identity.client.AccountAdapter$GuestAccountFilter.filter(:63)
    at com.microsoft.identity.client.AccountAdapter.filterCacheRecords(:356)
    at com.microsoft.identity.client.AccountAdapter.adapt(:161)
    at com.microsoft.identity.client.SingleAccountPublicClientApplication.getAccountFromICacheRecordList(:609)
    at com.microsoft.identity.client.SingleAccountPublicClientApplication.getPersistedCurrentAccount(:573)
    at com.microsoft.identity.client.SingleAccountPublicClientApplication.access$000(:84)
    at com.microsoft.identity.client.SingleAccountPublicClientApplication$1$1.onTaskCompleted(:139)
    at com.microsoft.identity.client.SingleAccountPublicClientApplication$1$1.onTaskCompleted(:133)
    at com.microsoft.identity.common.java.controllers.CommandDispatcher.commandCallbackOnTaskCompleted(:649)
    at com.microsoft.identity.common.java.controllers.CommandDispatcher.access$1000(:99)
    at com.microsoft.identity.common.java.controllers.CommandDispatcher$4.run(:625)
    at android.os.Handler.handleCallback(Handler.java:942)
    at android.os.Handler.dispatchMessage(Handler.java:99)
    at android.os.Looper.loopOnce(Looper.java:201)
    at android.os.Looper.loop(Looper.java:288)
    at android.app.ActivityThread.main(ActivityThread.java:8046)
    at java.lang.reflect.Method.invoke(Native Method)
    at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:703)
    at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:911)
```

I cannot repro this issue with both Broker 12.0.1 and dev branch.
(enroll device in Shared mode via BrokerHost, sign into MSALTestApp,
navigate to AzureSample and sign out, navigate back to MSALTestApp)

I'm suspecting that this could be due to a bad/malformed data that was
sent to MSAL some time back (i.e. older versions of broker), and is
still cached.

Therefore, I decide to go with a **_targeted_** fix (instead of making
changes in GuestAccountFilter - which is used in many places).

If that hypothesis is correct, then this should fix it.
If not, then we should see similar issues popping up somewhere down the
line. (e.g. when MSAL is serializing the value returned from Broker)
I'll also add logs on Broker side to see if we've ever store/send out
local account id.
AzureAD/ad-accounts-for-android#2592

(cherry picked from commit 5156729)
(cherry picked from commit 225ee67)
  • Loading branch information
rpdome committed Nov 6, 2023
1 parent 2b89182 commit b20d5e8
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 5 deletions.
4 changes: 4 additions & 0 deletions changelog
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
MSAL Wiki : https://github.com/AzureAD/microsoft-authentication-library-for-android/wiki
vNext
----------
- [PATCH] Fix NPE in SingleAccountPublicClientApplication.getPersistedCurrentAccount (#1933)

Version 4.9.0
----------
- [PATCH] Update common @16.1.0
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ public List<ICacheRecord> filter(@NonNull final List<ICacheRecord> records) {
for (final ICacheRecord cacheRecord : records) {
final String acctHomeAccountId = cacheRecord.getAccount().getHomeAccountId();
final String acctLocalAccountId = cacheRecord.getAccount().getLocalAccountId();
if (acctLocalAccountId != null && acctHomeAccountId.contains(acctLocalAccountId)) {
if (acctHomeAccountId.contains(acctLocalAccountId)) {
result.add(cacheRecord);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,8 @@ public void getCurrentAccountAsync(@NonNull final CurrentAccountCallback callbac

private void getCurrentAccountAsyncInternal(@NonNull final CurrentAccountCallback callback,
@NonNull final String publicApiId) {
final String methodTag = TAG + ":getCurrentAccountAsyncInternal";

TokenMigrationCallback migrationCallback = new TokenMigrationCallback() {
@Override
public void onMigrationFinished(int numberOfAccountsMigrated) {
Expand All @@ -136,9 +138,21 @@ public void onTaskCompleted(final List<ICacheRecord> result) {
// To simplify the logic, if more than one account is returned, the first account will be picked.
// We do not support switching from MULTIPLE to SINGLE.
// See getAccountFromICacheRecordList() for more details.
final MultiTenantAccount oldAccount = getPersistedCurrentAccount();

MultiTenantAccount oldAccount = null;
boolean forceNotify = false;
try {
oldAccount = getPersistedCurrentAccount();
} catch (final NullPointerException e){
// There is an issue where the cached value could be malformed.
// If that happens, wipe the value, and trigger the callback.
Logger.error(methodTag, "Failed to load Persisted Current Account", e);
sharedPreferencesFileManager.remove(CURRENT_ACCOUNT_SHARED_PREFERENCE_KEY);
forceNotify = true;
}

persistCurrentAccount(result);
checkCurrentAccountNotifyCallback(callback, result, oldAccount);
checkCurrentAccountNotifyCallback(callback, result, oldAccount, forceNotify);
}

@Override
Expand Down Expand Up @@ -220,12 +234,13 @@ public void onError(@NonNull final MsalException exception) {

private void checkCurrentAccountNotifyCallback(@NonNull final CurrentAccountCallback callback,
@Nullable final List<ICacheRecord> newAccountRecords,
@Nullable final MultiTenantAccount oldAccount) {
@Nullable final MultiTenantAccount oldAccount,
final boolean forceNotify) {
final MultiTenantAccount newAccount = newAccountRecords == null
? null
: getAccountFromICacheRecordList(newAccountRecords);

if (!isHomeAccountIdMatching(oldAccount, newAccount)) {
if (forceNotify || !isHomeAccountIdMatching(oldAccount, newAccount)) {
callback.onAccountChanged(oldAccount, newAccount);
}

Expand Down

0 comments on commit b20d5e8

Please sign in to comment.