fixes regarding code-review

src/main/java/net/hostsharing/hsadminng/rbac/rbacdef/InsertTriggerGenerator.java

@@ -161,23 +161,28 @@ public class InsertTriggerGenerator {
                     Checks if the user or assumed roles are allowed to insert a row to ${rawSubTable},
                     where the check is performed by an indirect role.
                     
-                    An indirect role is a role FIXME.
+                    An indirect role is a role which depends on an object uuid which is not a direct foreign key
+                    of the source entity, but needs to be fetched via joined tables.
                 */
                 create or replace function ${rawSubTable}_insert_permission_check_tf()
                     returns trigger
                     language plpgsql as $$
+
                 declare
                     superRoleObjectUuid uuid;
-                begin
 
+                begin
                 """,
                 with("rawSubTable", rbacDef.getRootEntityAlias().getRawTableName()));
+        plPgSql.chopEmptyLines();
         plPgSql.indented(2, () ->  {
             plPgSql.writeLn(
-                    "superRoleObjectUuid := (" + g.getSuperRoleDef().getEntityAlias().fetchSql().sql + ");",
+                    "superRoleObjectUuid := (" + g.getSuperRoleDef().getEntityAlias().fetchSql().sql + ");\n" +
+                    "assert superRoleObjectUuid is not null, 'superRoleObjectUuid must not be null';",
                     with("columns", g.getSuperRoleDef().getEntityAlias().aliasName() + ".uuid"),
                     with("ref", NEW.name()));
         });
+        plPgSql.writeLn();
         plPgSql.writeLn("""
                         if ( not hasInsertPermission(superRoleObjectUuid, 'INSERT', '${rawSubTable}') ) then
                             raise exception

src/main/java/net/hostsharing/hsadminng/rbac/rbacdef/RbacIdentityViewGenerator.java

@@ -26,18 +26,20 @@ public class RbacIdentityViewGenerator {
         plPgSql.writeLn(
             switch (rbacDef.getIdentityViewSqlQuery().part) {
             case SQL_PROJECTION -> """
-                    call generateRbacIdentityViewFromProjection('${rawTableName}', $idName$
+                    call generateRbacIdentityViewFromProjection('${rawTableName}', 
+                        $idName$
                     ${identityViewSqlPart}
                         $idName$);
                     """;
             case SQL_QUERY -> """
-                    call generateRbacIdentityViewFromQuery('${rawTableName}', $idName$
+                    call generateRbacIdentityViewFromQuery('${rawTableName}', 
+                        $idName$
                     ${identityViewSqlPart}
                         $idName$);
                 """;
             default -> throw new IllegalStateException("illegal SQL part given");
             },
-            with("identityViewSqlPart", rbacDef.getIdentityViewSqlQuery().sql),
+            with("identityViewSqlPart", StringWriter.indented(2, rbacDef.getIdentityViewSqlQuery().sql)),
             with("rawTableName", rawTableName));
 
         plPgSql.writeLn("--//");

src/main/java/net/hostsharing/hsadminng/rbac/rbacdef/RbacRestrictedViewGenerator.java

@@ -32,10 +32,10 @@ public class RbacRestrictedViewGenerator {
 
                 """,
                 with("liquibaseTagPrefix", liquibaseTagPrefix),
-                with("orderBy", indented(rbacDef.getOrderBySqlExpression().sql, 2)),
-                with("updates", indented(rbacDef.getUpdatableColumns().stream()
+                with("orderBy", indented(2, rbacDef.getOrderBySqlExpression().sql)),
+                with("updates", indented(2, rbacDef.getUpdatableColumns().stream()
                         .map(c -> c + " = new." + c)
-                        .collect(joining(",\n")), 2)),
+                        .collect(joining(",\n")))),
                 with("rawTableName", rawTableName));
     }
 }

src/main/java/net/hostsharing/hsadminng/rbac/rbacdef/RolesGrantsAndPermissionsGenerator.java

@@ -7,6 +7,7 @@ import java.util.List;
 import java.util.Set;
 import java.util.stream.Stream;
 
+import static java.util.Optional.ofNullable;
 import static java.util.stream.Collectors.joining;
 import static java.util.stream.Collectors.toSet;
 import static net.hostsharing.hsadminng.rbac.rbacdef.PostgresTriggerReference.NEW;
@@ -233,6 +234,7 @@ class RolesGrantsAndPermissionsGenerator {
             final PostgresTriggerReference old) {
         plPgSql.writeLn(
                 ea.fetchSql().sql + "    INTO " + entityRefVar(old, ea) + ";",
+                with("columns", ea.aliasName() + ".*"),
                 with("ref", old.name()));
         if (ea.nullable() == RbacView.Nullable.NOT_NULL) {
             plPgSql.writeLn(
@@ -244,14 +246,11 @@ class RolesGrantsAndPermissionsGenerator {
         }
     }
 
-    private boolean isUpdatable(final RbacView.Column c) {
-        return rbacDef.getUpdatableColumns().contains(c);
-    }
-
     private void updateGrantsDependingOn(final StringWriter plPgSql, final String columnName) {
         rbacDef.getGrantDefs().stream()
                 .filter(RbacView.RbacGrantDefinition::isToCreate)
                 .filter(g -> g.dependsOnColumn(columnName))
+                .filter(g -> !isInsertPermissionGrant(g))
                 .forEach(g -> {
                     plPgSql.ensureSingleEmptyLine();
                     plPgSql.writeLn(generateRevoke(g));
@@ -260,6 +259,11 @@ class RolesGrantsAndPermissionsGenerator {
                 });
     }
 
+    private static Boolean isInsertPermissionGrant(final RbacView.RbacGrantDefinition g) {
+        final var isInsertPermissionGrant = ofNullable(g.getPermDef()).map(RbacPermissionDefinition::getPermission).map(p -> p == INSERT).orElse(false);
+        return isInsertPermissionGrant;
+    }
+
     private void generateGrants(final StringWriter plPgSql, final RbacView.RbacGrantDefinition.GrantType grantType) {
         plPgSql.ensureSingleEmptyLine();
         rbacGrants.stream()
@@ -406,7 +410,7 @@ class RolesGrantsAndPermissionsGenerator {
         if (!incomingGrants.isEmpty()) {
             final var arrayElements = incomingGrants.stream()
                     .map(g -> toPlPgSqlReference(NEW, g.getSuperRoleDef(), g.isAssumed()))
-                    .toList();
+                    .sorted().toList();
             plPgSql.indented(() ->
                     plPgSql.writeLn("incomingSuperRoles => array[" + joinArrayElements(arrayElements, 1) + "],\n"));
             rbacGrants.removeAll(incomingGrants);
@@ -418,7 +422,7 @@ class RolesGrantsAndPermissionsGenerator {
         if (!outgoingGrants.isEmpty()) {
             final var arrayElements = outgoingGrants.stream()
                     .map(g -> toPlPgSqlReference(NEW, g.getSubRoleDef(), g.isAssumed()))
-                    .toList();
+                    .sorted().toList();
             plPgSql.indented(() ->
                     plPgSql.writeLn("outgoingSubRoles => array[" + joinArrayElements(arrayElements, 1) + "],\n"));
             rbacGrants.removeAll(outgoingGrants);

src/main/java/net/hostsharing/hsadminng/rbac/rbacdef/StringWriter.java

@@ -82,7 +82,7 @@ public class StringWriter {
         return string.toString();
     }
 
-    public static String indented(final String text, final int indentLevel) {
+    public static String indented(final int indentLevel, final String text) {
         final var indentation = StringUtils.repeat("    ", indentLevel);
         final var indented = stream(text.split("\n"))
                 .map(line -> line.trim().isBlank() ? "" : indentation + line)
@@ -94,7 +94,7 @@ public class StringWriter {
         if ( indentLevel == 0) {
             return text;
         }
-        return indented(text, indentLevel);
+        return indented(indentLevel, text);
     }
 
     record VarDef(String name, String value){}

src/main/java/net/hostsharing/hsadminng/rbac/rbacgrant/RbacGrantsDiagramService.java

@@ -71,10 +71,6 @@ public class RbacGrantsDiagramService {
     private void traverseGrantsTo(final Set<RawRbacGrantEntity> graph, final UUID refUuid, final EnumSet<Include> includes) {
         final var grants = rawGrantRepo.findByAscendingUuid(refUuid);
         grants.forEach(g -> {
-            if ( g.getDescendantIdName() == null ) {
-                // FIXME: what's that?
-                return;
-            }
             if (!includes.contains(PERMISSIONS) && g.getDescendantIdName().startsWith("perm ")) {
                 return;
             }

src/main/resources/db/changelog/050-rbac-base.sql

@@ -691,7 +691,7 @@ declare
     superRoleId uuid;
     subRoleId uuid;
 begin
-    -- FIXME: maybe separate method grantRoleToRoleIfNotNull(...)?
+    -- TODO: maybe separate method grantRoleToRoleIfNotNull(...) for NULLABLE references
     if superRole.objectUuid is null or subRole.objectuuid is null then
         return;
     end if;

src/main/resources/db/changelog/057-rbac-role-builder.sql

@@ -60,7 +60,7 @@ begin
     if cardinality(userUuids) > 0 then
         -- direct grants to users need a grantedByRole which can revoke the grant
         if grantedByRole is null then
-            userGrantsByRoleUuid := roleUuid; -- FIXME: or do we want to require an explicit userGrantsByRoleUuid?
+            userGrantsByRoleUuid := roleUuid; -- TODO: or do we want to require an explicit userGrantsByRoleUuid?
         else
             userGrantsByRoleUuid := getRoleId(grantedByRole);
         end if;

src/main/resources/db/changelog/113-test-customer-rbac.sql

@@ -143,7 +143,8 @@ create trigger test_customer_insert_permission_check_tg
 --changeset test-customer-rbac-IDENTITY-VIEW:1 endDelimiter:--//
 -- ----------------------------------------------------------------------------
 
-call generateRbacIdentityViewFromProjection('test_customer', $idName$
+call generateRbacIdentityViewFromProjection('test_customer',
+    $idName$
         prefix
     $idName$);
 --//

src/main/resources/db/changelog/123-test-package-rbac.sql

@@ -110,8 +110,6 @@ begin
 
     if NEW.customerUuid <> OLD.customerUuid then
 
-        call revokePermissionFromRole(getPermissionId(OLD.uuid, 'INSERT'), testCustomerAdmin(oldCustomer));
-
         call revokeRoleFromRole(testPackageOwner(OLD), testCustomerAdmin(oldCustomer));
         call grantRoleToRole(testPackageOwner(NEW), testCustomerAdmin(newCustomer));
 
@@ -212,7 +210,8 @@ create trigger test_package_insert_permission_check_tg
 --changeset test-package-rbac-IDENTITY-VIEW:1 endDelimiter:--//
 -- ----------------------------------------------------------------------------
 
-call generateRbacIdentityViewFromProjection('test_package', $idName$
+call generateRbacIdentityViewFromProjection('test_package',
+    $idName$
         name
     $idName$);
 --//

src/main/resources/db/changelog/133-test-domain-rbac.sql

@@ -106,8 +106,6 @@ begin
 
     if NEW.packageUuid <> OLD.packageUuid then
 
-        call revokePermissionFromRole(getPermissionId(OLD.uuid, 'INSERT'), testPackageAdmin(oldPackage));
-
         call revokeRoleFromRole(testDomainOwner(OLD), testPackageAdmin(oldPackage));
         call grantRoleToRole(testDomainOwner(NEW), testPackageAdmin(newPackage));
 
@@ -211,7 +209,8 @@ create trigger test_domain_insert_permission_check_tg
 --changeset test-domain-rbac-IDENTITY-VIEW:1 endDelimiter:--//
 -- ----------------------------------------------------------------------------
 
-call generateRbacIdentityViewFromProjection('test_domain', $idName$
+call generateRbacIdentityViewFromProjection('test_domain',
+    $idName$
         name
     $idName$);
 --//

src/main/resources/db/changelog/203-hs-office-contact-rbac-generated.sql

@@ -103,7 +103,8 @@ create trigger hs_office_contact_insert_permission_check_tg
 --changeset hs-office-contact-rbac-IDENTITY-VIEW:1 endDelimiter:--//
 -- ----------------------------------------------------------------------------
 
-call generateRbacIdentityViewFromProjection('hs_office_contact', $idName$
+call generateRbacIdentityViewFromProjection('hs_office_contact',
+    $idName$
         label
     $idName$);
 --//

src/main/resources/db/changelog/213-hs-office-person-rbac-generated.sql

@@ -103,7 +103,8 @@ create trigger hs_office_person_insert_permission_check_tg
 --changeset hs-office-person-rbac-IDENTITY-VIEW:1 endDelimiter:--//
 -- ----------------------------------------------------------------------------
 
-call generateRbacIdentityViewFromProjection('hs_office_person', $idName$
+call generateRbacIdentityViewFromProjection('hs_office_person',
+    $idName$
         concat(tradeName, familyName, givenName)
     $idName$);
 --//

src/main/resources/db/changelog/223-hs-office-relation-rbac-generated.sql

@@ -54,15 +54,15 @@ begin
         hsOfficeRelationAdmin(NEW),
             permissions => array['UPDATE'],
             incomingSuperRoles => array[
-            	hsOfficeRelationOwner(NEW),
-            	hsOfficePersonAdmin(newAnchorPerson)]
+            	hsOfficePersonAdmin(newAnchorPerson),
+            	hsOfficeRelationOwner(NEW)]
     );
 
     perform createRoleWithGrants(
         hsOfficeRelationAgent(NEW),
             incomingSuperRoles => array[
-            	hsOfficeRelationAdmin(NEW),
-            	hsOfficePersonAdmin(newHolderPerson)]
+            	hsOfficePersonAdmin(newHolderPerson),
+            	hsOfficeRelationAdmin(NEW)]
     );
 
     perform createRoleWithGrants(
@@ -73,9 +73,9 @@ begin
             	hsOfficePersonAdmin(newHolderPerson),
             	hsOfficeRelationAgent(NEW)],
             outgoingSubRoles => array[
-            	hsOfficePersonReferrer(newHolderPerson),
             	hsOfficeContactReferrer(newContact),
-            	hsOfficePersonReferrer(newAnchorPerson)]
+            	hsOfficePersonReferrer(newAnchorPerson),
+            	hsOfficePersonReferrer(newHolderPerson)]
     );
 
     call leaveTriggerForObjectUuid(NEW.uuid);
@@ -169,11 +169,11 @@ create trigger hs_office_relation_insert_permission_check_tg
 --changeset hs-office-relation-rbac-IDENTITY-VIEW:1 endDelimiter:--//
 -- ----------------------------------------------------------------------------
 
-call generateRbacIdentityViewFromProjection('hs_office_relation', $idName$
+call generateRbacIdentityViewFromProjection('hs_office_relation',
+    $idName$
              (select idName from hs_office_person_iv p where p.uuid = anchorUuid)
              || '-with-' || target.type || '-'
              || (select idName from hs_office_person_iv p where p.uuid = holderUuid)
-
     $idName$);
 --//
 

src/main/resources/db/changelog/233-hs-office-partner-rbac-generated.sql

@@ -220,12 +220,12 @@ create trigger hs_office_partner_insert_permission_check_tg
 --changeset hs-office-partner-rbac-IDENTITY-VIEW:1 endDelimiter:--//
 -- ----------------------------------------------------------------------------
 
-    call generateRbacIdentityViewFromQuery('hs_office_partner', $idName$
+    call generateRbacIdentityViewFromQuery('hs_office_partner',
+        $idName$
             SELECT partner.partnerNumber
             || ':' || (SELECT idName FROM hs_office_person_iv p WHERE p.uuid = partner.personUuid)
             || '-' || (SELECT idName FROM hs_office_contact_iv c WHERE c.uuid = partner.contactUuid)
             FROM hs_office_partner AS partner
-
         $idName$);
 --//
 

src/main/resources/db/changelog/234-hs-office-partner-details-rbac-generated.sql

@@ -35,7 +35,7 @@ declare
 begin
     call enterTriggerForObjectUuid(NEW.uuid);
 
-    SELECT ${columns}
+    SELECT partnerRel.*
         FROM hs_office_relation AS partnerRel
         JOIN hs_office_partner AS partner
             ON partner.detailsUuid = NEW.uuid
@@ -133,12 +133,12 @@ create trigger hs_office_partner_details_insert_permission_check_tg
 --changeset hs-office-partner-details-rbac-IDENTITY-VIEW:1 endDelimiter:--//
 -- ----------------------------------------------------------------------------
 
-    call generateRbacIdentityViewFromQuery('hs_office_partner_details', $idName$
+    call generateRbacIdentityViewFromQuery('hs_office_partner_details',
+        $idName$
             SELECT partner_iv.idName || '-details'
             FROM hs_office_partner_details AS partnerDetails
             JOIN hs_office_partner partner ON partner.detailsUuid = partnerDetails.uuid
             JOIN hs_office_partner_iv partner_iv ON partner_iv.uuid = partner.uuid
-
         $idName$);
 --//
 

src/main/resources/db/changelog/243-hs-office-bankaccount-rbac-generated.sql

@@ -103,7 +103,8 @@ create trigger hs_office_bankaccount_insert_permission_check_tg
 --changeset hs-office-bankaccount-rbac-IDENTITY-VIEW:1 endDelimiter:--//
 -- ----------------------------------------------------------------------------
 
-call generateRbacIdentityViewFromProjection('hs_office_bankaccount', $idName$
+call generateRbacIdentityViewFromProjection('hs_office_bankaccount',
+    $idName$
         iban || ':' || holder
     $idName$);
 --//

src/main/resources/db/changelog/253-hs-office-sepamandate-rbac-generated.sql

@@ -57,17 +57,17 @@ begin
         hsOfficeSepaMandateAgent(NEW),
             incomingSuperRoles => array[hsOfficeSepaMandateAdmin(NEW)],
             outgoingSubRoles => array[
-            	hsOfficeRelationAgent(newDebitorRel),
-            	hsOfficeBankAccountReferrer(newBankAccount)]
+            	hsOfficeBankAccountReferrer(newBankAccount),
+            	hsOfficeRelationAgent(newDebitorRel)]
     );
 
     perform createRoleWithGrants(
         hsOfficeSepaMandateReferrer(NEW),
             permissions => array['SELECT'],
             incomingSuperRoles => array[
-            	hsOfficeSepaMandateAgent(NEW),
+            	hsOfficeBankAccountAdmin(newBankAccount),
             	hsOfficeRelationAgent(newDebitorRel),
-            	hsOfficeBankAccountAdmin(newBankAccount)],
+            	hsOfficeSepaMandateAgent(NEW)],
             outgoingSubRoles => array[hsOfficeRelationTenant(newDebitorRel)]
     );
 
@@ -121,7 +121,8 @@ create trigger hs_office_sepamandate_insert_permission_check_tg
 --changeset hs-office-sepamandate-rbac-IDENTITY-VIEW:1 endDelimiter:--//
 -- ----------------------------------------------------------------------------
 
-call generateRbacIdentityViewFromProjection('hs_office_sepamandate', $idName$
+call generateRbacIdentityViewFromProjection('hs_office_sepamandate',
+    $idName$
         concat(tradeName, familyName, givenName)
     $idName$);
 --//

src/main/resources/db/changelog/273-hs-office-debitor-rbac-generated.sql

@@ -185,7 +185,8 @@ create trigger hs_office_debitor_insert_permission_check_tg
 --changeset hs-office-debitor-rbac-IDENTITY-VIEW:1 endDelimiter:--//
 -- ----------------------------------------------------------------------------
 
-    call generateRbacIdentityViewFromQuery('hs_office_debitor', $idName$
+    call generateRbacIdentityViewFromQuery('hs_office_debitor',
+        $idName$
                 SELECT debitor.uuid,
                         'D-' || (SELECT partner.partnerNumber
                                 FROM hs_office_partner partner
@@ -196,7 +197,6 @@ create trigger hs_office_debitor_insert_permission_check_tg
                                 WHERE debitorRel.uuid = debitor.debitorRelUuid)
                              || to_char(debitorNumberSuffix, 'fm00')
                 from hs_office_debitor as debitor
-
         $idName$);
 --//
 

src/test/java/net/hostsharing/hsadminng/hs/office/membership/HsOfficeMembershipControllerAcceptanceTest.java

@@ -462,7 +462,7 @@ class HsOfficeMembershipControllerAcceptanceTest extends ContextBasedTestWithCle
             RestAssured // @formatter:off
                 .given()
                     .header("current-user", "superuser-alex@hostsharing.net")
-                    .header("assumed-roles", "hs_office_partner#FirstGmbH-firstcontact.agent")
+                    .header("assumed-roles", "hs_office_partner#10001:FirstGmbH-firstcontact.admin")
                     .port(port)
                 .when()
                     .delete("http://localhost/api/hs/office/memberships/" + givenMembership.getUuid())