search

AzureAD / microsoft-authentication-library-common-for-android

Common code used by both the Active Directory Authentication Library (ADAL) and the Microsoft Authentication Library (MSAL)
MIT License
41 stars 35 forks source link

Handle Receiver Callback Exception in CommandDispatcher interactive command #2305

Closed fadidurah closed 8 months ago

fadidurah commented 8 months ago

Related Bug https://identitydivision.visualstudio.com/Engineering/_workitems/edit/2502607

Related PR https://github.com/AzureAD/ad-accounts-for-android/pull/2690

Teams is seeing a crash during LocalMSALController.onFinishAuthorizationSession(), where we occasionally get a NullPointerException on mAuthorizationStrategy.completeAuthorization(). It's unclear why this happens, but the root issue that such an exception should be routed as a BaseException and sent back as a command result. This PR handles this by routing the exception through the command result if it happens during the callback

Also add a check to see if the authorization future is initialized and waiting, provide an exception for it if we run into the NPE.

E2E testing I tested this with MSALTestApp by throwing a NPE explicitly in complereAuthorization() and caught the exception, routing through to a command result.

shahzaibj commented 8 months ago

Can you try reproducing the issue by just hard-code throwing an NPE from that place in the code? I think we need at least some manual validation of this change

fadidurah commented 8 months ago

Can you try reproducing the issue by just hard-code throwing an NPE from that place in the code? I think we need at least some manual validation of this change

I tried this, the application does not crash with my fix. Unfortunately in my case it hangs forever, because the result future that would be completed in completeAuthorization is not getting a result sent to it, so i'm guessing some background thread is hanging forever so nothing actually happens in my application. On the plus side i don't think this scenario would happen in the user scenario because if the strategy is never initialized, then the future would not be either. But i added a check for both anyway.