403 Forbidden when using controller methods with resource name contai…

…ning . (dot) (#86) * Added . <dot> to acceptable resource chars and Pattern validation to ObjectMeta name * Added missing regex end char
michelin · Jul 7, 2021 · 1939d41 · 1939d41
1 parent 6fc4451
commit 1939d41
Show file tree

Hide file tree

Showing 4 changed files with 75 additions and 2 deletions.
diff --git a/api/src/main/java/com/michelin/ns4kafka/models/ObjectMeta.java b/api/src/main/java/com/michelin/ns4kafka/models/ObjectMeta.java
@@ -5,6 +5,7 @@
 import lombok.*;
 
 import javax.validation.constraints.NotBlank;
+import javax.validation.constraints.Pattern;
 import java.util.Date;
 import java.util.Map;
 
@@ -15,6 +16,7 @@
 @Data
 public class ObjectMeta {
     @NotBlank
+    @Pattern(regexp = "^[a-zA-Z0-9_.-]+$")
     private String name;
     private String namespace;
     private String cluster;

diff --git a/api/src/main/java/com/michelin/ns4kafka/security/ResourceBasedSecurityRule.java b/api/src/main/java/com/michelin/ns4kafka/security/ResourceBasedSecurityRule.java
@@ -27,7 +27,7 @@ public class ResourceBasedSecurityRule implements SecurityRule {
 
     public static final String IS_ADMIN = "isAdmin()";
 
-    private final Pattern namespacedResourcePattern = Pattern.compile("^\\/api\\/namespaces\\/(?<namespace>[a-zA-Z0-9_-]+)\\/(?<resourceType>[a-z_-]+)(\\/([a-zA-Z0-9_-]+)(\\/(?<resourceSubtype>[a-z]+))?)?");
+    private final Pattern namespacedResourcePattern = Pattern.compile("^\\/api\\/namespaces\\/(?<namespace>[a-zA-Z0-9_-]+)\\/(?<resourceType>[a-z_-]+)(\\/([a-zA-Z0-9_.-]+)(\\/(?<resourceSubtype>[a-z]+))?)?$");
 
 
     SecurityConfig securityConfig;

diff --git a/api/src/test/java/com/michelin/ns4kafka/integration/TopicTest.java b/api/src/test/java/com/michelin/ns4kafka/integration/TopicTest.java
@@ -198,6 +198,31 @@ void createTopic() throws InterruptedException, ExecutionException {
         });
     }
 
+    @Test
+    void invalidTopicName() throws InterruptedException, ExecutionException {
+
+        Topic topicFirstCreate = Topic.builder()
+                .metadata(ObjectMeta.builder()
+                        .name("ns1-invalid-é")
+                        .namespace("ns1")
+                        .build())
+                .spec(TopicSpec.builder()
+                        .partitions(3)
+                        .replicationFactor(1)
+                        .configs(Map.of("cleanup.policy", "delete",
+                                "min.insync.replicas", "1",
+                                "retention.ms", "60000"))
+                        .build())
+                .build();
+        HttpClientResponseException exception = Assertions.assertThrows(HttpClientResponseException.class,
+                () -> client.exchange(HttpRequest.create(HttpMethod.POST,"/api/namespaces/ns1/topics")
+                        .bearerAuth(token)
+                        .body(topicFirstCreate))
+                        .blockingFirst());
+
+        Assertions.assertEquals("topic.metadata.name: must match \"^[a-zA-Z0-9_.-]+$\"", exception.getMessage());
+
+    }
 
     @Test
     void unauthorizedModifications() throws InterruptedException {

diff --git a/api/src/test/java/com/michelin/ns4kafka/security/ResourceBasedSecurityRuleTest.java b/api/src/test/java/com/michelin/ns4kafka/security/ResourceBasedSecurityRuleTest.java
@@ -174,8 +174,31 @@ void CheckReturnsAllowed_ResourceWithHyphen(){
         SecurityRuleResult actual = resourceBasedSecurityRule.check(HttpRequest.GET("/api/namespaces/test/role-bindings"),null, claims);
         Assertions.assertEquals(SecurityRuleResult.ALLOWED, actual);
     }
+    @Test
+    void CheckReturnsAllowed_ResourceNameWithDot(){
+        List<String> groups = List.of("group1");
+        Map<String,Object> claims = Map.of("sub","user", "groups", groups, "roles", List.of());
+        Mockito.when(roleBindingRepository.findAllForGroups(groups))
+                .thenReturn(List.of(RoleBinding.builder()
+                        .metadata(ObjectMeta.builder().namespace("test")
+                                .build())
+                        .spec(RoleBinding.RoleBindingSpec.builder()
+                                .role(RoleBinding.Role.builder()
+                                        .resourceTypes(List.of("topics"))
+                                        .verbs(List.of(RoleBinding.Verb.GET))
+                                        .build())
+                                .subject(RoleBinding.Subject.builder().subjectName("group1")
+                                        .build())
+                                .build())
+                        .build()));
+        Mockito.when(namespaceRepository.findByName("test"))
+                .thenReturn(Optional.of(Namespace.builder().build()));
 
-  @Test
+        SecurityRuleResult actual = resourceBasedSecurityRule.check(HttpRequest.GET("/api/namespaces/test/topics/topic.with.dots"),null, claims);
+        Assertions.assertEquals(SecurityRuleResult.ALLOWED, actual);
+    }
+
+    @Test
     void CheckReturnsUnknown_SubResource(){
         List<String> groups = List.of("group1");
         Map<String,Object> claims = Map.of("sub","user", "groups", groups, "roles", List.of());
@@ -198,6 +221,29 @@ void CheckReturnsUnknown_SubResource(){
         SecurityRuleResult actual = resourceBasedSecurityRule.check(HttpRequest.GET("/api/namespaces/test/connects/name/restart"),null, claims);
         Assertions.assertEquals(SecurityRuleResult.UNKNOWN, actual);
     }
+    @Test
+    void CheckReturnsUnknown_SubResourceWithDot(){
+        List<String> groups = List.of("group1");
+        Map<String,Object> claims = Map.of("sub","user", "groups", groups, "roles", List.of());
+        Mockito.when(namespaceRepository.findByName("test"))
+                .thenReturn(Optional.of(Namespace.builder().build()));
+        Mockito.when(roleBindingRepository.findAllForGroups(groups))
+                .thenReturn(List.of(RoleBinding.builder()
+                        .metadata(ObjectMeta.builder().namespace("test")
+                                .build())
+                        .spec(RoleBinding.RoleBindingSpec.builder()
+                                .role(RoleBinding.Role.builder()
+                                        .resourceTypes(List.of("connects"))
+                                        .verbs(List.of(RoleBinding.Verb.GET))
+                                        .build())
+                                .subject(RoleBinding.Subject.builder().subjectName("group1")
+                                        .build())
+                                .build())
+                        .build()));
+
+        SecurityRuleResult actual = resourceBasedSecurityRule.check(HttpRequest.GET("/api/namespaces/test/connects/name.with.dots/restart"),null, claims);
+        Assertions.assertEquals(SecurityRuleResult.UNKNOWN, actual);
+    }
 
     @Test
     void ComputeRoles_NoAdmin(){