From eb8781db77672fbb6edbc790197b948086b6fd2e Mon Sep 17 00:00:00 2001 From: Darin Kelkhoff Date: Wed, 2 Oct 2024 16:16:16 -0500 Subject: [PATCH 1/2] CE-1654 - Update joins built for security-purposes, that if they're for an all-access key, to be outer (LEFT); update tests to reflect this --- .../actions/tables/query/JoinsContext.java | 97 +++++++++++-------- .../rdbms/actions/RDBMSCountActionTest.java | 2 +- .../actions/RDBMSQueryActionJoinsTest.java | 65 ++++++++++++- 3 files changed, 122 insertions(+), 42 deletions(-) diff --git a/qqq-backend-core/src/main/java/com/kingsrook/qqq/backend/core/model/actions/tables/query/JoinsContext.java b/qqq-backend-core/src/main/java/com/kingsrook/qqq/backend/core/model/actions/tables/query/JoinsContext.java index 7f2adde6..f1c23ee2 100644 --- a/qqq-backend-core/src/main/java/com/kingsrook/qqq/backend/core/model/actions/tables/query/JoinsContext.java +++ b/qqq-backend-core/src/main/java/com/kingsrook/qqq/backend/core/model/actions/tables/query/JoinsContext.java @@ -82,7 +82,7 @@ public class JoinsContext ///////////////////////////////////////////////////////////////////////////// // we will get a TON of more output if this gets turned up, so be cautious // ///////////////////////////////////////////////////////////////////////////// - private Level logLevel = Level.OFF; + private Level logLevel = Level.OFF; private Level logLevelForFilter = Level.OFF; @@ -404,6 +404,12 @@ public class JoinsContext chainIsInner = false; } + if(hasAllAccessKey(recordSecurityLock)) + { + queryJoin.withType(QueryJoin.Type.LEFT); + chainIsInner = false; + } + addQueryJoin(queryJoin, "forRecordSecurityLock (non-flipped)", "- "); addedQueryJoins.add(queryJoin); tmpTable = instance.getTable(join.getRightTable()); @@ -423,6 +429,12 @@ public class JoinsContext chainIsInner = false; } + if(hasAllAccessKey(recordSecurityLock)) + { + queryJoin.withType(QueryJoin.Type.LEFT); + chainIsInner = false; + } + addQueryJoin(queryJoin, "forRecordSecurityLock (flipped)", "- "); addedQueryJoins.add(queryJoin); tmpTable = instance.getTable(join.getLeftTable()); @@ -456,44 +468,53 @@ public class JoinsContext + /*************************************************************************** + ** + ***************************************************************************/ + private boolean hasAllAccessKey(RecordSecurityLock recordSecurityLock) + { + ////////////////////////////////////////////////////////////////////////////////////////////////////////////// + // check if the key type has an all-access key, and if so, if it's set to true for the current user/session // + ////////////////////////////////////////////////////////////////////////////////////////////////////////////// + QSecurityKeyType securityKeyType = instance.getSecurityKeyType(recordSecurityLock.getSecurityKeyType()); + if(StringUtils.hasContent(securityKeyType.getAllAccessKeyName())) + { + QSession session = QContext.getQSession(); + if(session.hasSecurityKeyValue(securityKeyType.getAllAccessKeyName(), true, QFieldType.BOOLEAN)) + { + return (true); + } + } + + return (false); + } + + + /******************************************************************************* ** *******************************************************************************/ private void addSubFilterForRecordSecurityLock(RecordSecurityLock recordSecurityLock, QTableMetaData table, String tableNameOrAlias, boolean isOuter, QueryJoin sourceQueryJoin) { - QSession session = QContext.getQSession(); - - ////////////////////////////////////////////////////////////////////////////////////////////////////////////// - // check if the key type has an all-access key, and if so, if it's set to true for the current user/session // - ////////////////////////////////////////////////////////////////////////////////////////////////////////////// - QSecurityKeyType securityKeyType = instance.getSecurityKeyType(recordSecurityLock.getSecurityKeyType()); - boolean haveAllAccessKey = false; - if(StringUtils.hasContent(securityKeyType.getAllAccessKeyName())) + boolean haveAllAccessKey = hasAllAccessKey(recordSecurityLock); + if(haveAllAccessKey) { - ////////////////////////////////////////////////////////////////////////////////////////////////////////////////// - // if we have all-access on this key, then we don't need a criterion for it (as long as we're in an AND filter) // - ////////////////////////////////////////////////////////////////////////////////////////////////////////////////// - if(session.hasSecurityKeyValue(securityKeyType.getAllAccessKeyName(), true, QFieldType.BOOLEAN)) + if(sourceQueryJoin != null) { - haveAllAccessKey = true; + //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// + // in case the queryJoin object is re-used between queries, and its security criteria need to be different (!!), reset it // + // this can be exposed in tests - maybe not entirely expected in real-world, but seems safe enough // + //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// + sourceQueryJoin.withSecurityCriteria(new ArrayList<>()); + } - if(sourceQueryJoin != null) - { - //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// - // in case the queryJoin object is re-used between queries, and its security criteria need to be different (!!), reset it // - // this can be exposed in tests - maybe not entirely expected in real-world, but seems safe enough // - //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// - sourceQueryJoin.withSecurityCriteria(new ArrayList<>()); - } - - //////////////////////////////////////////////////////////////////////////////////////// - // if we're in an AND filter, then we don't need a criteria for this lock, so return. // - //////////////////////////////////////////////////////////////////////////////////////// - boolean inAnAndFilter = securityFilterCursor.getBooleanOperator() == QQueryFilter.BooleanOperator.AND; - if(inAnAndFilter) - { - return; - } + //////////////////////////////////////////////////////////////////////////////////////// + // if we're in an AND filter, then we don't need a criteria for this lock, so return. // + //////////////////////////////////////////////////////////////////////////////////////// + boolean inAnAndFilter = securityFilterCursor.getBooleanOperator() == QQueryFilter.BooleanOperator.AND; + if(inAnAndFilter) + { + return; } } @@ -545,7 +566,7 @@ public class JoinsContext } else { - List securityKeyValues = session.getSecurityKeyValues(recordSecurityLock.getSecurityKeyType(), type); + List securityKeyValues = QContext.getQSession().getSecurityKeyValues(recordSecurityLock.getSecurityKeyType(), type); if(CollectionUtils.nullSafeIsEmpty(securityKeyValues)) { /////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// @@ -1193,15 +1214,15 @@ public class JoinsContext if(isStart) { - System.out.println("\n" + StringUtils.safeTruncate("--- Start [main table: " + this.mainTableName + "] " + "-".repeat(full), full)); + System.out.println("\n" + StringUtils.safeTruncate("--- Start [main table: " + this.mainTableName + "] " + "-" .repeat(full), full)); } StringBuilder rs = new StringBuilder(); String formatString = "| %-" + md + "s | %-" + md + "s %-" + md + "s | %-" + lg + "s | %-" + sm + "s |\n"; rs.append(String.format(formatString, "Base Table", "Join Table", "(Alias)", "Join Meta Data", "Type")); - String dashesLg = "-".repeat(lg); - String dashesMd = "-".repeat(md); - String dashesSm = "-".repeat(sm); + String dashesLg = "-" .repeat(lg); + String dashesMd = "-" .repeat(md); + String dashesSm = "-" .repeat(sm); rs.append(String.format(formatString, dashesMd, dashesMd, dashesMd, dashesLg, dashesSm)); if(CollectionUtils.nullSafeHasContents(queryJoins)) { @@ -1227,11 +1248,11 @@ public class JoinsContext if(isEnd) { - System.out.println(StringUtils.safeTruncate("--- End " + "-".repeat(full), full) + "\n"); + System.out.println(StringUtils.safeTruncate("--- End " + "-" .repeat(full), full) + "\n"); } else { - System.out.println(StringUtils.safeTruncate("-".repeat(full), full)); + System.out.println(StringUtils.safeTruncate("-" .repeat(full), full)); } } } diff --git a/qqq-backend-module-rdbms/src/test/java/com/kingsrook/qqq/backend/module/rdbms/actions/RDBMSCountActionTest.java b/qqq-backend-module-rdbms/src/test/java/com/kingsrook/qqq/backend/module/rdbms/actions/RDBMSCountActionTest.java index 29247bc7..405c97c8 100644 --- a/qqq-backend-module-rdbms/src/test/java/com/kingsrook/qqq/backend/module/rdbms/actions/RDBMSCountActionTest.java +++ b/qqq-backend-module-rdbms/src/test/java/com/kingsrook/qqq/backend/module/rdbms/actions/RDBMSCountActionTest.java @@ -212,7 +212,7 @@ public class RDBMSCountActionTest extends RDBMSActionTest CountInput countInput = new CountInput(); countInput.setTableName(TestUtils.TABLE_NAME_WAREHOUSE); - assertThat(new CountAction().execute(countInput).getCount()).isEqualTo(1); + assertThat(new CountAction().execute(countInput).getCount()).isEqualTo(4); } } diff --git a/qqq-backend-module-rdbms/src/test/java/com/kingsrook/qqq/backend/module/rdbms/actions/RDBMSQueryActionJoinsTest.java b/qqq-backend-module-rdbms/src/test/java/com/kingsrook/qqq/backend/module/rdbms/actions/RDBMSQueryActionJoinsTest.java index ae659653..54e4afcd 100644 --- a/qqq-backend-module-rdbms/src/test/java/com/kingsrook/qqq/backend/module/rdbms/actions/RDBMSQueryActionJoinsTest.java +++ b/qqq-backend-module-rdbms/src/test/java/com/kingsrook/qqq/backend/module/rdbms/actions/RDBMSQueryActionJoinsTest.java @@ -771,6 +771,61 @@ public class RDBMSQueryActionJoinsTest extends RDBMSActionTest + /******************************************************************************* + ** Error seen in CTLive - query for a record in a sub-table, but whose security + ** key comes from a main table, but the main-table record doesn't exist. + ** + ** In this QInstance, our warehouse table's security key comes from + ** storeWarehouseInt.storeId - so if we insert a warehouse, but no stores, we + ** might not be able to find it (if this bug exists!) + *******************************************************************************/ + @Test + void testRequestedJoinWithTableWhoseSecurityFieldIsInMainTableAndNoRowIsInMainTable() throws Exception + { + runTestSql("INSERT INTO warehouse (name) VALUES ('Springfield')", null); + + QueryInput queryInput = new QueryInput(); + queryInput.setTableName(TestUtils.TABLE_NAME_WAREHOUSE); + queryInput.setFilter(new QQueryFilter(new QFilterCriteria("name", QCriteriaOperator.EQUALS, "Springfield"))); + + ///////////////////////////////////////// + // with all access key, should find it // + ///////////////////////////////////////// + QContext.setQSession(new QSession().withSecurityKeyValue(TestUtils.SECURITY_KEY_STORE_ALL_ACCESS, true)); + assertThat(new QueryAction().execute(queryInput).getRecords()).hasSize(1); + + //////////////////////////////////////////// + // with a regular key, should not find it // + //////////////////////////////////////////// + QContext.setQSession(new QSession().withSecurityKeyValue(TestUtils.TABLE_NAME_STORE, 1)); + assertThat(new QueryAction().execute(queryInput).getRecords()).hasSize(0); + + ///////////////////////////////////////// + // now assign the warehouse to a store // + ///////////////////////////////////////// + runTestSql("INSERT INTO warehouse_store_int (store_id, warehouse_id) SELECT 1, id FROM warehouse WHERE name='Springfield'", null); + + ///////////////////////////////////////// + // with all access key, should find it // + ///////////////////////////////////////// + QContext.setQSession(new QSession().withSecurityKeyValue(TestUtils.SECURITY_KEY_STORE_ALL_ACCESS, true)); + assertThat(new QueryAction().execute(queryInput).getRecords()).hasSize(1); + + /////////////////////////////////////////////////////// + // with a regular key, should find it if key matches // + /////////////////////////////////////////////////////// + QContext.setQSession(new QSession().withSecurityKeyValue(TestUtils.TABLE_NAME_STORE, 1)); + assertThat(new QueryAction().execute(queryInput).getRecords()).hasSize(1); + + ////////////////////////////////////////////////////////////////// + // with a regular key, should not find it if key does not match // + ////////////////////////////////////////////////////////////////// + QContext.setQSession(new QSession().withSecurityKeyValue(TestUtils.TABLE_NAME_STORE, 2)); + assertThat(new QueryAction().execute(queryInput).getRecords()).hasSize(0); + + } + + /******************************************************************************* ** *******************************************************************************/ @@ -888,7 +943,10 @@ public class RDBMSQueryActionJoinsTest extends RDBMSActionTest /******************************************************************************* - ** + ** Note, this test was originally written asserting size=1... but reading + ** the data, for an all-access key, that seems wrong - as the user should see + ** all the records in this table, not just ones associated with a store... + ** so, switching to 4 (same issue in CountActionTest too). *******************************************************************************/ @Test void testRecordSecurityWithLockFromJoinTableWhereTheKeyIsOnTheManySide() throws QException @@ -897,8 +955,9 @@ public class RDBMSQueryActionJoinsTest extends RDBMSActionTest QueryInput queryInput = new QueryInput(); queryInput.setTableName(TestUtils.TABLE_NAME_WAREHOUSE); - assertThat(new QueryAction().execute(queryInput).getRecords()) - .hasSize(1); + List records = new QueryAction().execute(queryInput).getRecords(); + assertThat(records) + .hasSize(4); } From b955a20e1890e0ddc106cf0b4ee26a0f27588cfe Mon Sep 17 00:00:00 2001 From: Darin Kelkhoff Date: Wed, 2 Oct 2024 16:22:41 -0500 Subject: [PATCH 2/2] CE-1654 - Checkstyle! --- .../model/actions/tables/query/JoinsContext.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/qqq-backend-core/src/main/java/com/kingsrook/qqq/backend/core/model/actions/tables/query/JoinsContext.java b/qqq-backend-core/src/main/java/com/kingsrook/qqq/backend/core/model/actions/tables/query/JoinsContext.java index f1c23ee2..3f085f0e 100644 --- a/qqq-backend-core/src/main/java/com/kingsrook/qqq/backend/core/model/actions/tables/query/JoinsContext.java +++ b/qqq-backend-core/src/main/java/com/kingsrook/qqq/backend/core/model/actions/tables/query/JoinsContext.java @@ -1214,15 +1214,15 @@ public class JoinsContext if(isStart) { - System.out.println("\n" + StringUtils.safeTruncate("--- Start [main table: " + this.mainTableName + "] " + "-" .repeat(full), full)); + System.out.println("\n" + StringUtils.safeTruncate("--- Start [main table: " + this.mainTableName + "] " + "-".repeat(full), full)); } StringBuilder rs = new StringBuilder(); String formatString = "| %-" + md + "s | %-" + md + "s %-" + md + "s | %-" + lg + "s | %-" + sm + "s |\n"; rs.append(String.format(formatString, "Base Table", "Join Table", "(Alias)", "Join Meta Data", "Type")); - String dashesLg = "-" .repeat(lg); - String dashesMd = "-" .repeat(md); - String dashesSm = "-" .repeat(sm); + String dashesLg = "-".repeat(lg); + String dashesMd = "-".repeat(md); + String dashesSm = "-".repeat(sm); rs.append(String.format(formatString, dashesMd, dashesMd, dashesMd, dashesLg, dashesSm)); if(CollectionUtils.nullSafeHasContents(queryJoins)) { @@ -1248,11 +1248,11 @@ public class JoinsContext if(isEnd) { - System.out.println(StringUtils.safeTruncate("--- End " + "-" .repeat(full), full) + "\n"); + System.out.println(StringUtils.safeTruncate("--- End " + "-".repeat(full), full) + "\n"); } else { - System.out.println(StringUtils.safeTruncate("-" .repeat(full), full)); + System.out.println(StringUtils.safeTruncate("-".repeat(full), full)); } } }