-
-
Notifications
You must be signed in to change notification settings - Fork 151
BE: RBAC: Impl default role #1056
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
8c4d0ff
55250e7
7ddaeb8
dad4a37
5a21c83
1bf5904
69045d5
273da6a
e0cba88
7cbf653
daac844
e5bb2fe
0c7a8a1
db75677
e320708
c8a15ef
e5bb106
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
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 List<Permission> permissions = new ArrayList<>(); | ||
|
||
public void validate() { | ||
permissions.forEach(Permission::validate); | ||
permissions.forEach(Permission::transform); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,5 +21,4 @@ public void validate() { | |
permissions.forEach(Permission::transform); | ||
subjects.forEach(Subject::validate); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ | |
import io.kafbat.ui.model.ConnectDTO; | ||
import io.kafbat.ui.model.InternalTopic; | ||
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; | ||
|
@@ -62,7 +63,7 @@ public class AccessControlService { | |
|
||
@PostConstruct | ||
public void init() { | ||
if (CollectionUtils.isEmpty(properties.getRoles())) { | ||
if (CollectionUtils.isEmpty(properties.getRoles()) && properties.getDefaultRole() == null) { | ||
log.trace("No roles provided, disabling RBAC"); | ||
return; | ||
} | ||
|
@@ -86,7 +87,7 @@ public void init() { | |
.flatMap(Set::stream) | ||
.collect(Collectors.toSet()); | ||
|
||
if (!properties.getRoles().isEmpty() | ||
if (!(properties.getRoles().isEmpty() && properties.getDefaultRole() == null) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While we're at it, can we refactor this monstrosity a bit? A negative check on a compound |
||
&& "oauth2".equalsIgnoreCase(environment.getProperty("auth.type")) | ||
&& (clientRegistrationRepository == null || !clientRegistrationRepository.iterator().hasNext())) { | ||
log.error("Roles are configured but no authentication methods are present. Authentication might fail."); | ||
|
@@ -114,12 +115,20 @@ private boolean isAccessible(AuthenticatedUser user, AccessContext context) { | |
} | ||
|
||
private List<Permission> getUserPermissions(AuthenticatedUser user, @Nullable String clusterName) { | ||
return properties.getRoles() | ||
.stream() | ||
.filter(filterRole(user)) | ||
.filter(role -> clusterName == null || role.getClusters().stream().anyMatch(clusterName::equalsIgnoreCase)) | ||
.flatMap(role -> role.getPermissions().stream()) | ||
.toList(); | ||
List<Role> filteredRoles = properties.getRoles() | ||
.stream() | ||
.filter(filterRole(user)) | ||
.filter(role -> clusterName == null || role.getClusters().stream().anyMatch(clusterName::equalsIgnoreCase)) | ||
.toList(); | ||
|
||
// if no roles are found, check if default role is set | ||
if (filteredRoles.isEmpty() && properties.getDefaultRole() != null) { | ||
return properties.getDefaultRole().getPermissions(); | ||
} | ||
|
||
return filteredRoles.stream() | ||
.flatMap(role -> role.getPermissions().stream()) | ||
.toList(); | ||
} | ||
|
||
public static Mono<AuthenticatedUser> getUser() { | ||
|
@@ -132,10 +141,12 @@ public static Mono<AuthenticatedUser> getUser() { | |
|
||
private boolean isClusterAccessible(String clusterName, AuthenticatedUser user) { | ||
germanosin marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Assert.isTrue(StringUtils.isNotEmpty(clusterName), "cluster value is empty"); | ||
return properties.getRoles() | ||
boolean isAccessible = properties.getRoles() | ||
.stream() | ||
.filter(filterRole(user)) | ||
.anyMatch(role -> role.getClusters().stream().anyMatch(clusterName::equalsIgnoreCase)); | ||
|
||
return isAccessible || properties.getDefaultRole() != null; | ||
} | ||
|
||
public Mono<Boolean> isClusterAccessible(ClusterDTO cluster) { | ||
|
@@ -200,6 +211,10 @@ public List<Role> getRoles() { | |
return Collections.unmodifiableList(properties.getRoles()); | ||
} | ||
|
||
public DefaultRole getDefaultRole() { | ||
return properties.getDefaultRole(); | ||
} | ||
|
||
private Predicate<Role> filterRole(AuthenticatedUser user) { | ||
return role -> user.groups().contains(role.getName()); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,102 @@ | ||
package io.kafbat.ui.service.rbac; | ||
|
||
import static io.kafbat.ui.service.rbac.MockedRbacUtils.DEFAULT_ROLE; | ||
import static io.kafbat.ui.service.rbac.MockedRbacUtils.PROD_CLUSTER; | ||
import static io.kafbat.ui.service.rbac.MockedRbacUtils.getAccessContext; | ||
import static org.mockito.Mockito.mock; | ||
import static org.mockito.Mockito.when; | ||
|
||
import io.kafbat.ui.AbstractIntegrationTest; | ||
import io.kafbat.ui.config.auth.RbacUser; | ||
import io.kafbat.ui.config.auth.RoleBasedAccessControlProperties; | ||
import io.kafbat.ui.model.ClusterDTO; | ||
import io.kafbat.ui.model.rbac.AccessContext; | ||
import io.kafbat.ui.model.rbac.DefaultRole; | ||
import java.util.List; | ||
import org.junit.jupiter.api.BeforeEach; | ||
import org.junit.jupiter.api.Test; | ||
import org.mockito.Mock; | ||
import org.mockito.MockedStatic; | ||
import org.mockito.Mockito; | ||
import org.springframework.beans.factory.annotation.Autowired; | ||
import org.springframework.security.core.Authentication; | ||
import org.springframework.security.core.context.ReactiveSecurityContextHolder; | ||
import org.springframework.security.core.context.SecurityContext; | ||
import org.springframework.test.annotation.DirtiesContext; | ||
import org.springframework.test.util.ReflectionTestUtils; | ||
import reactor.core.publisher.Mono; | ||
import reactor.test.StepVerifier; | ||
|
||
|
||
/** | ||
* Test class for AccessControlService with default role and RBAC enabled. | ||
*/ | ||
@DirtiesContext(classMode = DirtiesContext.ClassMode.AFTER_CLASS) | ||
public class AccessControlServiceDefaultRoleRbacEnabledTest extends AbstractIntegrationTest { | ||
|
||
@Autowired | ||
AccessControlService accessControlService; | ||
|
||
@Mock | ||
SecurityContext securityContext; | ||
|
||
@Mock | ||
Authentication authentication; | ||
|
||
@Mock | ||
RbacUser user; | ||
|
||
@Mock | ||
DefaultRole defaultRole; | ||
|
||
@BeforeEach | ||
void setUp() { | ||
|
||
RoleBasedAccessControlProperties properties = mock(); | ||
defaultRole = MockedRbacUtils.getDefaultRole(); | ||
when(properties.getDefaultRole()).thenReturn(defaultRole); | ||
when(properties.getRoles()).thenReturn(List.of()); // Return empty list for roles | ||
|
||
|
||
ReflectionTestUtils.setField(accessControlService, "properties", properties); | ||
ReflectionTestUtils.setField(accessControlService, "rbacEnabled", true); | ||
|
||
// Mock security context | ||
when(securityContext.getAuthentication()).thenReturn(authentication); | ||
when(authentication.getPrincipal()).thenReturn(user); | ||
} | ||
|
||
public void withSecurityContext(Runnable runnable) { | ||
try (MockedStatic<ReactiveSecurityContextHolder> ctxHolder = Mockito.mockStatic( | ||
ReactiveSecurityContextHolder.class)) { | ||
// Mock static method to get security context | ||
ctxHolder.when(ReactiveSecurityContextHolder::getContext).thenReturn(Mono.just(securityContext)); | ||
runnable.run(); | ||
} | ||
} | ||
|
||
@Test | ||
void validateAccess() { | ||
withSecurityContext(() -> { | ||
when(user.groups()).thenReturn(List.of(DEFAULT_ROLE)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not following here, what are we testing here for? Given that we mock users' groups, it seems that no newly implemented logic is gonna be tested against? |
||
AccessContext context = getAccessContext(PROD_CLUSTER, true); | ||
Mono<Void> validateAccessMono = accessControlService.validateAccess(context); | ||
StepVerifier.create(validateAccessMono) | ||
.expectComplete() | ||
.verify(); | ||
}); | ||
} | ||
|
||
@Test | ||
void isClusterAccessible() { | ||
withSecurityContext(() -> { | ||
ClusterDTO clusterDto = new ClusterDTO(); | ||
clusterDto.setName(PROD_CLUSTER); | ||
Mono<Boolean> clusterAccessibleMono = accessControlService.isClusterAccessible(clusterDto); | ||
StepVerifier.create(clusterAccessibleMono) | ||
.expectNext(true) | ||
.expectComplete() | ||
.verify(); | ||
}); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps an explicit setter is redundant here