From e554547593784f431723bb9c9f6a4ef9b77099c5 Mon Sep 17 00:00:00 2001 From: Joe Grandja Date: Mon, 8 Jul 2019 15:18:37 -0400 Subject: [PATCH] Revert Map constructor for InMemoryReactiveClientRegistrationRepository This commit reverts f6414e9a52f6a66dc8d21c0455c0b9ead7edc520 and partial revert of e1b095df3260c45c53408ef0a3360a7aa7c5073b. NOTE: InMemoryReactiveClientRegistrationRepository should not expose a Map constructor as it would allow the caller to pass in a 'distributed' (remote) Map, which would result in a blocking I/O operation. --- .../InMemoryClientRegistrationRepository.java | 27 ++++++------ ...yReactiveClientRegistrationRepository.java | 36 ++++++++-------- ...moryClientRegistrationRepositoryTests.java | 11 ++--- ...tiveClientRegistrationRepositoryTests.java | 43 +++++-------------- 4 files changed, 48 insertions(+), 69 deletions(-) diff --git a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/registration/InMemoryClientRegistrationRepository.java b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/registration/InMemoryClientRegistrationRepository.java index d1e8904242..456c68be7c 100644 --- a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/registration/InMemoryClientRegistrationRepository.java +++ b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/registration/InMemoryClientRegistrationRepository.java @@ -18,13 +18,16 @@ package org.springframework.security.oauth2.client.registration; import org.springframework.util.Assert; import java.util.Arrays; -import java.util.Collection; import java.util.Collections; import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.concurrent.ConcurrentMap; import java.util.function.Function; -import java.util.stream.Collectors; +import java.util.stream.Collector; + +import static java.util.stream.Collectors.collectingAndThen; +import static java.util.stream.Collectors.toConcurrentMap; /** * A {@link ClientRegistrationRepository} that stores {@link ClientRegistration}(s) in-memory. @@ -37,7 +40,6 @@ import java.util.stream.Collectors; * @see ClientRegistration */ public final class InMemoryClientRegistrationRepository implements ClientRegistrationRepository, Iterable { - private final Map registrations; /** @@ -46,8 +48,7 @@ public final class InMemoryClientRegistrationRepository implements ClientRegistr * @param registrations the client registration(s) */ public InMemoryClientRegistrationRepository(ClientRegistration... registrations) { - Assert.notEmpty(registrations, "registrations cannot be empty"); - this.registrations = createClientRegistrationIdToClientRegistration(Arrays.asList(registrations)); + this(Arrays.asList(registrations)); } /** @@ -56,7 +57,14 @@ public final class InMemoryClientRegistrationRepository implements ClientRegistr * @param registrations the client registration(s) */ public InMemoryClientRegistrationRepository(List registrations) { - this(createClientRegistrationIdToClientRegistration(registrations)); + this(createRegistrationsMap(registrations)); + } + + private static Map createRegistrationsMap(List registrations) { + Assert.notEmpty(registrations, "registrations cannot be empty"); + Collector> collector = + toConcurrentMap(ClientRegistration::getRegistrationId, Function.identity()); + return registrations.stream().collect(collectingAndThen(collector, Collections::unmodifiableMap)); } /** @@ -71,13 +79,6 @@ public final class InMemoryClientRegistrationRepository implements ClientRegistr this.registrations = registrations; } - private static Map createClientRegistrationIdToClientRegistration(Collection registrations) { - Assert.notNull(registrations, "registrations cannot be null"); - return Collections.unmodifiableMap(registrations.stream() - .peek(registration -> Assert.notNull(registration, "registrations cannot contain null values")) - .collect(Collectors.toMap(ClientRegistration::getRegistrationId, Function.identity()))); - } - @Override public ClientRegistration findByRegistrationId(String registrationId) { Assert.hasText(registrationId, "registrationId cannot be empty"); diff --git a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/registration/InMemoryReactiveClientRegistrationRepository.java b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/registration/InMemoryReactiveClientRegistrationRepository.java index 9d647a153e..c93a93e91c 100644 --- a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/registration/InMemoryReactiveClientRegistrationRepository.java +++ b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/registration/InMemoryReactiveClientRegistrationRepository.java @@ -18,6 +18,11 @@ package org.springframework.security.oauth2.client.registration; import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.function.Function; +import java.util.stream.Collectors; + +import org.springframework.util.Assert; +import org.springframework.util.ConcurrentReferenceHashMap; import reactor.core.publisher.Mono; @@ -25,7 +30,6 @@ import reactor.core.publisher.Mono; * A Reactive {@link ClientRegistrationRepository} that stores {@link ClientRegistration}(s) in-memory. * * @author Rob Winch - * @author Vedran Pavic * @since 5.1 * @see ClientRegistrationRepository * @see ClientRegistration @@ -33,7 +37,7 @@ import reactor.core.publisher.Mono; public final class InMemoryReactiveClientRegistrationRepository implements ReactiveClientRegistrationRepository, Iterable { - private final InMemoryClientRegistrationRepository delegate; + private final Map clientIdToClientRegistration; /** * Constructs an {@code InMemoryReactiveClientRegistrationRepository} using the provided parameters. @@ -41,7 +45,12 @@ public final class InMemoryReactiveClientRegistrationRepository * @param registrations the client registration(s) */ public InMemoryReactiveClientRegistrationRepository(ClientRegistration... registrations) { - this.delegate = new InMemoryClientRegistrationRepository(registrations); + Assert.notEmpty(registrations, "registrations cannot be empty"); + this.clientIdToClientRegistration = new ConcurrentReferenceHashMap<>(); + for (ClientRegistration registration : registrations) { + Assert.notNull(registration, "registrations cannot contain null values"); + this.clientIdToClientRegistration.put(registration.getRegistrationId(), registration); + } } /** @@ -50,24 +59,15 @@ public final class InMemoryReactiveClientRegistrationRepository * @param registrations the client registration(s) */ public InMemoryReactiveClientRegistrationRepository(List registrations) { - this.delegate = new InMemoryClientRegistrationRepository(registrations); + Assert.notEmpty(registrations, "registrations cannot be null or empty"); + this.clientIdToClientRegistration = registrations.stream() + .collect(Collectors.toConcurrentMap(ClientRegistration::getRegistrationId, Function.identity())); } - /** - * Constructs an {@code InMemoryReactiveClientRegistrationRepository} using the provided {@code Map} - * of {@link ClientRegistration#getRegistrationId() registration id} to {@link ClientRegistration}. - * NOTE: The supplied {@code Map} must be a non-blocking {@code Map}. - * - * @since 5.2 - * @param registrations the {@code Map} of client registration(s) - */ - public InMemoryReactiveClientRegistrationRepository( - Map registrations) { - this.delegate = new InMemoryClientRegistrationRepository(registrations); - } + @Override public Mono findByRegistrationId(String registrationId) { - return Mono.justOrEmpty(this.delegate.findByRegistrationId(registrationId)); + return Mono.justOrEmpty(this.clientIdToClientRegistration.get(registrationId)); } /** @@ -77,6 +77,6 @@ public final class InMemoryReactiveClientRegistrationRepository */ @Override public Iterator iterator() { - return delegate.iterator(); + return this.clientIdToClientRegistration.values().iterator(); } } diff --git a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/registration/InMemoryClientRegistrationRepositoryTests.java b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/registration/InMemoryClientRegistrationRepositoryTests.java index f1042aac04..f48b9210b5 100644 --- a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/registration/InMemoryClientRegistrationRepositoryTests.java +++ b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/registration/InMemoryClientRegistrationRepositoryTests.java @@ -19,6 +19,7 @@ package org.springframework.security.oauth2.client.registration; import org.junit.Test; import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -38,14 +39,14 @@ public class InMemoryClientRegistrationRepositoryTests { private InMemoryClientRegistrationRepository clients = new InMemoryClientRegistrationRepository(this.registration); @Test(expected = IllegalArgumentException.class) - public void constructorVarArgsListClientRegistrationWhenNullThenIllegalArgumentException() { - ClientRegistration nullRegistration = null; - new InMemoryClientRegistrationRepository(nullRegistration); + public void constructorListClientRegistrationWhenNullThenIllegalArgumentException() { + List registrations = null; + new InMemoryClientRegistrationRepository(registrations); } @Test(expected = IllegalArgumentException.class) - public void constructorListClientRegistrationWhenNullThenIllegalArgumentException() { - List registrations = null; + public void constructorListClientRegistrationWhenEmptyThenIllegalArgumentException() { + List registrations = Collections.emptyList(); new InMemoryClientRegistrationRepository(registrations); } diff --git a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/registration/InMemoryReactiveClientRegistrationRepositoryTests.java b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/registration/InMemoryReactiveClientRegistrationRepositoryTests.java index 208a84eb76..4c29adb86b 100644 --- a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/registration/InMemoryReactiveClientRegistrationRepositoryTests.java +++ b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/registration/InMemoryReactiveClientRegistrationRepositoryTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2019 the original author or authors. + * Copyright 2002-2018 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. @@ -16,21 +16,18 @@ package org.springframework.security.oauth2.client.registration; -import org.junit.Before; -import org.junit.Test; -import reactor.test.StepVerifier; - -import java.util.Arrays; -import java.util.HashMap; -import java.util.List; -import java.util.Map; - import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; +import java.util.List; + +import org.junit.Before; +import org.junit.Test; + +import reactor.test.StepVerifier; + /** * @author Rob Winch - * @author Vedran Pavic * @since 5.1 */ public class InMemoryReactiveClientRegistrationRepositoryTests { @@ -64,33 +61,13 @@ public class InMemoryReactiveClientRegistrationRepositoryTests { .isInstanceOf(IllegalArgumentException.class); } - @Test - public void constructorWhenClientRegistrationListHasNullThenIllegalArgumentException() { - List registrations = Arrays.asList(null, registration); - assertThatThrownBy(() -> new InMemoryReactiveClientRegistrationRepository(registrations)) - .isInstanceOf(IllegalArgumentException.class); - } - @Test public void constructorWhenClientRegistrationIsNullThenIllegalArgumentException() { - assertThatThrownBy(() -> new InMemoryReactiveClientRegistrationRepository(registration, null)) + ClientRegistration registration = null; + assertThatThrownBy(() -> new InMemoryReactiveClientRegistrationRepository(registration)) .isInstanceOf(IllegalArgumentException.class); } - @Test - public void constructorWhenClientRegistrationMapIsNullThenIllegalArgumentException() { - Map registrations = null; - assertThatThrownBy(() -> new InMemoryReactiveClientRegistrationRepository(registrations)) - .isInstanceOf(IllegalArgumentException.class); - } - - @Test - public void constructorWhenClientRegistrationMapIsEmptyThenRepositoryIsEmpty() { - InMemoryReactiveClientRegistrationRepository repository = new InMemoryReactiveClientRegistrationRepository( - new HashMap<>()); - assertThat(repository).isEmpty(); - } - @Test public void findByRegistrationIdWhenValidIdThenFound() { StepVerifier.create(this.repository.findByRegistrationId(this.registration.getRegistrationId()))