From 84ae5646a4b1c01fa8790f06defc1919efabb54a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Pfeuffer?= Date: Fri, 31 May 2019 10:20:44 +0200 Subject: [PATCH 1/3] Fix bug for repository owners without global role permission Repository owners got an frontend error when they hat no permission to read the global repository roles. We therefore remove the dedicated permission to read repository. Additionally we fix the 'write' permission to match the entry in the 'permissions.xml' file. --- .../java/sonia/scm/repository/RepositoryRole.java | 2 +- .../scm/api/v2/resources/IndexDtoGenerator.java | 4 +--- .../RepositoryRoleCollectionToDtoMapper.java | 2 +- .../RepositoryRoleToRepositoryRoleDtoMapper.java | 2 +- .../scm/repository/DefaultRepositoryRoleManager.java | 12 +++--------- .../src/main/resources/META-INF/scm/permissions.xml | 2 +- 6 files changed, 8 insertions(+), 16 deletions(-) diff --git a/scm-core/src/main/java/sonia/scm/repository/RepositoryRole.java b/scm-core/src/main/java/sonia/scm/repository/RepositoryRole.java index 1f8f638cb8..e5ca046aac 100644 --- a/scm-core/src/main/java/sonia/scm/repository/RepositoryRole.java +++ b/scm-core/src/main/java/sonia/scm/repository/RepositoryRole.java @@ -57,7 +57,7 @@ import static java.util.Collections.unmodifiableSet; * Custom role with specific permissions related to {@link Repository}. * This object should be immutable, but could not be due to mapstruct. */ -@StaticPermissions(value = "repositoryRole", permissions = {}, globalPermissions = {"read", "modify"}) +@StaticPermissions(value = "repositoryRole", permissions = {}, globalPermissions = {"write"}) @XmlRootElement(name = "roles") @XmlAccessorType(XmlAccessType.FIELD) public class RepositoryRole implements ModelObject, PermissionObject { diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/IndexDtoGenerator.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/IndexDtoGenerator.java index 634de9381c..00982a1609 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/IndexDtoGenerator.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/IndexDtoGenerator.java @@ -63,9 +63,7 @@ public class IndexDtoGenerator extends HalAppenderMapper { builder.single(link("repositoryTypes", resourceLinks.repositoryTypeCollection().self())); builder.single(link("namespaceStrategies", resourceLinks.namespaceStrategies().self())); - if (RepositoryRolePermissions.read().isPermitted()) { - builder.single(link("repositoryRoles", resourceLinks.repositoryRoleCollection().self())); - } + builder.single(link("repositoryRoles", resourceLinks.repositoryRoleCollection().self())); } else { builder.single(link("login", resourceLinks.authentication().jsonLogin())); } diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/RepositoryRoleCollectionToDtoMapper.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/RepositoryRoleCollectionToDtoMapper.java index ab34efd7fe..82af9a5cda 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/RepositoryRoleCollectionToDtoMapper.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/RepositoryRoleCollectionToDtoMapper.java @@ -25,7 +25,7 @@ public class RepositoryRoleCollectionToDtoMapper extends BasicCollectionToDtoMap } Optional createCreateLink() { - return RepositoryRolePermissions.modify().isPermitted() ? of(resourceLinks.repositoryRoleCollection().create()): empty(); + return RepositoryRolePermissions.write().isPermitted() ? of(resourceLinks.repositoryRoleCollection().create()): empty(); } String createSelfLink() { diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/RepositoryRoleToRepositoryRoleDtoMapper.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/RepositoryRoleToRepositoryRoleDtoMapper.java index 43d364c1b3..a0cc8bae69 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/RepositoryRoleToRepositoryRoleDtoMapper.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/RepositoryRoleToRepositoryRoleDtoMapper.java @@ -27,7 +27,7 @@ public abstract class RepositoryRoleToRepositoryRoleDtoMapper extends BaseMapper @ObjectFactory RepositoryRoleDto createDto(RepositoryRole repositoryRole) { Links.Builder linksBuilder = linkingTo().self(resourceLinks.repositoryRole().self(repositoryRole.getName())); - if (!"system".equals(repositoryRole.getType()) && RepositoryRolePermissions.modify().isPermitted()) { + if (!"system".equals(repositoryRole.getType()) && RepositoryRolePermissions.write().isPermitted()) { linksBuilder.single(link("delete", resourceLinks.repositoryRole().delete(repositoryRole.getName()))); linksBuilder.single(link("update", resourceLinks.repositoryRole().update(repositoryRole.getName()))); } diff --git a/scm-webapp/src/main/java/sonia/scm/repository/DefaultRepositoryRoleManager.java b/scm-webapp/src/main/java/sonia/scm/repository/DefaultRepositoryRoleManager.java index ab0ad16d0e..b725fb614e 100644 --- a/scm-webapp/src/main/java/sonia/scm/repository/DefaultRepositoryRoleManager.java +++ b/scm-webapp/src/main/java/sonia/scm/repository/DefaultRepositoryRoleManager.java @@ -88,7 +88,7 @@ public class DefaultRepositoryRoleManager extends AbstractRepositoryRoleManager return managerDaoAdapter.create( repositoryRole, - RepositoryRolePermissions::modify, + RepositoryRolePermissions::write, newRepositoryRole -> fireEvent(HandlerEventType.BEFORE_CREATE, newRepositoryRole), newRepositoryRole -> fireEvent(HandlerEventType.CREATE, newRepositoryRole) ); @@ -100,7 +100,7 @@ public class DefaultRepositoryRoleManager extends AbstractRepositoryRoleManager logger.info("delete repositoryRole {} of type {}", repositoryRole.getName(), repositoryRole.getType()); managerDaoAdapter.delete( repositoryRole, - RepositoryRolePermissions::modify, + RepositoryRolePermissions::write, toDelete -> fireEvent(HandlerEventType.BEFORE_DELETE, toDelete), toDelete -> fireEvent(HandlerEventType.DELETE, toDelete) ); @@ -116,7 +116,7 @@ public class DefaultRepositoryRoleManager extends AbstractRepositoryRoleManager logger.info("modify repositoryRole {} of type {}", repositoryRole.getName(), repositoryRole.getType()); managerDaoAdapter.modify( repositoryRole, - x -> RepositoryRolePermissions.modify(), + x -> RepositoryRolePermissions.write(), notModified -> fireEvent(HandlerEventType.BEFORE_MODIFY, repositoryRole, notModified), notModified -> fireEvent(HandlerEventType.MODIFY, repositoryRole, notModified)); } @@ -125,7 +125,6 @@ public class DefaultRepositoryRoleManager extends AbstractRepositoryRoleManager public void refresh(RepositoryRole repositoryRole) { logger.info("refresh repositoryRole {} of type {}", repositoryRole.getName(), repositoryRole.getType()); - RepositoryRolePermissions.read().check(); RepositoryRole fresh = repositoryRoleDAO.get(repositoryRole.getName()); if (fresh == null) { @@ -135,8 +134,6 @@ public class DefaultRepositoryRoleManager extends AbstractRepositoryRoleManager @Override public RepositoryRole get(String id) { - RepositoryRolePermissions.read().check(); - return findSystemRole(id).orElse(findCustomRole(id)); } @@ -168,9 +165,6 @@ public class DefaultRepositoryRoleManager extends AbstractRepositoryRoleManager public List getAll() { List repositoryRoles = new ArrayList<>(); - if (!RepositoryRolePermissions.read().isPermitted()) { - return Collections.emptyList(); - } for (RepositoryRole repositoryRole : repositoryPermissionProvider.availableRoles()) { repositoryRoles.add(repositoryRole.clone()); } diff --git a/scm-webapp/src/main/resources/META-INF/scm/permissions.xml b/scm-webapp/src/main/resources/META-INF/scm/permissions.xml index 620fc484b1..3598b178bf 100644 --- a/scm-webapp/src/main/resources/META-INF/scm/permissions.xml +++ b/scm-webapp/src/main/resources/META-INF/scm/permissions.xml @@ -67,7 +67,7 @@ configuration:read,write:* - repositoryRole:read,write + repositoryRole:write From 55069cf2dd97d55c223e4382a08228acc55d183a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Pfeuffer?= Date: Fri, 31 May 2019 10:47:50 +0200 Subject: [PATCH 2/3] Adapt unit test --- .../DefaultRepositoryRoleManagerTest.java | 33 +++++++++++++------ 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/scm-webapp/src/test/java/sonia/scm/repository/DefaultRepositoryRoleManagerTest.java b/scm-webapp/src/test/java/sonia/scm/repository/DefaultRepositoryRoleManagerTest.java index cd0ba963b9..9c6ab10041 100644 --- a/scm-webapp/src/test/java/sonia/scm/repository/DefaultRepositoryRoleManagerTest.java +++ b/scm-webapp/src/test/java/sonia/scm/repository/DefaultRepositoryRoleManagerTest.java @@ -89,8 +89,7 @@ class DefaultRepositoryRoleManagerTest { @BeforeEach void authorizeUser() { - when(subject.isPermitted("repositoryRole:read")).thenReturn(true); - when(subject.isPermitted("repositoryRole:modify")).thenReturn(true); + when(subject.isPermitted("repositoryRole:write")).thenReturn(true); } @Test @@ -184,8 +183,15 @@ class DefaultRepositoryRoleManagerTest { } @Test - void shouldThrowException_forGet() { - assertThrows(UnauthorizedException.class, () -> manager.get("any")); + void shouldReturnNull_forNotExistingRole() { + RepositoryRole role = manager.get("noSuchRole"); + assertThat(role).isNull(); + } + + @Test + void shouldReturnRole_forExistingRole() { + RepositoryRole role = manager.get(CUSTOM_ROLE_NAME); + assertThat(role).isNotNull(); } @Test @@ -201,18 +207,25 @@ class DefaultRepositoryRoleManagerTest { } @Test - void shouldReturnEmptyList() { - assertThat(manager.getAll()).isEmpty(); + void shouldReturnAllRoles() { + List allRoles = manager.getAll(); + assertThat(allRoles).containsExactly(CUSTOM_ROLE, SYSTEM_ROLE); } @Test - void shouldReturnEmptyFilteredList() { - assertThat(manager.getAll(x -> true, null)).isEmpty(); + void shouldReturnFilteredList() { + Collection allRoles = manager.getAll(role -> CUSTOM_ROLE_NAME.equals(role.getName()), null); + assertThat(allRoles).containsExactly(CUSTOM_ROLE); } @Test - void shouldReturnEmptyPaginatedList() { - assertThat(manager.getAll(1, 1)).isEmpty(); + void shouldReturnPaginatedRoles() { + Collection allRoles = + manager.getAll( + Comparator.comparing(RepositoryRole::getType), + 1, 1 + ); + assertThat(allRoles).containsExactly(CUSTOM_ROLE); } } } From 743feb27a8de2db4f03cc4656dd962804e2008a6 Mon Sep 17 00:00:00 2001 From: Sebastian Sdorra Date: Mon, 3 Jun 2019 13:56:11 +0000 Subject: [PATCH 3/3] Close branch bugfix/repo_permissions_without_admin