-
Notifications
You must be signed in to change notification settings - Fork 154
Lab Migration #1002
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.
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
base: dev
Are you sure you want to change the base?
Lab Migration #1002
Conversation
msal4j-sdk/src/integrationtest/java/com/microsoft/aad/msal4j/labapi/LabConstants.java
Outdated
Show resolved
Hide resolved
msal4j-sdk/src/integrationtest/java/com/microsoft/aad/msal4j/labapi/LabServiceParameters.java
Outdated
Show resolved
Hide resolved
msal4j-sdk/src/integrationtest/java/com/microsoft/aad/msal4j/labapi/LabUserHelper.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| //Avoids AADSTS7000218 credential errors in certain public client scenarios | ||
| public static LabResponse getDefaultUserMultiTenantAppPublicClient() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why we need dedicated user for PCA. I agree we need dedicated app, but not user. @RyAuld ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that there shouldn't be a reason for user unless there is a CA or role grant policy based on user involved with the test itself. I don't think there is in this instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mergeLabResponses() method that all of these helper methods use takes in the names of three key vault "secrets": one for the user, one for the lab, and one for the app, which are pretty much just the same sort of JSON responses from the old /user, /app, and /lab API endpoints.
These helper methods all use the same "MSAL-User-Default-JSON" user but different apps, and they just merge that user info with different app info.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the latest commit I renamed some of these helper classes/methods and added more comments to hopefully clarify what it's doing: it's not just fetching user config, rather it's getting whatever combination of user/app/lab config is needed for the test scenario.
MSAL Java version of the work done in AzureAD/microsoft-authentication-library-for-dotnet#5558
We are migrating from getting test config from the ID Labs API to a more Key Vault-based approach. This work was recently completed in MSAL .NET, and this PR makes similar changes in Java.
A lot of files were changed in this PR, reviews can focus on a single folder at a time since all the files in each received similar changes:
com.microsoft.aad.msal4j/com.microsoft.aad.msal4j/labapi/infrastructure/labapi/com.microsoft.aad.msal4j/labapi/pom.xml: updated with a few new test-only dependenciesAuthorizationResponseHandler: unrelated to the lab migration, this just fixes a long-neglected issue (Missing error and error_description when acquiring the token interactively #513) that I was reminded of when testing the new systemIn addition to the lab migration, some test scenarios were streamlined to avoid over-testing or testing deprecated services: