From 973a185f13d757ab54b34469e4e604a47da1508d Mon Sep 17 00:00:00 2001 From: Rui Hu Date: Thu, 24 Jul 2025 16:30:09 +0800 Subject: [PATCH] fix: fix permission check rule resolving This commit fix the way for resolving the permission check rule. Now it respect the request path instead of randomly picking one. --- .../controller/AuthController.java | 47 ++++++------ .../service/PermissionMapLookupService.java | 16 +--- .../FallbackPermissionMapLookupService.java | 28 +------ ...allbackPermissionMapLookupServiceTest.java | 44 +++-------- .../KeycloakIdentityManagerServiceTest.java | 73 +++++++++++++++++++ 5 files changed, 110 insertions(+), 98 deletions(-) diff --git a/src/main/java/com/cleverthis/authservice/controller/AuthController.java b/src/main/java/com/cleverthis/authservice/controller/AuthController.java index 90a1804..2046d0e 100644 --- a/src/main/java/com/cleverthis/authservice/controller/AuthController.java +++ b/src/main/java/com/cleverthis/authservice/controller/AuthController.java @@ -187,17 +187,19 @@ public class AuthController { log.info("Received forwardAuth request: Method='{}', Uri='{}'", originalMethod, originalUriString); // first detect the client id and service relative path - final var targetServiceClientIdOpt = getTargetServiceClientId(originalUriString); - if (targetServiceClientIdOpt.isEmpty()) { + final var optionalRule = getPermissionCheckRuleByUri(originalUriString); + if (optionalRule.isEmpty()) { log.warn("ForwardAuth: No Keycloak client mapping found for URI: {}", originalUriString); return Mono.just(ResponseEntity.status(HttpStatus.FORBIDDEN).build()); } - final var targetServiceClientId = targetServiceClientIdOpt.get(); + final var rule = optionalRule.get(); + + final var targetServiceClientId = rule.getKeycloakClientId(); final var serviceRelativePath = - extractServiceRelativePath(originalUriString, targetServiceClientId); + extractServiceRelativePath(originalUriString, rule); // then check pass-through rule - if (shouldPassThrough(targetServiceClientId, serviceRelativePath)) { + if (shouldPassThrough(rule, serviceRelativePath)) { return Mono.just(ResponseEntity.status(HttpStatus.NO_CONTENT).build()); } if (authorizationHeader == null) { @@ -276,16 +278,15 @@ public class AuthController { } private boolean shouldPassThrough( - final String serviceClientId, final String serviceRelativePath + final PermissionMappingConfigProperties.Rule rule, final String serviceRelativePath ) { - final var rules = this.permissionMapLookupService - .getPassThoughRulesByClientId(serviceClientId); + final var passthroughs = rule.getPassthroughs(); try { - return rules.stream() + return passthroughs.stream() .anyMatch(it -> executePassThroughRule(it, serviceRelativePath)); } catch (final Throwable throwable) { log.error("Error executing pass-through rule for client {} : {}", - serviceClientId, throwable.getMessage(), throwable); + rule.getKeycloakClientId(), throwable.getMessage(), throwable); return false; } } @@ -300,13 +301,15 @@ public class AuthController { }; } - private Optional getTargetServiceClientId(String fullUriString) { + private Optional getPermissionCheckRuleByUri( + String fullUriString + ) { try { URI uri = new URI(fullUriString); String path = uri.getPath(); // Get path part, e.g., /cleverswarm/api/jobs/123 log.debug("Mapping URI path: {}", path); - final var pathMatch = this.permissionMapLookupService.matchClientIdByPath(path); + final var pathMatch = this.permissionMapLookupService.matchRuleByPath(path); if (pathMatch.isPresent()) { return pathMatch; } @@ -315,7 +318,9 @@ public class AuthController { log.debug( "No rule matched for path '{}', using default Keycloak Client ID:{}", path, defaultValue); - return Optional.of(defaultValue); + return Optional.of(PermissionMappingConfigProperties.Rule.builder() + .keycloakClientId(defaultValue) + .build()); } } catch (URISyntaxException e) { log.error("Invalid URI syntax for X-Forwarded-Uri: {}", fullUriString, e); @@ -324,17 +329,11 @@ public class AuthController { return Optional.empty(); } - private String extractServiceRelativePath(String fullUriString, String mappedKeycloakClientId) { - // Find the rule that gave us this mappedKeycloakClientId to get its pathPrefix - Optional matchedRule = - this.permissionMapLookupService.matchRuleByClientIdAndPath( - mappedKeycloakClientId, fullUriString - ); - - String pathPrefixToRemove = ""; - if (matchedRule.isPresent()) { - pathPrefixToRemove = matchedRule.get().getPathPrefix(); - } else if (mappedKeycloakClientId + private String extractServiceRelativePath( + String fullUriString, PermissionMappingConfigProperties.Rule rule + ) { + String pathPrefixToRemove = rule.getPathPrefix(); + if (rule.getKeycloakClientId() .equals(this.permissionMapLookupService.getDefaultKeycloakClientId())) { // If it's a default mapping, there's no prefix to remove, use the whole path. // Or, you might decide that default mappings always apply to root paths. This diff --git a/src/main/java/com/cleverthis/authservice/service/PermissionMapLookupService.java b/src/main/java/com/cleverthis/authservice/service/PermissionMapLookupService.java index 6ad18ef..0037b60 100644 --- a/src/main/java/com/cleverthis/authservice/service/PermissionMapLookupService.java +++ b/src/main/java/com/cleverthis/authservice/service/PermissionMapLookupService.java @@ -1,7 +1,6 @@ package com.cleverthis.authservice.service; import com.cleverthis.authservice.config.PermissionMappingConfigProperties; -import java.util.List; import java.util.Optional; /** @@ -17,18 +16,5 @@ public interface PermissionMapLookupService { /** * Given the path, find the client id of the first rule matching the path prefix. */ - Optional matchClientIdByPath(String path); - - /** - * Given the client id and path, find the rule that both matches the client id exactly, - * and match the path prefix. - */ - Optional matchRuleByClientIdAndPath( - String clientId, String path); - - /** - * Return the list of pass-through rules by client id. - */ - List getPassThoughRulesByClientId( - String clientId); + Optional matchRuleByPath(String path); } diff --git a/src/main/java/com/cleverthis/authservice/service/impl/FallbackPermissionMapLookupService.java b/src/main/java/com/cleverthis/authservice/service/impl/FallbackPermissionMapLookupService.java index 4e6ffbc..ae2ef21 100644 --- a/src/main/java/com/cleverthis/authservice/service/impl/FallbackPermissionMapLookupService.java +++ b/src/main/java/com/cleverthis/authservice/service/impl/FallbackPermissionMapLookupService.java @@ -5,7 +5,6 @@ import com.cleverthis.authservice.service.PermissionMapLookupService; import com.ecwid.consul.v1.ConsulClient; import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; -import java.util.List; import java.util.Optional; import java.util.concurrent.atomic.AtomicReference; import lombok.extern.slf4j.Slf4j; @@ -102,38 +101,15 @@ public final class FallbackPermissionMapLookupService implements PermissionMapLo } @Override - public Optional matchClientIdByPath(final String path) { + public Optional matchRuleByPath(final String path) { final var config = getConfig(); for (final var rule : config.getRules()) { if (path.startsWith(rule.getPathPrefix())) { log.debug("Path '{}' matched prefix '{}', using Keycloak Client ID: {}", path, rule.getPathPrefix(), rule.getKeycloakClientId()); - return Optional.of(rule.getKeycloakClientId()); + return Optional.of(rule); } } return Optional.empty(); } - - @Override - public Optional matchRuleByClientIdAndPath( - final String clientId, final String path - ) { - final var config = getConfig(); - return config.getRules().stream() - .filter(rule -> - rule.getKeycloakClientId().equals(clientId) - && path.startsWith(rule.getPathPrefix())) - .findFirst(); - } - - @Override - public List getPassThoughRulesByClientId( - final String clientId - ) { - final var config = getConfig(); - return config.getRules().stream() - .filter(rule -> rule.getKeycloakClientId().equals(clientId)) - .flatMap(rule -> rule.getPassthroughs().stream()) - .toList(); - } } diff --git a/src/test/java/com/cleverthis/authservice/service/impl/FallbackPermissionMapLookupServiceTest.java b/src/test/java/com/cleverthis/authservice/service/impl/FallbackPermissionMapLookupServiceTest.java index c11cd36..5074cbc 100644 --- a/src/test/java/com/cleverthis/authservice/service/impl/FallbackPermissionMapLookupServiceTest.java +++ b/src/test/java/com/cleverthis/authservice/service/impl/FallbackPermissionMapLookupServiceTest.java @@ -55,7 +55,8 @@ class FallbackPermissionMapLookupServiceTest { assertNull(service.getConfig()); // fallback is null // has json content when(value.getDecodedValue()) - .thenReturn(this.objectMapper.writeValueAsString(new PermissionMappingConfigProperties())); + .thenReturn( + this.objectMapper.writeValueAsString(new PermissionMappingConfigProperties())); service.refreshConsul(); assertEquals(new PermissionMappingConfigProperties(), service.getConfig()); } @@ -76,45 +77,22 @@ class FallbackPermissionMapLookupServiceTest { } @Test - void testMatchClientIdByPath() { + void testMatchRuleByPath() { final var defaultConfig = new PermissionMappingConfigProperties(); - defaultConfig.setRules(List.of( - PermissionMappingConfigProperties.Rule.builder() - .keycloakClientId("service-A").pathPrefix("/a/").build(), - PermissionMappingConfigProperties.Rule.builder() - .keycloakClientId("service-B").pathPrefix("/b/").build() - )); + final var ruleA = PermissionMappingConfigProperties.Rule.builder() + .keycloakClientId("service-A").pathPrefix("/a/").build(); + final var ruleB = PermissionMappingConfigProperties.Rule.builder() + .keycloakClientId("service-B").pathPrefix("/b/").build(); + defaultConfig.setRules(List.of(ruleA, ruleB)); final var service = new FallbackPermissionMapLookupService( defaultConfig, this.consulClient, this.objectMapper); assertEquals( - "service-B", - service.matchClientIdByPath("/b/items/123").orElseThrow()); + ruleB, + service.matchRuleByPath("/b/items/123").orElseThrow()); assertEquals( Optional.empty(), - service.matchClientIdByPath("/c/items/123")); + service.matchRuleByPath("/c/items/123")); } - @Test - void testMatchRuleByClientIdAndPath() { - final var defaultConfig = new PermissionMappingConfigProperties(); - defaultConfig.setRules(List.of( - PermissionMappingConfigProperties.Rule.builder() - .keycloakClientId("service-A").pathPrefix("/a/").build(), - PermissionMappingConfigProperties.Rule.builder() - .keycloakClientId("service-B").pathPrefix("/b/").build() - )); - final var service = new FallbackPermissionMapLookupService( - defaultConfig, this.consulClient, this.objectMapper); - - service.matchRuleByClientIdAndPath("service-A", "/a/items/123") - .ifPresentOrElse( - rule -> assertEquals("service-A", rule.getKeycloakClientId()), - () -> Assertions.fail("No rule matched")); - service.matchRuleByClientIdAndPath("service-B", "/a/items/123") - .ifPresentOrElse( - it -> Assertions.fail("Rule matched but should not have"), - () -> {} - ); - } } \ No newline at end of file diff --git a/src/test/java/com/cleverthis/authservice/service/impl/KeycloakIdentityManagerServiceTest.java b/src/test/java/com/cleverthis/authservice/service/impl/KeycloakIdentityManagerServiceTest.java index a2ce6da..fc9f76a 100644 --- a/src/test/java/com/cleverthis/authservice/service/impl/KeycloakIdentityManagerServiceTest.java +++ b/src/test/java/com/cleverthis/authservice/service/impl/KeycloakIdentityManagerServiceTest.java @@ -28,6 +28,7 @@ import org.keycloak.representations.idm.GroupRepresentation; import org.keycloak.representations.idm.MemberRepresentation; import org.keycloak.representations.idm.OrganizationDomainRepresentation; import org.keycloak.representations.idm.OrganizationRepresentation; +import org.keycloak.representations.idm.UserRepresentation; import org.mockito.Mockito; import org.springframework.web.reactive.function.client.WebClient; import reactor.core.publisher.Mono; @@ -558,4 +559,76 @@ class KeycloakIdentityManagerServiceTest { Mockito.verify(this.adminClient).getGroupById(groupId); } + + @Test + void listSubGroups_success() { + // Arrange + final var parentGroupId = "parent-group-id"; + final var subGroups = List.of( + new GroupRepresentation() {{ + setId("sub-group-1"); + setName("SubGroup1"); + }}, + new GroupRepresentation() {{ + setId("sub-group-2"); + setName("SubGroup2"); + }} + ); + + Mockito.when(this.adminClient.listSubGroups(parentGroupId)) + .thenReturn(Mono.just(subGroups)); + + // Act + final var result = this.service.listSubGroups(parentGroupId); + + // Assert + StepVerifier.create(result) + .expectNextMatches(groups -> groups.size() == 2 && + "SubGroup1".equals(groups.getFirst().getName()) && + "sub-group-2".equals(groups.get(1).getId())) + .verifyComplete(); + + Mockito.verify(this.adminClient).listSubGroups(parentGroupId); + } + + @Test + void getGroupMembers_success() { + // Arrange + final var groupId = "test-group-id"; + final var mockMembers = List.of( + new UserRepresentation() {{ + setId("user1"); + setUsername("user1Username"); + setFirstName("User1"); + setLastName("One"); + setEmail("user1@example.com"); + setEnabled(true); + }}, + new UserRepresentation() {{ + setId("user2"); + setUsername("user2Username"); + setFirstName("User2"); + setLastName("Two"); + setEmail("user2@example.com"); + setEnabled(true); + }} + ); + + Mockito.when(this.adminClient.getGroupMembers(groupId)) + .thenReturn(Mono.just(mockMembers)); + + // Act + final var result = this.service.getGroupMembers(groupId); + + // Assert + StepVerifier.create(result) + .expectNextMatches(members -> members.size() == 2 + && "user1".equals(members.getFirst().getId()) + && "user1@example.com".equals(members.getFirst().getEmail()) + && "user2".equals(members.get(1).getId()) + && "user2@example.com".equals(members.get(1).getEmail())) + .verifyComplete(); + + Mockito.verify(this.adminClient).getGroupMembers(groupId); + } } \ No newline at end of file