-
Notifications
You must be signed in to change notification settings - Fork 46
lab tweak, one auth client, Fixes AB#3447250 #2824
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
Conversation
|
❌ Work item link check failed. Description does not contain AB#{ID}. Click here to Learn more. |
|
✅ Work item link check complete. Description contains link AB#3447250 to an Azure Boards work item. |
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.
Pull request overview
This PR consolidates authentication client usage in the Lab API utilities to avoid BuildConfig dependencies at library build time. The change removes a dedicated KeyVault authentication client and extends the existing authentication client to support custom scopes dynamically.
- Removed the separate
mLabApiAuthenticationClientForKeyVaultfield that required BuildConfig at construction time - Added
getAccessTokenForCustomScope()method to dynamically request tokens with custom scopes - Updated
getKeyVaultSecret()to use the single authentication client with a custom scope parameter
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| LabApiUtilities/src/main/com/microsoft/identity/labapi/utilities/client/LabClient.java | Removed dedicated KeyVault auth client field and updated getKeyVaultSecret to use main auth client with custom scope |
| LabApiUtilities/src/main/com/microsoft/identity/labapi/utilities/authentication/LabApiAuthenticationClient.java | Added getAccessTokenForCustomScope method and refactored internal token acquisition to support dynamic scope parameter |
.../main/com/microsoft/identity/labapi/utilities/authentication/LabApiAuthenticationClient.java
Show resolved
Hide resolved
.../main/com/microsoft/identity/labapi/utilities/authentication/LabApiAuthenticationClient.java
Show resolved
Hide resolved
let's get of rid of this and corresponding constructor. there's only one other caller of the constructor, which can use your new method. This will clean up the class and your change. Refers to: LabApiUtilities/src/main/com/microsoft/identity/labapi/utilities/authentication/LabApiAuthenticationClient.java:54 in 7e297b6. [](commit_id = 7e297b6, deletion_comment = False) |
I think we can remove mScope (and ability to pass it through constructor), and just have a default scope field for anyone using getAccessToken(). |
mohitc1
left a comment
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.
🕐
mohitc1
left a comment
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.
![]()
Only need one auth client. Having a second one referencing BuildConfigs will cause issues if the build configs are not passed at library build time.
https://identitydivision.visualstudio.com/Engineering/_build/results?buildId=1565324&view=results
https://identitydivision.visualstudio.com/Engineering/_build/results?buildId=1565339&view=results
AB#3447250