Support a new API Path for Account Transfer V2 in Broker #2375
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
…/ClientException.java Co-authored-by: Shahzaib <37125644+shahzaibj@users.noreply.github.com>
fadidurah
changed the title
fadidurah
commented
Apr 10, 2024
| @@ -1599,6 +1600,12 @@ public String getMsalVersion(){ | |||
| */ | |||
| public static final String MSAL_ACQUIRE_TOKEN_INTERACTIVE_PATH = "/acquireTokenInteractive"; | |||
|
|
|||
| /** | |||
| * URI Path constant for MSAL-to-Broker acquireTokenInteractive (for ATv2) request using ContentProvider. | |||
| * TODO: Not really sure how this works, this seems right | |||
fadidurah
commented
Apr 10, 2024
...n/src/main/java/com/microsoft/identity/common/internal/controllers/BrokerMsalController.java
Outdated
Show resolved
Hide resolved
fadidurah
commented
Apr 10, 2024
...n/src/main/java/com/microsoft/identity/common/internal/controllers/BrokerMsalController.java
Outdated
Show resolved
Hide resolved
mohitc1
reviewed
Apr 11, 2024
common/src/main/java/com/microsoft/identity/common/adal/internal/AuthenticationConstants.java
Outdated
Show resolved
Hide resolved
mohitc1
reviewed
Apr 11, 2024
common/src/main/aidl/com/microsoft/identity/client/IMicrosoftAuthService.aidl
Outdated
Show resolved
Hide resolved
mohitc1
reviewed
Apr 11, 2024
.../main/com/microsoft/identity/common/java/commands/parameters/ATv2TokenCommandParameters.java
Outdated
Show resolved
Hide resolved
git commit -m ":wq Merge branch 'dev' of https://github.com/AzureAD/microsoft-authentication-library-common-for-android into fadi/ATv2-API
mohitc1
reviewed
Apr 23, 2024
...n/src/main/java/com/microsoft/identity/common/internal/controllers/BrokerMsalController.java
Outdated
Show resolved
Hide resolved
mohitc1
reviewed
Apr 23, 2024
...n/src/main/java/com/microsoft/identity/common/internal/request/MsalBrokerRequestAdapter.java
Show resolved
Hide resolved
fadidurah
added
the
Skip-Consumers-Check
mohitc1
reviewed
Apr 26, 2024
...n/src/main/java/com/microsoft/identity/common/internal/request/MsalBrokerRequestAdapter.java
Outdated
Show resolved
Hide resolved
mohitc1
approved these changes
Apr 26, 2024
…tion-library-common-for-android into fadi/ATv2-API t status
shahzaibj
reviewed
Apr 29, 2024
common4j/src/main/com/microsoft/identity/common/java/controllers/BaseController.java
Outdated
Show resolved
Hide resolved
fadidurah
removed
the
Skip-Consumers-Check
fadidurah
added
the
Skip-Consumers-Check
rpdome
reviewed
May 23, 2024
| @@ -38,6 +38,8 @@ interface IMicrosoftAuthService { | |||
|
|
|||
| Intent getIntentForInteractiveRequest(); | |||
|
|
|||
| Intent getIntentForAccountTransferInteractiveRequest(); | |||
There was a problem hiding this comment.
add it at the bottom.
there might be some weird side effects if the order of aidl on broker and MSAL/OneAuth side mismatches.
| final Intent accountTransferIntent = microsoftAuthService.getIntentForAccountTransferInteractiveRequest(); | ||
| final Bundle accountTransferBundle = accountTransferIntent.getExtras(); | ||
|
|
||
| //older brokers (pre-ContentProvider) are ONLY sending these values in the intent itself. |
There was a problem hiding this comment.
this msg could be confusing - since this operation is just added (it's not pre-ContentProvider)
you actually can just expose
GetBundleForAccountTransferInteractiveRequestMsalBrokerOperation
on the Broker side, and return the resulted bundle as is.
The code will be cleaner on the Broker side, and you wouldn't need to do extra conversion here.
| @Override | ||
| public void performPrerequisites(final @NonNull IIpcStrategy strategy) throws BaseException { | ||
| verifyTokenParametersAreSupported(parameters); | ||
| negotiatedBrokerProtocolVersion = hello(strategy, parameters.getRequiredBrokerProtocolVersion()); |
There was a problem hiding this comment.
if the negotiatedBrokerProtocolVersion is lower than the new version you just added, it means the broker does NOT support this operation.
In that case, you might want to fail the request early. (no need to make the request to the broker because it'll just throw "not supported" back)
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.
In this PR, i'm adding a new API Path for Account Transfer V2 rather than adding the functionality through Account Chooser activity and Acquire Token. This will make it easier to track the code path for ATv2.
Changes:
acquireTokenForAccountTransferinBrokerMsalController, along with metho to get intent for ATv2. This method is defined inBaseController, although it throws an exception inBaseController, and is only implemented inBrokerMsalControllerAccountTransferTokenCommandParametersto store transfer tokenAuthenticationConstantsto support new APIBrokerRequestformAccountTransferTokenBroker PR to support these changes: https://github.com/AzureAD/ad-accounts-for-android/pull/2768
Consumers check (expecting broker to fail): https://identitydivision.visualstudio.com/Engineering/_build/results?buildId=1298934&view=results