Merge pull request quarkusio#45012 from michalvavrik/feature/apply-pe…

…rm-check-on-final-identity Use final SecurityIdentity augmented with all the augmentors in @permissionchecker methods
gastaldi · Dec 10, 2024 · 90dc755 · 90dc755
2 parents 3762314 + 8f44d0d
commit 90dc755
Show file tree

Hide file tree

Showing 4 changed files with 169 additions and 21 deletions.
diff --git a/...rity/test/permissionsallowed/checker/SecurityIdentityAugmentorsPermissionCheckerTest.java b/...rity/test/permissionsallowed/checker/SecurityIdentityAugmentorsPermissionCheckerTest.java
@@ -0,0 +1,127 @@
+package io.quarkus.security.test.permissionsallowed.checker;
+
+import static io.quarkus.security.test.utils.IdentityMock.ADMIN;
+import static io.quarkus.security.test.utils.IdentityMock.USER;
+import static io.quarkus.security.test.utils.SecurityTestUtils.assertFailureFor;
+import static io.quarkus.security.test.utils.SecurityTestUtils.assertSuccess;
+
+import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.inject.Inject;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+
+import io.quarkus.logging.Log;
+import io.quarkus.security.ForbiddenException;
+import io.quarkus.security.PermissionChecker;
+import io.quarkus.security.PermissionsAllowed;
+import io.quarkus.security.identity.AuthenticationRequestContext;
+import io.quarkus.security.identity.SecurityIdentity;
+import io.quarkus.security.identity.SecurityIdentityAugmentor;
+import io.quarkus.security.runtime.QuarkusSecurityIdentity;
+import io.quarkus.security.test.utils.AuthData;
+import io.quarkus.security.test.utils.IdentityMock;
+import io.quarkus.security.test.utils.SecurityTestUtils;
+import io.quarkus.test.QuarkusUnitTest;
+import io.smallrye.mutiny.Uni;
+
+public class SecurityIdentityAugmentorsPermissionCheckerTest {
+
+    private static final AuthData USER_WITH_AUGMENTORS = new AuthData(USER, true);
+    private static final AuthData ADMIN_WITH_AUGMENTORS = new AuthData(ADMIN, true);
+
+    @RegisterExtension
+    static final QuarkusUnitTest config = new QuarkusUnitTest()
+            .withApplicationRoot((jar) -> jar.addClasses(IdentityMock.class, AuthData.class, SecurityTestUtils.class));
+
+    @Inject
+    SecuredBean bean;
+
+    /**
+     * Tests that {@link SecurityIdentity} passed to the {@link PermissionChecker} methods is augmented by all the
+     * augmentors (because that's the last operation we do on the identity, then it's de facto final).
+     */
+    @Test
+    public void testPermissionCheckerUsesAugmentedIdentity() {
+        assertSuccess(bean::securedMethod, "secured", ADMIN_WITH_AUGMENTORS);
+        assertFailureFor(bean::securedMethod, ForbiddenException.class, USER_WITH_AUGMENTORS);
+    }
+
+    @ApplicationScoped
+    public static class SecuredBean {
+
+        @PermissionsAllowed("canCallSecuredMethod")
+        String securedMethod() {
+            return "secured";
+        }
+
+        @PermissionChecker("canCallSecuredMethod")
+        boolean canCallSecuredMethod(SecurityIdentity identity) {
+            if (!identity.hasRole("lowest-priority-augmentor")) {
+                Log.error("Role granted by the augmentor with the smallest priority is missing");
+                return false;
+            }
+            if (!identity.hasRole("default-priority-augmentor")) {
+                Log.error("Role granted by the augmentor with a default priority is missing");
+                return false;
+            }
+            if (!identity.hasRole("highest-priority-augmentor")) {
+                Log.error("Role granted by the augmentor with the highest priority is missing");
+                return false;
+            }
+            return "admin".equals(identity.getPrincipal().getName());
+        }
+    }
+
+    @ApplicationScoped
+    public static class AugmentorWithLowestPriority implements SecurityIdentityAugmentor {
+
+        @Override
+        public int priority() {
+            return Integer.MIN_VALUE;
+        }
+
+        @Override
+        public Uni<SecurityIdentity> augment(SecurityIdentity securityIdentity,
+                AuthenticationRequestContext authenticationRequestContext) {
+            return Uni.createFrom().item(
+                    QuarkusSecurityIdentity
+                            .builder(securityIdentity)
+                            .addRole("lowest-priority-augmentor")
+                            .build());
+        }
+    }
+
+    @ApplicationScoped
+    public static class AugmentorWithDefaultPriority implements SecurityIdentityAugmentor {
+
+        @Override
+        public Uni<SecurityIdentity> augment(SecurityIdentity securityIdentity,
+                AuthenticationRequestContext authenticationRequestContext) {
+            return Uni.createFrom().item(
+                    QuarkusSecurityIdentity
+                            .builder(securityIdentity)
+                            .addRole("default-priority-augmentor")
+                            .build());
+        }
+    }
+
+    @ApplicationScoped
+    public static class AugmentorWithHighestPriority implements SecurityIdentityAugmentor {
+
+        @Override
+        public int priority() {
+            return Integer.MAX_VALUE;
+        }
+
+        @Override
+        public Uni<SecurityIdentity> augment(SecurityIdentity securityIdentity,
+                AuthenticationRequestContext authenticationRequestContext) {
+            return Uni.createFrom().item(
+                    QuarkusSecurityIdentity
+                            .builder(securityIdentity)
+                            .addRole("highest-priority-augmentor")
+                            .build());
+        }
+    }
+}
diff --git a/...runtime/src/main/java/io/quarkus/security/runtime/QuarkusIdentityProviderManagerImpl.java b/...runtime/src/main/java/io/quarkus/security/runtime/QuarkusIdentityProviderManagerImpl.java
@@ -182,6 +182,7 @@ public static class Builder {
 
         private final Map<Class<? extends AuthenticationRequest>, List<IdentityProvider<? extends AuthenticationRequest>>> providers = new HashMap<>();
         private final List<SecurityIdentityAugmentor> augmentors = new ArrayList<>();
+        private QuarkusPermissionSecurityIdentityAugmentor quarkusPermissionAugmentor = null;
         private BlockingSecurityExecutor blockingExecutor;
         private boolean built = false;
 
@@ -206,7 +207,11 @@ public Builder addProvider(IdentityProvider<? extends AuthenticationRequest> pro
          * @return this builder
          */
         public Builder addSecurityIdentityAugmentor(SecurityIdentityAugmentor augmentor) {
-            augmentors.add(augmentor);
+            if (augmentor instanceof QuarkusPermissionSecurityIdentityAugmentor quarkusPermissionAugmentor) {
+                this.quarkusPermissionAugmentor = quarkusPermissionAugmentor;
+            } else {
+                augmentors.add(augmentor);
+            }
             return this;
         }
 
@@ -254,6 +259,10 @@ public int compare(SecurityIdentityAugmentor o1, SecurityIdentityAugmentor o2) {
                     return Integer.compare(o2.priority(), o1.priority());
                 }
             });
+            if (quarkusPermissionAugmentor != null) {
+                // @PermissionChecker methods must always run with the final SecurityIdentity
+                augmentors.add(quarkusPermissionAugmentor);
+            }
             return new QuarkusIdentityProviderManagerImpl(this);
         }
     }

diff --git a/...src/main/java/io/quarkus/security/runtime/QuarkusPermissionSecurityIdentityAugmentor.java b/...src/main/java/io/quarkus/security/runtime/QuarkusPermissionSecurityIdentityAugmentor.java
@@ -61,4 +61,10 @@ public Uni<Boolean> apply(Permission requiredpermission) {
                 })
                 .build());
     }
+
+    @Override
+    public int priority() {
+        // we do not rely on this value and always add this augmentor as the last one manually
+        return Integer.MAX_VALUE;
+    }
 }
diff --git a/...nsions/security/test-utils/src/main/java/io/quarkus/security/test/utils/IdentityMock.java b/...nsions/security/test-utils/src/main/java/io/quarkus/security/test/utils/IdentityMock.java
@@ -6,21 +6,19 @@
 import java.util.HashSet;
 import java.util.Map;
 import java.util.Set;
-import java.util.function.Supplier;
 
 import jakarta.annotation.Priority;
 import jakarta.enterprise.context.ApplicationScoped;
 import jakarta.enterprise.inject.Alternative;
-import jakarta.enterprise.inject.Instance;
 import jakarta.inject.Inject;
 
-import io.quarkus.arc.Arc;
 import io.quarkus.security.credential.Credential;
 import io.quarkus.security.identity.AuthenticationRequestContext;
+import io.quarkus.security.identity.IdentityProvider;
+import io.quarkus.security.identity.IdentityProviderManager;
 import io.quarkus.security.identity.SecurityIdentity;
-import io.quarkus.security.identity.SecurityIdentityAugmentor;
+import io.quarkus.security.identity.request.BaseAuthenticationRequest;
 import io.quarkus.security.runtime.SecurityIdentityAssociation;
-import io.quarkus.security.spi.runtime.BlockingSecurityExecutor;
 import io.smallrye.mutiny.Uni;
 
 /**
@@ -107,42 +105,50 @@ public Uni<Boolean> checkPermission(Permission permission) {
     @ApplicationScoped
     @Priority(1)
     public static class IdentityAssociationMock extends SecurityIdentityAssociation {
+
         @Inject
         IdentityMock identity;
 
         @Inject
-        Instance<SecurityIdentityAugmentor> augmentors;
+        IdentityProviderManager identityProviderManager;
 
         @Override
         public Uni<SecurityIdentity> getDeferredIdentity() {
             if (applyAugmentors) {
-                return augmentIdentity(identity);
+                return identityProviderManager.authenticate(new IdentityMockAuthenticationRequest());
             }
             return Uni.createFrom().item(identity);
         }
 
         @Override
         public SecurityIdentity getIdentity() {
             if (applyAugmentors) {
-                return augmentIdentity(identity).await().indefinitely();
+                return getDeferredIdentity().await().indefinitely();
             }
             return identity;
         }
 
-        private Uni<SecurityIdentity> augmentIdentity(SecurityIdentity identity) {
-            var authReqContexts = new TestAuthenticationRequestContext();
-            Uni<SecurityIdentity> result = Uni.createFrom().item(identity);
-            for (SecurityIdentityAugmentor augmentor : augmentors) {
-                result = result.flatMap(si -> augmentor.augment(si, authReqContexts, Map.of()));
-            }
-            return result;
+    }
+
+    public static final class IdentityMockAuthenticationRequest extends BaseAuthenticationRequest {
+
+    }
+
+    @ApplicationScoped
+    public static final class IdentityMockProvider implements IdentityProvider<IdentityMockAuthenticationRequest> {
+
+        @Inject
+        IdentityMock identity;
+
+        @Override
+        public Class<IdentityMockAuthenticationRequest> getRequestType() {
+            return IdentityMockAuthenticationRequest.class;
         }
 
-        private static final class TestAuthenticationRequestContext implements AuthenticationRequestContext {
-            @Override
-            public Uni<SecurityIdentity> runBlocking(Supplier<SecurityIdentity> function) {
-                return Arc.container().instance(BlockingSecurityExecutor.class).get().executeBlocking(function);
-            }
+        @Override
+        public Uni<SecurityIdentity> authenticate(IdentityMockAuthenticationRequest identityMockAuthenticationRequest,
+                AuthenticationRequestContext authenticationRequestContext) {
+            return Uni.createFrom().item(identity);
         }
     }
 }