Handle Receiver Callback Exception in CommandDispatcher interactive command #2305
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fadidurah
changed the title
|
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. |
shahzaibj
reviewed
Feb 1, 2024
...on/src/main/java/com/microsoft/identity/common/internal/controllers/LocalMSALController.java
Outdated
Show resolved
Hide resolved
shahzaibj
reviewed
Feb 1, 2024
common4j/src/main/com/microsoft/identity/common/java/controllers/CommandDispatcher.java
Outdated
Show resolved
Hide resolved
fadidurah
changed the title
…tion-library-common-for-android into fadi/receiver-exception
fadidurah
added
the
Skip-Consumers-Check
shahzaibj
reviewed
Feb 1, 2024
...on/src/main/java/com/microsoft/identity/common/internal/controllers/LocalMSALController.java
Outdated
Show resolved
Hide resolved
shahzaibj
reviewed
Feb 1, 2024
common4j/src/main/com/microsoft/identity/common/java/controllers/CommandDispatcher.java
Outdated
Show resolved
Hide resolved
shahzaibj
reviewed
Feb 1, 2024
common4j/src/main/com/microsoft/identity/common/java/controllers/CommandDispatcher.java
Outdated
Show resolved
Hide resolved
shahzaibj
reviewed
Feb 1, 2024
...on/src/main/java/com/microsoft/identity/common/internal/controllers/LocalMSALController.java
Outdated
Show resolved
Hide resolved
shahzaibj
approved these changes
Feb 1, 2024
melissaahn
approved these changes
Feb 1, 2024
fadidurah
added a commit
that referenced
this pull request
Feb 5, 2024
Revert changes made to method signature to return back a ResultFuture instead of a Future. It seems to have introduced a breaking change in MSAL, instead of debugging this during release time, I rolled back part of the solution for the original google crash. The fix still does the same thing, just by using a type casting rather than explicit return type being set to ResultFuture. As mentioned in the original PR, this scenario should theoretically be impossible anyway, as if we fail out of authorization because the strategy is null, then the future would not be initialized, but keeping the result future check there for additional security. Original PR #2305
fadidurah
added a commit
that referenced
this pull request
Feb 9, 2024
Revert changes made to method signature to return back a ResultFuture instead of a Future. It seems to have introduced a breaking change in MSAL, instead of debugging this during release time, I rolled back part of the solution for the original google crash. In this PR i'm rolling back the whole fix for better tracking, will have a following PR to reintroduce the smaller fix into common to address the issue without adjusting method signatures that could affect msal or broker. Original PR #2305
Merged
fadidurah
added a commit
that referenced
this pull request
Feb 13, 2024
Previously implemented a fix for common to address this bug https://identitydivision.visualstudio.com/Engineering/_workitems/edit/2502607. Original common PR #2305, which has now been reverted. During release process in february, realized that the original fix introduced a break in MSAL. Not totally clear where the break was, but realized that parts of the fix that were causing the break were unnecessary. This PR is to add back the parts of the fix that would address the teams bug, without introducing the breaking change in MSAL and Broker (we don't change the method signatures in `OAuth2Strategy.requestAuthorization()` or `IAuthorizationStrategy.requestAuthorization()`). Instead of changing signature, we use a type check. Testing: Tested manually, was able to recover after throwing NPE Also ran common consumers pipeline check
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 aNullPointerExceptiononmAuthorizationStrategy.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 callbackAlso 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.