From b0d4e500a82b2125b42382a4f0e202cc1f243cb0 Mon Sep 17 00:00:00 2001 From: Josh Cummings Date: Tue, 24 Nov 2020 14:39:00 -0700 Subject: [PATCH] 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 --- ...egatingJwtGrantedAuthoritiesConverter.java | 54 ++++++------ ...ingJwtGrantedAuthoritiesConverterTest.java | 79 ------------------ ...ngJwtGrantedAuthoritiesConverterTests.java | 82 +++++++++++++++++++ 3 files changed, 112 insertions(+), 103 deletions(-) delete mode 100644 oauth2/oauth2-resource-server/src/test/java/org/springframework/security/oauth2/server/resource/authentication/DelegatingJwtGrantedAuthoritiesConverterTest.java create mode 100644 oauth2/oauth2-resource-server/src/test/java/org/springframework/security/oauth2/server/resource/authentication/DelegatingJwtGrantedAuthoritiesConverterTests.java diff --git a/oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/authentication/DelegatingJwtGrantedAuthoritiesConverter.java b/oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/authentication/DelegatingJwtGrantedAuthoritiesConverter.java index 0bfd1b5dce..5e99547f67 100644 --- a/oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/authentication/DelegatingJwtGrantedAuthoritiesConverter.java +++ b/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> { - private final Collection>> converters = new HashSet<>(); + private final Collection>> 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>> converters) { - this.converters.addAll(converters); + public DelegatingJwtGrantedAuthoritiesConverter( + Collection>> 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>... converters) { - this(Arrays.asList(converters)); + public DelegatingJwtGrantedAuthoritiesConverter( + Converter>... 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}. + *

+ * 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 convert(Jwt source) { + public Collection convert(Jwt jwt) { Collection result = new LinkedHashSet<>(); - for (Converter> converter: this.converters) { - Collection authorities = converter.convert(source); + for (Converter> authoritiesConverter : this.authoritiesConverters) { + Collection authorities = authoritiesConverter.convert(jwt); if (authorities != null) { result.addAll(authorities); } @@ -77,4 +82,5 @@ public class DelegatingJwtGrantedAuthoritiesConverter implements Converter - 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") - ); - } -} diff --git a/oauth2/oauth2-resource-server/src/test/java/org/springframework/security/oauth2/server/resource/authentication/DelegatingJwtGrantedAuthoritiesConverterTests.java b/oauth2/oauth2-resource-server/src/test/java/org/springframework/security/oauth2/server/resource/authentication/DelegatingJwtGrantedAuthoritiesConverterTests.java new file mode 100644 index 0000000000..9bda7545ca --- /dev/null +++ b/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 authorities = converter.convert(jwt); + assertThat(authorityListToOrderedSet(authorities)).containsExactly("one"); + } + + @Test + public void convertWhenMultipleConvertersThenDuplicatesRemoved() { + Converter> one = (jwt) -> AuthorityUtils.createAuthorityList("one", "two"); + Converter> two = (jwt) -> AuthorityUtils.createAuthorityList("one", "three"); + DelegatingJwtGrantedAuthoritiesConverter composite = new DelegatingJwtGrantedAuthoritiesConverter(one, two); + Jwt jwt = TestJwts.jwt().build(); + Collection authorities = composite.convert(jwt); + assertThat(authorityListToOrderedSet(authorities)).containsExactly("one", "two", "three"); + } + + @Test + public void constructorWhenAuthoritiesConverterIsNullThenIllegalArgumentException() { + assertThatExceptionOfType(IllegalArgumentException.class) + .isThrownBy(() -> new DelegatingJwtGrantedAuthoritiesConverter( + (Collection>>) null)); + } + + private Collection authorityListToOrderedSet(Collection grantedAuthorities) { + Collection authorities = new LinkedHashSet<>(grantedAuthorities.size()); + for (GrantedAuthority authority : grantedAuthorities) { + authorities.add(authority.getAuthority()); + } + return authorities; + } + +}