BE: address PR review comments

seono · seono · commit 7cbf653915c9 · 2025-06-12T23:21:56.000+09:00
diff --git a/api/src/main/java/io/kafbat/ui/config/auth/RoleBasedAccessControlProperties.java b/api/src/main/java/io/kafbat/ui/config/auth/RoleBasedAccessControlProperties.java
@@ -1,37 +1,38 @@
 package io.kafbat.ui.config.auth;
 
+import io.kafbat.ui.model.rbac.DefaultRole;
 import io.kafbat.ui.model.rbac.Role;
+import jakarta.annotation.Nullable;
 import jakarta.annotation.PostConstruct;
 import java.util.ArrayList;
 import java.util.List;
-import javax.annotation.Nullable;
 import org.springframework.boot.context.properties.ConfigurationProperties;
 
 @ConfigurationProperties("rbac")
 public class RoleBasedAccessControlProperties {
 
   private final List<Role> roles = new ArrayList<>();
 
-  private Role defaultRole;
+  private DefaultRole defaultRole;
 
   @PostConstruct
   public void init() {
     roles.forEach(Role::validate);
     if (defaultRole != null) {
-      defaultRole.validateDefaultRole();
+      defaultRole.validate();
     }
   }
 
   public List<Role> getRoles() {
     return roles;
   }
 
-  public void setDefaultRole(Role defaultRole) {
+  public void setDefaultRole(DefaultRole defaultRole) {
     this.defaultRole = defaultRole;
   }
 
   @Nullable
-  public Role getDefaultRole() {
+  public DefaultRole getDefaultRole() {
     return defaultRole;
   }
 }
diff --git a/api/src/main/java/io/kafbat/ui/model/rbac/DefaultRole.java b/api/src/main/java/io/kafbat/ui/model/rbac/DefaultRole.java
@@ -0,0 +1,21 @@
+package io.kafbat.ui.model.rbac;
+
+import static com.google.common.base.Preconditions.checkArgument;
+
+import java.util.ArrayList;
+import java.util.List;
+import lombok.Data;
+
+@Data
+public class DefaultRole {
+
+  private String name;
+  private List<String> clusters;
+  private List<Permission> permissions = new ArrayList<>();
+
+  public void validate() {
+    checkArgument(clusters != null && !clusters.isEmpty(), "Default role clusters cannot be empty");
+    permissions.forEach(Permission::validate);
+    permissions.forEach(Permission::transform);
+  }
+}
diff --git a/api/src/main/java/io/kafbat/ui/model/rbac/Role.java b/api/src/main/java/io/kafbat/ui/model/rbac/Role.java
@@ -21,10 +21,4 @@ public void validate() {
     permissions.forEach(Permission::transform);
     subjects.forEach(Subject::validate);
   }
-
-  // default role need only permissions
-  public void validateDefaultRole() {
-    permissions.forEach(Permission::validate);
-    permissions.forEach(Permission::transform);
-  }
 }
diff --git a/api/src/main/java/io/kafbat/ui/service/rbac/AccessControlService.java b/api/src/main/java/io/kafbat/ui/service/rbac/AccessControlService.java
@@ -6,8 +6,8 @@
 import io.kafbat.ui.model.ClusterDTO;
 import io.kafbat.ui.model.ConnectDTO;
 import io.kafbat.ui.model.InternalTopic;
-import io.kafbat.ui.model.KafkaCluster;
 import io.kafbat.ui.model.rbac.AccessContext;
+import io.kafbat.ui.model.rbac.DefaultRole;
 import io.kafbat.ui.model.rbac.Permission;
 import io.kafbat.ui.model.rbac.Role;
 import io.kafbat.ui.model.rbac.Subject;
@@ -69,27 +69,9 @@ public void init() {
       log.trace("No roles provided, disabling RBAC");
       return;
     }
-    if (properties.getDefaultRole() != null) {
-      log.trace("Set Default Role Clusters");
-      // set default role for all clusters
-      properties.getDefaultRole().setClusters(
-          clustersStorage.getKafkaClusters().stream()
-            .map(KafkaCluster::getName)
-            .collect(Collectors.toList())
-      );
-    }
     rbacEnabled = true;
 
-    if (properties.getDefaultRole() != null) {
-      // set all extractors for default role because it is applied to all clusters
-      this.oauthExtractors = Set.of(
-        new CognitoAuthorityExtractor(),
-        new GoogleAuthorityExtractor(),
-        new GithubAuthorityExtractor(),
-        new OauthAuthorityExtractor()
-      );
-    } else {
-      this.oauthExtractors = properties.getRoles()
+    this.oauthExtractors = properties.getRoles()
         .stream()
         .map(role -> role.getSubjects()
             .stream()
@@ -106,7 +88,6 @@ public void init() {
             .collect(Collectors.toSet()))
         .flatMap(Set::stream)
         .collect(Collectors.toSet());
-    }
 
     if (!(properties.getRoles().isEmpty() && properties.getDefaultRole() == null)
         && "oauth2".equalsIgnoreCase(environment.getProperty("auth.type"))
@@ -162,13 +143,15 @@ public static Mono<AuthenticatedUser> getUser() {
 
   private boolean isClusterAccessible(String clusterName, AuthenticatedUser user) {
     Assert.isTrue(StringUtils.isNotEmpty(clusterName), "cluster value is empty");
-    if (properties.getDefaultRole() != null) {
-      return true;
-    }
-    return properties.getRoles()
+    boolean isAccessible = properties.getRoles()
         .stream()
         .filter(filterRole(user))
         .anyMatch(role -> role.getClusters().stream().anyMatch(clusterName::equalsIgnoreCase));
+    
+    if (!isAccessible && properties.getDefaultRole() != null) {
+      return properties.getDefaultRole().getClusters().stream().anyMatch(clusterName::equalsIgnoreCase);
+    }
+    return isAccessible;
   }
 
   public Mono<Boolean> isClusterAccessible(ClusterDTO cluster) {
@@ -233,7 +216,7 @@ public List<Role> getRoles() {
     return Collections.unmodifiableList(properties.getRoles());
   }
 
-  public Role getDefaultRole() {
+  public DefaultRole getDefaultRole() {
     return properties.getDefaultRole();
   }
 
diff --git a/api/src/main/java/io/kafbat/ui/service/rbac/extractor/CognitoAuthorityExtractor.java b/api/src/main/java/io/kafbat/ui/service/rbac/extractor/CognitoAuthorityExtractor.java
@@ -39,13 +39,8 @@ public Mono<Set<String>> extract(AccessControlService acs, Object value, Map<Str
 
     var usernameRoles = extractUsernameRoles(acs, principal);
     var groupRoles = extractGroupRoles(acs, principal);
-    var unionRoles = Sets.union(usernameRoles, groupRoles);
 
-    if (unionRoles.isEmpty() && acs.getDefaultRole() != null) {
-      return Mono.just(Set.of(acs.getDefaultRole().getName()));
-    }
-
-    return Mono.just(unionRoles);
+    return Mono.just(Sets.union(usernameRoles, groupRoles));
   }
 
   private Set<String> extractUsernameRoles(AccessControlService acs, DefaultOAuth2User principal) {
diff --git a/api/src/main/java/io/kafbat/ui/service/rbac/extractor/GithubAuthorityExtractor.java b/api/src/main/java/io/kafbat/ui/service/rbac/extractor/GithubAuthorityExtractor.java
@@ -71,15 +71,10 @@ public Mono<Set<String>> extract(AccessControlService acs, Object value, Map<Str
     Mono<Set<String>> organizationRoles = getOrganizationRoles(principal, additionalParams, acs, webClient);
     Mono<Set<String>> teamRoles = getTeamRoles(webClient, additionalParams, acs);
 
-    Set<String> defaultRoles = acs.getDefaultRole() == null
-            ? Collections.emptySet()
-            : Set.of(acs.getDefaultRole().getName());
-
     return Mono.zip(organizationRoles, teamRoles)
         .map((t) -> Stream.of(t.getT1(), t.getT2(), usernameRoles)
             .flatMap(Collection::stream)
-            .collect(Collectors.toSet()))
-            .map(roles -> roles.isEmpty() ? defaultRoles : roles);
+            .collect(Collectors.toSet()));
   }
 
   private Set<String> extractUsernameRoles(DefaultOAuth2User principal, AccessControlService acs) {
diff --git a/api/src/main/java/io/kafbat/ui/service/rbac/extractor/GoogleAuthorityExtractor.java b/api/src/main/java/io/kafbat/ui/service/rbac/extractor/GoogleAuthorityExtractor.java
@@ -39,13 +39,8 @@ public Mono<Set<String>> extract(AccessControlService acs, Object value, Map<Str
 
     var usernameRoles = extractUsernameRoles(acs, principal);
     var domainRoles = extractDomainRoles(acs, principal);
-    var unionRoles = Sets.union(usernameRoles, domainRoles);
 
-    if (unionRoles.isEmpty() && acs.getDefaultRole() != null) {
-      return Mono.just(Set.of(acs.getDefaultRole().getName()));
-    }
-
-    return Mono.just(unionRoles);
+    return Mono.just(Sets.union(usernameRoles, domainRoles));
   }
 
   private Set<String> extractUsernameRoles(AccessControlService acs, DefaultOAuth2User principal) {
diff --git a/api/src/main/java/io/kafbat/ui/service/rbac/extractor/OauthAuthorityExtractor.java b/api/src/main/java/io/kafbat/ui/service/rbac/extractor/OauthAuthorityExtractor.java
@@ -43,13 +43,8 @@ public Mono<Set<String>> extract(AccessControlService acs, Object value, Map<Str
 
     var usernameRoles = extractUsernameRoles(acs, principal);
     var roles = extractRoles(acs, principal, additionalParams);
-    var unionRoles = Sets.union(usernameRoles, roles);
 
-    if (unionRoles.isEmpty() && acs.getDefaultRole() != null) {
-      return Mono.just(Set.of(acs.getDefaultRole().getName()));
-    }
-
-    return Mono.just(unionRoles);
+    return Mono.just(Sets.union(usernameRoles, roles));
   }
 
   private Set<String> extractUsernameRoles(AccessControlService acs, DefaultOAuth2User principal) {
diff --git a/api/src/main/java/io/kafbat/ui/service/rbac/extractor/RbacActiveDirectoryAuthoritiesExtractor.java b/api/src/main/java/io/kafbat/ui/service/rbac/extractor/RbacActiveDirectoryAuthoritiesExtractor.java
@@ -4,7 +4,6 @@
 import io.kafbat.ui.model.rbac.provider.Provider;
 import io.kafbat.ui.service.rbac.AccessControlService;
 import java.util.Collection;
-import java.util.Set;
 import java.util.stream.Collectors;
 import lombok.extern.slf4j.Slf4j;
 import org.springframework.context.ApplicationContext;
@@ -32,7 +31,7 @@ public Collection<? extends GrantedAuthority> getGrantedAuthorities(DirContextOp
         .peek(group -> log.trace("Found AD group [{}] for user [{}]", group, username))
         .collect(Collectors.toSet());
 
-    var grantedAuthorities = acs.getRoles()
+    return acs.getRoles()
         .stream()
         .filter(r -> r.getSubjects()
             .stream()
@@ -47,11 +46,5 @@ public Collection<? extends GrantedAuthority> getGrantedAuthorities(DirContextOp
         .peek(role -> log.trace("Mapped role [{}] for user [{}]", role, username))
         .map(SimpleGrantedAuthority::new)
         .collect(Collectors.toSet());
-
-    // If no roles are found, return default role
-    if (grantedAuthorities.isEmpty() && acs.getDefaultRole() != null) {
-      return Set.of(new SimpleGrantedAuthority(acs.getDefaultRole().getName()));
-    }
-    return grantedAuthorities;
   }
 }
diff --git a/api/src/main/java/io/kafbat/ui/service/rbac/extractor/RbacLdapAuthoritiesExtractor.java b/api/src/main/java/io/kafbat/ui/service/rbac/extractor/RbacLdapAuthoritiesExtractor.java
@@ -3,7 +3,6 @@
 import io.kafbat.ui.model.rbac.Role;
 import io.kafbat.ui.model.rbac.provider.Provider;
 import io.kafbat.ui.service.rbac.AccessControlService;
-import java.util.HashSet;
 import java.util.Set;
 import java.util.stream.Collectors;
 import lombok.extern.slf4j.Slf4j;
@@ -34,7 +33,7 @@ protected Set<GrantedAuthority> getAdditionalRoles(DirContextOperations user, St
         .peek(group -> log.trace("Found LDAP group [{}] for user [{}]", group, username))
         .collect(Collectors.toSet());
 
-    var simpleGrantedAuthorities =  acs.getRoles()
+    return acs.getRoles()
         .stream()
         .filter(r -> r.getSubjects()
             .stream()
@@ -49,11 +48,5 @@ protected Set<GrantedAuthority> getAdditionalRoles(DirContextOperations user, St
         .peek(role -> log.trace("Mapped role [{}] for user [{}]", role, username))
         .map(SimpleGrantedAuthority::new)
         .collect(Collectors.toSet());
-
-    // If no roles are found, return default role
-    if (simpleGrantedAuthorities.isEmpty() && acs.getDefaultRole() != null) {
-      return Set.of(new SimpleGrantedAuthority(acs.getDefaultRole().getName()));
-    }
-    return new HashSet<>(simpleGrantedAuthorities);
   }
 }
diff --git a/api/src/test/java/io/kafbat/ui/service/rbac/AccessControlServiceDefaultRoleRbacEnabledTest.java b/api/src/test/java/io/kafbat/ui/service/rbac/AccessControlServiceDefaultRoleRbacEnabledTest.java
@@ -14,6 +14,7 @@
 import io.kafbat.ui.model.ClusterDTO;
 import io.kafbat.ui.model.KafkaCluster;
 import io.kafbat.ui.model.rbac.AccessContext;
+import io.kafbat.ui.model.rbac.DefaultRole;
 import io.kafbat.ui.model.rbac.Role;
 import io.kafbat.ui.service.ClustersStorage;
 import java.util.List;
@@ -23,6 +24,7 @@
 import org.mockito.MockedStatic;
 import org.mockito.Mockito;
 import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.security.access.AccessDeniedException;
 import org.springframework.security.core.Authentication;
 import org.springframework.security.core.context.ReactiveSecurityContextHolder;
 import org.springframework.security.core.context.SecurityContext;
@@ -51,7 +53,7 @@ public class AccessControlServiceDefaultRoleRbacEnabledTest extends AbstractInte
   RbacUser user;
 
   @Mock
-  Role defaultRole;
+  DefaultRole defaultRole;
 
   @Mock
   ClustersStorage clustersStorage;
@@ -67,14 +69,6 @@ void setUp() {
 
     ReflectionTestUtils.setField(accessControlService, "properties", properties);
     ReflectionTestUtils.setField(accessControlService, "rbacEnabled", true);
-    ReflectionTestUtils.setField(accessControlService, "clustersStorage", clustersStorage);
-
-    KafkaCluster prodCluster = KafkaCluster.builder().name(PROD_CLUSTER).build();
-    KafkaCluster devCluster = KafkaCluster.builder().name(DEV_CLUSTER).build();
-
-    // set default role for all clusters
-    when(clustersStorage.getKafkaClusters()).thenReturn(List.of(prodCluster, devCluster));
-    accessControlService.init();
 
     // Mock security context
     when(securityContext.getAuthentication()).thenReturn(authentication);
@@ -90,17 +84,6 @@ public void withSecurityContext(Runnable runnable) {
     }
   }
 
-  @Test
-  void validateSetCluster() {
-    withSecurityContext(() -> {
-
-      List<String> clusters = defaultRole.getClusters();
-      assertThat(clusters)
-          .isNotNull()
-          .containsExactlyInAnyOrder(PROD_CLUSTER, DEV_CLUSTER);
-    });
-  }
-
   @Test
   void validateAccess() {
     withSecurityContext(() -> {
@@ -125,11 +108,4 @@ void isClusterAccessible() {
           .verify();
     });
   }
-
-  @Test
-  void testGetDefaultRole() {
-    Role defaultRole = accessControlService.getDefaultRole();
-    assertThat(defaultRole).isNotNull()
-        .isEqualTo(this.defaultRole);
-  }
 }
diff --git a/api/src/test/java/io/kafbat/ui/service/rbac/MockedRbacUtils.java b/api/src/test/java/io/kafbat/ui/service/rbac/MockedRbacUtils.java
@@ -5,6 +5,7 @@
 import static org.mockito.Mockito.when;
 
 import io.kafbat.ui.model.rbac.AccessContext;
+import io.kafbat.ui.model.rbac.DefaultRole;
 import io.kafbat.ui.model.rbac.Permission;
 import io.kafbat.ui.model.rbac.Resource;
 import io.kafbat.ui.model.rbac.Role;
@@ -100,9 +101,10 @@ public static Role getDevRole() {
     return role;
   }
 
-  public static Role getDefaultRole() {
-    Role role = new Role();
+  public static DefaultRole getDefaultRole() {
+    DefaultRole role = new DefaultRole();
     role.setName(DEFAULT_ROLE);
+    role.setClusters(List.of(DEV_CLUSTER, PROD_CLUSTER));
 
     Permission topicViewPermission = new Permission();
     topicViewPermission.setResource(Resource.TOPIC.name());
@@ -131,7 +133,7 @@ public static Role getDefaultRole() {
         connectPermission
     );
     role.setPermissions(permissions);
-    role.validateDefaultRole();
+    role.validate();
     return role;
   }
 

Original file line number	Diff line number	Diff line change
`@@ -21,10 +21,4 @@ public void validate() {`
`21`	`21`	`permissions.forEach(Permission::transform);`
`22`	`22`	`subjects.forEach(Subject::validate);`
`23`	`23`	`}`
`24`		`-`
`25`		`- // default role need only permissions`
`26`		`- public void validateDefaultRole() {`
`27`		`- permissions.forEach(Permission::validate);`
`28`		`- permissions.forEach(Permission::transform);`
`29`		`- }`
`30`	`24`	`}`