Skip to content

Conversation

@shahzaibj
Copy link
Contributor

@shahzaibj shahzaibj commented Jun 24, 2025

This basically does a couple of changes:

  • 1st: Received feedback from eSTS folks to move app links under androidbroker/ path as opposed to the root of the eSTS domain.
  • 2nd: for each broker app specific app link, we were using a friendly identifier at the end such as /authenticator to denote authapp and /ltw to denote ltw. I've moved away from these custom friendly names and decided to just put the pkg name into the path. This allows us to declare an intent filter for it directly in our broker's intent filter as opposed to asking partner teams to put an intent filter in their manifest.

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

Copilot AI review requested due to automatic review settings June 24, 2025 01:46
@shahzaibj shahzaibj requested review from a team as code owners June 24, 2025 01:46
@github-actions
Copy link

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

Click here to Learn more.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR reworks the construction of broker app link redirect URIs based on partner feedback. The changes include:

  • Removing the appLinkRedirectUri parameter and adding a new method in BrokerData.kt to generate the URI using the package name.
  • Splitting the previous hardcoded redirect URL constants into scheme, host, and path prefix constants in AuthenticationConstants.java.
  • Updating the changelog to reflect the changes.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
common/src/main/java/com/microsoft/identity/common/internal/broker/BrokerData.kt Removed the appLinkRedirectUri parameter and added getAppLinkRedirectUri() to construct URIs dynamically.
common/src/main/java/com/microsoft/identity/common/adal/internal/AuthenticationConstants.java Replaced old redirect URL constants with distinct scheme, host, and path prefix constants.
changelog.txt Updated changelog with minor version note on broker app link redirect changes.
Comments suppressed due to low confidence (2)

common/src/main/java/com/microsoft/identity/common/internal/broker/BrokerData.kt:42

  • Update the class documentation to indicate that the app link redirect URI is now generated dynamically via getAppLinkRedirectUri(), as the previous appLinkRedirectUri parameter has been removed.
                      private val nickName: String?) {

common/src/main/java/com/microsoft/identity/common/adal/internal/AuthenticationConstants.java:1489

  • Consider extending the documentation for the newly introduced constants (scheme, host, and path prefix) to clarify how they should be combined to construct the complete app link redirect URI.
        public static final String BROKER_APP_LINK_REDIRECT_URL_SCHEME = "https";

return "$packageName::$signingCertificateThumbprint"
}

fun getAppLinkRedirectUri(): String {
Copy link

Copilot AI Jun 24, 2025

Choose a reason for hiding this comment

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

Consider using a URI builder or equivalent mechanism to handle URL encoding for the packageName to ensure the generated URI is valid in all cases.

Copilot uses AI. Check for mistakes.
@shahzaibj shahzaibj changed the title Shahzaibj/app link changes Construct broker app link redirect URI based on broker pkg name + move under /androidbroker path Jun 24, 2025
@shahzaibj shahzaibj added Skip-Consumers-Check Only include this if making a breaking change purposefully, and there is an MSAL/ADAL/Broker PR skip AB ID validation labels Jun 24, 2025
Copy link
Contributor

@p3dr0rv p3dr0rv left a comment

Choose a reason for hiding this comment

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

:shipit:

@shahzaibj shahzaibj merged commit 2f0712f into dev Jun 27, 2025
19 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Skip-Consumers-Check Only include this if making a breaking change purposefully, and there is an MSAL/ADAL/Broker PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants