From c2ad5a7e28bea2d37f8cc9a4f45eae410c060e98 Mon Sep 17 00:00:00 2001 From: Michael Hoennig Date: Fri, 8 Mar 2024 16:04:58 +0100 Subject: [PATCH] fix Debitor RBAC system --- .../debitor/HsOfficeDebitorController.java | 1 - .../resources/db/changelog/050-rbac-base.sql | 37 +----- .../db/changelog/057-rbac-role-builder.sql | 11 +- .../changelog/118-test-customer-test-data.sql | 4 +- .../changelog/128-test-package-test-data.sql | 2 +- .../changelog/233-hs-office-partner-rbac.sql | 6 +- .../313-hs-office-coopshares-rbac.sql | 2 +- .../323-hs-office-coopassets-rbac.sql | 2 +- .../hsadminng/arch/ArchitectureTest.java | 7 +- ...fficeDebitorRepositoryIntegrationTest.java | 8 +- ...acGrantsDiagramServiceIntegrationTest.java | 106 ++++++------------ ...t.java => TestCustomerEntityUnitTest.java} | 2 +- ...st.java => TestPackageEntityUnitTest.java} | 2 +- 13 files changed, 64 insertions(+), 126 deletions(-) rename src/test/java/net/hostsharing/hsadminng/test/cust/{TestCustomerEntityTest.java => TestCustomerEntityUnitTest.java} (98%) rename src/test/java/net/hostsharing/hsadminng/test/pac/{TestPackageEntityTest.java => TestPackageEntityUnitTest.java} (98%) diff --git a/src/main/java/net/hostsharing/hsadminng/hs/office/debitor/HsOfficeDebitorController.java b/src/main/java/net/hostsharing/hsadminng/hs/office/debitor/HsOfficeDebitorController.java index 91e2785c..bc4175ca 100644 --- a/src/main/java/net/hostsharing/hsadminng/hs/office/debitor/HsOfficeDebitorController.java +++ b/src/main/java/net/hostsharing/hsadminng/hs/office/debitor/HsOfficeDebitorController.java @@ -62,7 +62,6 @@ public class HsOfficeDebitorController implements HsOfficeDebitorsApi { final var entityToSave = mapper.map(body, HsOfficeDebitorEntity.class); final var saved = debitorRepo.save(entityToSave); - em.flush(); // FIXME: remove final var uri = MvcUriComponentsBuilder.fromController(getClass()) diff --git a/src/main/resources/db/changelog/050-rbac-base.sql b/src/main/resources/db/changelog/050-rbac-base.sql index 2d851197..4e63700f 100644 --- a/src/main/resources/db/changelog/050-rbac-base.sql +++ b/src/main/resources/db/changelog/050-rbac-base.sql @@ -86,29 +86,6 @@ create or replace function findRbacUserId(userName varchar) language sql as $$ select uuid from RbacUser where name = userName $$; - -create type RbacWhenNotExists as enum ('fail', 'create'); - -create or replace function getRbacUserId(userName varchar, whenNotExists RbacWhenNotExists) - returns uuid - returns null on null input - language plpgsql as $$ -declare - userUuid uuid; -begin - userUuid = findRbacUserId(userName); - if (userUuid is null) then - if (whenNotExists = 'fail') then - raise exception 'RbacUser with name="%" not found', userName; - end if; - if (whenNotExists = 'create') then - userUuid = createRbacUser(userName); - end if; - end if; - return userUuid; -end; -$$; - --// -- ============================================================================ @@ -293,24 +270,18 @@ create or replace function findRoleId(roleDescriptor RbacRoleDescriptor) select uuid from RbacRole where objectUuid = roleDescriptor.objectUuid and roleType = roleDescriptor.roleType; $$; -create or replace function getRoleId(roleDescriptor RbacRoleDescriptor, whenNotExists RbacWhenNotExists) +create or replace function getRoleId(roleDescriptor RbacRoleDescriptor) returns uuid - returns null on null input language plpgsql as $$ declare roleUuid uuid; begin + assert roleDescriptor is not null, 'roleDescriptor must not be null'; + roleUuid := findRoleId(roleDescriptor); - assert roleUuid is not null, 'roleUuid must not be null'; -- FIXME: remove if (roleUuid is null) then - if (whenNotExists = 'fail') then - raise exception 'RbacRole "%#%.%" not found', roleDescriptor.objectTable, roleDescriptor.objectUuid, roleDescriptor.roleType; - end if; - if (whenNotExists = 'create') then - roleUuid := createRole(roleDescriptor); - end if; + raise exception 'RbacRole "%#%.%" not found', roleDescriptor.objectTable, roleDescriptor.objectUuid, roleDescriptor.roleType; end if; - assert roleUuid is not null, 'roleUuid must not be null'; -- FIXME: remove return roleUuid; end; $$; diff --git a/src/main/resources/db/changelog/057-rbac-role-builder.sql b/src/main/resources/db/changelog/057-rbac-role-builder.sql index 49975123..1a7da953 100644 --- a/src/main/resources/db/changelog/057-rbac-role-builder.sql +++ b/src/main/resources/db/changelog/057-rbac-role-builder.sql @@ -45,16 +45,15 @@ begin call grantPermissionsToRole(roleUuid, toPermissionUuids(roleDescriptor.objectuuid, permissions)); end if; - foreach superRoleDesc in array incomingSuperRoles + foreach superRoleDesc in array array_remove(incomingSuperRoles, null) loop - superRoleUuid := getRoleId(superRoleDesc, 'fail'); + superRoleUuid := getRoleId(superRoleDesc); call grantRoleToRole(roleUuid, superRoleUuid, superRoleDesc.assumed); end loop; - foreach subRoleDesc in array outgoingSubRoles + foreach subRoleDesc in array array_remove(outgoingSubRoles, null) loop - subRoleUuid := getRoleId(subRoleDesc, 'fail'); - assert subRoleUuid is not null, 'subRoleUuid must not be null'; -- FIXME: remove + subRoleUuid := getRoleId(subRoleDesc); call grantRoleToRole(subRoleUuid, roleUuid, subRoleDesc.assumed); end loop; @@ -62,7 +61,7 @@ begin if grantedByRole is null then grantedByRoleUuid := roleUuid; else - grantedByRoleUuid := getRoleId(grantedByRole, 'fail'); + grantedByRoleUuid := getRoleId(grantedByRole); end if; foreach userUuid in array userUuids loop diff --git a/src/main/resources/db/changelog/118-test-customer-test-data.sql b/src/main/resources/db/changelog/118-test-customer-test-data.sql index 1e239001..85c34ac6 100644 --- a/src/main/resources/db/changelog/118-test-customer-test-data.sql +++ b/src/main/resources/db/changelog/118-test-customer-test-data.sql @@ -46,8 +46,8 @@ begin select * into newCust from test_customer where reference=custReference; call grantRoleToUser( - getRoleId(testCustomerOwner(newCust), 'fail'), - getRoleId(testCustomerAdmin(newCust), 'fail'), + getRoleId(testCustomerOwner(newCust)), + getRoleId(testCustomerAdmin(newCust)), custAdminUuid, true); end; $$; diff --git a/src/main/resources/db/changelog/128-test-package-test-data.sql b/src/main/resources/db/changelog/128-test-package-test-data.sql index 8c6568f3..9abba772 100644 --- a/src/main/resources/db/changelog/128-test-package-test-data.sql +++ b/src/main/resources/db/changelog/128-test-package-test-data.sql @@ -35,7 +35,7 @@ begin returning * into pac; call grantRoleToUser( - getRoleId(testCustomerAdmin(cust), 'fail'), + getRoleId(testCustomerAdmin(cust)), findRoleId(testPackageAdmin(pac)), createRbacUser('pac-admin-' || pacName || '@' || cust.prefix || '.example.com'), true); diff --git a/src/main/resources/db/changelog/233-hs-office-partner-rbac.sql b/src/main/resources/db/changelog/233-hs-office-partner-rbac.sql index a6ad3733..e7634d46 100644 --- a/src/main/resources/db/changelog/233-hs-office-partner-rbac.sql +++ b/src/main/resources/db/changelog/233-hs-office-partner-rbac.sql @@ -98,12 +98,12 @@ begin --Attention: Cannot be in partner-details because of insert order (partner is not in database yet) call grantPermissionsToRole( - getRoleId(hsOfficePartnerOwner(NEW), 'fail'), + getRoleId(hsOfficePartnerOwner(NEW)), createPermissions(NEW.detailsUuid, array ['DELETE']) ); call grantPermissionsToRole( - getRoleId(hsOfficePartnerAdmin(NEW), 'fail'), + getRoleId(hsOfficePartnerAdmin(NEW)), createPermissions(NEW.detailsUuid, array ['UPDATE']) ); @@ -111,7 +111,7 @@ begin -- Yes, here hsOfficePartnerAGENT is used, not hsOfficePartnerTENANT. -- Do NOT grant view permission on partner-details to hsOfficePartnerTENANT! -- Otherwise package-admins etc. would be able to read the data. - getRoleId(hsOfficePartnerAgent(NEW), 'fail'), + getRoleId(hsOfficePartnerAgent(NEW)), createPermissions(NEW.detailsUuid, array ['SELECT']) ); diff --git a/src/main/resources/db/changelog/313-hs-office-coopshares-rbac.sql b/src/main/resources/db/changelog/313-hs-office-coopshares-rbac.sql index a79d354e..5082a3ca 100644 --- a/src/main/resources/db/changelog/313-hs-office-coopshares-rbac.sql +++ b/src/main/resources/db/changelog/313-hs-office-coopshares-rbac.sql @@ -42,7 +42,7 @@ begin -- coopsharestransactions cannot be edited nor deleted, just created+viewed call grantPermissionsToRole( - getRoleId(hsOfficeMembershipTenant(newHsOfficeMembership), 'fail'), + getRoleId(hsOfficeMembershipTenant(newHsOfficeMembership)), createPermissions(NEW.uuid, array ['SELECT']) ); diff --git a/src/main/resources/db/changelog/323-hs-office-coopassets-rbac.sql b/src/main/resources/db/changelog/323-hs-office-coopassets-rbac.sql index 38fec4ff..6fbdc5ce 100644 --- a/src/main/resources/db/changelog/323-hs-office-coopassets-rbac.sql +++ b/src/main/resources/db/changelog/323-hs-office-coopassets-rbac.sql @@ -42,7 +42,7 @@ begin -- coopassetstransactions cannot be edited nor deleted, just created+viewed call grantPermissionsToRole( - getRoleId(hsOfficeMembershipTenant(newHsOfficeMembership), 'fail'), + getRoleId(hsOfficeMembershipTenant(newHsOfficeMembership)), createPermissions(NEW.uuid, array ['SELECT']) ); diff --git a/src/test/java/net/hostsharing/hsadminng/arch/ArchitectureTest.java b/src/test/java/net/hostsharing/hsadminng/arch/ArchitectureTest.java index fe50ccf1..82856124 100644 --- a/src/test/java/net/hostsharing/hsadminng/arch/ArchitectureTest.java +++ b/src/test/java/net/hostsharing/hsadminng/arch/ArchitectureTest.java @@ -49,6 +49,8 @@ public class ArchitectureTest { "..rbac.rbacuser", "..rbac.rbacgrant", "..rbac.rbacrole", + "..rbac.rbacobject", + "..rbac.rbacdef", "..stringify" // ATTENTION: Don't simply add packages here, also add arch rules for the new package! ); @@ -116,7 +118,10 @@ public class ArchitectureTest { public static final ArchRule hsAdminPackagesRule = classes() .that().resideInAPackage("..hs.office.(*)..") .should().onlyBeAccessed().byClassesThat() - .resideInAnyPackage("..hs.office.(*).."); + .resideInAnyPackage( + "..hs.office.(*)..", + "..rbac.rbacgrant" // TODO: just because of RbacGrantsDiagramServiceIntegrationTest + ); @ArchTest @SuppressWarnings("unused") diff --git a/src/test/java/net/hostsharing/hsadminng/hs/office/debitor/HsOfficeDebitorRepositoryIntegrationTest.java b/src/test/java/net/hostsharing/hsadminng/hs/office/debitor/HsOfficeDebitorRepositoryIntegrationTest.java index c703c31a..7c2420ee 100644 --- a/src/test/java/net/hostsharing/hsadminng/hs/office/debitor/HsOfficeDebitorRepositoryIntegrationTest.java +++ b/src/test/java/net/hostsharing/hsadminng/hs/office/debitor/HsOfficeDebitorRepositoryIntegrationTest.java @@ -119,7 +119,7 @@ class HsOfficeDebitorRepositoryIntegrationTest extends ContextBasedTestWithClean // then System.out.println("ok"); -// result.assertExceptionWithRootCauseMessage(org.hibernate.exception.ConstraintViolationException.class); + result.assertExceptionWithRootCauseMessage(org.hibernate.exception.ConstraintViolationException.class); } @Test @@ -167,12 +167,12 @@ class HsOfficeDebitorRepositoryIntegrationTest extends ContextBasedTestWithClean .containsExactlyInAnyOrder(Array.fromFormatted( initialGrantNames, // owner - "{ grant perm * on debitor#1000422:FeG to role debitor#1000422:FeG.owner by system and assume }", + "{ grant perm DELETE on debitor#1000422:FeG to role debitor#1000422:FeG.owner by system and assume }", "{ grant role debitor#1000422:FeG.owner to role global#global.admin by system and assume }", "{ grant role debitor#1000422:FeG.owner to user superuser-alex by global#global.admin and assume }", // admin - "{ grant perm edit on debitor#1000422:FeG to role debitor#1000422:FeG.admin by system and assume }", + "{ grant perm UPDATE on debitor#1000422:FeG to role debitor#1000422:FeG.admin by system and assume }", "{ grant role debitor#1000422:FeG.admin to role debitor#1000422:FeG.owner by system and assume }", // agent @@ -187,7 +187,7 @@ class HsOfficeDebitorRepositoryIntegrationTest extends ContextBasedTestWithClean "{ grant role partner#10004:FeG.tenant to role debitor#1000422:FeG.tenant by system and assume }", // guest - "{ grant perm view on debitor#1000422:FeG to role debitor#1000422:FeG.guest by system and assume }", + "{ grant perm SELECT on debitor#1000422:FeG to role debitor#1000422:FeG.guest by system and assume }", "{ grant role debitor#1000422:FeG.guest to role debitor#1000422:FeG.tenant by system and assume }", null)); diff --git a/src/test/java/net/hostsharing/hsadminng/rbac/rbacgrant/RbacGrantsDiagramServiceIntegrationTest.java b/src/test/java/net/hostsharing/hsadminng/rbac/rbacgrant/RbacGrantsDiagramServiceIntegrationTest.java index 442f4979..0ed953ca 100644 --- a/src/test/java/net/hostsharing/hsadminng/rbac/rbacgrant/RbacGrantsDiagramServiceIntegrationTest.java +++ b/src/test/java/net/hostsharing/hsadminng/rbac/rbacgrant/RbacGrantsDiagramServiceIntegrationTest.java @@ -4,7 +4,10 @@ import net.hostsharing.hsadminng.context.Context; import net.hostsharing.hsadminng.hs.office.test.ContextBasedTestWithCleanup; import net.hostsharing.hsadminng.rbac.rbacgrant.RbacGrantsDiagramService.Include; import net.hostsharing.test.JpaAttempt; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInfo; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.autoconfigure.orm.jpa.DataJpaTest; import org.springframework.boot.test.mock.mockito.MockBean; @@ -28,6 +31,27 @@ class RbacGrantsDiagramServiceIntegrationTest extends ContextBasedTestWithCleanu @MockBean HttpServletRequest request; + @Autowired + Context context; + + @Autowired + RbacGrantsDiagramService diagramService; + + TestInfo test; + + @BeforeEach + void init(TestInfo testInfo) { + this.test = testInfo; + } + + protected void context(final String currentUser, final String assumedRoles) { + context.define(test.getDisplayName(), null, currentUser, assumedRoles); + } + + protected void context(final String currentUser) { + context(currentUser, null); + } + @Test void allGrantsToCurrentUser() { context("superuser-alex@hostsharing.net", "test_domain#xxx00-aaaa.owner"); @@ -36,27 +60,9 @@ class RbacGrantsDiagramServiceIntegrationTest extends ContextBasedTestWithCleanu assertThat(graph).isEqualTo(""" flowchart TB - role:test_package#xxx00.tenant[ - test_package - xxx00.t - tenant] --> role:test_customer#xxx.tenant[ - test_customer - xxx.t - tenant] - role:test_domain#xxx00-aaaa.owner[ - test_domain - xxx00-aaaa.o - owner] --> role:test_domain#xxx00-aaaa.admin[ - test_domain - xxx00-aaaa.a - admin] - role:test_domain#xxx00-aaaa.admin[ - test_domain - xxx00-aaaa.a - admin] --> role:test_package#xxx00.tenant[ - test_package - xxx00.t - tenant] + role:test_domain#xxx00-aaaa.admin --> role:test_package#xxx00.tenant + role:test_domain#xxx00-aaaa.owner --> role:test_domain#xxx00-aaaa.admin + role:test_package#xxx00.tenant --> role:test_customer#xxx.tenant """.trim()); } @@ -68,60 +74,18 @@ class RbacGrantsDiagramServiceIntegrationTest extends ContextBasedTestWithCleanu assertThat(graph).isEqualTo(""" flowchart TB - role:test_domain#xxx00-aaaa.owner[ - test_domain - xxx00-aaaa.o - owner] --> perm:*:on:test_domain#xxx00-aaaa{{ - test_domain - xxx00-aaaa - *}} - role:test_customer#xxx.tenant[ - test_customer - xxx.t - tenant] --> perm:view:on:test_customer#xxx{{ - test_customer - xxx - view}} - role:test_domain#xxx00-aaaa.admin[ - test_domain - xxx00-aaaa.a - admin] --> perm:edit:on:test_domain#xxx00-aaaa{{ - test_domain - xxx00-aaaa - edit}} - role:test_package#xxx00.tenant[ - test_package - xxx00.t - tenant] --> role:test_customer#xxx.tenant[ - test_customer - xxx.t - tenant] - role:test_domain#xxx00-aaaa.owner[ - test_domain - xxx00-aaaa.o - owner] --> role:test_domain#xxx00-aaaa.admin[ - test_domain - xxx00-aaaa.a - admin] - role:test_package#xxx00.tenant[ - test_package - xxx00.t - tenant] --> perm:view:on:test_package#xxx00{{ - test_package - xxx00 - view}} - role:test_domain#xxx00-aaaa.admin[ - test_domain - xxx00-aaaa.a - admin] --> role:test_package#xxx00.tenant[ - test_package - xxx00.t - tenant] + role:test_customer#xxx.tenant --> perm:SELECT:on:test_customer#xxx + role:test_domain#xxx00-aaaa.admin --> perm:UPDATE:on:test_domain#xxx00-aaaa + role:test_domain#xxx00-aaaa.admin --> role:test_package#xxx00.tenant + role:test_domain#xxx00-aaaa.owner --> perm:DELETE:on:test_domain#xxx00-aaaa + role:test_domain#xxx00-aaaa.owner --> role:test_domain#xxx00-aaaa.admin + role:test_package#xxx00.tenant --> perm:SELECT:on:test_package#xxx00 + role:test_package#xxx00.tenant --> role:test_customer#xxx.tenant """.trim()); } @Test -// @Disabled + @Disabled // enable to generate from a real database void print() throws IOException { //context("superuser-alex@hostsharing.net", "hs_office_person#FirbySusan.admin"); context("superuser-alex@hostsharing.net"); diff --git a/src/test/java/net/hostsharing/hsadminng/test/cust/TestCustomerEntityTest.java b/src/test/java/net/hostsharing/hsadminng/test/cust/TestCustomerEntityUnitTest.java similarity index 98% rename from src/test/java/net/hostsharing/hsadminng/test/cust/TestCustomerEntityTest.java rename to src/test/java/net/hostsharing/hsadminng/test/cust/TestCustomerEntityUnitTest.java index 7094530e..4dfcc183 100644 --- a/src/test/java/net/hostsharing/hsadminng/test/cust/TestCustomerEntityTest.java +++ b/src/test/java/net/hostsharing/hsadminng/test/cust/TestCustomerEntityUnitTest.java @@ -5,7 +5,7 @@ import org.junit.jupiter.api.Test; import static org.assertj.core.api.Assertions.assertThat; -class TestCustomerEntityTest { +class TestCustomerEntityUnitTest { @Test void definesRbac() { diff --git a/src/test/java/net/hostsharing/hsadminng/test/pac/TestPackageEntityTest.java b/src/test/java/net/hostsharing/hsadminng/test/pac/TestPackageEntityUnitTest.java similarity index 98% rename from src/test/java/net/hostsharing/hsadminng/test/pac/TestPackageEntityTest.java rename to src/test/java/net/hostsharing/hsadminng/test/pac/TestPackageEntityUnitTest.java index 392d0274..2546a8e8 100644 --- a/src/test/java/net/hostsharing/hsadminng/test/pac/TestPackageEntityTest.java +++ b/src/test/java/net/hostsharing/hsadminng/test/pac/TestPackageEntityUnitTest.java @@ -5,7 +5,7 @@ import org.junit.jupiter.api.Test; import static org.assertj.core.api.Assertions.assertThat; -class TestPackageEntityTest { +class TestPackageEntityUnitTest { @Test void definesRbac() {