Rapong/release/4.9.1 #1939
Rapong/release/4.9.1 #1939
Conversation
…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)
| @@ -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)) { | |||
There was a problem hiding this comment.
Why removing this null check? Isn't this going to actually cause NPE now?
There was a problem hiding this comment.
@shahzaibj This was actually a band-aid fix of the same issue.
account local id is not supposed to be null. If we're getting null, it's likely that the data is malformed.
the fix I made targeted the malformed data in a specific flow. We probably shouldn't add a null check to the layer which is used throughout the code base.
see melissa's suggestion in this PR
#1933
There was a problem hiding this comment.
I looked into the code, the getLocalAccountId method on AccountRecord is not marked NonNull - so that means it is Nullable and that means that caller should perform the null check before using the value
There was a problem hiding this comment.
@shahzaibj the class AccountRecord was written in 2018. IIRC, that was before we introduced Nullable/NonNull (lombok) into the code.
Therefore, I'm not sure if we can conclude that every field in that class is meant to be nullable.
My concern is that suppressing the missing value in that layer could lead to unexpected behavior in other flows. (This is why I switched to have a flow-specific exception handler in #1933 instead)
The PR that introduced this null check was #1847, which had the same stack trace as #1933. We never had/needed a null check here before then.
Hotfix.