This is an automated email from the ASF dual-hosted git repository. lprimak pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/shiro.git
commit 8a92a71007c4552b604f1d67effffada6c51f0ad Author: lprimak <[email protected]> AuthorDate: Sat Oct 24 18:11:27 2026 -0500 enh: implement strict handling for empty authorization configurations in filters and annotation handlers --- .../authz/aop/AuthorizingAnnotationHandler.java | 80 +++++++++++- .../authz/aop/PermissionAnnotationHandler.java | 5 +- .../shiro/authz/aop/RoleAnnotationHandler.java | 5 +- .../shiro/authz/permission/WildcardPermission.java | 81 +++++++++--- .../shiro/authz/aop/EmptyAnnotationValueTest.java | 137 +++++++++++++++++++++ .../authz/permission/WildcardPermissionTest.java | 77 ++++++++++++ .../apache/shiro/cdi/ShiroSecurityExtension.java | 5 +- .../org/apache/shiro/guice/aop/ShiroAopModule.java | 7 +- .../web/jaxrs/AnnotationAuthorizationFilter.java | 2 + .../AuthorizationAttributeSourceAdvisor.java | 3 + .../authz/PermissionsAuthorizationFilter.java | 12 +- .../web/filter/authz/RolesAuthorizationFilter.java | 13 +- .../authz/EmptyAuthorizationConfigFilterTest.java | 104 ++++++++++++++++ 13 files changed, 505 insertions(+), 26 deletions(-) diff --git a/core/src/main/java/org/apache/shiro/authz/aop/AuthorizingAnnotationHandler.java b/core/src/main/java/org/apache/shiro/authz/aop/AuthorizingAnnotationHandler.java index 898479636..e0b70ab74 100644 --- a/core/src/main/java/org/apache/shiro/authz/aop/AuthorizingAnnotationHandler.java +++ b/core/src/main/java/org/apache/shiro/authz/aop/AuthorizingAnnotationHandler.java @@ -19,9 +19,17 @@ package org.apache.shiro.authz.aop; import java.lang.annotation.Annotation; - +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import lombok.Getter; +import lombok.Setter; import org.apache.shiro.aop.AnnotationHandler; import org.apache.shiro.authz.AuthorizationException; +import org.apache.shiro.authz.annotation.RequiresPermissions; +import org.apache.shiro.authz.annotation.RequiresRoles; +import org.apache.shiro.config.ConfigurationException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * An AnnotationHandler that executes authorization (access control) behavior based on directive(s) found in a @@ -52,4 +60,74 @@ public abstract class AuthorizingAnnotationHandler extends AnnotationHandler { * @throws org.apache.shiro.authz.AuthorizationException if the class/instance/method is not allowed to proceed/execute. */ public abstract void assertAuthorized(Annotation a) throws AuthorizationException; + + /** + * What to do with authorization configuration that names no permissions or roles, such as the {@code perms} and + * {@code roles} filters without arguments, or {@code @RequiresPermissions({})}. + * <p/> + * Such configuration performs no check, which is almost always a mistake. By default a warning is logged and the + * previous behavior is kept. If the {@value #STRICT_PROPERTY} system property is {@code true}, the + * configuration is rejected with a {@link ConfigurationException} when the integration reads it at startup. + * + * @since 3.0.2 + */ + public static final class AuthorizationConfigPolicy { + /** + * System property that, when {@code true}, makes empty authorization configuration fail at startup + * instead of only logging a warning. Also enables strict {@link org.apache.shiro.authz.permission.WildcardPermission} + * parsing, rejecting empty or blank permission parts and subparts, including trailing separators. + */ + private static final String STRICT_PROPERTY = "shiro.authz.strictConfig"; + @Getter @Setter + // Setter is for testing only, to avoid having to set a system property in the test JVM. + private static boolean strictParsing = Boolean.getBoolean(STRICT_PROPERTY); + + private static final Logger LOGGER = LoggerFactory.getLogger(AuthorizingAnnotationHandler.class); + private static final Set<String> WARNED = ConcurrentHashMap.newKeySet(); + + private AuthorizationConfigPolicy() { + } + + /** + * Validates the permissions or roles of an authorization annotation. Annotations other than + * {@link RequiresPermissions} and {@link RequiresRoles} are ignored. + * + * @param annotation the annotation found on a class or method + * @throws ConfigurationException if the annotation is empty and {@link #isStrictParsing()} + * is set + */ + public static <T extends Annotation> T validate(T annotation) { + if (annotation instanceof RequiresPermissions permissions) { + validate(permissions.value(), "@RequiresPermissions"); + } else if (annotation instanceof RequiresRoles roles) { + validate(roles.value(), "@RequiresRoles"); + } + return annotation; + } + + /** + * Logs a warning, once per {@code what}, if the configuration is empty, and rejects it by throwing if + * {@link #isStrictParsing()} is set. Integrations call this when they read the configuration + * at startup, and the annotation handlers call it again when they check, for configuration that no + * startup hook saw. + * + * @param values the configured permissions or roles + * @param what describes the configuration, for the message + * @throws ConfigurationException if the configuration is empty and {@link #isStrictParsing()} + * is set + */ + public static void validate(String[] values, String what) { + if (values != null && values.length > 0) { + return; + } + boolean strict = isStrictParsing(); + if (WARNED.add(what)) { + LOGGER.warn("{} specifies no permissions or roles, so it {}. Specify at least one.", what, + strict ? "is rejected" : "performs no check"); + } + if (strict) { + throw new ConfigurationException(what + " must specify at least one permission or role"); + } + } + } } diff --git a/core/src/main/java/org/apache/shiro/authz/aop/PermissionAnnotationHandler.java b/core/src/main/java/org/apache/shiro/authz/aop/PermissionAnnotationHandler.java index e96a11364..87c9103de 100644 --- a/core/src/main/java/org/apache/shiro/authz/aop/PermissionAnnotationHandler.java +++ b/core/src/main/java/org/apache/shiro/authz/aop/PermissionAnnotationHandler.java @@ -63,11 +63,12 @@ public class PermissionAnnotationHandler extends AuthorizingAnnotationHandler { */ @Override public void assertAuthorized(Annotation a) throws AuthorizationException { - if (!(a instanceof RequiresPermissions)) { + if (!(a instanceof RequiresPermissions rpAnnotation)) { return; } - RequiresPermissions rpAnnotation = (RequiresPermissions) a; + AuthorizationConfigPolicy.validate(a); + String[] perms = getAnnotationValue(a); Subject subject = getSubject(); diff --git a/core/src/main/java/org/apache/shiro/authz/aop/RoleAnnotationHandler.java b/core/src/main/java/org/apache/shiro/authz/aop/RoleAnnotationHandler.java index 14fb68b3d..c63fe0263 100644 --- a/core/src/main/java/org/apache/shiro/authz/aop/RoleAnnotationHandler.java +++ b/core/src/main/java/org/apache/shiro/authz/aop/RoleAnnotationHandler.java @@ -51,11 +51,12 @@ public class RoleAnnotationHandler extends AuthorizingAnnotationHandler { */ @Override public void assertAuthorized(Annotation a) throws AuthorizationException { - if (!(a instanceof RequiresRoles)) { + if (!(a instanceof RequiresRoles rrAnnotation)) { return; } - RequiresRoles rrAnnotation = (RequiresRoles) a; + AuthorizationConfigPolicy.validate(a); + String[] roles = rrAnnotation.value(); if (roles.length == 1) { diff --git a/core/src/main/java/org/apache/shiro/authz/permission/WildcardPermission.java b/core/src/main/java/org/apache/shiro/authz/permission/WildcardPermission.java index 08ab55f8d..d9180da51 100644 --- a/core/src/main/java/org/apache/shiro/authz/permission/WildcardPermission.java +++ b/core/src/main/java/org/apache/shiro/authz/permission/WildcardPermission.java @@ -19,11 +19,14 @@ package org.apache.shiro.authz.permission; import org.apache.shiro.authz.Permission; +import org.apache.shiro.authz.aop.AuthorizingAnnotationHandler.AuthorizationConfigPolicy; +import org.apache.shiro.lang.util.Assert; import org.apache.shiro.util.CollectionUtils; import org.apache.shiro.lang.util.StringUtils; import java.io.Serializable; import java.util.ArrayList; +import java.util.Arrays; import java.util.Iterator; import java.util.List; import java.util.Locale; @@ -101,11 +104,21 @@ import java.util.Set; * There is no limit to the number of tokens that can be used, so it is up to your imagination in terms of ways that * this could be used in your application. However, the Shiro team likes to standardize some common usages shown * above to help people get started and provide consistency in the Shiro community. + * <p/> + * Permission strings are case-insensitive by default, using {@link Locale#ROOT}. Use the + * {@link #WildcardPermission(String, boolean)} constructor for case-sensitive identifiers. + * <p/> + * Do not concatenate untrusted values into permission strings: {@code :}, {@code ,}, and {@code *} are permission + * syntax, not literal data. Use {@link #fromLiteralParts(String...)} for both grants and checks built from data. + * This factory rejects reserved characters instead of escaping them. + * <p/> + * Setting {@code -Dshiro.authz.strictConfig=true} rejects empty or blank parts and subparts, including trailing + * separators such as {@code newsletter:edit:}. Without it, legacy parsing is preserved, including discarding trailing + * separators, which can broaden a grant. Intentionally omitted parts and explicit wildcards remain supported. * * @since 0.9 */ public class WildcardPermission implements Permission, Serializable { - /*-------------------------------------------- | C O N S T A N T S | ============================================*/ @@ -143,6 +156,44 @@ public class WildcardPermission implements Permission, Serializable { setParts(wildcardString, caseSensitive); } + /** + * Creates a case-insensitive permission from literal values, without interpreting them as permission syntax. + * + * @param parts one literal value per permission level, such as domain, action, and instance ID + * @return the permission + * @throws IllegalArgumentException if no parts are supplied or a part is null, blank, has surrounding whitespace, + * or contains {@code :}, {@code ,}, or {@code *} + * @since 3.0.2 + */ + public static WildcardPermission fromLiteralParts(String... parts) { + return fromLiteralParts(DEFAULT_CASE_SENSITIVE, parts); + } + + /** + * Creates a permission from literal values. Validation is unconditional, regardless of the + * {@code shiro.authz.strictConfig} system property. No escaping is supported. + * <p/> + * Supply every level needed by the application's permission model: an omitted level still implies all values at + * that level. Use the string constructors only for trusted permission expressions requiring wildcards or lists. + * + * @param caseSensitive whether values are case-sensitive + * @param parts one literal value per permission level + * @return the permission + * @throws IllegalArgumentException if no parts are supplied or a part is null, blank, has surrounding whitespace, + * or contains {@code :}, {@code ,}, or {@code *} + * @since 3.0.2 + */ + public static WildcardPermission fromLiteralParts(boolean caseSensitive, String... parts) { + Assert.notEmpty(parts, "At least one literal permission part is required."); + Arrays.stream(parts).forEach(part -> { + Assert.hasText(part, "Literal permission parts cannot be null, empty, or blank."); + Assert.isTrue(part.equals(part.trim()), "Literal permission parts cannot have surrounding whitespace."); + List.of(PART_DIVIDER_TOKEN, SUBPART_DIVIDER_TOKEN, WILDCARD_TOKEN).forEach(token -> + Assert.doesNotContain(part, token, "Literal permission parts cannot contain ':', ',', or '*'.")); + }); + return new WildcardPermission(String.join(PART_DIVIDER_TOKEN, parts), caseSensitive); + } + protected void setParts(String wildcardString) { setParts(wildcardString, DEFAULT_CASE_SENSITIVE); } @@ -160,18 +211,20 @@ public class WildcardPermission implements Permission, Serializable { wildcardString = wildcardString.toLowerCase(Locale.ROOT); } - List<String> parts = CollectionUtils.asList(wildcardString.split(PART_DIVIDER_TOKEN)); - - this.parts = new ArrayList<Set<String>>(); - for (String part : parts) { - Set<String> subparts = CollectionUtils.asSet(part.split(SUBPART_DIVIDER_TOKEN)); + List<String> parts = CollectionUtils.asList(wildcardString.split(PART_DIVIDER_TOKEN, + AuthorizationConfigPolicy.isStrictParsing() ? -1 : 0)); - if (subparts.isEmpty()) { - throw new IllegalArgumentException("Wildcard string cannot contain parts with only dividers. " + this.parts = new ArrayList<>(); + parts.stream().map(part -> CollectionUtils.asSet(part.split(SUBPART_DIVIDER_TOKEN, + AuthorizationConfigPolicy.isStrictParsing() ? -1 : 0))) + .forEach(subparts -> { + if (subparts.isEmpty() || (AuthorizationConfigPolicy.isStrictParsing() + && subparts.stream().anyMatch(value -> !StringUtils.hasText(value)))) { + throw new IllegalArgumentException("Wildcard string cannot contain empty or blank parts or subparts. " + "Make sure permission strings are properly formatted."); } this.parts.add(subparts); - } + }); if (this.parts.isEmpty()) { throw new IllegalArgumentException("Wildcard string cannot contain only dividers. " @@ -202,18 +255,16 @@ public class WildcardPermission implements Permission, Serializable { @Override public boolean implies(Permission p) { - // By default only supports comparisons with other WildcardPermissions - if (!(p instanceof WildcardPermission)) { + // By default, only supports comparisons with other WildcardPermissions + if (!(p instanceof WildcardPermission wp)) { return false; } - WildcardPermission wp = (WildcardPermission) p; - List<Set<String>> otherParts = wp.getParts(); int i = 0; for (Set<String> otherPart : otherParts) { - // If this permission has less parts than the other permission, everything after the number of parts contained + // If this permission has fewer parts than the other permission, everything after the number of parts contained // in this permission is automatically implied, so return true if (getParts().size() - 1 < i) { return true; @@ -241,7 +292,7 @@ public class WildcardPermission implements Permission, Serializable { public String toString() { StringBuilder buffer = new StringBuilder(); for (Set<String> part : parts) { - if (buffer.length() > 0) { + if (!buffer.isEmpty()) { buffer.append(PART_DIVIDER_TOKEN); } Iterator<String> partIt = part.iterator(); diff --git a/core/src/test/java/org/apache/shiro/authz/aop/EmptyAnnotationValueTest.java b/core/src/test/java/org/apache/shiro/authz/aop/EmptyAnnotationValueTest.java new file mode 100644 index 000000000..caccb846a --- /dev/null +++ b/core/src/test/java/org/apache/shiro/authz/aop/EmptyAnnotationValueTest.java @@ -0,0 +1,137 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 + * + * http://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.apache.shiro.authz.aop; + +import org.apache.shiro.authz.aop.AuthorizingAnnotationHandler.AuthorizationConfigPolicy; +import org.apache.shiro.authz.annotation.Logical; +import org.apache.shiro.authz.annotation.RequiresPermissions; +import org.apache.shiro.authz.annotation.RequiresRoles; +import org.apache.shiro.config.ConfigurationException; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.parallel.ResourceLock; + +import java.lang.annotation.Annotation; + +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatExceptionOfType; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +/** + * Tests that annotations without any roles or permissions only warn by default, and are rejected at startup when so configured. + */ +@ResourceLock("AuthorizationConfigPolicy.strictParsing") +public class EmptyAnnotationValueTest { + + private boolean strictParsing; + + @BeforeEach + void saveStrictParsing() { + strictParsing = AuthorizationConfigPolicy.isStrictParsing(); + AuthorizationConfigPolicy.setStrictParsing(false); + } + + private static RequiresRoles roles(String... values) { + return new RequiresRoles() { + @Override + public String[] value() { + return values; + } + + @Override + public Logical logical() { + return Logical.AND; + } + + @Override + public Class<? extends Annotation> annotationType() { + return RequiresRoles.class; + } + }; + } + + private static RequiresPermissions permissions(String... values) { + return new RequiresPermissions() { + @Override + public String[] value() { + return values; + } + + @Override + public Logical logical() { + return Logical.AND; + } + + @Override + public Class<? extends Annotation> annotationType() { + return RequiresPermissions.class; + } + }; + } + + @AfterEach + void restoreStrictParsing() { + AuthorizationConfigPolicy.setStrictParsing(strictParsing); + } + + @Test + public void handlersOnlyWarnByDefault() { + // the empty check does not stop the call, it only fails later for lack of a subject + assertThatThrownBy(() -> new RoleAnnotationHandler().assertAuthorized(roles())) + .isNotInstanceOf(ConfigurationException.class); + assertThatThrownBy(() -> new PermissionAnnotationHandler().assertAuthorized(permissions())) + .isNotInstanceOf(ConfigurationException.class); + } + + @Test + public void handlersRejectEmptyWhenStrict() { + AuthorizationConfigPolicy.setStrictParsing(true); + assertThatExceptionOfType(ConfigurationException.class) + .isThrownBy(() -> new RoleAnnotationHandler().assertAuthorized(roles())); + assertThatExceptionOfType(ConfigurationException.class) + .isThrownBy(() -> new PermissionAnnotationHandler().assertAuthorized(permissions())); + } + + @Test + public void emptyAnnotationsOnlyWarnByDefault() { + assertThatCode(() -> { + AuthorizationConfigPolicy.validate(roles()); + AuthorizationConfigPolicy.validate(permissions()); + }).doesNotThrowAnyException(); + } + + @Test + public void emptyAnnotationsFailWhenStrict() { + AuthorizationConfigPolicy.setStrictParsing(true); + assertThatExceptionOfType(ConfigurationException.class) + .isThrownBy(() -> AuthorizationConfigPolicy.validate(roles())); + assertThatExceptionOfType(ConfigurationException.class) + .isThrownBy(() -> AuthorizationConfigPolicy.validate(permissions())); + } + + @Test + public void nonEmptyAnnotationsPassWhenStrict() { + AuthorizationConfigPolicy.setStrictParsing(true); + assertThatCode(() -> { + AuthorizationConfigPolicy.validate(roles("admin")); + AuthorizationConfigPolicy.validate(permissions("user:read")); + }).doesNotThrowAnyException(); + } +} diff --git a/core/src/test/java/org/apache/shiro/authz/permission/WildcardPermissionTest.java b/core/src/test/java/org/apache/shiro/authz/permission/WildcardPermissionTest.java index e1102c394..841eee327 100644 --- a/core/src/test/java/org/apache/shiro/authz/permission/WildcardPermissionTest.java +++ b/core/src/test/java/org/apache/shiro/authz/permission/WildcardPermissionTest.java @@ -18,7 +18,11 @@ */ package org.apache.shiro.authz.permission; +import org.apache.shiro.authz.aop.AuthorizingAnnotationHandler.AuthorizationConfigPolicy; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.parallel.ResourceLock; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.AssertionsForClassTypes.assertThatExceptionOfType; @@ -26,8 +30,81 @@ import static org.assertj.core.api.AssertionsForClassTypes.assertThatExceptionOf /** * @since 0.9 */ +@ResourceLock("AuthorizationConfigPolicy.strictParsing") public class WildcardPermissionTest { + private boolean strictParsing; + + @BeforeEach + void saveStrictProperty() { + strictParsing = AuthorizationConfigPolicy.isStrictParsing(); + } + + @AfterEach + void restoreStrictProperty() { + AuthorizationConfigPolicy.setStrictParsing(strictParsing); + } + + @Test + void strictParsingRejectsEmptyPartsAndSubparts() { + AuthorizationConfigPolicy.setStrictParsing(true); + for (String permission : new String[] {"doc:read:", "doc:read::", ":doc:read", "doc::read", "doc:read,", + "doc:,read", "doc:read,,write", "doc:read: ", "doc: :id", "doc:read, ,write", ":", ","}) { + assertThatExceptionOfType(IllegalArgumentException.class) + .as("constructor rejects '%s'", permission) + .isThrownBy(() -> new WildcardPermission(permission)); + assertThatExceptionOfType(IllegalArgumentException.class) + .as("resolver rejects '%s'", permission) + .isThrownBy(() -> new WildcardPermissionResolver().resolvePermission(permission)); + } + } + + @Test + void legacyParsingRetainsTrailingSeparatorBehavior() { + AuthorizationConfigPolicy.setStrictParsing(false); + assertThat(new WildcardPermission("doc:read:")).isEqualTo(new WildcardPermission("doc:read")); + assertThat(new WildcardPermission("doc:read,")).isEqualTo(new WildcardPermission("doc:read")); + assertThat(new WildcardPermission("doc:read:").implies(new WildcardPermission("doc:read:any"))).isTrue(); + } + + @Test + void literalPartsRejectUnsafeValuesRegardlessOfStrictMode() { + for (boolean strict : new boolean[] {false, true}) { + AuthorizationConfigPolicy.setStrictParsing(strict); + for (String value : new String[] {null, "", " ", "\t", "alice:admin", "alice,bob", "*", "alice*", + " alice", "alice "}) { + assertThatExceptionOfType(IllegalArgumentException.class) + .as("literal instance rejects '%s' with strict=%s", value, strict) + .isThrownBy(() -> WildcardPermission.fromLiteralParts("doc", "read", value)); + assertThatExceptionOfType(IllegalArgumentException.class) + .as("literal domain rejects '%s' with strict=%s", value, strict) + .isThrownBy(() -> WildcardPermission.fromLiteralParts(true, value, "read", "id")); + } + } + } + + @Test + void literalPartsRequireAtLeastOnePart() { + assertThatExceptionOfType(IllegalArgumentException.class) + .isThrownBy(() -> WildcardPermission.fromLiteralParts()); + assertThatExceptionOfType(IllegalArgumentException.class) + .isThrownBy(() -> WildcardPermission.fromLiteralParts((String[]) null)); + } + + @Test + void literalPermissionsInteroperateWithWildcardGrantsWithoutBroadeningAccess() { + AuthorizationConfigPolicy.setStrictParsing(true); + WildcardPermission permission = WildcardPermission.fromLiteralParts("doc", "read", "Alice"); + assertThat(permission).isEqualTo(new WildcardPermission("doc:read:alice")); + assertThat(new WildcardPermission(permission.toString())).isEqualTo(permission); + assertThat(permission.implies(WildcardPermission.fromLiteralParts("doc", "read", "Bob"))).isFalse(); + assertThat(permission.implies(new WildcardPermission("doc:read:*"))).isFalse(); + assertThat(new WildcardPermission("doc:read:*").implies(permission)).isTrue(); + assertThat(new WildcardPermission("doc:read").implies(permission)).isTrue(); + assertThat(WildcardPermission.fromLiteralParts(true, "doc", "read", "Alice") + .implies(new WildcardPermission("doc:read:alice", true))).isFalse(); + } + @Test void testNull() { assertThatExceptionOfType(IllegalArgumentException.class).isThrownBy(() -> { diff --git a/support/cdi/src/main/java/org/apache/shiro/cdi/ShiroSecurityExtension.java b/support/cdi/src/main/java/org/apache/shiro/cdi/ShiroSecurityExtension.java index 2e0ac0c06..4dcfe6569 100644 --- a/support/cdi/src/main/java/org/apache/shiro/cdi/ShiroSecurityExtension.java +++ b/support/cdi/src/main/java/org/apache/shiro/cdi/ShiroSecurityExtension.java @@ -20,12 +20,12 @@ import jakarta.enterprise.event.Observes; import jakarta.enterprise.inject.spi.Extension; import jakarta.enterprise.inject.spi.ProcessAnnotatedType; import jakarta.enterprise.inject.spi.WithAnnotations; - import org.apache.shiro.authz.annotation.RequiresAuthentication; import org.apache.shiro.authz.annotation.RequiresGuest; import org.apache.shiro.authz.annotation.RequiresPermissions; import org.apache.shiro.authz.annotation.RequiresRoles; import org.apache.shiro.authz.annotation.RequiresUser; +import org.apache.shiro.authz.aop.AuthorizingAnnotationHandler.AuthorizationConfigPolicy; /** * Automatically apply Shiro security to all appropriate beans @@ -39,6 +39,9 @@ public class ShiroSecurityExtension implements Extension { RequiresAuthentication.class, RequiresGuest.class, RequiresPermissions.class, RequiresRoles.class, RequiresUser.class, RolesAllowed.class, PermitAll.class, DenyAll.class}) ProcessAnnotatedType<T> pat) { + pat.getAnnotatedType().getAnnotations().forEach(AuthorizationConfigPolicy::validate); + pat.getAnnotatedType().getMethods().forEach(method -> + method.getAnnotations().forEach(AuthorizationConfigPolicy::validate)); pat.setAnnotatedType(new AnnotatedTypeWrapper<>(pat.getAnnotatedType(), ShiroSecureAnnotated.class.getDeclaredAnnotations()[0])); } diff --git a/support/guice/src/main/java/org/apache/shiro/guice/aop/ShiroAopModule.java b/support/guice/src/main/java/org/apache/shiro/guice/aop/ShiroAopModule.java index 39500d014..3b14432ee 100644 --- a/support/guice/src/main/java/org/apache/shiro/guice/aop/ShiroAopModule.java +++ b/support/guice/src/main/java/org/apache/shiro/guice/aop/ShiroAopModule.java @@ -24,12 +24,14 @@ import org.apache.shiro.aop.AnnotationMethodInterceptor; import org.apache.shiro.aop.AnnotationResolver; import org.apache.shiro.aop.DefaultAnnotationResolver; import org.apache.shiro.authz.aop.AuthenticatedAnnotationMethodInterceptor; +import org.apache.shiro.authz.aop.AuthorizingAnnotationHandler.AuthorizationConfigPolicy; import org.apache.shiro.authz.aop.GuestAnnotationMethodInterceptor; import org.apache.shiro.authz.aop.PermissionAnnotationMethodInterceptor; import org.apache.shiro.authz.aop.RoleAnnotationMethodInterceptor; import org.apache.shiro.authz.aop.UserAnnotationMethodInterceptor; import java.lang.annotation.Annotation; +import java.util.Optional; import static org.apache.shiro.aop.DefaultAnnotationResolver.getAnnotationFromClassHierarchy; /** @@ -47,8 +49,9 @@ public class ShiroAopModule extends AbstractModule { protected final void bindShiroInterceptor(final AnnotationMethodInterceptor methodInterceptor) { bindInterceptor(Matchers.any(), method -> { Class<? extends Annotation> annotation = methodInterceptor.getHandler().getAnnotationClass(); - return method.getAnnotation(annotation) != null - || getAnnotationFromClassHierarchy(method.getDeclaringClass(), annotation) != null; + return Optional.<Annotation>ofNullable(method.getAnnotation(annotation)) + .or(() -> Optional.ofNullable(getAnnotationFromClassHierarchy(method.getDeclaringClass(), annotation))) + .map(AuthorizationConfigPolicy::validate).isPresent(); }, new AopAllianceMethodInterceptorAdapter(methodInterceptor)); } diff --git a/support/jaxrs/src/main/java/org/apache/shiro/web/jaxrs/AnnotationAuthorizationFilter.java b/support/jaxrs/src/main/java/org/apache/shiro/web/jaxrs/AnnotationAuthorizationFilter.java index c51ac4c6e..3f519a48d 100644 --- a/support/jaxrs/src/main/java/org/apache/shiro/web/jaxrs/AnnotationAuthorizationFilter.java +++ b/support/jaxrs/src/main/java/org/apache/shiro/web/jaxrs/AnnotationAuthorizationFilter.java @@ -26,6 +26,7 @@ import org.apache.shiro.authz.annotation.RequiresRoles; import org.apache.shiro.authz.annotation.RequiresUser; import org.apache.shiro.authz.aop.AuthenticatedAnnotationHandler; import org.apache.shiro.authz.aop.AuthorizingAnnotationHandler; +import org.apache.shiro.authz.aop.AuthorizingAnnotationHandler.AuthorizationConfigPolicy; import org.apache.shiro.authz.aop.DenyAllAnnotationHandler; import org.apache.shiro.authz.aop.GuestAnnotationHandler; import org.apache.shiro.authz.aop.PermissionAnnotationHandler; @@ -60,6 +61,7 @@ public class AnnotationAuthorizationFilter implements ContainerRequestFilter { Map<AuthorizingAnnotationHandler, Annotation> authChecks = new HashMap<AuthorizingAnnotationHandler, Annotation>(authzSpecs.size()); for (Annotation authSpec : authzSpecs) { + AuthorizationConfigPolicy.validate(authSpec); authChecks.put(createHandler(authSpec), authSpec); } this.authzChecks = Collections.unmodifiableMap(authChecks); diff --git a/support/spring/src/main/java/org/apache/shiro/spring/security/interceptor/AuthorizationAttributeSourceAdvisor.java b/support/spring/src/main/java/org/apache/shiro/spring/security/interceptor/AuthorizationAttributeSourceAdvisor.java index 5fb466c45..2f8234ace 100644 --- a/support/spring/src/main/java/org/apache/shiro/spring/security/interceptor/AuthorizationAttributeSourceAdvisor.java +++ b/support/spring/src/main/java/org/apache/shiro/spring/security/interceptor/AuthorizationAttributeSourceAdvisor.java @@ -18,6 +18,7 @@ */ package org.apache.shiro.spring.security.interceptor; +import org.apache.shiro.authz.aop.AuthorizingAnnotationHandler.AuthorizationConfigPolicy; import org.apache.shiro.authz.annotation.RequiresAuthentication; import org.apache.shiro.authz.annotation.RequiresGuest; import org.apache.shiro.authz.annotation.RequiresPermissions; @@ -109,6 +110,7 @@ public class AuthorizationAttributeSourceAdvisor extends StaticMethodMatcherPoin for (Class<? extends Annotation> annClass : AUTHZ_ANNOTATION_CLASSES) { Annotation a = AnnotationUtils.findAnnotation(targetClazz, annClass); if (a != null) { + AuthorizationConfigPolicy.validate(a); return true; } } @@ -119,6 +121,7 @@ public class AuthorizationAttributeSourceAdvisor extends StaticMethodMatcherPoin for (Class<? extends Annotation> annClass : AUTHZ_ANNOTATION_CLASSES) { Annotation a = AnnotationUtils.findAnnotation(method, annClass); if (a != null) { + AuthorizationConfigPolicy.validate(a); return true; } } diff --git a/web/src/main/java/org/apache/shiro/web/filter/authz/PermissionsAuthorizationFilter.java b/web/src/main/java/org/apache/shiro/web/filter/authz/PermissionsAuthorizationFilter.java index d6fe79afd..f1608a4f6 100644 --- a/web/src/main/java/org/apache/shiro/web/filter/authz/PermissionsAuthorizationFilter.java +++ b/web/src/main/java/org/apache/shiro/web/filter/authz/PermissionsAuthorizationFilter.java @@ -19,11 +19,14 @@ package org.apache.shiro.web.filter.authz; import java.io.IOException; +import jakarta.servlet.Filter; import jakarta.servlet.ServletRequest; import jakarta.servlet.ServletResponse; - +import org.apache.shiro.authz.aop.AuthorizingAnnotationHandler.AuthorizationConfigPolicy; import org.apache.shiro.subject.Subject; +import static org.apache.shiro.lang.util.StringUtils.split; + /** * Filter that allows access if the current user has the permissions specified by the mapped value, or denies access * if the user does not have all of the permissions specified. @@ -34,6 +37,13 @@ public class PermissionsAuthorizationFilter extends AuthorizationFilter { //TODO - complete JavaDoc + @Override + public Filter processPathConfig(String path, String config) { + AuthorizationConfigPolicy.validate(split(config), + "Path '" + path + "' of " + getClass().getSimpleName()); + return super.processPathConfig(path, config); + } + @Override public boolean isAccessAllowed(ServletRequest request, ServletResponse response, Object mappedValue) throws IOException { diff --git a/web/src/main/java/org/apache/shiro/web/filter/authz/RolesAuthorizationFilter.java b/web/src/main/java/org/apache/shiro/web/filter/authz/RolesAuthorizationFilter.java index a3995c4fc..09f92fd99 100644 --- a/web/src/main/java/org/apache/shiro/web/filter/authz/RolesAuthorizationFilter.java +++ b/web/src/main/java/org/apache/shiro/web/filter/authz/RolesAuthorizationFilter.java @@ -20,13 +20,16 @@ package org.apache.shiro.web.filter.authz; import java.io.IOException; import java.util.Set; +import jakarta.servlet.Filter; import jakarta.servlet.ServletRequest; import jakarta.servlet.ServletResponse; - +import org.apache.shiro.authz.aop.AuthorizingAnnotationHandler.AuthorizationConfigPolicy; import org.apache.shiro.subject.Subject; import org.apache.shiro.util.CollectionUtils; +import static org.apache.shiro.lang.util.StringUtils.split; + /** * Filter that allows access if the current user has the roles specified by the mapped value, or denies access * if the user does not have all of the roles specified. @@ -37,7 +40,13 @@ public class RolesAuthorizationFilter extends AuthorizationFilter { //TODO - complete JavaDoc - @SuppressWarnings({"unchecked"}) + @Override + public Filter processPathConfig(String path, String config) { + AuthorizationConfigPolicy.validate(split(config), + "Path '" + path + "' of " + getClass().getSimpleName()); + return super.processPathConfig(path, config); + } + @Override public boolean isAccessAllowed(ServletRequest request, ServletResponse response, Object mappedValue) throws IOException { diff --git a/web/src/test/java/org/apache/shiro/web/filter/authz/EmptyAuthorizationConfigFilterTest.java b/web/src/test/java/org/apache/shiro/web/filter/authz/EmptyAuthorizationConfigFilterTest.java new file mode 100644 index 000000000..2c8ca8f98 --- /dev/null +++ b/web/src/test/java/org/apache/shiro/web/filter/authz/EmptyAuthorizationConfigFilterTest.java @@ -0,0 +1,104 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 + * + * http://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.apache.shiro.web.filter.authz; + +import org.apache.shiro.subject.Subject; +import org.apache.shiro.util.ThreadContext; +import org.apache.shiro.authz.aop.AuthorizingAnnotationHandler.AuthorizationConfigPolicy; +import org.apache.shiro.config.ConfigurationException; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.parallel.ResourceLock; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +import jakarta.servlet.ServletRequest; +import jakarta.servlet.ServletResponse; + +import java.util.stream.Stream; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatExceptionOfType; +import static org.easymock.EasyMock.createNiceMock; + +/** + * Tests that the {@code perms} and {@code roles} filters warn and allow by default, and reject the configuration + * at startup when so configured, if no permissions or roles are configured. + */ +@ResourceLock("AuthorizationConfigPolicy.strictParsing") +class EmptyAuthorizationConfigFilterTest { + private boolean strictParsing; + private final ServletRequest request = createNiceMock(ServletRequest.class); + private final ServletResponse response = createNiceMock(ServletResponse.class); + + @BeforeEach + void bindSubject() { + strictParsing = AuthorizationConfigPolicy.isStrictParsing(); + AuthorizationConfigPolicy.setStrictParsing(false); + Subject subject = createNiceMock(Subject.class); + ThreadContext.bind(subject); + } + + @AfterEach + void unbind() { + AuthorizationConfigPolicy.setStrictParsing(strictParsing); + ThreadContext.remove(); + } + + @FunctionalInterface + private interface AccessCheck { + boolean isAccessAllowed(ServletRequest request, ServletResponse response, Object mappedValue) + throws Exception; + } + + static Stream<Arguments> filters() { + PermissionsAuthorizationFilter perms = new PermissionsAuthorizationFilter(); + RolesAuthorizationFilter roles = new RolesAuthorizationFilter(); + return Stream.of( + Arguments.of(perms, (AccessCheck) perms::isAccessAllowed), + Arguments.of(roles, (AccessCheck) roles::isAccessAllowed)); + } + + @ParameterizedTest + @MethodSource("filters") + void allowsEmptyConfigByDefault(AuthorizationFilter filter, AccessCheck check) throws Exception { + filter.processPathConfig("/admin/**", null); + assertThat(check.isAccessAllowed(request, response, null)).isTrue(); + assertThat(check.isAccessAllowed(request, response, new String[0])).isTrue(); + } + + @ParameterizedTest + @MethodSource("filters") + void rejectsEmptyConfigAtStartupWhenStrict(AuthorizationFilter filter, AccessCheck check) { + AuthorizationConfigPolicy.setStrictParsing(true); + assertThatExceptionOfType(ConfigurationException.class) + .isThrownBy(() -> filter.processPathConfig("/admin/**", null)); + assertThatExceptionOfType(ConfigurationException.class) + .isThrownBy(() -> filter.processPathConfig("/admin/**", "")); + } + + @ParameterizedTest + @MethodSource("filters") + void acceptsConfigWhenStrict(AuthorizationFilter filter, AccessCheck check) { + AuthorizationConfigPolicy.setStrictParsing(true); + assertThatCode(() -> filter.processPathConfig("/admin/**", "admin")).doesNotThrowAnyException(); + } +}
