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

Fix NPE in SingleAccountPublicClientApplication.getPersistedCurrentAccount #1933

Merged
merged 6 commits into from
Oct 31, 2023

Conversation

rpdome
Copy link
Member

@rpdome rpdome commented Oct 31, 2023

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. https://github.com/AzureAD/ad-accounts-for-android/pull/2592

@rpdome rpdome requested a review from a team as a code owner October 31, 2023 18:11
@github-actions github-actions bot added the msal label Oct 31, 2023
@rpdome rpdome changed the title wip Fix NPE in SingleAccountPublicClientApplication.getPersistedCurrentAccount Oct 31, 2023
Copy link
Contributor

@melissaahn melissaahn left a comment

Choose a reason for hiding this comment

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

LGTM; might we want to delete the null check on acctLocalAccountId in the HomeAccountFilter of AccountAdapter? An NPE will most likely be thrown anyway down the line if not thrown there, but maybe for consistency it would be good to remove it.

@rpdome
Copy link
Member Author

rpdome commented Oct 31, 2023

@somalaya I'm undoing the change in #1847 as suggested above.

@rpdome rpdome merged commit 5156729 into dev Oct 31, 2023
9 checks passed
rpdome added a commit that referenced this pull request Nov 1, 2023
…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)
rpdome added a commit that referenced this pull request Nov 6, 2023
…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)
This was referenced Nov 6, 2023
rpdome added a commit that referenced this pull request Nov 7, 2023
Undo the removal in
#1933
- as there is a chance that this is a separate issue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants