-
Notifications
You must be signed in to change notification settings - Fork 46
[Common] Update Broker ATS for Resource Accounts, Fixes AB#3230426 #2704
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 complete. Description contains link AB#3230426 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 enhances the Broker ATS flow to support Resource Accounts by introducing a new flag on silent token parameters and updating how token errors are adapted, plus new tests, a JWT constant, and changelog entry.
- Add
isRequestForResourceAccounttoBrokerSilentTokenCommandParametersto signal Resource Account requests. - Extend
ExceptionAdapterwith a boolean flag to conditionally convert toUiRequiredExceptionand wire it through silent- and non-silent-token flows. - Update tests to cover the new flag and error-conversion behavior, add a JWT “use” constant, and bump the changelog.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| common4j/src/test/com/microsoft/identity/common/java/controllers/ExceptionAdapterTests.java | Convert JUnit asserts to static imports and add tests for resource-account and policy-required flows |
| common4j/src/main/com/microsoft/identity/common/java/jwt/JwtKeyUseTypes.kt | Introduce RESOURCE_ACCOUNT constant with KDoc |
| common4j/src/main/com/microsoft/identity/common/java/controllers/ExceptionAdapter.java | Add overload with allowConvertToUiRequiredException, handle silent-resource-account logic, adjust visibility |
| common4j/src/main/com/microsoft/identity/common/java/commands/parameters/BrokerSilentTokenCommandParameters.java | Add isRequestForResourceAccount flag and documentation |
| changelog.txt | Document “Update Broker ATS flow for Resource Account” |
Comments suppressed due to low confidence (5)
common4j/src/main/com/microsoft/identity/common/java/controllers/ExceptionAdapter.java:225
- Changing this method from public to package-private may break existing public API consumers. Consider restoring its public visibility or marking it deprecated before removal.
static ServiceException getExceptionFromTokenErrorResponse(@NonNull final TokenErrorResponse errorResponse) {
common4j/src/test/com/microsoft/identity/common/java/controllers/ExceptionAdapterTests.java:108
- The assertion message mentions UiRequiredException, but you’re checking for ServiceException. Update the message to reference ServiceException for clarity.
assertTrue("Expected exception of UiRequiredException type", e instanceof ServiceException);
common4j/src/test/com/microsoft/identity/common/java/controllers/ExceptionAdapterTests.java:85
- [nitpick] Method names mix camelCase and underscores. Consider renaming to
testMfaTokenErrorResponseAllowUiRequiredExceptionTruefor consistency with other test names.
public void testMFATokenErrorResponse_allowUiRequiredException_True() {
common4j/src/main/com/microsoft/identity/common/java/controllers/ExceptionAdapter.java:293
- There’s no test covering the path where
commandParametersis aBrokerInteractiveTokenCommandParametersandallowConvertToUiRequiredExceptionis true. Add a test to validate that branch.
if (isBrokerTokenCommandParameters(commandParameters)) {
common4j/src/main/com/microsoft/identity/common/java/jwt/JwtKeyUseTypes.kt:26
- [nitpick] The comment has a redundant phrase “to be used with for.” Consider rephrasing to something like: “Constants for JWT key use types for the
useclaim in the JWT header.”
* Constants for JWT key use types to be used with for "use" claim in JWT header.
Don't return UiRequiredException when the acquire token silent is for resource account. It's not expected to start interactive request for an RA account, so instead we return ServiceException.
On top of it, there will be broker side PR to handle interrupt internally if ATS fails with relevant errors. This will be separate change.
Fixes AB#3230426