Skip to content

Homogenize permissions checks #215

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.

Sign up for GitHub

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

Merged
merged 11 commits into from
Oct 10, 2025
Merged

Homogenize permissions checks #215

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
9b2b559
Homogenize permissions checks
Oct 6, 2025
53dc492
Fix test
Oct 6, 2025
543dac3
Set hasReadPermissions deeper
Oct 6, 2025
b9ea629
Add permission check to getSubDirectoriesCounts
Oct 7, 2025
49b9bbb
Remove useless permission check to getSubDirectoriesCounts which is i…
Oct 7, 2025
6b84bbb
Rewrite getSubDirectoriesCounts
Oct 7, 2025
e8d5488
Simplify
Oct 7, 2025
b3bf66a
Reduce memory overhead by loading only element's id and their parent id
Oct 7, 2025
0af499b
Checkstyle
Oct 7, 2025
d4ef8f0
add unit test for the issue
dbraquart Oct 10, 2025
32bea87
Check directories permission before retrieving their elements in getS…
Oct 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 27 additions & 25 deletions src/main/java/org/gridsuite/directory/server/DirectoryService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,11 +248,13 @@ public void createElementInDirectoryPath(String directoryPath, ElementAttributes
insertElement(elementAttributes, parentDirectoryUuid);
}

private Map<UUID, Long> getSubDirectoriesCounts(List<UUID> subDirectories, List<String> types) {
List<DirectoryElementRepository.SubDirectoryCount> subdirectoriesCountsList = repositoryService.getSubDirectoriesCounts(subDirectories, types);
Map<UUID, Long> subdirectoriesCountsMap = new HashMap<>();
subdirectoriesCountsList.forEach(e -> subdirectoriesCountsMap.put(e.getId(), e.getCount()));
return subdirectoriesCountsMap;
private Map<UUID, Long> getSubDirectoriesCounts(List<UUID> subDirectories, List<String> types, String userId) {
return repositoryService.findAllByParentIdInAndTypeIn(subDirectories, types).stream()
.filter(child -> hasReadPermissions(userId, List.of(child.getId())))
Copy link
Contributor

@dbraquart dbraquart Oct 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All children are in readable dirs, so we dont need to check (their parent dir) anymore.
Or maybe we have to filter/check in case the child is a sub-dir ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ye a subdirectory can be read protected while its parent isn't so I don't think we can bypass this check

.collect(Collectors.groupingBy(
DirectoryElementRepository.ElementParentage::getParentId,
Collectors.counting()
));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can do differently: rather than finding all children in N directories (each child will be checked by checking its parent directory), we could first eliminate the non-readable dirs among the N, and then retrieve the children ?
Ex:

List<UUID> readableSubDirectories = subDirectories.stream().filter(dirId -> checkPermission(userId, List.of(dirId), READ)).toList();
return repositoryService.findAllByParentIdInAndTypeIn(readableSubDirectories, types).stream()
        .collect(Collectors.groupingBy(
                DirectoryElementRepository.ElementParentage::getParentId,
                Collectors.counting()
        ));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In our current uses cases it might be a bit overkill because we already check directories permissions in functions calling getSubDirectoriesCounts but I reckon it's still worth because by adding it here too we're avoiding future permissions mistakes related to it if it gets involved in new use cases


public List<ElementAttributes> getDirectoryElements(UUID directoryUuid, List<String> types, Boolean recursive, String userId) {
Expand All @@ -270,25 +272,25 @@ public List<ElementAttributes> getDirectoryElements(UUID directoryUuid, List<Str
List<DirectoryElementEntity> descendents = repositoryService.findAllDescendants(directoryUuid).stream().toList();
return descendents
.stream()
.filter(e -> types.isEmpty() || types.contains(e.getType()))
.filter(e -> (types.isEmpty() || types.contains(e.getType())) && hasReadPermissions(userId, List.of(e.getId())))
.map(ElementAttributes::toElementAttributes)
.toList();
} else {
return getAllDirectoryElementsStream(directoryUuid, types).toList();
return getAllDirectoryElementsStream(directoryUuid, types, userId).toList();
}
}

private Stream<ElementAttributes> getOnlyElementsStream(UUID directoryUuid, List<String> types) {
return getAllDirectoryElementsStream(directoryUuid, types)
private Stream<ElementAttributes> getOnlyElementsStream(UUID directoryUuid, List<String> types, String userId) {
return getAllDirectoryElementsStream(directoryUuid, types, userId)
.filter(elementAttributes -> !elementAttributes.getType().equals(DIRECTORY));
}

private Stream<ElementAttributes> getAllDirectoryElementsStream(UUID directoryUuid, List<String> types) {
private Stream<ElementAttributes> getAllDirectoryElementsStream(UUID directoryUuid, List<String> types, String userId) {
List<DirectoryElementEntity> directoryElements = repositoryService.findAllByParentId(directoryUuid);
Map<UUID, Long> subdirectoriesCountsMap = getSubDirectoriesCountsMap(types, directoryElements);
Map<UUID, Long> subdirectoriesCountsMap = getSubDirectoriesCountsMap(types, directoryElements, userId);
return directoryElements
.stream()
.filter(e -> e.getType().equals(DIRECTORY) || types.isEmpty() || types.contains(e.getType()))
.filter(e -> (e.getType().equals(DIRECTORY) || types.isEmpty() || types.contains(e.getType())) && hasReadPermissions(userId, List.of(e.getId())))
.map(e -> toElementAttributes(e, subdirectoriesCountsMap.getOrDefault(e.getId(), 0L)));
}

Expand All @@ -299,21 +301,21 @@ public List<ElementAttributes> getRootDirectories(List<String> types, String use
if (!roleService.isUserExploreAdmin()) {
directoryElements = directoryElements.stream().filter(directoryElementEntity -> hasReadPermissions(userId, List.of(directoryElementEntity.getId()))).toList();
}
Map<UUID, Long> subdirectoriesCountsMap = getSubDirectoriesCountsMap(types, directoryElements);
Map<UUID, Long> subdirectoriesCountsMap = getSubDirectoriesCountsMap(types, directoryElements, userId);
return directoryElements.stream()
.map(e -> toElementAttributes(e, subdirectoriesCountsMap.getOrDefault(e.getId(), 0L)))
.toList();
}

private Map<UUID, Long> getSubDirectoriesCountsMap(List<String> types, List<DirectoryElementEntity> directoryElements) {
return getSubDirectoriesCounts(directoryElements.stream().map(DirectoryElementEntity::getId).toList(), types);
private Map<UUID, Long> getSubDirectoriesCountsMap(List<String> types, List<DirectoryElementEntity> directoryElements, String userId) {
return getSubDirectoriesCounts(directoryElements.stream().map(DirectoryElementEntity::getId).toList(), types, userId);
}

public void updateElement(UUID elementUuid, ElementAttributes newElementAttributes, String userId) {
DirectoryElementEntity directoryElement = getDirectoryElementEntity(elementUuid);
if (!directoryElement.isAttributesUpdatable(newElementAttributes, userId) ||
!directoryElement.getName().equals(newElementAttributes.getElementName()) &&
directoryHasElementOfNameAndType(directoryElement.getParentId(), newElementAttributes.getElementName(), directoryElement.getType())) {
directoryHasElementOfNameAndType(directoryElement.getParentId(), newElementAttributes.getElementName(), directoryElement.getType(), userId)) {
throw new DirectoryException(NOT_ALLOWED);
}

Expand Down Expand Up @@ -350,7 +352,7 @@ private void moveElementDirectory(DirectoryElementEntity element, UUID newDirect
List<DirectoryElementEntity> descendents = isDirectory ? repositoryService.findAllDescendants(element.getId()).stream().toList() : List.of();

// validate move elements
validateElementForMove(element, newDirectoryUuid, descendents.stream().map(DirectoryElementEntity::getId).collect(Collectors.toSet()));
validateElementForMove(element, newDirectoryUuid, descendents.stream().map(DirectoryElementEntity::getId).collect(Collectors.toSet()), userId);

// we update the parent of the moving element
updateElementParentDirectory(element, newDirectoryUuid);
Expand All @@ -369,12 +371,12 @@ private void moveElementDirectory(DirectoryElementEntity element, UUID newDirect

}

private void validateElementForMove(DirectoryElementEntity element, UUID newDirectoryUuid, Set<UUID> descendentsUuids) {
private void validateElementForMove(DirectoryElementEntity element, UUID newDirectoryUuid, Set<UUID> descendentsUuids, String userId) {
if (newDirectoryUuid == element.getId() || descendentsUuids.contains(newDirectoryUuid)) {
throw new DirectoryException(MOVE_IN_DESCENDANT_NOT_ALLOWED);
}

if (directoryHasElementOfNameAndType(newDirectoryUuid, element.getName(), element.getType())) {
if (directoryHasElementOfNameAndType(newDirectoryUuid, element.getName(), element.getType(), userId)) {
throw DirectoryException.createElementNameAlreadyExists(element.getName());
}
}
Expand All @@ -393,8 +395,8 @@ private void validateNewDirectory(UUID newDirectoryUuid) {
}
}

private boolean directoryHasElementOfNameAndType(UUID directoryUUID, String elementName, String elementType) {
return getOnlyElementsStream(directoryUUID, List.of(elementType))
private boolean directoryHasElementOfNameAndType(UUID directoryUUID, String elementName, String elementType, String userId) {
return getOnlyElementsStream(directoryUUID, List.of(elementType), userId)
.anyMatch(
e -> e.getElementName().equals(elementName)
);
Expand Down Expand Up @@ -429,7 +431,7 @@ private void deleteElement(ElementAttributes elementAttributes, String userId) {
}

private void deleteSubElements(UUID elementUuid, String userId) {
getAllDirectoryElementsStream(elementUuid, List.of()).forEach(elementAttributes -> deleteElement(elementAttributes, userId));
getAllDirectoryElementsStream(elementUuid, List.of(), userId).forEach(elementAttributes -> deleteElement(elementAttributes, userId));
}

/**
Expand Down Expand Up @@ -502,15 +504,15 @@ public List<ElementAttributes> getElements(List<UUID> ids, boolean strictMode, L
//if the user is not an admin we filter out elements he doesn't have the permission on
if (!roleService.isUserExploreAdmin()) {
elementEntities = elementEntities.stream().filter(directoryElementEntity ->
hasReadPermissions(userId, List.of(directoryElementEntity.getId()))
).toList();
hasReadPermissions(userId, List.of(directoryElementEntity.getId()))
).toList();
}

if (strictMode && elementEntities.size() != ids.stream().distinct().count()) {
throw new DirectoryException(NOT_FOUND);
}

Map<UUID, Long> subElementsCount = getSubDirectoriesCounts(elementEntities.stream().map(DirectoryElementEntity::getId).toList(), types);
Map<UUID, Long> subElementsCount = getSubDirectoriesCounts(elementEntities.stream().map(DirectoryElementEntity::getId).toList(), types, userId);

return elementEntities.stream()
.map(attribute -> toElementAttributes(attribute, subElementsCount.getOrDefault(attribute.getId(), 0L)))
Expand Down
18 changes: 9 additions & 9 deletions src/main/java/org/gridsuite/directory/server/repository/DirectoryElementRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,15 +57,6 @@ public interface DirectoryElementRepository extends JpaRepository<DirectoryEleme

boolean existsByIdAndOwnerOrId(UUID id, String owner, UUID id2);

interface SubDirectoryCount {
UUID getId();

Long getCount();
}

@Query("SELECT d.parentId AS id, COUNT(*) AS count FROM DirectoryElementEntity d WHERE d.parentId IN :subDirectories AND (d.type = 'DIRECTORY' OR d.type IN :elementTypes) GROUP BY d.parentId")
List<SubDirectoryCount> getSubDirectoriesCounts(List<UUID> subDirectories, List<String> elementTypes);

@Transactional
void deleteById(UUID id);

Expand Down Expand Up @@ -101,4 +92,13 @@ interface SubDirectoryCount {
"WHERE e.id IN (SELECT dh.element_id FROM DescendantHierarchy dh) AND e.id != :elementId"
)
List<DirectoryElementEntity> findAllDescendants(@Param("elementId") UUID elementId);

interface ElementParentage {
UUID getId();

UUID getParentId();
}

@Query("SELECT d.id AS id, d.parentId AS parentId FROM DirectoryElementEntity d WHERE d.parentId IN :parentIds AND (d.type = 'DIRECTORY' OR d.type IN :elementTypes)")
List<ElementParentage> findAllByParentIdsAndElementTypes(List<UUID> parentIds, List<String> elementTypes);
}
8 changes: 4 additions & 4 deletions src/main/java/org/gridsuite/directory/server/services/DirectoryRepositoryService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,10 +105,6 @@ public UUID getParentUuid(UUID elementUuid) {
.orElse(null);
}

public List<DirectoryElementRepository.SubDirectoryCount> getSubDirectoriesCounts(List<UUID> subDirectories, List<String> elementTypes) {
return directoryElementRepository.getSubDirectoriesCounts(subDirectories, elementTypes);
}

public List<DirectoryElementEntity> findAllByIdIn(List<UUID> uuids) {
return directoryElementRepository.findAllByIdIn(uuids);
}
Expand All @@ -117,6 +113,10 @@ public List<DirectoryElementEntity> findAllByParentId(UUID parentId) {
return directoryElementRepository.findAllByParentId(parentId);
}

public List<DirectoryElementRepository.ElementParentage> findAllByParentIdInAndTypeIn(List<UUID> parentIds, List<String> types) {
return directoryElementRepository.findAllByParentIdsAndElementTypes(parentIds, types);
}

public List<DirectoryElementEntity> findRootDirectories() {
return directoryElementRepository.findRootDirectories();
}
Expand Down
33 changes: 33 additions & 0 deletions src/test/java/org/gridsuite/directory/server/AccessRightsControlTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,9 @@ public class AccessRightsControlTest {
@Autowired
private DirectoryElementRepository directoryElementRepository;

@Autowired
private DirectoryService directoryService;

@Autowired
private PermissionRepository permissionRepository;

Expand Down Expand Up @@ -318,6 +321,36 @@ public void testExistence() throws Exception {
insertSubElement(dirUuid1, toElementAttributes(null, "elementName1", TYPE_01, ADMIN_USER), HttpStatus.CONFLICT);
}

@Test
public void testDirectoryContentWithPermissions() throws Exception {
// user1 owns a dir with 2 sub-dirs
UUID rootUuid1 = insertRootDirectory("user1", "root1");
UUID dirUuid1 = insertSubElement(rootUuid1, toElementAttributes(null, "dir1", DIRECTORY, "user1"));
UUID dirUuid2 = insertSubElement(rootUuid1, toElementAttributes(null, "dir2", DIRECTORY, "user1"));

// both user1 ans user2 can see them
assertEquals(2, directoryService.getDirectoryElements(rootUuid1, List.of(DIRECTORY), false, "user1").size());
assertEquals(2, directoryService.getDirectoryElements(rootUuid1, List.of(DIRECTORY), false, "user2").size());

// user1 restraints access to dir2
List<PermissionDTO> newPermissions = List.of(
new PermissionDTO(false, List.of(), PermissionType.READ),
new PermissionDTO(false, List.of(), PermissionType.WRITE)
);
updateDirectoryPermissions("user1", dirUuid2, newPermissions).andExpect(status().isOk());

// only user1 still sees 2 sub-dirs ; users2 only 1
assertEquals(2, directoryService.getDirectoryElements(rootUuid1, List.of(DIRECTORY), false, "user1").size());
assertEquals(1, directoryService.getDirectoryElements(rootUuid1, List.of(DIRECTORY), false, "user2").size());

// user1 restraints access to dir1
updateDirectoryPermissions("user1", dirUuid1, newPermissions).andExpect(status().isOk());

// only user1 still sees 2 sub-dirs ; users2 nothing
assertEquals(2, directoryService.getDirectoryElements(rootUuid1, List.of(DIRECTORY), false, "user1").size());
assertEquals(0, directoryService.getDirectoryElements(rootUuid1, List.of(DIRECTORY), false, "user2").size());
}

@Test
public void testSetDirectoryPermissions() throws Exception {
UUID rootDirectoryUuid = insertRootDirectory(ADMIN_USER, "root1");
Expand Down
8 changes: 4 additions & 4 deletions src/test/java/org/gridsuite/directory/server/DirectoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1121,18 +1121,18 @@ public void testGetElement() {
@Test
public void testGetElementWithNonAdminUser() {
// Insert a root directory by the user1
UUID rootDirUuid = insertAndCheckRootDirectory("rootDir1", "user1");
UUID rootDirUuid = insertAndCheckRootDirectory("rootDir1", ADMIN_USER);

// Insert an element of type TYPE_02 in the root directory by the user1
ElementAttributes element1Attributes = toElementAttributes(TYPE_02_UUID, "elementName1", TYPE_02, "user1");
ElementAttributes element1Attributes = toElementAttributes(TYPE_02_UUID, "elementName1", TYPE_02, ADMIN_USER);
insertAndCheckSubElementInRootDir(rootDirUuid, element1Attributes);

// Insert an element of type TYPE_03 in the root directory by the user1
ElementAttributes element2Attributes = toElementAttributes(TYPE_03_UUID, "elementName2", TYPE_03, "user1");
ElementAttributes element2Attributes = toElementAttributes(TYPE_03_UUID, "elementName2", TYPE_03, ADMIN_USER);
insertAndCheckSubElementInRootDir(rootDirUuid, element2Attributes);

// Insert an element of type TYPE_03 in the root directory by the user1
ElementAttributes element3Attributes = toElementAttributes(UUID.randomUUID(), "elementName3", TYPE_03, "user1");
ElementAttributes element3Attributes = toElementAttributes(UUID.randomUUID(), "elementName3", TYPE_03, ADMIN_USER);
insertAndCheckSubElementInRootDir(rootDirUuid, element3Attributes);

ElementAttributes rootDirectory = getElements(List.of(rootDirUuid), "user1", false, 200).get(0);
Expand Down