fix(auth): add the default group during the SecurityRule check (#1816)

Fix #1786
tchiotludo · Jun 27, 2024 · ef8e8f4 · ef8e8f4
1 parent 0be719d
commit ef8e8f4
Show file tree

Hide file tree

Showing 10 changed files with 89 additions and 82 deletions.
diff --git a/src/main/java/org/akhq/controllers/AbstractController.java b/src/main/java/org/akhq/controllers/AbstractController.java
@@ -42,33 +42,29 @@ protected URI uri(String path) throws URISyntaxException {
     }
 
     protected List<Group> getUserGroups() {
-        Optional<Authentication> authentication = applicationContext.containsBean(SecurityService.class) ?
-            applicationContext.getBean(SecurityService.class).getAuthentication() :
-            Optional.empty();
-
-        if (authentication.isEmpty()) {
+        // Authentication disabled, no groups to return
+        if (!applicationContext.containsBean(SecurityService.class)) {
             return List.of();
         }
 
-        var groups = AKHQSecurityRule.decompressGroups(authentication.get());
-
-        if (groups == null) {
-           return List.of();
-        }
+        Optional<Authentication> authentication = applicationContext.getBean(SecurityService.class).getAuthentication();
 
-        List<Group> groupBindings = groups.values()
-            .stream()
-            .flatMap(Collection::stream)
-            .map(gb -> new ObjectMapper().convertValue(gb, Group.class))
-            .collect(Collectors.toList());
+        var groups = new ArrayList<Group>();
 
         // Add the default group if there is one
-        if (groupBindings.isEmpty() && StringUtils.isNotEmpty(securityProperties.getDefaultGroup())
-            && securityProperties.getGroups().get(securityProperties.getDefaultGroup()) != null) {
-            groupBindings.addAll(securityProperties.getGroups().get(securityProperties.getDefaultGroup()));
+        if (securityProperties.getGroups().get(securityProperties.getDefaultGroup()) != null) {
+            groups.addAll(securityProperties.getGroups().get(securityProperties.getDefaultGroup()));
         }
 
-        return groupBindings;
+        // Add user groups
+        authentication.ifPresent(value -> groups.addAll(
+            AKHQSecurityRule.decompressGroups(value).values().stream()
+                .flatMap(Collection::stream)
+                .map(gb -> new ObjectMapper().convertValue(gb, Group.class))
+                .toList())
+        );
+
+        return groups;
     }
 
     /**
@@ -78,8 +74,8 @@ protected List<Group> getUserGroups() {
      * @return
      */
     protected List<String> buildUserBasedResourceFilters(String cluster) {
-        if (!applicationContext.containsBean(SecurityService.class)
-            || applicationContext.getBean(SecurityService.class).getAuthentication().isEmpty())
+        // Authentication disabled, we allow everything
+        if (!applicationContext.containsBean(SecurityService.class))
             return List.of();
 
         AKHQSecured annotation;
@@ -141,26 +137,16 @@ protected void checkIfClusterAndResourceAllowed(String cluster, List<String> res
     }
 
     protected void checkIfClusterAndResourceAllowed(String cluster, String resource) {
-        Optional<Authentication> authentication = applicationContext.containsBean(SecurityService.class) ?
-            applicationContext.getBean(SecurityService.class).getAuthentication() :
-            Optional.empty();
-
-        if (authentication.isEmpty())
+        // Authentication disabled, we allow everything
+        if (!applicationContext.containsBean(SecurityService.class))
             return;
 
-        StackWalker.StackFrame sf = walker.walk(frames ->
-            frames.filter(frame -> frame.getDeclaringClass().equals(getClass()))
-                .findFirst()
-                .orElseThrow());
-
         boolean isAllowed;
 
         try {
             AKHQSecured annotation = getCallingAKHQSecuredAnnotation();
 
-            isAllowed = AKHQSecurityRule.decompressGroups(authentication.get()).values().stream()
-                .flatMap(Collection::stream)
-                .map(gb -> new ObjectMapper().convertValue(gb, Group.class))
+            isAllowed = getUserGroups().stream()
                 // Get only group with role matching the method annotation resource and action
                 .filter(groupBinding -> securityProperties.getRoles().entrySet().stream()
                     .filter(role -> groupBinding.getRole().equals(role.getKey()))
@@ -184,7 +170,8 @@ protected void checkIfClusterAndResourceAllowed(String cluster, String resource)
         }
 
         if (!isAllowed) {
-            throw new AuthorizationException(authentication.get());
+            throw new AuthorizationException(applicationContext.getBean(SecurityService.class).getAuthentication()
+                .orElse(null));
         }
     }
 }
diff --git a/src/main/java/org/akhq/controllers/AkhqController.java b/src/main/java/org/akhq/controllers/AkhqController.java
@@ -4,6 +4,7 @@
 import io.micronaut.context.ApplicationContext;
 import io.micronaut.core.annotation.Introspected;
 import io.micronaut.core.annotation.Nullable;
+import io.micronaut.core.util.StringUtils;
 import io.micronaut.http.HttpResponse;
 import io.micronaut.http.MediaType;
 import io.micronaut.http.annotation.Controller;
@@ -25,10 +26,7 @@
 
 import jakarta.inject.Inject;
 
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.List;
-import java.util.Map;
+import java.util.*;
 import java.util.stream.Collectors;
 
 @Controller
@@ -207,7 +205,9 @@ private List<AuthUser.AuthPermissions> expandRoles(List<Group> groupBindings) {
             throw new RuntimeException("Roles has not been defined properly. Please check the documentation");
         }
 
-        return groupBindings.stream()
+        return Optional.ofNullable(groupBindings)
+            .orElseGet(Collections::emptyList)
+            .stream()
             .map(binding -> securityProperties.getRoles().entrySet().stream()
                 .filter(role -> role.getKey().equals(binding.getRole()))
                 .map(Map.Entry::getValue)
@@ -219,10 +219,14 @@ private List<AuthUser.AuthPermissions> expandRoles(List<Group> groupBindings) {
     }
 
     protected List<AuthUser.AuthPermissions> getRights() {
-        if (!applicationContext.containsBean(SecurityService.class)) {
+        // Authentication disabled or authentication enabled but user not logged in
+        // return rights for the default group
+        if (!applicationContext.containsBean(SecurityService.class)
+            || applicationContext.getBean(SecurityService.class).getAuthentication().isEmpty()) {
             return expandRoles(securityProperties.getGroups().get(securityProperties.getDefaultGroup()));
         }
 
+        // Authentication enabled and user logged in
         return expandRoles(getUserGroups());
     }
 

diff --git a/src/main/java/org/akhq/security/authentication/BasicAuthAuthenticationProvider.java b/src/main/java/org/akhq/security/authentication/BasicAuthAuthenticationProvider.java
@@ -2,32 +2,37 @@
 
 import io.micronaut.core.annotation.Nullable;
 import io.micronaut.http.HttpRequest;
-import io.micronaut.security.authentication.*;
+import io.micronaut.security.authentication.AuthenticationFailed;
+import io.micronaut.security.authentication.AuthenticationFailureReason;
+import io.micronaut.security.authentication.AuthenticationRequest;
+import io.micronaut.security.authentication.AuthenticationResponse;
+import io.micronaut.security.authentication.provider.HttpRequestReactiveAuthenticationProvider;
 import io.micronaut.security.rules.SecurityRule;
 import io.reactivex.Flowable;
 import jakarta.inject.Inject;
 import jakarta.inject.Singleton;
 import org.akhq.configs.security.BasicAuth;
 import org.akhq.configs.security.SecurityProperties;
-import org.akhq.models.security.ClaimRequest;
-import org.akhq.models.security.ClaimResponse;
 import org.akhq.models.security.ClaimProvider;
 import org.akhq.models.security.ClaimProviderType;
+import org.akhq.models.security.ClaimRequest;
+import org.akhq.models.security.ClaimResponse;
 import org.reactivestreams.Publisher;
 
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 
 @Singleton
-public class BasicAuthAuthenticationProvider implements AuthenticationProvider<HttpRequest<?>> {
+public class BasicAuthAuthenticationProvider<B> implements HttpRequestReactiveAuthenticationProvider<B> {
     @Inject
     private SecurityProperties securityProperties;
     @Inject
     private ClaimProvider claimProvider;
 
     @Override
-    public Publisher<AuthenticationResponse> authenticate(@Nullable HttpRequest<?> httpRequest, AuthenticationRequest<?, ?> authenticationRequest) {
+    public Publisher<AuthenticationResponse> authenticate(@Nullable HttpRequest<B> httpRequest,
+                                                          AuthenticationRequest<String, String> authenticationRequest) {
         String username = String.valueOf(authenticationRequest.getIdentity());
         Optional<BasicAuth> optionalBasicAuth = securityProperties.getBasicAuth()
                 .stream()

diff --git a/src/main/java/org/akhq/security/rule/AKHQSecurityRule.java b/src/main/java/org/akhq/security/rule/AKHQSecurityRule.java
@@ -65,17 +65,29 @@ public Publisher<SecurityRuleResult> check(HttpRequest<?> request, Authenticatio
         }
         String cluster = routeMatch.getVariableValues().get("cluster").toString();
 
-        if (authentication == null) {
+        // No authentication information provided or no existing default group, we reject the request
+        if (authentication == null && (securityProperties.getDefaultGroup() == null
+            || securityProperties.getGroups().get(securityProperties.getDefaultGroup()) == null)) {
             log.warn("No authentication information provided");
             return Flowable.just(SecurityRuleResult.REJECTED);
         }
 
-        List<Group> userGroups = decompressGroups(authentication).values().stream()
-            .flatMap(Collection::stream)
-            // Type mismatch during serialization from LinkedTreeMap to Group if we use List<Group>
-            // Need to serialize Object to Group manually in the stream
-            .map(gb -> new ObjectMapper().convertValue(gb, Group.class))
-            .collect(Collectors.toList());
+        List<Group> userGroups = new ArrayList<>();
+
+        if (authentication != null) {
+            // Add user groups from the user token
+            userGroups = decompressGroups(authentication).values().stream()
+                .flatMap(Collection::stream)
+                // Type mismatch during serialization from LinkedTreeMap to Group if we use List<Group>
+                // Need to serialize Object to Group manually in the stream
+                .map(gb -> new ObjectMapper().convertValue(gb, Group.class))
+                .collect(Collectors.toList());
+        }
+
+        // Add default group anyway
+        if (securityProperties.getGroups().get(securityProperties.getDefaultGroup()) != null) {
+            userGroups.addAll(securityProperties.getGroups().get(securityProperties.getDefaultGroup()));
+        }
 
         boolean allowed = userGroups.stream()
             // Keep only bindings matching on cluster name

diff --git a/src/main/java/org/akhq/security/rule/SecuredAnnotationRuleWithDefault.java b/src/main/java/org/akhq/security/rule/SecuredAnnotationRuleWithDefault.java
@@ -13,6 +13,7 @@
 import io.reactivex.Flowable;
 import jakarta.inject.Inject;
 import jakarta.inject.Singleton;
+import org.akhq.configs.security.SecurityProperties;
 import org.akhq.security.annotation.HasAnyPermission;
 import org.reactivestreams.Publisher;
 
@@ -21,6 +22,9 @@
 @Singleton
 @Replaces(SecuredAnnotationRule.class)
 public class SecuredAnnotationRuleWithDefault extends SecuredAnnotationRule {
+    @Inject
+    protected SecurityProperties securityProperties;
+
     @Inject
     SecuredAnnotationRuleWithDefault(RolesFinder rolesFinder) {
         super(rolesFinder);
@@ -40,10 +44,7 @@ public Publisher<SecurityRuleResult> check(HttpRequest<?> request, Authenticatio
 
         MethodBasedRouteMatch<?, ?> methodRoute = ((MethodBasedRouteMatch<?, ?>) routeMatch);
         if (methodRoute.hasAnnotation(HasAnyPermission.class)) {
-            if (getRoles(authentication)
-                .stream()
-                .anyMatch(s -> !s.equals(SecurityRule.IS_ANONYMOUS))
-            ) {
+            if (authentication != null || securityProperties.getDefaultGroup() != null) {
                 return Flowable.just(SecurityRuleResult.ALLOWED);
             } else {
                 return Flowable.just(SecurityRuleResult.REJECTED);

diff --git a/src/test/java/org/akhq/controllers/AkhqControllerTest.java b/src/test/java/org/akhq/controllers/AkhqControllerTest.java
@@ -51,10 +51,10 @@ void user() {
         );
 
         assertEquals("admin", result.getUsername());
-        assertEquals(2, result.getRoles().size());
+        assertEquals(3, result.getRoles().size());
         assertThat(result.getRoles().stream().map(AkhqController.AuthUser.AuthPermissions::getPatterns).flatMap(Collection::stream).collect(Collectors.toList()),
-            containsInAnyOrder(".*", "user.*"));
+            containsInAnyOrder(".*", "user.*", "public.*"));
         assertThat(result.getRoles().stream().map(AkhqController.AuthUser.AuthPermissions::getClusters).flatMap(Collection::stream).collect(Collectors.toList()),
-            containsInAnyOrder(".*", ".*"));
+            containsInAnyOrder(".*", ".*", "pub.*"));
     }
 }
diff --git a/src/test/java/org/akhq/controllers/HeaderAuthControllerTest.java b/src/test/java/org/akhq/controllers/HeaderAuthControllerTest.java
@@ -33,11 +33,11 @@ void user() {
         );
 
         assertEquals("header-user", result.getUsername());
-        assertEquals(2, result.getRoles().size());
+        assertEquals(3, result.getRoles().size());
         assertThat(result.getRoles().stream().map(AkhqController.AuthUser.AuthPermissions::getPatterns).flatMap(Collection::stream).collect(Collectors.toList()),
-            containsInAnyOrder("test-operator.*", "test-operator.*"));
+            containsInAnyOrder("test-operator.*", "test-operator.*", "public.*"));
         assertThat(result.getRoles().stream().map(AkhqController.AuthUser.AuthPermissions::getClusters).flatMap(Collection::stream).collect(Collectors.toList()),
-            containsInAnyOrder(".*", ".*"));
+            containsInAnyOrder(".*", ".*", "pub.*"));
     }
 
     @Test
@@ -50,11 +50,11 @@ void admin() {
         );
 
         assertEquals("header-admin", result.getUsername());
-        assertEquals(2, result.getRoles().size());
+        assertEquals(3, result.getRoles().size());
         assertThat(result.getRoles().stream().map(AkhqController.AuthUser.AuthPermissions::getPatterns).flatMap(Collection::stream).collect(Collectors.toList()),
-            containsInAnyOrder(".*", "user.*"));
+            containsInAnyOrder(".*", "user.*", "public.*"));
         assertThat(result.getRoles().stream().map(AkhqController.AuthUser.AuthPermissions::getClusters).flatMap(Collection::stream).collect(Collectors.toList()),
-            containsInAnyOrder(".*", ".*"));
+            containsInAnyOrder(".*", ".*", "pub.*"));
     }
 
     @Test
@@ -68,11 +68,11 @@ void externalUserAndGroup() {
         );
 
         assertEquals("header-user-operator", result.getUsername());
-        assertEquals(5, result.getRoles().size());
+        assertEquals(6, result.getRoles().size());
         assertThat(result.getRoles().stream().map(AkhqController.AuthUser.AuthPermissions::getPatterns).flatMap(Collection::stream).collect(Collectors.toList()),
-            containsInAnyOrder("test.*", "test.*", "user.*", "test-operator.*", "test-operator.*"));
+            containsInAnyOrder("test.*", "test.*", "user.*", "test-operator.*", "test-operator.*", "public.*"));
         assertThat(result.getRoles().stream().map(AkhqController.AuthUser.AuthPermissions::getClusters).flatMap(Collection::stream).collect(Collectors.toList()),
-            containsInAnyOrder("pub.*", "pub.*", "pub.*", ".*", ".*"));
+            containsInAnyOrder("pub.*", "pub.*", "pub.*", ".*", ".*", "pub.*"));
     }
 
     @Test
@@ -87,11 +87,11 @@ void userWithAdditionalExternalGroup() {
 
         assertEquals("header-user", result.getUsername());
         // operator from 'users' and externally provided 'limited'
-        assertEquals(5, result.getRoles().size());
+        assertEquals(6, result.getRoles().size());
         assertThat(result.getRoles().stream().map(AkhqController.AuthUser.AuthPermissions::getPatterns).flatMap(Collection::stream).collect(Collectors.toList()),
-            containsInAnyOrder("test.*", "test.*", "user.*", "test-operator.*", "test-operator.*"));
+            containsInAnyOrder("test.*", "test.*", "user.*", "test-operator.*", "test-operator.*", "public.*"));
         assertThat(result.getRoles().stream().map(AkhqController.AuthUser.AuthPermissions::getClusters).flatMap(Collection::stream).collect(Collectors.toList()),
-            containsInAnyOrder("pub.*", "pub.*", "pub.*", ".*", ".*"));
+            containsInAnyOrder("pub.*", "pub.*", "pub.*", ".*", ".*", "pub.*"));
     }
 
     @Test
@@ -104,7 +104,7 @@ void userWithoutAnyGroup() {
         );
 
         assertEquals("header-invalid", result.getUsername());
-        assertEquals(4, result.getRoles().size());
+        assertEquals(1, result.getRoles().size());
     }
 
     @MicronautTest(environments = "header-ip-disallow")

diff --git a/src/test/java/org/akhq/security/authentication/BasicAuthAuthenticationProviderTest.java b/src/test/java/org/akhq/security/authentication/BasicAuthAuthenticationProviderTest.java
@@ -24,7 +24,7 @@ class BasicAuthAuthenticationProviderTest {
 
     @Test
     void success() {
-        AuthenticationResponse response = Flowable
+        AuthenticationResponse response = (AuthenticationResponse) Flowable
             .fromPublisher(auth.authenticate(null, new UsernamePasswordCredentials(
                 "user",
                 "pass"
@@ -47,7 +47,7 @@ void success() {
 
     @Test
     void successCase() {
-        AuthenticationResponse response = Flowable
+        AuthenticationResponse response = (AuthenticationResponse) Flowable
             .fromPublisher(auth.authenticate(null, new UsernamePasswordCredentials(
                 "MyUser3!@yàhöù.com",
                 "pass"
@@ -71,7 +71,7 @@ void successCase() {
 
     @Test
     void failed_UserNotFound() {
-        AuthenticationResponse response = Flowable
+        AuthenticationResponse response = (AuthenticationResponse) Flowable
             .fromPublisher(auth.authenticate(null, new UsernamePasswordCredentials(
                 "user2",
                 "pass2"
@@ -84,7 +84,7 @@ void failed_UserNotFound() {
 
     @Test
     void failed_PasswordInvalid() {
-        AuthenticationResponse response = Flowable
+        AuthenticationResponse response = (AuthenticationResponse) Flowable
                 .fromPublisher(auth.authenticate(null, new UsernamePasswordCredentials(
                         "user",
                         "invalid-pass"

diff --git a/src/test/java/org/akhq/security/claim/GroovyClaimProviderTest.java b/src/test/java/org/akhq/security/claim/GroovyClaimProviderTest.java
@@ -24,7 +24,7 @@ class GroovyClaimProviderTest {
 
     @Test
     void successUser() {
-        AuthenticationResponse response = Flowable
+        AuthenticationResponse response = (AuthenticationResponse) Flowable
                 .fromPublisher(auth.authenticate(null, new UsernamePasswordCredentials(
                         "user",
                         "pass"

diff --git a/src/test/resources/application.yml b/src/test/resources/application.yml
@@ -117,10 +117,8 @@ akhq:
           patterns: [ "test-operator.*" ]
       no-filter:
         - role: topic-read
-        - role: topic-write
-        - role: schema-delete
-        - role: acl-read
-          patterns: [ "user.*" ]
+          patterns: [ "public.*" ]
+          clusters: [ "pub.*" ]
     default-group: no-filter
 
     basic-auth: