From 9fdeb047ee1774cae6fea0acba2117cb723f14ef Mon Sep 17 00:00:00 2001 From: Michael Hoennig Date: Mon, 25 Mar 2024 11:46:39 +0100 Subject: [PATCH] avoid nested subselect for insert permission check --- .../partner/HsOfficePartnerDetailsEntity.java | 2 +- .../rbac/rbacdef/InsertTriggerGenerator.java | 24 +++++++++---------- .../hsadminng/rbac/rbacdef/RbacView.java | 24 ++++++++++++++----- .../db/changelog/123-test-package-rbac.sql | 18 ++++---------- .../db/changelog/133-test-domain-rbac.sql | 18 ++++---------- .../223-hs-office-relation-rbac-generated.sql | 12 +++++----- ...-office-partner-details-rbac-generated.sql | 2 +- ...3-hs-office-sepamandate-rbac-generated.sql | 8 +++---- 8 files changed, 51 insertions(+), 57 deletions(-) diff --git a/src/main/java/net/hostsharing/hsadminng/hs/office/partner/HsOfficePartnerDetailsEntity.java b/src/main/java/net/hostsharing/hsadminng/hs/office/partner/HsOfficePartnerDetailsEntity.java index 31ff56ab..acf39249 100644 --- a/src/main/java/net/hostsharing/hsadminng/hs/office/partner/HsOfficePartnerDetailsEntity.java +++ b/src/main/java/net/hostsharing/hsadminng/hs/office/partner/HsOfficePartnerDetailsEntity.java @@ -88,7 +88,7 @@ public class HsOfficePartnerDetailsEntity implements HasUuid, Stringifyable { .importRootEntityAliasProxy("partnerRel", HsOfficeRelationEntity.class, fetchedBySql(""" - SELECT partnerRel.* + SELECT ${columns} FROM hs_office_relation AS partnerRel JOIN hs_office_partner AS partner ON partner.detailsUuid = ${ref}.uuid diff --git a/src/main/java/net/hostsharing/hsadminng/rbac/rbacdef/InsertTriggerGenerator.java b/src/main/java/net/hostsharing/hsadminng/rbac/rbacdef/InsertTriggerGenerator.java index 000988fa..85810d1a 100644 --- a/src/main/java/net/hostsharing/hsadminng/rbac/rbacdef/InsertTriggerGenerator.java +++ b/src/main/java/net/hostsharing/hsadminng/rbac/rbacdef/InsertTriggerGenerator.java @@ -114,8 +114,7 @@ public class InsertTriggerGenerator { } } } else { - final var superRoleEntityAlias = g.getSuperRoleDef().getEntityAlias(); - if (superRoleEntityAlias.fetchSql().part == RbacView.SQL.Part.AUTO_FETCH) { + if (g.getSuperRoleDef().getEntityAlias().isFetchedByDirectForeignKey()) { generateInsertPermissionTriggerAllowByRoleOfDirectForeignKey(plPgSql, g); } else { generateInsertPermissionTriggerAllowByRoleOfIndirectForeignKey(plPgSql, g); @@ -164,24 +163,23 @@ public class InsertTriggerGenerator { An indirect role is a role FIXME. */ - create or replace function ${rawSubTable}_insert_permission_missing_tf() + create or replace function ${rawSubTable}_insert_permission_check_tf() returns trigger language plpgsql as $$ + declare + superRoleObjectUuid uuid; begin - if ( not hasInsertPermission( - ( SELECT ${varName}.uuid FROM + """, - with("rawSubTable", rbacDef.getRootEntityAlias().getRawTableName()), - with("varName", g.getSuperRoleDef().getEntityAlias().aliasName())); - plPgSql.indented(3, () -> { + with("rawSubTable", rbacDef.getRootEntityAlias().getRawTableName())); + plPgSql.indented(2, () -> { plPgSql.writeLn( - "(" + g.getSuperRoleDef().getEntityAlias().fetchSql().sql + ") AS ${varName}", - with("varName", g.getSuperRoleDef().getEntityAlias().aliasName()), + "superRoleObjectUuid := (" + g.getSuperRoleDef().getEntityAlias().fetchSql().sql + ");", + with("columns", g.getSuperRoleDef().getEntityAlias().aliasName() + ".uuid"), with("ref", NEW.name())); }); plPgSql.writeLn(""" - - ), 'INSERT', '${rawSubTable}') ) then + if ( not hasInsertPermission(superRoleObjectUuid, 'INSERT', '${rawSubTable}') ) then raise exception '[403] insert into ${rawSubTable} not allowed for current subjects % (%)', currentSubjects(), currentSubjectsUuids(); @@ -192,7 +190,7 @@ public class InsertTriggerGenerator { create trigger ${rawSubTable}_insert_permission_check_tg before insert on ${rawSubTable} for each row - execute procedure ${rawSubTable}_insert_permission_missing_tf(); + execute procedure ${rawSubTable}_insert_permission_check_tf(); """, with("rawSubTable", rbacDef.getRootEntityAlias().getRawTableName())); diff --git a/src/main/java/net/hostsharing/hsadminng/rbac/rbacdef/RbacView.java b/src/main/java/net/hostsharing/hsadminng/rbac/rbacdef/RbacView.java index 99acade9..d6fe2ab3 100644 --- a/src/main/java/net/hostsharing/hsadminng/rbac/rbacdef/RbacView.java +++ b/src/main/java/net/hostsharing/hsadminng/rbac/rbacdef/RbacView.java @@ -34,6 +34,7 @@ import static java.util.Arrays.stream; import static java.util.Optional.ofNullable; import static net.hostsharing.hsadminng.rbac.rbacdef.RbacView.Nullable.NOT_NULL; import static net.hostsharing.hsadminng.rbac.rbacdef.RbacView.RbacUserReference.UserRole.CREATOR; +import static net.hostsharing.hsadminng.rbac.rbacdef.RbacView.SQL.Part.AUTO_FETCH; import static net.hostsharing.hsadminng.rbac.rbacdef.RbacView.SQL.directlyFetchedByDependsOnColumn; import static org.apache.commons.lang3.StringUtils.uncapitalize; @@ -839,8 +840,8 @@ public class RbacView { }; } - public boolean hasFetchSql() { - return fetchSql != null; + boolean isFetchedByDirectForeignKey() { + return fetchSql != null && fetchSql.part == AUTO_FETCH; } private String withoutEntitySuffix(final String simpleEntityName) { @@ -909,14 +910,25 @@ public class RbacView { /** * DSL method to specify an SQL SELECT expression which fetches the related entity, - * using the reference `${ref}` of the root entity. - * `${ref}` is going to be replaced by either `NEW` or `OLD` of the trigger function. - * `into ...` will be added with a variable name prefixed with either `new` or `old`. + * using the reference `${ref}` of the root entity and `${columns}` for the projection. + * + *

The query must define the entity alias name of the fetched table + * as its alias for, so it can be used in the generated projection (the columns between + * `SELECT` and `FROM`.

+ * + *

`${ref}` is going to be replaced by either `NEW` or `OLD` of the trigger function. + * `into ...` will be added with a variable name prefixed with either `new` or `old`.

+ * + *

`${columns}` is going to be replaced by the columns which are needed for the query, + * e.g. `*` or `uuid`.

* * @param sql an SQL SELECT expression (not ending with ';) * @return the wrapped SQL expression */ public static SQL fetchedBySql(final String sql) { + if ( !sql.startsWith("SELECT ${columns}") ) { + throw new IllegalArgumentException("SQL SELECT expression must start with 'SELECT ${columns}', but is: " + sql); + } validateExpression(sql); return new SQL(sql, Part.SQL_QUERY); } @@ -929,7 +941,7 @@ public class RbacView { * @return the wrapped SQL definition object */ public static SQL directlyFetchedByDependsOnColumn() { - return new SQL(null, Part.AUTO_FETCH); + return new SQL(null, AUTO_FETCH); } /** diff --git a/src/main/resources/db/changelog/123-test-package-rbac.sql b/src/main/resources/db/changelog/123-test-package-rbac.sql index dc4d042f..dc0cb457 100644 --- a/src/main/resources/db/changelog/123-test-package-rbac.sql +++ b/src/main/resources/db/changelog/123-test-package-rbac.sql @@ -189,30 +189,22 @@ execute procedure test_package_test_customer_insert_tf(); /** Checks if the user or assumed roles are allowed to insert a row to test_package, - where the check is performed by an indirect role. + where the check is performed by a direct role. - An indirect role is a role FIXME. + A direct role is a role depending on a foreign key directly available in the NEW row. */ create or replace function test_package_insert_permission_missing_tf() returns trigger language plpgsql as $$ begin - if ( not hasInsertPermission( - ( SELECT customer.uuid FROM - - (SELECT * FROM test_customer WHERE uuid = NEW.customerUuid) AS customer - - ), 'INSERT', 'test_package') ) then - raise exception - '[403] insert into test_package not allowed for current subjects % (%)', - currentSubjects(), currentSubjectsUuids(); - end if; - return NEW; + raise exception '[403] insert into test_package not allowed for current subjects % (%)', + currentSubjects(), currentSubjectsUuids(); end; $$; create trigger test_package_insert_permission_check_tg before insert on test_package for each row + when ( not hasInsertPermission(NEW.customerUuid, 'INSERT', 'test_package') ) execute procedure test_package_insert_permission_missing_tf(); --// diff --git a/src/main/resources/db/changelog/133-test-domain-rbac.sql b/src/main/resources/db/changelog/133-test-domain-rbac.sql index e87adb9c..bb8c515d 100644 --- a/src/main/resources/db/changelog/133-test-domain-rbac.sql +++ b/src/main/resources/db/changelog/133-test-domain-rbac.sql @@ -188,30 +188,22 @@ execute procedure test_domain_test_package_insert_tf(); /** Checks if the user or assumed roles are allowed to insert a row to test_domain, - where the check is performed by an indirect role. + where the check is performed by a direct role. - An indirect role is a role FIXME. + A direct role is a role depending on a foreign key directly available in the NEW row. */ create or replace function test_domain_insert_permission_missing_tf() returns trigger language plpgsql as $$ begin - if ( not hasInsertPermission( - ( SELECT package.uuid FROM - - (SELECT * FROM test_package WHERE uuid = NEW.packageUuid) AS package - - ), 'INSERT', 'test_domain') ) then - raise exception - '[403] insert into test_domain not allowed for current subjects % (%)', - currentSubjects(), currentSubjectsUuids(); - end if; - return NEW; + raise exception '[403] insert into test_domain not allowed for current subjects % (%)', + currentSubjects(), currentSubjectsUuids(); end; $$; create trigger test_domain_insert_permission_check_tg before insert on test_domain for each row + when ( not hasInsertPermission(NEW.packageUuid, 'INSERT', 'test_domain') ) execute procedure test_domain_insert_permission_missing_tf(); --// diff --git a/src/main/resources/db/changelog/223-hs-office-relation-rbac-generated.sql b/src/main/resources/db/changelog/223-hs-office-relation-rbac-generated.sql index 1b72bc0a..766722be 100644 --- a/src/main/resources/db/changelog/223-hs-office-relation-rbac-generated.sql +++ b/src/main/resources/db/changelog/223-hs-office-relation-rbac-generated.sql @@ -54,8 +54,8 @@ begin hsOfficeRelationAdmin(NEW), permissions => array['UPDATE'], incomingSuperRoles => array[ - hsOfficePersonAdmin(newAnchorPerson), - hsOfficeRelationOwner(NEW)] + hsOfficeRelationOwner(NEW), + hsOfficePersonAdmin(newAnchorPerson)] ); perform createRoleWithGrants( @@ -69,13 +69,13 @@ begin hsOfficeRelationTenant(NEW), permissions => array['SELECT'], incomingSuperRoles => array[ - hsOfficeRelationAgent(NEW), hsOfficeContactAdmin(newContact), - hsOfficePersonAdmin(newHolderPerson)], + hsOfficePersonAdmin(newHolderPerson), + hsOfficeRelationAgent(NEW)], outgoingSubRoles => array[ - hsOfficePersonReferrer(newAnchorPerson), + hsOfficePersonReferrer(newHolderPerson), hsOfficeContactReferrer(newContact), - hsOfficePersonReferrer(newHolderPerson)] + hsOfficePersonReferrer(newAnchorPerson)] ); call leaveTriggerForObjectUuid(NEW.uuid); diff --git a/src/main/resources/db/changelog/234-hs-office-partner-details-rbac-generated.sql b/src/main/resources/db/changelog/234-hs-office-partner-details-rbac-generated.sql index 04fc7be7..e3846a95 100644 --- a/src/main/resources/db/changelog/234-hs-office-partner-details-rbac-generated.sql +++ b/src/main/resources/db/changelog/234-hs-office-partner-details-rbac-generated.sql @@ -35,7 +35,7 @@ declare begin call enterTriggerForObjectUuid(NEW.uuid); - SELECT partnerRel.* + SELECT ${columns} FROM hs_office_relation AS partnerRel JOIN hs_office_partner AS partner ON partner.detailsUuid = NEW.uuid diff --git a/src/main/resources/db/changelog/253-hs-office-sepamandate-rbac-generated.sql b/src/main/resources/db/changelog/253-hs-office-sepamandate-rbac-generated.sql index 5edf9454..ac4e9fee 100644 --- a/src/main/resources/db/changelog/253-hs-office-sepamandate-rbac-generated.sql +++ b/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[ - hsOfficeBankAccountReferrer(newBankAccount), - hsOfficeRelationAgent(newDebitorRel)] + hsOfficeRelationAgent(newDebitorRel), + hsOfficeBankAccountReferrer(newBankAccount)] ); perform createRoleWithGrants( hsOfficeSepaMandateReferrer(NEW), permissions => array['SELECT'], incomingSuperRoles => array[ + hsOfficeSepaMandateAgent(NEW), hsOfficeRelationAgent(newDebitorRel), - hsOfficeBankAccountAdmin(newBankAccount), - hsOfficeSepaMandateAgent(NEW)], + hsOfficeBankAccountAdmin(newBankAccount)], outgoingSubRoles => array[hsOfficeRelationTenant(newDebitorRel)] );