Refresh tokens when request contains claims #811
Conversation
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AcquireTokenSilentSupplier.java
Outdated
Show resolved
Hide resolved
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AcquireTokenSilentSupplier.java
Show resolved
Hide resolved
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AcquireTokenSilentSupplier.java
Outdated
Show resolved
Hide resolved
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AcquireTokenSilentSupplier.java
Show resolved
Hide resolved
bgavrilMS
left a comment
bgavrilMS
left a comment
There was a problem hiding this comment.
Missing unit tests is primary concern.
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AcquireTokenSilentSupplier.java
Outdated
Show resolved
Hide resolved
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AcquireTokenSilentSupplier.java
Outdated
Show resolved
Hide resolved
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AcquireTokenSilentSupplier.java
Show resolved
Hide resolved
bgavrilMS
left a comment
bgavrilMS
left a comment
There was a problem hiding this comment.
More test coverage (unit test) needed.
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AcquireTokenSilentSupplier.java
Show resolved
Hide resolved
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AcquireTokenSilentSupplier.java
Outdated
Show resolved
Hide resolved
| User user = labUserProvider.getDefaultUser(AzureEnvironment.AZURE); | ||
|
|
||
| Set<String> scopes = new HashSet<>(); | ||
| PublicClientApplication pca = PublicClientApplication.builder( |
There was a problem hiding this comment.
I think unit tests should cover this not integration tests. Integration tests are expensive. There is no integration point that is worth testing unless you pass in some well-known claims and can assert the result.
Unit tests should cover:
- cca.AcquireTokenSilent
- cca.AcquireTokenForClient
- pca.AcquireTokenSilent
There was a problem hiding this comment.
In the latest commit I added extra tests using some dummy cache data, to show that a token is returned when using the AcquireTokenSilent API without claims and not returned when claims are added to the request.
I did not add a specific test for the implicit silent call in the 'cca.AcquireTokenForClient' scenario, because internally it reaches the exact same code as the explicit AcquireTokenSilent call and that behavior is covered by other tests.
|
Any updates on this @Avery-Dunn ? |
…tion-library-for-java into avdunn/claims-refresh-fix
| assertNotNull(result); | ||
| assertEquals("token", result.accessToken()); | ||
|
|
||
| ClaimsRequest cr = new ClaimsRequest(); |
There was a problem hiding this comment.
This is not good API experience ( I know it exists already).
Is there an overload where the claims are injected as a string? After all, this is what the app developers will use.|
Later edit: can we pls deprecate Claims|Request and associated APIs? And just keep claims(string) ?"
There was a problem hiding this comment.
For simplicity I'm not deprecating in this PR/release, but I created this task to get it deprecated in the next one: #855
| InteractiveRequestParameters parameters = InteractiveRequestParameters.builder(new URI("http://localhost:8080")) | ||
| .claimsChallenge("{\"id_token\":{\"auth_time\":{\"essential\":true}},\"access_token\":{\"auth_time\":{\"essential\":true},\"xms_cc\":{\"values\":[\"abc\"]}}}") | ||
| .claimsChallenge(TestConfiguration.CLAIMS_CHALLENGE) | ||
| .claims(cr) |
There was a problem hiding this comment.
Why both claims and claims channelge?
Can we deprecate this claims and ClaimsRequest object model?
There was a problem hiding this comment.
For simplicity I'm not deprecating in this PR/release, but I created this task to get it deprecated in the next one: #855
bgavrilMS
left a comment
bgavrilMS
left a comment
There was a problem hiding this comment.
Unsure about claims(string) and claimsrequest(CR) - propose to deprecate strongly typed model and it is inconsistent with other MSALs.
The first commit in this PR refactors the logic for determining when a refresh should be performed, since it had gotten complex as different scenarios and edge cases were introduced over the years. That refactor should not affect the existing refresh behavior.
The second commit adds the existence of the claims parameter as reason to refresh, as per #794. With this change app developers will no longer need to explicitly use the forceRefresh() API when dealing with tokens that have claims/client capabilities/etc.