Skip to content

Commit

Permalink
Fix obsolete ACLs after namespace deletion (#518)
Browse files Browse the repository at this point in the history 
* Fix ACL not deleted after namespace deletion

* add unit tests

* fix variable position

* rename test + add deleted resources arg

* remove test
  • Loading branch information
@ThomasCAI-mlv
ThomasCAI-mlv authored Feb 26, 2025
1 parent eb34048 commit c51cfc6
Show file tree
Hide file tree
Showing 4 changed files with 163 additions and 13 deletions.
47 changes: 35 additions & 12 deletions src/main/java/com/michelin/ns4kafka/service/AclService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,16 @@ public class AclService {
@Inject
ApplicationContext applicationContext;

/**
* Is public ACL.
*
* @param accessControlEntry The ACL
* @return A list of validation errors
*/
public boolean isPublicAcl(AccessControlEntry accessControlEntry) {
return accessControlEntry.getSpec().getGrantedTo().equals(PUBLIC_GRANTED_TO);
}

/**
* Validate a new ACL.
*
Expand Down Expand Up @@ -103,7 +113,7 @@ public List<String> validate(AccessControlEntry accessControlEntry, Namespace na
Optional<Namespace> granteeNamespace =
namespaceService.findByName(accessControlEntry.getSpec().getGrantedTo());
boolean granteeExists = granteeNamespace.isPresent();
boolean granteeIsPublic = accessControlEntry.getSpec().getGrantedTo().equals(PUBLIC_GRANTED_TO);
boolean granteeIsPublic = isPublicAcl(accessControlEntry);

if (!granteeExists && !granteeIsPublic) {
validationErrors.add(invalidNotFound("grantedTo", accessControlEntry.getSpec().getGrantedTo()));
Expand Down Expand Up @@ -228,9 +238,9 @@ public boolean isOwnerOfTopLevelAcl(AccessControlEntry accessControlEntry, Names
// Grantor Namespace is OWNER of Resource + ResourcePattern ?
return findAllGrantedToNamespace(namespace)
.stream()
.filter(ace -> ace.getSpec().getResourceType() == accessControlEntry.getSpec().getResourceType()
&& ace.getSpec().getPermission() == AccessControlEntry.Permission.OWNER)
.anyMatch(ace -> {
.filter(acl -> acl.getSpec().getResourceType() == accessControlEntry.getSpec().getResourceType()
&& acl.getSpec().getPermission() == AccessControlEntry.Permission.OWNER)
.anyMatch(acl -> {
// if grantor is owner of PREFIXED resource that starts with
// owner PREFIXED: priv_bsm_
// grants LITERAL : priv_bsm_topic OK
Expand All @@ -239,8 +249,8 @@ public boolean isOwnerOfTopLevelAcl(AccessControlEntry accessControlEntry, Names
// grants LITERAL : priv_b NO
// grants PREFIXED: priv_bsm_ OK
// grants LITERAL : pric_bsm_ OK
if (ace.getSpec().getResourcePatternType() == AccessControlEntry.ResourcePatternType.PREFIXED
&& accessControlEntry.getSpec().getResource().startsWith(ace.getSpec().getResource())) {
if (acl.getSpec().getResourcePatternType() == AccessControlEntry.ResourcePatternType.PREFIXED
&& accessControlEntry.getSpec().getResource().startsWith(acl.getSpec().getResource())) {
// if so, either patternType are fine (LITERAL/PREFIXED)
return true;
}
Expand All @@ -253,10 +263,10 @@ public boolean isOwnerOfTopLevelAcl(AccessControlEntry accessControlEntry, Names
// grants LITERAL : priv_b NO
// grants PREFIXED: priv_bsm_topic2 NO
// grants LITERAL : pric_bsm_topic2 NO
return ace.getSpec().getResourcePatternType() == AccessControlEntry.ResourcePatternType.LITERAL
return acl.getSpec().getResourcePatternType() == AccessControlEntry.ResourcePatternType.LITERAL
&& accessControlEntry.getSpec().getResourcePatternType()
== AccessControlEntry.ResourcePatternType.LITERAL
&& accessControlEntry.getSpec().getResource().equals(ace.getSpec().getResource());
&& accessControlEntry.getSpec().getResource().equals(acl.getSpec().getResource());
});
}

Expand Down Expand Up @@ -295,7 +305,7 @@ public List<AccessControlEntry> findAllGrantedToNamespace(Namespace namespace) {
return findAllForCluster(namespace.getMetadata().getCluster())
.stream()
.filter(acl -> acl.getSpec().getGrantedTo().equals(namespace.getMetadata().getName())
|| acl.getSpec().getGrantedTo().equals(PUBLIC_GRANTED_TO))
|| isPublicAcl(acl))
.toList();
}

Expand Down Expand Up @@ -337,7 +347,7 @@ public List<AccessControlEntry> findAllRelatedToNamespace(Namespace namespace) {
.stream()
.filter(acl -> acl.getMetadata().getNamespace().equals(namespace.getMetadata().getName())
|| acl.getSpec().getGrantedTo().equals(namespace.getMetadata().getName())
|| acl.getSpec().getGrantedTo().equals(PUBLIC_GRANTED_TO))
|| isPublicAcl(acl))
.toList();
}

Expand Down Expand Up @@ -427,7 +437,7 @@ public List<AccessControlEntry> findResourceOwnerGrantedToNamespace(Namespace na
public List<AccessControlEntry> findAllPublicGrantedTo() {
return accessControlEntryRepository.findAll()
.stream()
.filter(accessControlEntry -> accessControlEntry.getSpec().getGrantedTo().equals(PUBLIC_GRANTED_TO))
.filter(this::isPublicAcl)
.toList();
}

Expand Down Expand Up @@ -475,7 +485,8 @@ public List<AccessControlEntry> findAll() {
* @param resource The resource name
* @return true if it is, false otherwise
*/
public boolean isNamespaceOwnerOfResource(String namespace, AccessControlEntry.ResourceType resourceType,
public boolean isNamespaceOwnerOfResource(String namespace,
AccessControlEntry.ResourceType resourceType,
String resource) {
return accessControlEntryRepository.findAll()
.stream()
Expand Down Expand Up @@ -515,4 +526,16 @@ public boolean isResourceCoveredByAcls(List<AccessControlEntry> acls, String res
case LITERAL -> resourceName.equals(acl.getSpec().getResource());
});
}

/**
* Delete all the ACLs granted to the given namespace.
*
* @param namespace The namespace
*/
public void deleteAllGrantedToNamespace(Namespace namespace) {
findAllGrantedToNamespace(namespace)
.stream()
.filter(acl -> !isPublicAcl(acl))
.forEach(this::delete);
}
}
1 change: 1 addition & 0 deletions src/main/java/com/michelin/ns4kafka/service/NamespaceService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,7 @@ public Namespace createOrUpdate(Namespace namespace) {
* @param namespace The namespace to delete
*/
public void delete(Namespace namespace) {
aclService.deleteAllGrantedToNamespace(namespace);
namespaceRepository.delete(namespace);
}

Expand Down
122 changes: 122 additions & 0 deletions src/test/java/com/michelin/ns4kafka/service/AclServiceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,21 @@
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertLinesMatch;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.argThat;
import static org.mockito.Mockito.doNothing;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

import com.michelin.ns4kafka.model.AccessControlEntry;
import com.michelin.ns4kafka.model.Metadata;
import com.michelin.ns4kafka.model.Namespace;
import com.michelin.ns4kafka.repository.AccessControlEntryRepository;
import com.michelin.ns4kafka.service.executor.AccessControlEntryAsyncExecutor;
import io.micronaut.context.ApplicationContext;
import io.micronaut.inject.qualifiers.Qualifiers;
import java.util.List;
import java.util.Optional;
import org.junit.jupiter.api.Test;
Expand All @@ -45,6 +53,9 @@ class AclServiceTest {
@Mock
AccessControlEntryRepository accessControlEntryRepository;

@Mock
AccessControlEntryAsyncExecutor accessControlEntryAsyncExecutor;

@Mock
ApplicationContext applicationContext;

Expand Down Expand Up @@ -1680,4 +1691,115 @@ void shouldLiteralAclsMatchResource() {
assertTrue(aclService.isResourceCoveredByAcls(acls, "abc.topic1"));
assertTrue(aclService.isResourceCoveredByAcls(acls, "abc-topic1"));
}

@Test
void shouldDeleteAllGrantedToAclsForNamespace() {
AccessControlEntry acl1 = AccessControlEntry.builder()
.spec(AccessControlEntry.AccessControlEntrySpec.builder()
.grantedTo("namespace1")
.build())
.metadata(Metadata.builder()
.cluster("cluster")
.build())
.build();

AccessControlEntry acl2 = AccessControlEntry.builder()
.spec(AccessControlEntry.AccessControlEntrySpec.builder()
.grantedTo("namespace1")
.build())
.metadata(Metadata.builder()
.cluster("cluster")
.build())
.build();

AccessControlEntry acl3 = AccessControlEntry.builder()
.spec(AccessControlEntry.AccessControlEntrySpec.builder()
.grantedTo("namespace2")
.build())
.metadata(Metadata.builder()
.cluster("cluster")
.build())
.build();

when(accessControlEntryRepository.findAll())
.thenReturn(List.of(acl1, acl2, acl3));
when(applicationContext.getBean(AccessControlEntryAsyncExecutor.class, Qualifiers.byName("cluster")))
.thenReturn(accessControlEntryAsyncExecutor);
doNothing().when(accessControlEntryRepository).delete(any());

Namespace namespace = Namespace.builder()
.metadata(Metadata.builder()
.name("namespace1")
.cluster("cluster")
.build())
.build();

aclService.deleteAllGrantedToNamespace(namespace);

verify(accessControlEntryRepository, times(2)).delete(argThat(arg -> arg.equals(acl1) || arg.equals(acl2)));
verify(accessControlEntryRepository, never()).delete(acl3);
}

@Test
void shouldDeleteAllGrantedToAclsForNamespaceAndNotDeletePublicAcl() {
AccessControlEntry acl1 = AccessControlEntry.builder()
.spec(AccessControlEntry.AccessControlEntrySpec.builder()
.grantedTo("namespace1")
.build())
.metadata(Metadata.builder()
.cluster("cluster")
.build())
.build();

AccessControlEntry acl2 = AccessControlEntry.builder()
.spec(AccessControlEntry.AccessControlEntrySpec.builder()
.grantedTo("namespace1")
.build())
.metadata(Metadata.builder()
.cluster("cluster")
.build())
.build();

AccessControlEntry publicAcl = AccessControlEntry.builder()
.spec(AccessControlEntry.AccessControlEntrySpec.builder()
.grantedTo("*")
.build())
.metadata(Metadata.builder()
.cluster("cluster")
.build())
.build();

when(accessControlEntryRepository.findAll()).thenReturn(List.of(acl1, acl2, publicAcl));
when(applicationContext.getBean(AccessControlEntryAsyncExecutor.class, Qualifiers.byName("cluster")))
.thenReturn(accessControlEntryAsyncExecutor);
doNothing().when(accessControlEntryRepository).delete(any());

Namespace namespace = Namespace.builder()
.metadata(Metadata.builder()
.name("namespace1")
.cluster("cluster")
.build())
.build();

aclService.deleteAllGrantedToNamespace(namespace);

verify(accessControlEntryRepository, times(2)).delete(argThat(arg -> arg.equals(acl1) || arg.equals(acl2)));
verify(accessControlEntryRepository, never()).delete(publicAcl);
}

@Test
void shouldNotDeleteAllAclsForNamespaceWhenNoAcl() {
Namespace namespace = Namespace.builder()
.metadata(Metadata.builder()
.name("namespace1")
.cluster("cluster")
.build())
.build();

when(accessControlEntryRepository.findAll()).thenReturn(List.of());

aclService.deleteAllGrantedToNamespace(namespace);

verify(accessControlEntryRepository, never()).delete(any());
}
}
6 changes: 5 additions & 1 deletion src/test/java/com/michelin/ns4kafka/service/NamespaceServiceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import static com.michelin.ns4kafka.property.ManagedClusterProperties.KafkaProvider.SELF_MANAGED;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.doNothing;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

Expand Down Expand Up @@ -864,7 +865,7 @@ void shouldListAllNamespaceResourcesOfTypeQuota() {
}

@Test
void shouldDeleteNamespace() {
void shouldDeleteGrantedToAclAndNamespace() {
Namespace ns = Namespace.builder()
.metadata(Metadata.builder()
.name("namespace")
Expand All @@ -876,8 +877,11 @@ void shouldDeleteNamespace() {
.build())
.build();

doNothing().when(aclService).deleteAllGrantedToNamespace(ns);

namespaceService.delete(ns);

verify(aclService).deleteAllGrantedToNamespace(ns);
verify(namespaceRepository).delete(ns);
}
}

0 comments on commit c51cfc6

Please sign in to comment.