Added OAuth2TokenAttributes to wrap attributes #7014

clementkng · 2019-06-18T08:02:20Z

To simplify access to OAuth 2.0 token attributes

jzheaux

@clementkng thanks for this! I've left a few pieces of feedback inline to align the PR with the ticket description as well as team standards.

jzheaux · 2019-06-18T17:19:51Z

...auth2-core/src/main/java/org/springframework/security/oauth2/core/OAuth2TokenAttributes.java

+public class OAuth2TokenAttributes {
+	private Map<String, Object> attributes;
+
+	public OAuth2TokenAttributes() {


It doesn't really make sense to have one constructor that allows for empty attributes and another constructor that doesn't.

For now, let's leave the default constructor out - we can add it in later if it's needed.

I added the default constructor because constructorWhenAttributesAreNullOrEmptyThenThrowsException() tests that an empty attributes map throws an illegal argument exception, but the setup for all the tests originally set attributes to an empty map (which is OK because it doesn't go through the constructor for OAuth2IntrospectionAuthenticationToken). One potential alternative is to have the ``OAuth2IntrospectionAuthenticationToken.attributesmethod do the checking that the map is not empty and theOAuth2TokenAttributes` are not null. Would this be better?

...auth2-core/src/main/java/org/springframework/security/oauth2/core/OAuth2TokenAttributes.java

jzheaux · 2019-06-18T18:38:24Z

...auth2-core/src/main/java/org/springframework/security/oauth2/core/OAuth2TokenAttributes.java

+/**
+ *
+ * @author Clement Ng
+ *


Please add @since 5.2. Also, I'm sure you are already planning on adding a description of the class, but just calling it out here.

jzheaux · 2019-06-18T18:38:44Z

...auth2-core/src/main/java/org/springframework/security/oauth2/core/OAuth2TokenAttributes.java

+		return attributes;
+	}
+
+	public void putAttributes(String key, String value) {


Let's please leave the class immutable.

jzheaux · 2019-06-18T18:39:02Z

...auth2-core/src/main/java/org/springframework/security/oauth2/core/OAuth2TokenAttributes.java

+
+	public OAuth2TokenAttributes(Map<String, Object> attributes) {
+		Assert.notEmpty(attributes, "attributes cannot be empty");
+		this.attributes = attributes;


Please make a copy of the attributes and make it an immutable copy.

If we make the attributes map immutable via Collections.unmodifiableMap, I believe the setUp() method in the OAuth2IntrospectionAuthenticationTokenTests will have to be collapsed into the creation of the instance variables (since right now it puts values into an OAuth2.0 attributes map, but now we need the map to be prepopulated w/ data before constructing the OAuth2.0TokenAttributes). Would this be OK?

Yes, that is fine. We do want to be careful when modifying tests, but it is better than breaking encapsulation.

jzheaux · 2019-06-18T18:40:43Z

...auth2-core/src/main/java/org/springframework/security/oauth2/core/OAuth2TokenAttributes.java

+ * @author Clement Ng
+ *
+ */
+public class OAuth2TokenAttributes {


It'd be great to have a test class for OAuth2TokenAttributes.

I was considering it, but wasn't sure what would be tested w/i the test class that wasn't already covered in OAuth2IntrospectionAuthenticationTokenTests. Should I pull some attribute-specific tests from OAuth2IntrospectionAuthenticationTokenTests?

Fair point, we can add tests for this class if and when it becomes more sophisticated.

clementkng · 2019-06-25T05:06:56Z

Thanks @jzheaux I've addressed the feedback above. Let me know if you have any additional comments!

jzheaux

@clementkng Thanks for the revisions, they look good! I've left some more feedback inline.

jzheaux · 2019-06-26T03:26:55Z

...2-core/src/test/java/org/springframework/security/oauth2/core/OAuth2TokenAttributesTest.java

+ *
+ * @author Clement Ng
+ */
+public class OAuth2TokenAttributesTest {


I think this is a stray file, let's remove it.

jzheaux · 2019-06-26T03:27:41Z

...auth2-core/src/main/java/org/springframework/security/oauth2/core/OAuth2TokenAttributes.java

+ * @since 5.2
+ */
+public final class OAuth2TokenAttributes {
+	private Map<String, Object> attributes;


Since it's intended for this to be immutable, let's make it final.

jzheaux · 2019-06-26T03:28:41Z

...auth2-core/src/main/java/org/springframework/security/oauth2/core/OAuth2TokenAttributes.java

+import java.util.Map;
+
+/**
+ * A domain object that wraps around the attributes in an {@link org.springframework.security.core.Authentication}


Let's mention OAuth 2.0 in this description. Maybe:

Suggested change

* A domain object that wraps around the attributes in an {@link org.springframework.security.core.Authentication}

* A domain object that wraps the attributes of an OAuth 2.0 token

...auth2-core/src/main/java/org/springframework/security/oauth2/core/OAuth2TokenAttributes.java

jzheaux · 2019-06-26T03:33:36Z

...urity/oauth2/server/resource/authentication/OAuth2IntrospectionAuthenticationTokenTests.java

-		this.attributes.put(CLIENT_ID, "client_id");
-		this.attributes.put(USERNAME, "username");
-	}
+	private Map<String, Object> attributesMap = new HashMap<String, Object>() {{


While this is a clever construct for initializing a Map, it's not used across the codebase. So, just for consistency, let's please still use the setUp method.

Sounds good, I didn't know @before would allow setUp to populate the attributesMap before the OAuth2TokenAttributes attributes was made.

jzheaux · 2019-06-26T03:34:16Z

...urity/oauth2/server/resource/authentication/OAuth2IntrospectionAuthenticationTokenTests.java

@@ -76,7 +75,7 @@ public void getNameWhenHasNoSubjectThenReturnsNull() {
 	public void getNameWhenTokenHasUsernameThenReturnsUsernameAttribute() {
 		OAuth2IntrospectionAuthenticationToken authenticated =
 				new OAuth2IntrospectionAuthenticationToken(this.token, this.attributes, Collections.emptyList());
-		assertThat(authenticated.getName()).isEqualTo(this.attributes.get(SUBJECT));
+		assertThat(authenticated.getName()).isEqualTo(this.attributes.getAttributes().get(SUBJECT));


I think you could call this.attributes.getAttribute(SUBJECT), right?

To simplify access to OAuth 2.0 token attributes Fixes spring-projectsgh-6498

clementkng · 2019-06-26T08:13:16Z

@jzheaux Thanks for the feedback, I've incorporated it into the PR.

jzheaux · 2019-06-26T16:55:39Z

Looks great, @clementkng. The ticket outlines that the sample should also be updated - would you mind rolling that into this PR so that we can close out the ticket all at once?

Placing that in a separate commit probably makes sense, just with the annotation "Issue: gh-6498" instead of "Fixes: gh-6498".

clementkng · 2019-06-26T19:08:55Z

@jzheaux I'm not sure how to update a ticket by rolling that into the PR. I assume you mean check off the ticket items, but how do I connect that to the PR via a commit?

jzheaux · 2019-06-26T19:27:01Z

@clementkng, sorry, what I mean is just add a commit to this PR that updates the oauth2resourceserver-opaque sample. So after making the appropriate changes to the sample, you'd do:

git add samples/
git commit
git push origin gh-6498-OAuth2TokenAttributes-class

Where your commit message would be something like:

Update Opaque Token Sample

Issue: gh-6948

clementkng · 2019-06-26T21:46:10Z

@jzheaux Sorry, I think I misunderstood the item on that ticket. I've looked through the sample folders w/i the oauth2resourceserver-opaque (as well as the entire directory itself), and can't seem to find usages of OAuth2IntrospectionAuthenticationToken to update. I quickly scanned through the README.adoc as well and couldn't find anything to update. I'm not sure what the appropriate changes to the sample should be.

jzheaux · 2019-06-26T22:58:20Z

@clementkng no problem, sorry that's not so clear, let's see if I can help.

So, we want to change this here:

- public String index(@AuthenticationPrincipal(expression="['sub']") String subject) {
+ public String index(@AuthenticationPrincipal OAuth2TokenAttributes attributes) {

In OAuth2ResourceServerController.

clementkng · 2019-06-26T23:48:49Z

@jzheaux Just to check my understanding, once we add the attributes argument, we have the method return String.format("Hello, %s!", attributes.getAttribute("sub"));, which is to demonstrate the pattern we wanted to achieve at the end of the Extra Information section of the original ticket?

jzheaux · 2019-06-27T16:28:25Z

Correct, @clementkng

clementkng · 2019-06-27T19:06:02Z

@jzheaux Got it, there's currently an NullPointerException at OAuth2ResourceServerApplicationITests.java:58 and an AssertionError at NimbusReactiveJwtDecoderTests.java:122 that I'm going to dig into.

clementkng · 2019-06-28T00:48:02Z

@jzheaux is there a way to ./gradlew run individual integration tests? I'm currently having a hard time rooting out the issues in the sample.

clementkng · 2019-06-28T01:57:39Z

Actually, I was able to figure out how to run the individual test. I've been able to narrow the issue down to the fact that the OAuth2TokenAttributes attributes being passed into the index method is null, and was able to see that the test fails when .with(bearerToken(this.noScopesToken)) is called. For now, I'm not quite able to link up the two b/c I'm not very familiar w/ how the resource server and integration test code works. Would you happen to have any advice on how to parse this?

jzheaux · 2019-06-28T17:32:46Z

When you use @AuthenticationPrincipal SomeType someObject, and the value comes back as null, it means that either the user is not authenticated or that SomeType is not the actual type of the principal.

Since that endpoint requires authentication, we know that it's because the principal is not actually of type OAuth2TokenAttributes.

So, it's probably best to check the OAuth2IntrospectionAuthenticationToken to see what is being set as the principal value. The principal needs to be of type OAuth2TokenAttributes.

clementkng · 2019-06-28T22:14:21Z

@jzheaux Thanks for explaining! My understanding was that changing the constructor of OAuth2IntrospectionAuthenticationToken to take in a OAuth2TokenAttributes rather than a Map was enough to ensure that the principal value taken in was an OAuth2TokenAttributes. Am I missing something there?

Another thing I noticed was that the tests still pass w/ the old @AuthenticationPrincipal access pattern (expression="['sub']") String subject), but should this pass in a value of null since we've changed the aforementioned SomeType to be OAuth2TokenAttributes? If so, this might indicate something else is wrong.

jzheaux · 2019-07-01T11:27:14Z

@clementkng, if you take a look at the super constructor that's invoked, you'll see that the second parameter matches up with the principal. Because the code sends the result of the attributes method call as that second parameter, a Map is getting set as the principal.

The reason that the ['sub'] expression continues to work is because the principal hasn't changed yet from a Map.

Issue: spring-projectsgh-6498

clementkng · 2019-07-02T00:25:58Z

@jzheaux I see, thanks for pointing that out to me. I was able to update the sample. Please let me know if you have any other feedback!

jzheaux · 2019-07-02T13:47:01Z

Thanks, @clementkng, this is now merged into master. And congrats on another great contribution!

clementkng · 2019-07-02T17:15:00Z

@jzheaux Thanks for your help as well! When I first started out on this ticket, I had no idea how much the OAuth2IntrospectionAuthenticationToken class was involved in the code, so thanks for guiding me throughout trickier parts of the ticket.

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 18, 2019

clementkng mentioned this pull request Jun 18, 2019

Simplify access to OAuth 2.0 token attributes #6498

Closed

3 tasks

rwinch requested a review from jzheaux June 18, 2019 17:15

rwinch assigned jzheaux Jun 18, 2019

jzheaux requested changes Jun 18, 2019

View reviewed changes

clementkng force-pushed the gh-6498-OAuth2TokenAttributes-class branch from 148cd99 to e58b959 Compare June 25, 2019 04:30

jzheaux requested changes Jun 26, 2019

View reviewed changes

jzheaux added in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: duplicate A duplicate of another issue type: improvement and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 26, 2019

jzheaux added this to the 5.2.0.RC1 milestone Jun 26, 2019

Added OAuth2TokenAttributes to wrap attributes

b4cf8d8

To simplify access to OAuth 2.0 token attributes Fixes spring-projectsgh-6498

clementkng force-pushed the gh-6498-OAuth2TokenAttributes-class branch from e58b959 to b4cf8d8 Compare June 26, 2019 06:54

Update Opaque Token Sample and tests

0e0865f

Issue: spring-projectsgh-6498

jzheaux approved these changes Jul 2, 2019

View reviewed changes

jzheaux merged commit cd54808 into spring-projects:master Jul 2, 2019

	* A domain object that wraps around the attributes in an {@link org.springframework.security.core.Authentication}
	* A domain object that wraps the attributes of an OAuth 2.0 token

Added OAuth2TokenAttributes to wrap attributes #7014

Added OAuth2TokenAttributes to wrap attributes #7014

Uh oh!

Conversation

clementkng commented Jun 18, 2019

Uh oh!

jzheaux left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clementkng commented Jun 25, 2019

Uh oh!

jzheaux left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clementkng commented Jun 26, 2019

Uh oh!

jzheaux commented Jun 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clementkng commented Jun 26, 2019

Uh oh!

jzheaux commented Jun 26, 2019

Uh oh!

clementkng commented Jun 26, 2019

Uh oh!

jzheaux commented Jun 26, 2019

Uh oh!

clementkng commented Jun 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jzheaux commented Jun 27, 2019

Uh oh!

clementkng commented Jun 27, 2019

Uh oh!

clementkng commented Jun 28, 2019

Uh oh!

clementkng commented Jun 28, 2019

Uh oh!

jzheaux commented Jun 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clementkng commented Jun 28, 2019

Uh oh!

jzheaux commented Jul 1, 2019

Uh oh!

clementkng commented Jul 2, 2019

jzheaux commented Jun 26, 2019 •

edited

Loading

clementkng commented Jun 26, 2019 •

edited

Loading

jzheaux commented Jun 28, 2019 •

edited

Loading