Polish Add DelegatingJwtGrantedAuthoritiesConverter

- Adjusted internal logic to follow DelegatingOAuth2TokenValidator
- Changed JavaDoc to align more closely with
JwtGrantedAuthoritiesConverter
- Polished test names to follow Spring Security naming convention
- Updated test class name to follow Spring Security naming convention
- Polished tests to use TestJwts
- Added tests to address additional use cases

Closes gh-7596

oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/authentication/DelegatingJwtGrantedAuthoritiesConverter.java

@@ -16,60 +16,65 @@
 
 package org.springframework.security.oauth2.server.resource.authentication;
 
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
-import java.util.HashSet;
 import java.util.LinkedHashSet;
 
 import org.springframework.core.convert.converter.Converter;
 import org.springframework.security.core.GrantedAuthority;
 import org.springframework.security.oauth2.jwt.Jwt;
+import org.springframework.util.Assert;
 
 /**
- * Implementation of {@link Converter} that wraps multiple {@link Converter} instances into one.
+ * A {@link Jwt} to {@link GrantedAuthority} {@link Converter} that is a composite of
+ * converters.
  *
  * @author Laszlo Stahorszki
+ * @author Josh Cummings
  * @since 5.5
+ * @see org.springframework.security.oauth2.server.resource.authentication.JwtGrantedAuthoritiesConverter
  */
 public class DelegatingJwtGrantedAuthoritiesConverter implements Converter<Jwt, Collection<GrantedAuthority>> {
 
-	private final Collection<Converter<Jwt, Collection<GrantedAuthority>>> converters = new HashSet<>();
+	private final Collection<Converter<Jwt, Collection<GrantedAuthority>>> authoritiesConverters;
 
 	/**
-	 * Constructs a {@link DelegatingJwtGrantedAuthoritiesConverter} using the provided {@link Collection} of
-	 * {@link Converter}s
-	 *
-	 * @param converters the {@link Collection} of {@link Converter}s to use
+	 * Constructs a {@link DelegatingJwtGrantedAuthoritiesConverter} using the provided
+	 * {@link Collection} of {@link Converter}s
+	 * @param authoritiesConverters the {@link Collection} of {@link Converter}s to use
 	 */
-	public DelegatingJwtGrantedAuthoritiesConverter(Collection<Converter<Jwt, Collection<GrantedAuthority>>> converters) {
-		this.converters.addAll(converters);
+	public DelegatingJwtGrantedAuthoritiesConverter(
+			Collection<Converter<Jwt, Collection<GrantedAuthority>>> authoritiesConverters) {
+		Assert.notNull(authoritiesConverters, "authoritiesConverters cannot be null");
+		this.authoritiesConverters = new ArrayList<>(authoritiesConverters);
 	}
 
 	/**
-	 * Constructs a {@link DelegatingJwtGrantedAuthoritiesConverter} using the provided array of
-	 * {@link Converter}s
-	 *
-	 * @param converters the array of {@link Converter}s to use
+	 * Constructs a {@link DelegatingJwtGrantedAuthoritiesConverter} using the provided
+	 * array of {@link Converter}s
+	 * @param authoritiesConverters the array of {@link Converter}s to use
 	 */
 	@SafeVarargs
-	public DelegatingJwtGrantedAuthoritiesConverter(Converter<Jwt, Collection<GrantedAuthority>>... converters) {
-		this(Arrays.asList(converters));
+	public DelegatingJwtGrantedAuthoritiesConverter(
+			Converter<Jwt, Collection<GrantedAuthority>>... authoritiesConverters) {
+		this(Arrays.asList(authoritiesConverters));
 	}
 
 	/**
-	 * Collects the {@link Collection} of authorities from the provided {@link Jwt} token. The method iterates through
-	 * all the {@link Converter}s provided during construction and returns the union of {@link GrantedAuthority}s
-	 * they extract.
-	 * @param source the source object to convert, which must be an instance of {@code S} (never {@code null})
-	 * @return the converted object, which must be an instance of {@code T} (potentially {@code null})
-	 * @throws IllegalArgumentException if the source cannot be converted to the desired target type
+	 * Extract {@link GrantedAuthority}s from the given {@link Jwt}.
+	 * <p>
+	 * The authorities are extracted from each delegated {@link Converter} one at a time.
+	 * For each converter, its authorities are added in order, with duplicates removed.
+	 * @param jwt The {@link Jwt} token
+	 * @return The {@link GrantedAuthority authorities} read from the token scopes
 	 */
 	@Override
-	public Collection<GrantedAuthority> convert(Jwt source) {
+	public Collection<GrantedAuthority> convert(Jwt jwt) {
 		Collection<GrantedAuthority> result = new LinkedHashSet<>();
 
-		for (Converter<Jwt, Collection<GrantedAuthority>> converter: this.converters) {
-			Collection<GrantedAuthority> authorities = converter.convert(source);
+		for (Converter<Jwt, Collection<GrantedAuthority>> authoritiesConverter : this.authoritiesConverters) {
+			Collection<GrantedAuthority> authorities = authoritiesConverter.convert(jwt);
 			if (authorities != null) {
 				result.addAll(authorities);
 			}
@@ -77,4 +82,5 @@ public class DelegatingJwtGrantedAuthoritiesConverter implements Converter<Jwt,
 
 		return result;
 	}
+
 }

oauth2/oauth2-resource-server/src/test/java/org/springframework/security/oauth2/server/resource/authentication/DelegatingJwtGrantedAuthoritiesConverterTest.java

@@ -1,79 +0,0 @@
-/*
- * Copyright 2002-2020 the original author or authors.
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *      https://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-package org.springframework.security.oauth2.server.resource.authentication;
-
-import java.util.Arrays;
-import java.util.Collections;
-import java.util.Map;
-import java.util.stream.Collectors;
-
-import org.junit.Test;
-
-import org.springframework.security.core.authority.SimpleGrantedAuthority;
-import org.springframework.security.oauth2.jwt.Jwt;
-
-import static org.assertj.core.api.Assertions.assertThat;
-
-/**
- * Tests for verifying {@link DelegatingJwtGrantedAuthoritiesConverter}
- *
- * @author Laszlo Stahorszki
- */
-public class DelegatingJwtGrantedAuthoritiesConverterTest {
-
-	@Test
-	public void convertNoConverters() {
-		DelegatingJwtGrantedAuthoritiesConverter subject = new DelegatingJwtGrantedAuthoritiesConverter();
-
-		assertThat(subject.convert(Jwt.withTokenValue("some-token-value")
-				.header("header", "value")
-				.claim("claim", "value")
-				.build())).isEmpty();
-	}
-
-	@Test
-	public void convert() {
-		DelegatingJwtGrantedAuthoritiesConverter subject = new DelegatingJwtGrantedAuthoritiesConverter(((source) ->
-				Collections.singletonList(new SimpleGrantedAuthority(source.getClaim("claim")))));
-
-		assertThat(subject.convert(Jwt.withTokenValue("some-token-value")
-				.header("header", "value")
-				.claim("claim", "value")
-				.build())).containsExactlyInAnyOrder(new SimpleGrantedAuthority("value"));
-	}
-
-	@Test
-	public void convertMultipleConverters() {
-		DelegatingJwtGrantedAuthoritiesConverter subject = new DelegatingJwtGrantedAuthoritiesConverter(
-				(source) -> Collections.singletonList(new SimpleGrantedAuthority(source.getClaim("claim"))),
-				(source) -> Arrays.stream(source.getHeaders().entrySet().toArray(new Map.Entry[]{}))
-						.map(Map.Entry::getValue)
-						.map(Object::toString)
-						.map(SimpleGrantedAuthority::new)
-						.collect(Collectors.toList()));
-
-		assertThat(subject.convert(Jwt.withTokenValue("some-token-value")
-				.header("header", "value")
-				.header("header2", "value2")
-				.claim("claim", "value3")
-				.build())).containsExactlyInAnyOrder(
-						new SimpleGrantedAuthority("value"),
-						new SimpleGrantedAuthority("value2"),
-						new SimpleGrantedAuthority("value3")
-		);
-	}
-}

oauth2/oauth2-resource-server/src/test/java/org/springframework/security/oauth2/server/resource/authentication/DelegatingJwtGrantedAuthoritiesConverterTests.java

@@ -0,0 +1,82 @@
+/*
+ * Copyright 2002-2020 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.springframework.security.oauth2.server.resource.authentication;
+
+import java.util.Collection;
+import java.util.LinkedHashSet;
+
+import org.junit.Test;
+
+import org.springframework.core.convert.converter.Converter;
+import org.springframework.security.core.GrantedAuthority;
+import org.springframework.security.core.authority.AuthorityUtils;
+import org.springframework.security.oauth2.jwt.Jwt;
+import org.springframework.security.oauth2.jwt.TestJwts;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
+
+/**
+ * Tests for verifying {@link DelegatingJwtGrantedAuthoritiesConverter}
+ *
+ * @author Laszlo Stahorszki
+ * @author Josh Cummings
+ */
+public class DelegatingJwtGrantedAuthoritiesConverterTests {
+
+	@Test
+	public void convertWhenNoConvertersThenNoAuthorities() {
+		DelegatingJwtGrantedAuthoritiesConverter converter = new DelegatingJwtGrantedAuthoritiesConverter();
+		Jwt jwt = TestJwts.jwt().build();
+		assertThat(converter.convert(jwt)).isEmpty();
+	}
+
+	@Test
+	public void convertWhenConverterThenAuthorities() {
+		DelegatingJwtGrantedAuthoritiesConverter converter = new DelegatingJwtGrantedAuthoritiesConverter(
+				((jwt) -> AuthorityUtils.createAuthorityList("one")));
+		Jwt jwt = TestJwts.jwt().build();
+		Collection<GrantedAuthority> authorities = converter.convert(jwt);
+		assertThat(authorityListToOrderedSet(authorities)).containsExactly("one");
+	}
+
+	@Test
+	public void convertWhenMultipleConvertersThenDuplicatesRemoved() {
+		Converter<Jwt, Collection<GrantedAuthority>> one = (jwt) -> AuthorityUtils.createAuthorityList("one", "two");
+		Converter<Jwt, Collection<GrantedAuthority>> two = (jwt) -> AuthorityUtils.createAuthorityList("one", "three");
+		DelegatingJwtGrantedAuthoritiesConverter composite = new DelegatingJwtGrantedAuthoritiesConverter(one, two);
+		Jwt jwt = TestJwts.jwt().build();
+		Collection<GrantedAuthority> authorities = composite.convert(jwt);
+		assertThat(authorityListToOrderedSet(authorities)).containsExactly("one", "two", "three");
+	}
+
+	@Test
+	public void constructorWhenAuthoritiesConverterIsNullThenIllegalArgumentException() {
+		assertThatExceptionOfType(IllegalArgumentException.class)
+				.isThrownBy(() -> new DelegatingJwtGrantedAuthoritiesConverter(
+						(Collection<Converter<Jwt, Collection<GrantedAuthority>>>) null));
+	}
+
+	private Collection<String> authorityListToOrderedSet(Collection<GrantedAuthority> grantedAuthorities) {
+		Collection<String> authorities = new LinkedHashSet<>(grantedAuthorities.size());
+		for (GrantedAuthority authority : grantedAuthorities) {
+			authorities.add(authority.getAuthority());
+		}
+		return authorities;
+	}
+
+}