Client Capabilities Support by Avery-Dunn · Pull Request #226 · AzureAD/microsoft-authentication-library-for-java

Avery-Dunn · 2020-04-30T15:49:53Z

Added support for client capabilities, as per #197

Main functionality is to create a new parameter that can be set as part of an authorization request, and this new parameter is merged into the existing claims parameter.

See AzureAD/microsoft-authentication-library-for-dotnet#1641 for a related implementation in the .NET library

sangonzal

Overall looks good.

sangonzal · 2020-05-04T15:01:09Z

src/integrationtest/java/com.microsoft.aad.msal4j/AuthorizationCodeIT.java

+        Assert.assertNotNull(result.accessToken());
+        Assert.assertNotNull(result.idToken());
+        Assert.assertEquals(user.getUpn(), result.account().username());
+    }


I think you should be able to assert that the id token that gets returned has the claims that you requested.

src/integrationtest/java/com.microsoft.aad.msal4j/AuthorizationCodeIT.java

src/main/java/com/microsoft/aad/msal4j/JsonHelper.java

src/integrationtest/java/com.microsoft.aad.msal4j/AuthorizationCodeIT.java

src/main/java/com/microsoft/aad/msal4j/AuthorizationRequestUrlParameters.java

bgavrilMS · 2020-05-07T09:49:51Z

Although slightly off-topic, I encountered the following issue while implementing this on .NET - it looks like there is an expectation that if the user configures claims, then the token cache is bypassed (AcquireTokenSilent will behave as if ForceRefres=true). But if they configure only client capabilities, then the token cache is not bypassed. I believe this is how Android team also implemented this.

bgavrilMS

Send claims to /token endpoint as well
If claims are configured, token cache must be bypassed. If client capabilities are configured, it should not.

SomkaPe · 2020-05-11T20:38:19Z

@bgavrilMS Not sure about sending claims to token endpoint - according to spec https://openid.net/specs/openid-connect-core-1_0.html#ClaimsParameter
it is an optional parameter for Authorization Request

Not sure about bypassing cache when "claims" is used - i would imagine that most of applications use claims parameter always or not use it at all.
So, applications configured with "claims" will never reuse AT ? Sounds wrong

bgavrilMS

I think we ironed out all the details. I'm afraid I don't have time to do review this in detail though, so removing my review so as not to block the PR>

sangonzal

Requirements have changed multiple times during the life of the this PR, so if it would be helpful lets schedule a meeting between you, @SomkaPe, and I so we can clearly define the requirements.

src/main/java/com/microsoft/aad/msal4j/AuthorizationRequestUrlParameters.java

src/integrationtest/java/com.microsoft.aad.msal4j/AuthorizationCodeIT.java

src/main/java/com/microsoft/aad/msal4j/AuthorizationRequestUrlParameters.java

src/test/java/com/microsoft/aad/msal4j/AuthorizationRequestUrlParametersTest.java

…tion-library-for-java into avdunn/claims-and-capabilities � Conflicts: � src/integrationtest/java/com.microsoft.aad.msal4j/AuthorizationCodeIT.java � src/main/java/com/microsoft/aad/msal4j/AbstractClientApplicationBase.java � src/main/java/com/microsoft/aad/msal4j/AuthenticationErrorCode.java

src/integrationtest/java/com.microsoft.aad.msal4j/AuthorizationCodeIT.java

src/main/java/com/microsoft/aad/msal4j/AbstractClientApplicationBase.java

src/test/java/com/microsoft/aad/msal4j/AuthorizationRequestUrlParametersTest.java

These changes are covered under another set of requested changes

sangonzal

Avery-Dunn added 3 commits March 18, 2020 10:48

Minor typo fix

9c84702

Add client-capabilities optional parameter

bf9ad41

Add tests for client-capabilities

2da4bb7

Avery-Dunn added Enhancement A request or suggestion to improve some aspect of the library SDK-Consistency Items that deal with consistency between all MSALs labels Apr 30, 2020

Avery-Dunn requested review from SomkaPe, henrik-me and sangonzal April 30, 2020 15:49

Avery-Dunn linked an issue Apr 30, 2020 that may be closed by this pull request

[Feature Request] Client capabilities support #197

Closed

Better iterator handling

e7d38b8

sangonzal reviewed May 4, 2020

View reviewed changes

src/integrationtest/java/com.microsoft.aad.msal4j/AuthorizationCodeIT.java Outdated Show resolved Hide resolved

Avery-Dunn added 4 commits May 5, 2020 14:13

Refactor claims-and-capabilities test case

7102501

Code cleanup

47eb200

Refactor variable to TestConstants

5378764

Remove need for null checks

22d940a

henrik-me requested a review from bgavrilMS May 7, 2020 05:02

bgavrilMS reviewed May 7, 2020

View reviewed changes

src/main/java/com/microsoft/aad/msal4j/AuthorizationRequestUrlParameters.java Outdated Show resolved Hide resolved

bgavrilMS previously requested changes May 7, 2020

View reviewed changes

Avery-Dunn added 3 commits May 13, 2020 14:45

Refactor claims and capabilities parameters

ddaf080

Add claims and capabilities parameters

d890075

Fix capabilities test

b3f4146

bgavrilMS reviewed May 15, 2020

View reviewed changes

Handle null exception

c2f97c3

sangonzal suggested changes May 20, 2020

View reviewed changes

src/main/java/com/microsoft/aad/msal4j/AuthorizationRequestUrlParameters.java Outdated Show resolved Hide resolved

src/main/java/com/microsoft/aad/msal4j/AuthorizationRequestUrlParameters.java Outdated Show resolved Hide resolved

SomkaPe reviewed May 20, 2020

View reviewed changes

Avery-Dunn added 2 commits May 20, 2020 15:59

Move client capabilities to application level

482d2cc

Improve claims/capabilities test coverage

713c497

Avery-Dunn added 3 commits May 21, 2020 12:10

Disable claims/capabilities test

122789f

Fix merge conflict

a297942

SomkaPe reviewed May 22, 2020

View reviewed changes

Deprecate claims parameter and refactor into claimsChallenge

3af478d

Avery-Dunn requested a review from sangonzal May 26, 2020 15:48

Add client capabilities to token endpoint request

9edf87c

SomkaPe approved these changes May 26, 2020

View reviewed changes

sangonzal approved these changes May 26, 2020

View reviewed changes

Avery-Dunn merged commit 2fd7db4 into dev May 26, 2020

Avery-Dunn deleted the avdunn/claims-and-capabilities branch October 13, 2020 18:24

Conversation

Avery-Dunn commented Apr 30, 2020

Uh oh!

sangonzal left a comment

Choose a reason for hiding this comment

Uh oh!

sangonzal May 4, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bgavrilMS commented May 7, 2020

Uh oh!

bgavrilMS left a comment

Choose a reason for hiding this comment

Uh oh!

SomkaPe commented May 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bgavrilMS left a comment

Choose a reason for hiding this comment

Uh oh!

sangonzal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sangonzal left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

SomkaPe commented May 11, 2020 •

edited

Loading