Skip to content

MSI Refactoring#715

Merged
Avery-Dunn merged 7 commits intonebharg/MSIfrom
avdunn/msi-refactoring
Oct 5, 2023
Merged

MSI Refactoring#715
Avery-Dunn merged 7 commits intonebharg/MSIfrom
avdunn/msi-refactoring

Conversation

@Avery-Dunn
Copy link
Copy Markdown
Contributor

@Avery-Dunn Avery-Dunn commented Sep 25, 2023 •
edited
Loading

Refactoring the codebase to better accommodate the new MSI code. The main goals of this PR:

  • Make no changes to the functionality or visibility of any API that MSAL Java has already released, and do not break any of the newer MSI-related tests
  • Add a new IApplicationBase interface above the existing IClientApplicationBase, to have better control over the visibility of APIs for the managed identity application vs. the existing public/confidential client applications (commit 3715e77)
  • Refactor the new MSI-related classes to better follow Java best practices for naming/visibility/etc., and match the coding styles used throughout the rest of MSAL Java (commit 6835ef0)

@Avery-Dunn
Refactor the Application class hierarchy to better separate Public/Co…
3715e77 
…nfidentialClientApplication and ManagedIdentityApplication
@Avery-Dunn
Refactor ManagedIdentity classes to better follow Java best-practices…
6835ef0 
…, reduce use of public scope, and generally match the library's existing code styles
bgavrilMS
bgavrilMS reviewed Sep 25, 2023
View reviewed changes
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AbstractApplicationBase.java Outdated

@Accessors(fluent = true)
@Getter
private String applicationName;
Copy link
Copy Markdown
Member

@bgavrilMS bgavrilMS Sep 25, 2023

Choose a reason for hiding this comment

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

Please leave appname and appver out, as we're not hitting ESTS.

PS: they are not actually used :)

Copy link
Copy Markdown
Contributor Author

@Avery-Dunn Avery-Dunn Oct 4, 2023

Choose a reason for hiding this comment

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

Moved it back to AbstractClientApplicationBase in the latest commit (they were referenced in the RequestContext class which now uses AbstractApplicationBase, so I figured they could've been relevant for managed identity).

However, since they aren't actually used should we start marking them as deprecated? (in a different PR)

@neha-bhargava
Copy link
Copy Markdown
Contributor

neha-bhargava commented Sep 29, 2023

We will also need to hide the API's at the request level. Right now the ManagedIdentityParameters implements the IAcquireTokenParamters which have APIs WithClaims, extraQueryParams etc. Which are not relevant to managed identity. Please add a base level to IAcquireTokenParameters as well.

bgavrilMS
bgavrilMS requested changes Oct 3, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@bgavrilMS bgavrilMS left a comment

Choose a reason for hiding this comment

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

SILENT flow and REFREH_TOKEN flow are not part of the base.

@Avery-Dunn
Merge branch 'nebharg/MSI' of https://github.com/AzureAD/microsoft-au…
c499019 
…thentication-library-for-java into avdunn/msi-refactoring

# Conflicts:
#	msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AppServiceManagedIdentitySource.java
#	msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/IMDSManagedIdentitySource.java
#	msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/ManagedIdentityErrorResponse.java
#	msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/ManagedIdentityTestDataProvider.java
#	msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/ManagedIdentityTests.java
@Avery-Dunn
Merge cloud shell changes
d231b6c
@Avery-Dunn
Address code review comments
bdc4192
@Avery-Dunn Avery-Dunn requested a review from bgavrilMS October 4, 2023 15:38
@Avery-Dunn Avery-Dunn merged commit 450757a into nebharg/MSI Oct 5, 2023
@Avery-Dunn Avery-Dunn deleted the avdunn/msi-refactoring branch December 7, 2023 17:46
@Avery-Dunn Avery-Dunn mentioned this pull request Jun 18, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bgavrilMS bgavrilMS bgavrilMS approved these changes

@neha-bhargava neha-bhargava neha-bhargava 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

@Avery-Dunn @neha-bhargava @bgavrilMS