Edge TB: changes related to GetToken, Fixes AB#3317102 #2803

melissaahn · 2025-11-04T12:16:33Z

Summary

This PR creates some new data objects and contains changes to BrokerMsalController and some validation logic in BrokerSsoController in order to accommodate the webapps API. Please see the main broker PR for a more detailed description:
Broker PR: https://github.com/AzureAD/ad-accounts-for-android/pull/3267

AB#3317102

…poseWebApis Dev

dev

…pers dev

github-actions · 2025-11-04T12:16:50Z

❌ Work item link check failed. Description does not contain AB#{ID}.

Click here to Learn more.

Copilot

Pull Request Overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 9 comments.

common4j/src/main/com/microsoft/identity/common/java/util/IPlatformUtil.java

Copilot · 2025-11-05T04:06:16Z

...n/src/main/java/com/microsoft/identity/common/internal/controllers/BrokerMsalController.java

+            final WebAppsTokenResponse response = new WebAppsTokenResponse(
+                    interactiveResult.getAccountRecord() != null ? interactiveResult.getAccountRecord().getUsername() : null,
+                    interactiveResult.getUniqueId(),
+                    interactiveResult.getAccessTokenRecord() != null ? interactiveResult.getAccessTokenRecord().getExpiresOn() : null,
+                    interactiveResult.getIdToken(),
+                    interactiveResult.getAccessToken(),
+                    null
+            );


Severity: High – Raw access token and ID token are being included in the response object that will be serialized and transmitted. Issue: Lines 1734-1735 include raw token values (interactiveResult.getIdToken() and interactiveResult.getAccessToken()) in the WebAppsTokenResponse. Impact: These sensitive tokens will be logged if serialization occurs in any logging/telemetry path, violating the security principle that raw tokens must never be logged (per guidelines §2.2). Recommendation: Verify that the serialization path for WebAppsTokenResponse and its transmission to the broker does not pass through any logging or telemetry emission points. If there's any risk of exposure, redact or exclude tokens from logged representations.

While this is a good warning, don't we already do this in our regular token operation logic? Is this really a big concern?

...n/src/main/java/com/microsoft/identity/common/internal/controllers/BrokerMsalController.java

...c/main/com/microsoft/identity/common/java/providers/oauth2/OpenIdConnectPromptParameter.java

...n/src/main/java/com/microsoft/identity/common/internal/controllers/BrokerMsalController.java

Copilot · 2025-11-05T04:06:17Z

...n/src/main/java/com/microsoft/identity/common/internal/controllers/BrokerMsalController.java

+    private String handleInteractivePhase(@NonNull final String request,
+                                          @NonNull final Bundle phaseOneResultBundle,
+                                          @NonNull final String negotiatedBrokerProtocolVersion,
+                                          @NonNull final String minBrokerProtocolVersion,
+                                          @NonNull final WebAppsAdditionalRequiredParameters additionalRequiredParams,
+                                          @Nullable final Map<String, String> additionalArgs) throws BaseException {


Complex two-phase authentication flow lacks test coverage. Issue: The handleInteractivePhase() method implements critical two-phase flow logic including deserialization, token acquisition, error handling, and response serialization, but no tests exist for this method. Impact: This complex control flow with multiple error paths (lines 1707-1709, 1718-1720, 1726-1728, 1743-1754) and serialization operations is untested, leaving high-risk authentication logic unverified. Recommendation: Add unit tests covering: (1) successful interactive token acquisition and response serialization, (2) missing interactive request payload, (3) null deserialized request, (4) token acquisition failure with ClientException, (5) token acquisition failure with ServiceException, (6) serialization errors in phase 2 request.

Copilot · 2025-11-05T04:06:17Z

...n/src/main/java/com/microsoft/identity/common/internal/controllers/BrokerMsalController.java

+    private String performSecondPhaseWebAppRequest(@NonNull final String request,
+                                                   @NonNull final String negotiatedBrokerProtocolVersion,
+                                                   @NonNull final String minBrokerProtocolVersion,
+                                                   @NonNull final WebAppsAdditionalRequiredParameters additionalRequiredParams,
+                                                   @NonNull final Map<String, String> extraArgs) throws BaseException {


Second phase web app request flow lacks test coverage. Issue: The performSecondPhaseWebAppRequest() method implements the critical second phase of the two-phase authentication flow but has no test coverage. Impact: This method handles bundle construction, result extraction, and version verification (lines 1809, 1810) without verification, leaving critical authentication flow logic untested. Recommendation: Add unit tests covering: (1) successful second phase request and response, (2) null result bundle handling, (3) broker version verification failure, (4) bundle construction errors.

Copilot · 2025-11-05T04:06:17Z

...n/src/main/java/com/microsoft/identity/common/internal/controllers/BrokerMsalController.java

+    private BrokerInteractiveTokenCommandParameters buildInteractiveTokenParametersForWebApps(@NonNull final WebAppsInteractiveRequest webAppsRequest,
+                                                                                              @NonNull final WebAppsAdditionalRequiredParameters requiredParams,
+                                                                                              @NonNull final String minBrokerProtocolVersion) throws BaseException {


Parameter building logic with complex validation lacks test coverage. Issue: The buildInteractiveTokenParametersForWebApps() method contains extensive validation logic (lines 1845-1864), scope parsing (lines 1866-1877), authority parsing (lines 1879-1886), and parameter construction (lines 1901-1918) but has no test coverage. Impact: The complex validation and transformation logic with multiple error paths is untested, increasing risk of parameter construction bugs. Recommendation: Add unit tests covering: (1) null/missing required parameter fields, (2) null/empty authority, clientId, redirect, (3) empty vs valid scope parsing, (4) null vs non-null nonce handling, (5) instanceAware true/false, (6) authority parsing failures, (7) successful parameter construction with all fields.

I will be writing unit tests for this path (it will take some time) and it may not come in this PR due to the size.

…pers dev

melissaahn · 2025-11-06T17:35:23Z

Regarding the failing assemble consumers being caused by the validate broker step, it's failing due to a breaking change:

  D:\a\_work\1\s\AADAuthenticator\src\msal\java\com\microsoft\identity\client\MicrosoftAuthServiceOperation.java:251: error: unreported exception ClientException; must be caught or declared to be thrown
              parameters.validate();
                                 ^
  D:\a\_work\1\s\AADAuthenticator\src\msal\java\com\microsoft\identity\client\MicrosoftAuthServiceOperation.java:294: error: unreported exception ClientException; must be caught or declared to be thrown
              parameters.validate();
                                 ^
  D:\a\_work\1\s\AADAuthenticator\src\msal\java\com\microsoft\identity\client\MicrosoftAuthServiceOperation.java:458: error: unreported exception ClientException; must be caught or declared to be thrown
              parameters.validate();

The fix is to declare ClientException to be thrown, and that is made in the broker PR: https://github.com/AzureAD/ad-accounts-for-android/pull/3267/files#diff-e67c2e127c25d14625ad43c557f2aeb3357591fb8c965b6df8f641c880a2dc86

So this is why I will add the "skips consumer check" (once I get this PR reviewed)

fadidurah · 2025-11-06T18:14:09Z

...4j/src/main/com/microsoft/identity/common/java/commands/webapps/WebAppsSupportedContracts.kt

+            prefix = "[",
+            postfix = "]")
+    }
+}


fadidurah · 2025-11-06T18:19:05Z

...in/com/microsoft/identity/common/java/commands/webapps/WebAppsGetTokenSubOperationRequest.kt

+        const val FIELD_EXTRA_PARAMETERS = "extraParameters"
+        const val DEFAULT_AUTHORITY = "https://login.microsoftonline.com/common"
+    }
+}


some files missing EOF new line

fadidurah · 2025-11-06T18:20:06Z

...in/com/microsoft/identity/common/java/commands/webapps/WebAppsGetTokenSubOperationRequest.kt

+        const val FIELD_HOME_ACCOUNT_ID = "accountId"
+        const val FIELD_CLIENT_ID = "clientId"
+        const val FIELD_AUTHORITY = "authority"
+        const val FIELD_SCOPES = "scope"


scope or scopes?

mohitc1 · 2025-11-06T18:23:58Z

...n/src/main/java/com/microsoft/identity/common/internal/controllers/BrokerMsalController.java

+                                if (shouldForceInteractive(getTokenRequest, additionalRequiredParams.getCanShowUi())) {
+                                    mergedExtraArgs.putAll(
+                                            performInteractiveAndSerialize(envelope, additionalRequiredParams, minBrokerProtocolVersion)
+                                    );


Not clear here. Why do you need to send it to broker?
Don't you have full result already?

fadidurah · 2025-11-06T18:25:41Z

common/src/main/java/com/microsoft/identity/common/internal/platform/AndroidPlatformUtil.java

                || redirectUri.equals("https://login.microsoftonline.com/common/oauth2/nativeclient"));
    }
+
+    protected Context getContext() {


Where is this one used?

fadidurah · 2025-11-06T18:37:29Z

...soft/identity/common/java/providers/microsoft/azureactivedirectory/AzureActiveDirectory.java

    }

+    /**
+     * Builds and validates the authority from the WebApp sender URL.


Is this method only for this webApp functionality? we can rename it to include that i think. Maybe rename parameter to mention web app

fadidurah · 2025-11-06T18:58:02Z

...n/src/main/java/com/microsoft/identity/common/internal/controllers/BrokerMsalController.java

+     * @return The result string from the broker after the second phase completes.
+     * @throws BaseException If the interactive request payload is missing or token acquisition fails.
+     */
+    private String handleInteractivePhase(@NonNull final String request,


Let's mention webapp in method name

"interactivePhase" can be vague i think

…pers dev

melissaahn added 28 commits October 17, 2025 10:07

start

21c3ed2

cleaning up

1276de7

changelog

cf872e1

suggestion

1378c50

exposing api

8f04dd0

reentrant readwrite lock

addd84a

removing cache

3672ba1

changed structure

075440c

unused imports

f813536

renames

6e5b3c3

Merge branch 'melissaahn/WebAppsAccountIdRegistry' into melissaahn/Ex…

456e0d5

…poseWebApis Dev

Merge remote-tracking branch 'origin/dev' into melissaahn/ExposeWebApis

c7cb41d

dev

min version

935091d

addressing comments

3eae88b

changelog

b53404d

changelog

6fbaa31

Merge remote-tracking branch 'origin/dev' into melissaahn/ExposeWebApis

34d9d6f

dev

dev

7b2876d

dev

84ea614

Merge branch 'dev' into melissaahn/ExposeWebApis

17b851a

Merge remote-tracking branch 'origin/dev' into melissaahn/ExposeWebApis

262e689

dev

saving progress

a763095

saving progress

19e573f

saving progress again

8a1d9ff

clean up

27f6cf2

Merge remote-tracking branch 'origin/dev' into melissaahn/GetTokenHel…

827d081

…pers dev

removed

7f0003d

more

94338cd

melissaahn requested a review from Copilot November 4, 2025 12:16

melissaahn added 4 commits November 4, 2025 13:41

comments

699c860

more constants

14ae0ea

updated error handling

3669d6d

failures

0eebf03

melissaahn requested a review from Copilot November 5, 2025 04:00

Copilot AI reviewed Nov 5, 2025

View reviewed changes

melissaahn added 2 commits November 4, 2025 23:46

cleaned up some moree

65ad998

some unit tests

4f6a4e6

melissaahn marked this pull request as ready for review November 5, 2025 09:25

melissaahn requested review from a team as code owners November 5, 2025 09:25

melissaahn added 4 commits November 5, 2025 22:42

refactoring

5309bb5

Merge remote-tracking branch 'origin/dev' into melissaahn/GetTokenHel…

d8f7af8

…pers dev

spotbug

938d27e

edits

a53b257

fadidurah reviewed Nov 6, 2025

View reviewed changes

mohitc1 reviewed Nov 6, 2025

View reviewed changes

fadidurah reviewed Nov 6, 2025

View reviewed changes

fadidurah approved these changes Nov 6, 2025

View reviewed changes

melissaahn added 6 commits November 7, 2025 10:18

moving files around

7f362b5

dev

a98544a

big refactor

391eab5

Merge remote-tracking branch 'origin/dev' into melissaahn/GetTokenHel…

3c3b36d

…pers dev

edits

191ad5b

testing

629b232

Edge TB: changes related to GetToken, Fixes AB#3317102 #2803

Are you sure you want to change the base?

Edge TB: changes related to GetToken, Fixes AB#3317102 #2803

Uh oh!

Conversation

melissaahn commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

github-actions bot commented Nov 4, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

melissaahn commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

melissaahn commented Nov 4, 2025 •

edited

Loading

melissaahn commented Nov 6, 2025 •

edited

Loading