-
Notifications
You must be signed in to change notification settings - Fork 148
Add APIs to add client capabilities and claims #929
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
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 adds APIs to support client capabilities and claims for the managed identity application, along with test coverage to verify token acquisition with claims.
- Introduces a new test case in ManagedIdentityTests.java to validate acquiring a token with claims.
- Updates ManagedIdentityParameters.java to include a claims field and modify the claims() method.
- Enhances ManagedIdentityApplication.java with a new builder method to set client capabilities.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
File | Description |
---|---|
msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/ManagedIdentityTests.java | Added a new parameterized test to verify token acquisition when claims are provided. |
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/ManagedIdentityParameters.java | Extended parameters to support claims and updated the associated methods accordingly. |
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/ManagedIdentityApplication.java | Added a clientCapabilities field and a new builder method to support client capabilities in the application. |
Comments suppressed due to low confidence (1)
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/ManagedIdentityApplication.java:119
- The Builder's clientCapabilities field is set but not explicitly propagated to the ManagedIdentityApplication instance in its constructor. Verify that the clientCapabilities value is assigned and used appropriately in the token acquisition logic.
return new ManagedIdentityApplication(this);
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/ManagedIdentityApplication.java
Outdated
Show resolved
Hide resolved
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/ManagedIdentityApplication.java
Outdated
Show resolved
Hide resolved
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/ManagedIdentityApplication.java
Outdated
Show resolved
Hide resolved
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/ManagedIdentityParameters.java
Show resolved
Hide resolved
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/ManagedIdentityParameters.java
Show resolved
Hide resolved
msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/ManagedIdentityTests.java
Outdated
Show resolved
Hide resolved
msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/ManagedIdentityTests.java
Show resolved
Hide resolved
msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/ManagedIdentityTests.java
Outdated
Show resolved
Hide resolved
msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/ManagedIdentityTests.java
Show resolved
Hide resolved
msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/ManagedIdentityTests.java
Show resolved
Hide resolved
The new features for client capabilities and claims looks great. Adding tests for edge cases (like empty or malformed JSON) will strengthen the code coverage further. Excellent job!!! |
nit: would be also good to have a tracking item in the PR description. |
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/ManagedIdentityApplication.java
Outdated
Show resolved
Hide resolved
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.
when claims are present, we need to bypass MSALs cache
Add APIs to the managed identity app and request to add client capabilities and claims. Bypass the cache when claims are passed.