Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Organize browser selection classes and change signature for get AuthorizationStrategy, Fixes AB#3119409 #2564

Merged
merged 18 commits into from
Jan 24, 2025
Merged

Organize browser selection classes and change signature for get AuthorizationStrategy, Fixes AB#3119409 #2564

merged 18 commits into from
Jan 24, 2025

Conversation

p3dr0rv
Copy link
Contributor

@p3dr0rv p3dr0rv commented Jan 3, 2025 •
edited by azure-boards bot
Loading

The changes here are the preparatory work for the DUNA feature. Splitting this PR from DUNA to simplify the review process.

  • Update Browser Selector and move it to Platform Components
  • Change getAuthorizationStrategy signature from (InteractiveTokenCommandParameters) to (AuthorizationAgent , Browser, isBrokerRequest)
  • Move getBrowserSafeListForBroker from Platform Util to BrowserDescriptor
  • Move Browser to common4j

AB#3119409

@p3dr0rv p3dr0rv changed the title [WIP]draft Organize browser selection classes and change signature for get AuthorizationStrategy Jan 3, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 3, 2025

✅ Work item link check complete. Description contains link AB#3119409 to an Azure Boards work item.

@AzureAD AzureAD deleted a comment from github-actions bot Jan 3, 2025
@github-actions github-actions bot changed the title Organize browser selection classes and change signature for get AuthorizationStrategy Organize browser selection classes and change signature for get AuthorizationStrategy, Fixes AB#3119409 Jan 3, 2025
@p3dr0rv p3dr0rv marked this pull request as ready for review January 3, 2025 21:19
@p3dr0rv p3dr0rv requested a review from a team as a code owner January 3, 2025 21:19
shahzaibj
shahzaibj reviewed Jan 7, 2025
View reviewed changes
...main/java/com/microsoft/identity/common/internal/ui/AndroidAuthorizationStrategyFactory.java
if (parameters.getAuthorizationAgent() == AuthorizationAgent.WEBVIEW) {
Logger.info(methodTag, "Use webView for authorization.");
// Use embedded webView if no browser available or authorization agent is webView
if (authorizationAgent == AuthorizationAgent.WEBVIEW || browser == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If someone passes authorization agent as BROWSER but passes null browser, then what strategy is picked?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

embedded web view authorization strategy

Add Javadoc:

  • Get the authorization strategy based on the authorization agent and browser.
  • If the authorization agent is WEBVIEW or the browser is null,
  • return the embedded web view authorization strategy.
  • Otherwise, return the browser authorization strategy.

shahzaibj
shahzaibj reviewed Jan 7, 2025
View reviewed changes
...main/java/com/microsoft/identity/common/internal/ui/AndroidAuthorizationStrategyFactory.java Outdated
Comment on lines 68 to 69
@NonNull final AuthorizationAgent authorizationAgent,
@Nullable final Browser browser,
Copy link
Contributor

Choose a reason for hiding this comment

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

If someone passes authorization agent as WEBVIEW but also a non-null browser then what does that mean? What strategy gets picked?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

embedded web view authorization strategy

Add Javadoc:
* Get the authorization strategy based on the authorization agent and browser.
* If the authorization agent is WEBVIEW or the browser is null,
* return the embedded web view authorization strategy.
* Otherwise, return the browser authorization strategy.

@p3dr0rv
Copy link
Contributor Author

p3dr0rv commented Jan 8, 2025

@shahzaibj, it seems there is some confusion about the getAuthorizationStrategy method. I’ve rephrased my comment and included it as Javadoc. Does that address the issue, or were you suggesting a different approach?

@mohitc1
Copy link
Contributor

mohitc1 commented Jan 15, 2025 

                                .fragment(fragment)

how about we set browserSelector on AndroidAuthorizationStrategyFactory here itself.

with that you may avoid calling browselect every time before creating AndroidAuthorizationStrategy

Refers to: common/src/main/java/com/microsoft/identity/common/components/AndroidPlatformComponentsFactory.java:139 in 6aabd68. [](commit_id = 6aabd68, deletion_comment = False)

@p3dr0rv
Copy link
Contributor Author

p3dr0rv commented Jan 17, 2025 

                                .fragment(fragment)

how about we set browserSelector on AndroidAuthorizationStrategyFactory here itself.

with that you may avoid calling browselect every time before creating AndroidAuthorizationStrategy

Refers to: common/src/main/java/com/microsoft/identity/common/components/AndroidPlatformComponentsFactory.java:139 in 6aabd68. [](commit_id = 6aabd68, deletion_comment = False)

done

mohitc1
mohitc1 reviewed Jan 22, 2025
View reviewed changes
...src/main/java/com/microsoft/identity/common/components/AndroidPlatformComponentsFactory.java
@@ -127,14 +128,16 @@ public static void fillBuilderWithBasicImplementations(
.storageSupplier(new AndroidStorageSupplier(context,
new AndroidAuthSdkStorageEncryptionManager(context)))
.platformUtil(new AndroidPlatformUtil(context, activity))
.httpClientWrapper(new DefaultHttpClientWrapper());
.httpClientWrapper(new DefaultHttpClientWrapper())
.browserSelector(new AndroidBrowserSelector(context));
Copy link
Contributor

Choose a reason for hiding this comment

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

.browserSelector(new AndroidBrowserSelector(context));

would you still need here? If so, can we set same object on both and not create new ?

Copy link
Contributor Author

@p3dr0rv p3dr0rv Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

YES.
when duna is enabled, we need to use the browser selector to check if we have a duna broker available.
see: https://github.com/AzureAD/ad-accounts-for-android/pull/3017/files/70382d1f9a954f545ecb58e7396e54c678a7a802#r1890858348

yes, I can use the same object here

mohitc1
mohitc1 approved these changes Jan 22, 2025
View reviewed changes
Copy link
Contributor

@mohitc1 mohitc1 left a comment

Choose a reason for hiding this comment

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

:shipit:

@p3dr0rv p3dr0rv merged commit 99a061e into dev Jan 24, 2025
22 of 24 checks passed
@p3dr0rv p3dr0rv deleted the pedroro/browser-selector branch January 24, 2025 19:32
@p3dr0rv p3dr0rv mentioned this pull request Jan 24, 2025
Merged
p3dr0rv added a commit to AzureAD/microsoft-authentication-library-for-android that referenced this pull request Jan 24, 2025
@p3dr0rv
Update browser selector (#2250)
5c0ff4e 
With the changes introduced in
AzureAD/microsoft-authentication-library-common-for-android#2564,
I broke the MSAL test app, we need to update the signature for Browser
selector.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shahzaibj shahzaibj shahzaibj approved these changes

@mohitc1 mohitc1 mohitc1 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@p3dr0rv @mohitc1 @shahzaibj