From 488e55032ebd8fe7f1031aa92ab2926a5f8fb81a Mon Sep 17 00:00:00 2001 From: Rob Winch <362503+rwinch@users.noreply.github.com> Date: Wed, 8 Oct 2025 15:26:26 -0500 Subject: [PATCH] AllFactorsAuthorizationManager->AllRequiredFactorsAuthorizationManager This allows the authorization logic to be relaxed so that if RequiredFactor only has an authority specified, then the GrantedAuthority can be of any type. Closes gh-18031 --- ...lRequiredFactorsAuthorizationManager.java} | 49 ++++++------ .../authorization/RequiredFactor.java | 14 +++- ...iredFactorsAuthorizationManagerTests.java} | 75 +++++++++---------- 3 files changed, 75 insertions(+), 63 deletions(-) rename core/src/main/java/org/springframework/security/authorization/{AllFactorsAuthorizationManager.java => AllRequiredFactorsAuthorizationManager.java} (77%) rename core/src/test/java/org/springframework/security/authorization/{AllFactorsAuthorizationManagerTests.java => AllRequiredFactorsAuthorizationManagerTests.java} (75%) diff --git a/core/src/main/java/org/springframework/security/authorization/AllFactorsAuthorizationManager.java b/core/src/main/java/org/springframework/security/authorization/AllRequiredFactorsAuthorizationManager.java similarity index 77% rename from core/src/main/java/org/springframework/security/authorization/AllFactorsAuthorizationManager.java rename to core/src/main/java/org/springframework/security/authorization/AllRequiredFactorsAuthorizationManager.java index a31117c656..432fc943f1 100644 --- a/core/src/main/java/org/springframework/security/authorization/AllFactorsAuthorizationManager.java +++ b/core/src/main/java/org/springframework/security/authorization/AllRequiredFactorsAuthorizationManager.java @@ -30,6 +30,7 @@ import java.util.stream.Collectors; import org.jspecify.annotations.Nullable; import org.springframework.security.core.Authentication; +import org.springframework.security.core.GrantedAuthority; import org.springframework.security.core.authority.FactorGrantedAuthority; import org.springframework.util.Assert; @@ -42,7 +43,7 @@ import org.springframework.util.Assert; * @since 7.0 * @see AuthorityAuthorizationManager */ -public final class AllFactorsAuthorizationManager implements AuthorizationManager { +public final class AllRequiredFactorsAuthorizationManager implements AuthorizationManager { private Clock clock = Clock.systemUTC(); @@ -52,7 +53,7 @@ public final class AllFactorsAuthorizationManager implements AuthorizationMan * Creates a new instance. * @param requiredFactors the authorities that are required. */ - private AllFactorsAuthorizationManager(List requiredFactors) { + private AllRequiredFactorsAuthorizationManager(List requiredFactors) { Assert.notEmpty(requiredFactors, "requiredFactors cannot be empty"); Assert.noNullElements(requiredFactors, "requiredFactors must not contain null elements"); this.requiredFactors = Collections.unmodifiableList(requiredFactors); @@ -80,7 +81,7 @@ public final class AllFactorsAuthorizationManager implements AuthorizationMan @Override public FactorAuthorizationDecision authorize(Supplier authentication, T object) { - List currentFactorAuthorities = getFactorGrantedAuthorities(authentication.get()); + List currentFactorAuthorities = getFactorGrantedAuthorities(authentication.get()); List factorErrors = this.requiredFactors.stream() .map((factor) -> requiredFactorError(factor, currentFactorAuthorities)) .filter(Objects::nonNull) @@ -96,8 +97,8 @@ public final class AllFactorsAuthorizationManager implements AuthorizationMan * @return the {@link RequiredFactor} or null if granted. */ private @Nullable RequiredFactorError requiredFactorError(RequiredFactor requiredFactor, - List currentFactors) { - Optional matchingAuthority = currentFactors.stream() + List currentFactors) { + Optional matchingAuthority = currentFactors.stream() .filter((authority) -> authority.getAuthority().equals(requiredFactor.getAuthority())) .findFirst(); if (!matchingAuthority.isPresent()) { @@ -108,13 +109,16 @@ public final class AllFactorsAuthorizationManager implements AuthorizationMan // granted (only requires authority to match) return null; } - Instant now = this.clock.instant(); - Instant expiresAt = authority.getIssuedAt().plus(requiredFactor.getValidDuration()); - if (now.isBefore(expiresAt)) { - // granted - return null; + else if (authority instanceof FactorGrantedAuthority factorAuthority) { + Instant now = this.clock.instant(); + Instant expiresAt = factorAuthority.getIssuedAt().plus(requiredFactor.getValidDuration()); + if (now.isBefore(expiresAt)) { + // granted + return null; + } } - // denied (expired) + + // denied (expired or no issuedAt to compare) return RequiredFactorError.createExpired(requiredFactor); }).orElse(null); } @@ -128,14 +132,12 @@ public final class AllFactorsAuthorizationManager implements AuthorizationMan * @return all of the {@link FactorGrantedAuthority} instances from * {@link Authentication#getAuthorities()}. */ - private List getFactorGrantedAuthorities(@Nullable Authentication authentication) { + private List getFactorGrantedAuthorities(@Nullable Authentication authentication) { if (authentication == null || !authentication.isAuthenticated()) { return Collections.emptyList(); } // @formatter:off return authentication.getAuthorities().stream() - .filter(FactorGrantedAuthority.class::isInstance) - .map(FactorGrantedAuthority.class::cast) .collect(Collectors.toList()); // @formatter:on } @@ -149,7 +151,7 @@ public final class AllFactorsAuthorizationManager implements AuthorizationMan } /** - * A builder for {@link AllFactorsAuthorizationManager}. + * A builder for {@link AllRequiredFactorsAuthorizationManager}. * * @author Rob Winch * @since 7.0 @@ -160,15 +162,15 @@ public final class AllFactorsAuthorizationManager implements AuthorizationMan /** * Allows the user to consume the {@link RequiredFactor.Builder} that is passed in - * and then adds the result to the {@link #requiredFactor(RequiredFactor)}. + * and then adds the result to the {@link #requireFactor(RequiredFactor)}. * @param requiredFactor the {@link Consumer} to invoke. * @return the builder. */ - public Builder requiredFactor(Consumer requiredFactor) { + public Builder requireFactor(Consumer requiredFactor) { Assert.notNull(requiredFactor, "requiredFactor cannot be null"); RequiredFactor.Builder builder = RequiredFactor.builder(); requiredFactor.accept(builder); - return requiredFactor(builder.build()); + return requireFactor(builder.build()); } /** @@ -176,19 +178,20 @@ public final class AllFactorsAuthorizationManager implements AuthorizationMan * @param requiredFactor the requiredFactor to add. Cannot be null. * @return the builder. */ - public Builder requiredFactor(RequiredFactor requiredFactor) { + public Builder requireFactor(RequiredFactor requiredFactor) { Assert.notNull(requiredFactor, "requiredFactor cannot be null"); this.requiredFactors.add(requiredFactor); return this; } /** - * Builds the {@link AllFactorsAuthorizationManager}. + * Builds the {@link AllRequiredFactorsAuthorizationManager}. * @param the type. - * @return the {@link AllFactorsAuthorizationManager} + * @return the {@link AllRequiredFactorsAuthorizationManager} */ - public AllFactorsAuthorizationManager build() { - return new AllFactorsAuthorizationManager(this.requiredFactors); + public AllRequiredFactorsAuthorizationManager build() { + Assert.state(!this.requiredFactors.isEmpty(), "requiredFactors cannot be empty"); + return new AllRequiredFactorsAuthorizationManager(this.requiredFactors); } } diff --git a/core/src/main/java/org/springframework/security/authorization/RequiredFactor.java b/core/src/main/java/org/springframework/security/authorization/RequiredFactor.java index b5a1389596..a579325966 100644 --- a/core/src/main/java/org/springframework/security/authorization/RequiredFactor.java +++ b/core/src/main/java/org/springframework/security/authorization/RequiredFactor.java @@ -21,11 +21,21 @@ import java.util.Objects; import org.jspecify.annotations.Nullable; +import org.springframework.security.core.GrantedAuthority; import org.springframework.security.core.authority.FactorGrantedAuthority; import org.springframework.util.Assert; /** - * The requirements for an {@link FactorGrantedAuthority} to be considered valid. + * The requirements for an {@link GrantedAuthority} to be considered a valid factor. + * + *
    + *
  • If the {@link #getAuthority()} is specified, then it must match + * {@link GrantedAuthority#getAuthority()}
    • + *
  • If {@link #getValidDuration()} is specified, the matching {@link GrantedAuthority} + * must be of type {@link FactorGrantedAuthority} and + * {@link FactorGrantedAuthority#getIssuedAt()} must be such that it is not considered + * expired when compared to {@link #getValidDuration()}.
    • + *
* * @author Rob Winch * @since 7.0 @@ -43,7 +53,7 @@ public final class RequiredFactor { } /** - * The {@link FactorGrantedAuthority#getAuthority()}. + * The expected {@link GrantedAuthority#getAuthority()}. * @return the authority. */ public String getAuthority() { diff --git a/core/src/test/java/org/springframework/security/authorization/AllFactorsAuthorizationManagerTests.java b/core/src/test/java/org/springframework/security/authorization/AllRequiredFactorsAuthorizationManagerTests.java similarity index 75% rename from core/src/test/java/org/springframework/security/authorization/AllFactorsAuthorizationManagerTests.java rename to core/src/test/java/org/springframework/security/authorization/AllRequiredFactorsAuthorizationManagerTests.java index 3e89612c9a..34a34d3660 100644 --- a/core/src/test/java/org/springframework/security/authorization/AllFactorsAuthorizationManagerTests.java +++ b/core/src/test/java/org/springframework/security/authorization/AllRequiredFactorsAuthorizationManagerTests.java @@ -32,12 +32,12 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; /** - * Test {@link AllFactorsAuthorizationManager}. + * Test {@link AllRequiredFactorsAuthorizationManager}. * * @author Rob Winch * @since 7.0 */ -class AllFactorsAuthorizationManagerTests { +class AllRequiredFactorsAuthorizationManagerTests { private static final Object DOES_NOT_MATTER = new Object(); @@ -52,8 +52,8 @@ class AllFactorsAuthorizationManagerTests { @Test void authorizeWhenGranted() { - AllFactorsAuthorizationManager allFactors = AllFactorsAuthorizationManager.builder() - .requiredFactor(REQUIRED_PASSWORD) + AllRequiredFactorsAuthorizationManager allFactors = AllRequiredFactorsAuthorizationManager.builder() + .requireFactor(REQUIRED_PASSWORD) .build(); FactorGrantedAuthority passwordFactor = FactorGrantedAuthority.withAuthority(REQUIRED_PASSWORD.getAuthority()) .issuedAt(Instant.now()) @@ -65,8 +65,8 @@ class AllFactorsAuthorizationManagerTests { @Test void authorizeWhenConsumerGranted() { - AllFactorsAuthorizationManager allFactors = AllFactorsAuthorizationManager.builder() - .requiredFactor((required) -> required.authority(FactorGrantedAuthority.PASSWORD_AUTHORITY)) + AllRequiredFactorsAuthorizationManager allFactors = AllRequiredFactorsAuthorizationManager.builder() + .requireFactor((required) -> required.authority(FactorGrantedAuthority.PASSWORD_AUTHORITY)) .build(); FactorGrantedAuthority passwordFactor = FactorGrantedAuthority .withAuthority(FactorGrantedAuthority.PASSWORD_AUTHORITY) @@ -79,8 +79,8 @@ class AllFactorsAuthorizationManagerTests { @Test void authorizeWhenUnauthenticated() { - AllFactorsAuthorizationManager allFactors = AllFactorsAuthorizationManager.builder() - .requiredFactor(REQUIRED_PASSWORD) + AllRequiredFactorsAuthorizationManager allFactors = AllRequiredFactorsAuthorizationManager.builder() + .requireFactor(REQUIRED_PASSWORD) .build(); FactorGrantedAuthority passwordFactor = FactorGrantedAuthority.withAuthority(REQUIRED_PASSWORD.getAuthority()) .issuedAt(Instant.now()) @@ -94,8 +94,8 @@ class AllFactorsAuthorizationManagerTests { @Test void authorizeWhenNullAuthentication() { - AllFactorsAuthorizationManager allFactors = AllFactorsAuthorizationManager.builder() - .requiredFactor(EXPIRING_PASSWORD) + AllRequiredFactorsAuthorizationManager allFactors = AllRequiredFactorsAuthorizationManager.builder() + .requireFactor(EXPIRING_PASSWORD) .build(); Authentication authentication = null; FactorAuthorizationDecision result = allFactors.authorize(() -> authentication, DOES_NOT_MATTER); @@ -105,8 +105,8 @@ class AllFactorsAuthorizationManagerTests { @Test void authorizeWhenRequiredFactorHasNullDurationThenNullIssuedAtGranted() { - AllFactorsAuthorizationManager allFactors = AllFactorsAuthorizationManager.builder() - .requiredFactor(REQUIRED_PASSWORD) + AllRequiredFactorsAuthorizationManager allFactors = AllRequiredFactorsAuthorizationManager.builder() + .requireFactor(REQUIRED_PASSWORD) .build(); FactorGrantedAuthority passwordFactor = FactorGrantedAuthority.withAuthority(REQUIRED_PASSWORD.getAuthority()) .build(); @@ -116,21 +116,21 @@ class AllFactorsAuthorizationManagerTests { } @Test - void authorizeWhenRequiredFactorHasDurationAndNotFactorGrantedAuthorityThenMissing() { - AllFactorsAuthorizationManager allFactors = AllFactorsAuthorizationManager.builder() - .requiredFactor(EXPIRING_PASSWORD) + void authorizeWhenRequiredFactorHasDurationAndNotFactorGrantedAuthorityThenExpired() { + AllRequiredFactorsAuthorizationManager allFactors = AllRequiredFactorsAuthorizationManager.builder() + .requireFactor(EXPIRING_PASSWORD) .build(); Authentication authentication = new TestingAuthenticationToken("user", "password", EXPIRING_PASSWORD.getAuthority()); FactorAuthorizationDecision result = allFactors.authorize(() -> authentication, DOES_NOT_MATTER); assertThat(result.isGranted()).isFalse(); - assertThat(result.getFactorErrors()).containsExactly(RequiredFactorError.createMissing(EXPIRING_PASSWORD)); + assertThat(result.getFactorErrors()).containsExactly(RequiredFactorError.createExpired(EXPIRING_PASSWORD)); } @Test void authorizeWhenFactorAuthorityMissingThenMissing() { - AllFactorsAuthorizationManager allFactors = AllFactorsAuthorizationManager.builder() - .requiredFactor(REQUIRED_PASSWORD) + AllRequiredFactorsAuthorizationManager allFactors = AllRequiredFactorsAuthorizationManager.builder() + .requireFactor(REQUIRED_PASSWORD) .build(); Authentication authentication = new TestingAuthenticationToken("user", "password", "ROLE_USER"); FactorAuthorizationDecision result = allFactors.authorize(() -> authentication, DOES_NOT_MATTER); @@ -139,21 +139,20 @@ class AllFactorsAuthorizationManagerTests { } @Test - void authorizeWhenFactorGrantedAuthorityMissingThenMissing() { - AllFactorsAuthorizationManager allFactors = AllFactorsAuthorizationManager.builder() - .requiredFactor(REQUIRED_PASSWORD) + void authorizeWhenGrantedAuthorityThenGranted() { + AllRequiredFactorsAuthorizationManager allFactors = AllRequiredFactorsAuthorizationManager.builder() + .requireFactor(REQUIRED_PASSWORD) .build(); Authentication authentication = new TestingAuthenticationToken("user", "password", REQUIRED_PASSWORD.getAuthority()); FactorAuthorizationDecision result = allFactors.authorize(() -> authentication, DOES_NOT_MATTER); - assertThat(result.isGranted()).isFalse(); - assertThat(result.getFactorErrors()).containsExactly(RequiredFactorError.createMissing(REQUIRED_PASSWORD)); + assertThat(result.isGranted()).isTrue(); } @Test void authorizeWhenExpired() { - AllFactorsAuthorizationManager allFactors = AllFactorsAuthorizationManager.builder() - .requiredFactor(EXPIRING_PASSWORD) + AllRequiredFactorsAuthorizationManager allFactors = AllRequiredFactorsAuthorizationManager.builder() + .requireFactor(EXPIRING_PASSWORD) .build(); FactorGrantedAuthority passwordFactor = FactorGrantedAuthority.withAuthority(EXPIRING_PASSWORD.getAuthority()) .issuedAt(Instant.now().minus(Duration.ofHours(2))) @@ -173,8 +172,8 @@ class AllFactorsAuthorizationManagerTests { RequiredFactor expiringPassword = RequiredFactor.withAuthority(FactorGrantedAuthority.PASSWORD_AUTHORITY) .validDuration(expiresIn) .build(); - AllFactorsAuthorizationManager allFactors = AllFactorsAuthorizationManager.builder() - .requiredFactor(expiringPassword) + AllRequiredFactorsAuthorizationManager allFactors = AllRequiredFactorsAuthorizationManager.builder() + .requireFactor(expiringPassword) .build(); allFactors.setClock(clock); FactorGrantedAuthority passwordFactor = FactorGrantedAuthority.withAuthority(expiringPassword.getAuthority()) @@ -195,8 +194,8 @@ class AllFactorsAuthorizationManagerTests { RequiredFactor expiringPassword = RequiredFactor.withAuthority(FactorGrantedAuthority.PASSWORD_AUTHORITY) .validDuration(expiresIn) .build(); - AllFactorsAuthorizationManager allFactors = AllFactorsAuthorizationManager.builder() - .requiredFactor(expiringPassword) + AllRequiredFactorsAuthorizationManager allFactors = AllRequiredFactorsAuthorizationManager.builder() + .requireFactor(expiringPassword) .build(); allFactors.setClock(clock); FactorGrantedAuthority passwordFactor = FactorGrantedAuthority.withAuthority(expiringPassword.getAuthority()) @@ -209,8 +208,8 @@ class AllFactorsAuthorizationManagerTests { @Test void authorizeWhenDifferentFactorGrantedAuthorityThenMissing() { - AllFactorsAuthorizationManager allFactors = AllFactorsAuthorizationManager.builder() - .requiredFactor(REQUIRED_PASSWORD) + AllRequiredFactorsAuthorizationManager allFactors = AllRequiredFactorsAuthorizationManager.builder() + .requireFactor(REQUIRED_PASSWORD) .build(); Authentication authentication = new TestingAuthenticationToken("user", "password", FactorGrantedAuthority.fromAuthority(REQUIRED_PASSWORD.getAuthority()) + "DIFFERENT"); @@ -221,28 +220,28 @@ class AllFactorsAuthorizationManagerTests { @Test void setClockWhenNullThenIllegalArgumentException() { - AllFactorsAuthorizationManager allFactors = AllFactorsAuthorizationManager.builder() - .requiredFactor(REQUIRED_PASSWORD) + AllRequiredFactorsAuthorizationManager allFactors = AllRequiredFactorsAuthorizationManager.builder() + .requireFactor(REQUIRED_PASSWORD) .build(); assertThatIllegalArgumentException().isThrownBy(() -> allFactors.setClock(null)); } @Test void builderBuildWhenEmpty() { - assertThatIllegalArgumentException().isThrownBy(() -> AllFactorsAuthorizationManager.builder().build()); + assertThatIllegalArgumentException().isThrownBy(() -> AllRequiredFactorsAuthorizationManager.builder().build()); } @Test void builderWhenNullRequiredFactor() { - AllFactorsAuthorizationManager.Builder builder = AllFactorsAuthorizationManager.builder(); - assertThatIllegalArgumentException().isThrownBy(() -> builder.requiredFactor((RequiredFactor) null)); + AllRequiredFactorsAuthorizationManager.Builder builder = AllRequiredFactorsAuthorizationManager.builder(); + assertThatIllegalArgumentException().isThrownBy(() -> builder.requireFactor((RequiredFactor) null)); } @Test void builderWhenNullConsumerRequiredFactorBuilder() { - AllFactorsAuthorizationManager.Builder builder = AllFactorsAuthorizationManager.builder(); + AllRequiredFactorsAuthorizationManager.Builder builder = AllRequiredFactorsAuthorizationManager.builder(); assertThatIllegalArgumentException() - .isThrownBy(() -> builder.requiredFactor((Consumer) null)); + .isThrownBy(() -> builder.requireFactor((Consumer) null)); } }