BE: RBAC: Subject type/value is unintended to be optional #719
+365
−14
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| package io.kafbat.ui.service.rbac.extractor; | ||
|
|
||
| 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.ArrayList; | ||
| import java.util.Arrays; | ||
| import java.util.Collection; | ||
| import java.util.List; | ||
| import java.util.stream.Collectors; | ||
| import lombok.extern.slf4j.Slf4j; | ||
| import org.springframework.ldap.core.DirContextOperations; | ||
| import org.springframework.ldap.core.DistinguishedName; | ||
| import org.springframework.security.core.GrantedAuthority; | ||
| import org.springframework.security.core.authority.AuthorityUtils; | ||
| import org.springframework.security.core.authority.SimpleGrantedAuthority; | ||
| import org.springframework.security.ldap.userdetails.LdapAuthoritiesPopulator; | ||
|
|
||
| @Slf4j | ||
| public class RbacActiveDirectoryAuthoritiesExtractor implements LdapAuthoritiesPopulator { | ||
| private final AccessControlService controlService; | ||
|
|
||
| public RbacActiveDirectoryAuthoritiesExtractor(AccessControlService controlService) { | ||
| this.controlService = controlService; | ||
| } | ||
|
|
||
| @Override | ||
| public Collection<? extends GrantedAuthority> getGrantedAuthorities(DirContextOperations userData, String username) { | ||
| String[] groups = userData.getStringAttributes("memberOf"); | ||
|
|
||
| if (log.isDebugEnabled() && groups != null && groups.length > 0) { | ||
| log.debug("'memberOf' attribute values: {}", Arrays.asList(groups)); | ||
| } | ||
|
|
||
| if (controlService != null) { | ||
| return controlService.getRoles().stream() | ||
| .filter(role -> role.getSubjects().stream() | ||
| .filter(subject -> Provider.LDAP_AD.equals(subject.getProvider())) | ||
| .anyMatch(subject -> switch (subject.getType()) { | ||
| case "user" -> username.equals(subject.getValue()); | ||
| case "group" -> groups != null && Arrays.stream(groups) | ||
| .map(this::groupName) | ||
| .anyMatch(name -> name.equals(subject.getValue())); | ||
| default -> false; | ||
| }) | ||
| ) | ||
| .map(Role::getName) | ||
| .peek(role -> log.trace("Mapped role [{}] for user [{}]", role, username)) | ||
| .map(SimpleGrantedAuthority::new) | ||
| .collect(Collectors.toList()); | ||
| } else { | ||
| if (groups == null) { | ||
| return AuthorityUtils.NO_AUTHORITIES; | ||
| } | ||
|
|
||
| List<GrantedAuthority> authorities = new ArrayList<>(groups.length); | ||
|
|
||
| for (String group : groups) { | ||
| authorities.add(new SimpleGrantedAuthority(groupName(group))); | ||
| } | ||
|
|
||
| return authorities; | ||
| } | ||
| } | ||
|
|
||
| @SuppressWarnings("deprecation") | ||
| private String groupName(String group) { | ||
| return new DistinguishedName(group).removeLast().getValue(); | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,157 @@ | ||
| package io.kafbat.ui; | ||
|
|
||
| import static io.kafbat.ui.AbstractIntegrationTest.CONFLUENT_PLATFORM_VERSION; | ||
| import static io.kafbat.ui.AbstractIntegrationTest.LOCAL; | ||
| import static io.kafbat.ui.container.ActiveDirectoryContainer.DOMAIN; | ||
| import static io.kafbat.ui.container.ActiveDirectoryContainer.EMPTY_PERMISSIONS_USER; | ||
| import static io.kafbat.ui.container.ActiveDirectoryContainer.FIRST_GROUP_USER; | ||
| import static io.kafbat.ui.container.ActiveDirectoryContainer.PASSWORD; | ||
| import static io.kafbat.ui.container.ActiveDirectoryContainer.SECOND_GROUP_USER; | ||
| import static io.kafbat.ui.container.ActiveDirectoryContainer.USER_WITHOUT_GROUP; | ||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertFalse; | ||
| import static org.junit.jupiter.api.Assertions.assertNotNull; | ||
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
|
||
| import io.kafbat.ui.container.ActiveDirectoryContainer; | ||
| import io.kafbat.ui.model.AuthenticationInfoDTO; | ||
| import io.kafbat.ui.model.ResourceTypeDTO; | ||
| import io.kafbat.ui.model.TopicCreationDTO; | ||
| import io.kafbat.ui.model.TopicDTO; | ||
| import io.kafbat.ui.model.UserPermissionDTO; | ||
| import java.util.List; | ||
| import java.util.Objects; | ||
| import org.jetbrains.annotations.NotNull; | ||
| import org.junit.jupiter.api.AfterAll; | ||
| import org.junit.jupiter.api.BeforeAll; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.springframework.beans.factory.annotation.Autowired; | ||
| import org.springframework.boot.test.autoconfigure.web.reactive.AutoConfigureWebTestClient; | ||
| import org.springframework.boot.test.context.SpringBootTest; | ||
| import org.springframework.context.ApplicationContextInitializer; | ||
| import org.springframework.context.ConfigurableApplicationContext; | ||
| import org.springframework.http.MediaType; | ||
| import org.springframework.test.context.ActiveProfiles; | ||
| import org.springframework.test.context.ContextConfiguration; | ||
| import org.springframework.test.web.reactive.server.WebTestClient; | ||
| import org.springframework.web.reactive.function.BodyInserters; | ||
| import org.testcontainers.containers.KafkaContainer; | ||
| import org.testcontainers.containers.Network; | ||
| import org.testcontainers.utility.DockerImageName; | ||
|
|
||
| @SpringBootTest | ||
| @ActiveProfiles("rbac-ad") | ||
| @AutoConfigureWebTestClient(timeout = "60000") | ||
| @ContextConfiguration(initializers = {ActiveDirectoryIntegrationTest.Initializer.class}) | ||
| public class ActiveDirectoryIntegrationTest { | ||
|
There was a problem hiding this comment. This one is really cool tho, can you raise a separate PR with these tests so we can merge this? Btw, we don't quite need kafka container here, the app can perfectly start with no defined clusters. There was a problem hiding this comment.
Yes, but there's a test that checks the creation of a topic on a cluster. There was a problem hiding this comment.
this sounds redundant. Authenticating successfully should suffice There was a problem hiding this comment.
Opened the PR with the tests #726 |
||
| private static final String SESSION = "SESSION"; | ||
|
|
||
| private static final KafkaContainer KAFKA = new KafkaContainer( | ||
| DockerImageName.parse("confluentinc/cp-kafka").withTag(CONFLUENT_PLATFORM_VERSION)) | ||
| .withNetwork(Network.SHARED); | ||
|
|
||
| private static final ActiveDirectoryContainer ACTIVE_DIRECTORY = new ActiveDirectoryContainer(); | ||
|
|
||
| @Autowired | ||
| private WebTestClient webTestClient; | ||
|
|
||
| @BeforeAll | ||
| public static void setup() { | ||
| KAFKA.start(); | ||
| ACTIVE_DIRECTORY.start(); | ||
| } | ||
|
|
||
| @AfterAll | ||
| public static void shutdown() { | ||
| ACTIVE_DIRECTORY.stop(); | ||
| KAFKA.stop(); | ||
| } | ||
|
|
||
| @Test | ||
| public void testUserPermissions() { | ||
| AuthenticationInfoDTO info = authenticationInfo(FIRST_GROUP_USER); | ||
|
|
||
| assertNotNull(info); | ||
| assertTrue(info.getRbacEnabled()); | ||
|
|
||
| List<UserPermissionDTO> permissions = info.getUserInfo().getPermissions(); | ||
|
|
||
| assertFalse(permissions.isEmpty()); | ||
| assertTrue(permissions.stream().anyMatch(permission -> | ||
| permission.getClusters().contains(LOCAL) && permission.getResource() == ResourceTypeDTO.TOPIC)); | ||
| assertEquals(permissions, authenticationInfo(SECOND_GROUP_USER).getUserInfo().getPermissions()); | ||
| assertEquals(permissions, authenticationInfo(USER_WITHOUT_GROUP).getUserInfo().getPermissions()); | ||
| } | ||
|
|
||
| @Test | ||
| public void testCreateTopic() { | ||
| TopicCreationDTO topic = new TopicCreationDTO() | ||
| .name("new") | ||
| .partitions(10); | ||
|
|
||
| TopicDTO result = webTestClient | ||
| .post() | ||
| .uri("/api/clusters/{clusterName}/topics", LOCAL) | ||
| .cookie(SESSION, session(FIRST_GROUP_USER)) | ||
| .bodyValue(topic) | ||
| .exchange() | ||
| .expectStatus() | ||
| .isOk() | ||
| .returnResult(TopicDTO.class) | ||
| .getResponseBody() | ||
| .blockFirst(); | ||
|
|
||
| assertNotNull(result); | ||
| assertEquals(topic.getName(), result.getName()); | ||
| assertEquals(topic.getPartitions(), result.getPartitionCount()); | ||
| } | ||
|
|
||
| @Test | ||
| public void testEmptyPermissions() { | ||
| assertTrue(Objects.requireNonNull(authenticationInfo(EMPTY_PERMISSIONS_USER)) | ||
| .getUserInfo() | ||
| .getPermissions() | ||
| .isEmpty() | ||
| ); | ||
| } | ||
|
|
||
| private String session(String name) { | ||
| return Objects.requireNonNull( | ||
| webTestClient | ||
| .post() | ||
| .uri("/#") | ||
| .contentType(MediaType.APPLICATION_FORM_URLENCODED) | ||
| .body(BodyInserters.fromFormData("username", name).with("password", PASSWORD)) | ||
| .exchange() | ||
| .expectStatus() | ||
| .isFound() | ||
| .returnResult(String.class) | ||
| .getResponseCookies() | ||
| .getFirst(SESSION)) | ||
| .getValue(); | ||
| } | ||
|
|
||
| private AuthenticationInfoDTO authenticationInfo(String name) { | ||
| return webTestClient | ||
| .get() | ||
| .uri("/api/authorization") | ||
| .cookie(SESSION, session(name)) | ||
| .exchange() | ||
| .expectStatus() | ||
| .isOk() | ||
| .returnResult(AuthenticationInfoDTO.class) | ||
| .getResponseBody() | ||
| .blockFirst(); | ||
| } | ||
|
|
||
| public static class Initializer implements ApplicationContextInitializer<ConfigurableApplicationContext> { | ||
| @Override | ||
| public void initialize(@NotNull ConfigurableApplicationContext context) { | ||
| System.setProperty("kafka.clusters.0.name", LOCAL); | ||
| System.setProperty("kafka.clusters.0.bootstrapServers", KAFKA.getBootstrapServers()); | ||
| System.setProperty("spring.ldap.urls", ACTIVE_DIRECTORY.getLdapUrl()); | ||
| System.setProperty("oauth2.ldap.activeDirectory", "true"); | ||
| System.setProperty("oauth2.ldap.activeDirectory.domain", DOMAIN); | ||
| } | ||
| } | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Let's not mix the distinct issues in one PR, please. #54 is to be solved within #717 (which was planned as more of a refactoring PR first, but, welp, it's another story).
#716 has nothing to do with LDAP in particular, we'd need RBAC subject validation. In
io.kafbat.ui.model.rbac.PermissionandRoleclasses there's avalidatemethod, perhaps implementing one forSubjectas well is the way here.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.
Maybe we can merge this PR as is for the task fix #54 and I'll do the RBAC subject validation (#716) in a separate PR, what do you say?
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.
Let's rather not. This is not quite the changes we need to resolve #54, I've already started implementing the required adjustments in #717