perf(security): authorize each distinct parent once per child result page
ParentRowCustomizer and AttachmentPreviewCustomizer read the parent of every child row to decide whether the row is visible, so rows that share a parent repeated the same read: 61 tasks on two parents ran 61 parent reads (TagLookupStatementBudgetTest), now 2. The answer is kept only for the one postQuery call, where the session and transaction cannot change. Also correct the README: the Tags field is selectable. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
This commit is contained in:
1 parent
50226fbabc
commit
b9137b3379
6 files changed
+77
-8
No files matched your search
@@ -30,6 +30,7 @@ All notable changes are documented here, following [Keep a Changelog](https://ke
|
||||
### Fixed
|
||||
|
||||
- Reads of a table that has no tag assignment no longer query `collaboration_record_tag`. Core selects the Tags field for every query or get that names no fields, so each of them (including per-record gets and the stored-record read before an update) ran one assignment statement whether or not the table had ever been tagged: 600 gets of an untagged table ran 600 statements, now none. Each node remembers which tables have no assignment (`TaggedTables`, core `Memoization` registered with `ClusterCaches`) for `CollaborationQBitConfig.withTagLookupCacheSeconds` (default 30, 0 disables); writing an assignment clears it on the node and through the ClusterBus. Tables with assignments are read as before, one lookup per 500 parents. No API or schema change. See "What the Tags field costs a read" in the README.
|
||||
- Browsing one of the qbit's child tables (comments, tasks, record tags, attachments and the other parent-referencing tables) reads each distinct parent once per result page to authorize its rows, instead of once per row: 61 tasks on two parents ran 61 parent reads, now 2. The check itself and its outcome are unchanged.
|
||||
- The retained upload journal's PostgreSQL 17 qualification accepts Amazon Aurora PostgreSQL, which reports `full_page_writes=off` and does not let a deployment change it. `off` is accepted only when the settings query detects Aurora (`to_regprocedure('aurora_version()') IS NOT NULL`); stock PostgreSQL with `full_page_writes=off` is still refused, and every other requirement is unchanged on Aurora. See `docs/RETAINED-UPLOAD-JOURNAL.md`.
|
||||
- Bounded STRING custom fields carry QQQ's native `ValueTooLongBehavior.ERROR`, so a write longer than `maxLength` is rejected with a field error and the stored value is preserved, instead of passing through unchecked (the QQQ default `PASS_THROUGH`).
|
||||
- Persisted help content is reloaded after each committed runtime metadata refresh, so sections and fields created by publishing a custom-field definition receive their stored `helpContent` rows without a restart.
|
||||
|
||||
@@ -409,7 +409,7 @@ size, pending suggestions, and the five most-used approved tags.
|
||||
|
||||
`collaborationTags` is a virtual STRING field labeled Tags with possible-value source `collaboration_tag`. `TagCriteriaExpander` supports IN, NOT_IN, EQUALS, NOT_EQUALS, IS_BLANK, IS_NOT_BLANK and CONTAINS_ALL using core `QSubQuery` membership filters. Ordinary quick search matches labels; `#name` matches normalized names. Numeric criteria are tag IDs; string criteria are names. The old ID materializer is removed. Core owns SQL execution or bounded cross-backend materialization; its bound applies to inner rows, not final parent matches.
|
||||
|
||||
The runtime provider adds this field and its search entry where TAGS is enabled. It is queryable but not selectable: the overlay has no virtual-field post-query hook and existing host customizers must be preserved.
|
||||
The runtime provider adds this field and its search entry where TAGS is enabled. It is queryable and selectable (`TagValueProvider` fills it after host customizers have run).
|
||||
|
||||
### What the Tags field costs a read
|
||||
|
||||
|
||||
+2
-1
@@ -55,6 +55,7 @@ public class AttachmentPreviewCustomizer implements TableCustomizerInterface
|
||||
List<QRecord> filter(List<QRecord> records, boolean isGet, boolean generateUrls, AttachmentsService service) throws QException
|
||||
{
|
||||
List<QRecord> visible = new ArrayList<>();
|
||||
ParentRecordAccess.ResultSetParents parents = new ParentRecordAccess.ResultSetParents();
|
||||
for(QRecord record : records)
|
||||
{
|
||||
try
|
||||
@@ -63,7 +64,7 @@ public class AttachmentPreviewCustomizer implements TableCustomizerInterface
|
||||
{
|
||||
throw (new QNotFoundException("Not found"));
|
||||
}
|
||||
ParentRecordAccess.requireVisibleParent(record.getValueString("tableName"), record.getValueString("recordId"));
|
||||
parents.requireVisible(record.getValueString("tableName"), record.getValueString("recordId"));
|
||||
}
|
||||
catch(QNotFoundException expected)
|
||||
{
|
||||
|
||||
@@ -81,6 +81,42 @@ public final class ParentRecordAccess
|
||||
}
|
||||
}
|
||||
|
||||
/*******************************************************************************
|
||||
** One result set's parent checks: rows that share a parent cost one lookup.
|
||||
** The session and transaction do not change within the call that creates it,
|
||||
** so never keep an instance beyond that call.
|
||||
*******************************************************************************/
|
||||
public static final class ResultSetParents
|
||||
{
|
||||
private final java.util.Map<java.util.List<String>, Boolean> visible = new java.util.HashMap<>();
|
||||
|
||||
/*******************************************************************************
|
||||
** Same outcome as ParentRecordAccess.requireVisibleParent for every row.
|
||||
*******************************************************************************/
|
||||
public void requireVisible(String tableName, String recordId) throws QException
|
||||
{
|
||||
java.util.List<String> parent = java.util.Arrays.asList(tableName, recordId);
|
||||
Boolean known = visible.get(parent);
|
||||
if(known == null)
|
||||
{
|
||||
try
|
||||
{
|
||||
requireVisibleParent(tableName, recordId);
|
||||
known = true;
|
||||
}
|
||||
catch(QNotFoundException expected)
|
||||
{
|
||||
known = false;
|
||||
}
|
||||
visible.put(parent, known);
|
||||
}
|
||||
if(!known)
|
||||
{
|
||||
throw (new QNotFoundException("Not found"));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/*******************************************************************************
|
||||
** Core record capability checks include parent WRITE locks for implied grants.
|
||||
*******************************************************************************/
|
||||
|
||||
@@ -43,6 +43,7 @@ public class ParentRowCustomizer implements TableCustomizerInterface
|
||||
public List<QRecord> postQuery(QueryOrGetInputInterface input, List<QRecord> records) throws QException
|
||||
{
|
||||
List<QRecord> visible = new ArrayList<>();
|
||||
ParentRecordAccess.ResultSetParents parents = new ParentRecordAccess.ResultSetParents();
|
||||
for(QRecord record : records)
|
||||
{
|
||||
try
|
||||
@@ -51,7 +52,7 @@ public class ParentRowCustomizer implements TableCustomizerInterface
|
||||
{
|
||||
throw (new QNotFoundException("Not found"));
|
||||
}
|
||||
ParentRecordAccess.requireVisibleParent(record.getValueString("tableName"), record.getValueString("recordId"));
|
||||
parents.requireVisible(record.getValueString("tableName"), record.getValueString("recordId"));
|
||||
QRecord row = new QRecord(record);
|
||||
if(row.getValue("deletedAt") != null)
|
||||
{
|
||||
|
||||
@@ -27,6 +27,7 @@ import java.util.List;
|
||||
import java.util.Map;
|
||||
import java.util.TreeMap;
|
||||
import com.kingsrook.qbits.collaboration.model.CollaborationTables;
|
||||
import com.kingsrook.qbits.collaboration.permissions.CollaborationPermissions;
|
||||
import com.kingsrook.qbits.collaboration.tags.TagEntry;
|
||||
import com.kingsrook.qbits.collaboration.tags.TaggedTables;
|
||||
import com.kingsrook.qbits.collaboration.tags.TagsService;
|
||||
@@ -251,6 +252,27 @@ class TagLookupStatementBudgetTest extends PersonalServiceFixture
|
||||
|
||||
|
||||
|
||||
/*******************************************************************************
|
||||
** Browsing one of this qbit's child tables authorizes each distinct parent
|
||||
** once, not once per row (61 rows on two parents ran 61 parent reads before).
|
||||
*******************************************************************************/
|
||||
@Test
|
||||
void childRowsSharingAParentCostOneParentLookup() throws Exception
|
||||
{
|
||||
List<QRecord> tasks = new ArrayList<>();
|
||||
for(int id = 1; id <= 61; id++)
|
||||
{
|
||||
tasks.add(new QRecord().withValue("id", id).withValue("tableName", "document").withValue("recordId", id == 61 ? "2" : "1")
|
||||
.withValue("title", "Task " + id).withValue("assigneeUserId", "alice").withValue("createdByUserId", "alice")
|
||||
.withValue("status", "OPEN").withValue("parentDeleted", false));
|
||||
}
|
||||
MemoryRecordStore.getInstance().insert(new InsertInput(CollaborationTables.TASK).withRecords(tasks), true);
|
||||
login("alice", CollaborationPermissions.VIEW);
|
||||
assertThat(backendQueries(() -> assertThat(new QueryAction().execute(new QueryInput(CollaborationTables.TASK)).getRecords()).hasSize(61)).get("document")).isEqualTo(2L);
|
||||
}
|
||||
|
||||
|
||||
|
||||
/*******************************************************************************
|
||||
** A static cache that is not registered would go stale on the other nodes.
|
||||
*******************************************************************************/
|
||||
@@ -311,6 +333,18 @@ class TagLookupStatementBudgetTest extends PersonalServiceFixture
|
||||
** Backend queries against this qbit's tables while the work runs, by table.
|
||||
*******************************************************************************/
|
||||
private Map<String, Long> collaborationQueries(UnsafeVoidVoidMethod<Exception> work) throws Exception
|
||||
{
|
||||
Map<String, Long> result = backendQueries(work);
|
||||
result.keySet().removeIf(table -> !table.startsWith(CollaborationQBitConfig.TABLE_NAME_PREFIX));
|
||||
return (result);
|
||||
}
|
||||
|
||||
|
||||
|
||||
/*******************************************************************************
|
||||
** Every backend query while the work runs, by table.
|
||||
*******************************************************************************/
|
||||
private Map<String, Long> backendQueries(UnsafeVoidVoidMethod<Exception> work) throws Exception
|
||||
{
|
||||
MemoryRecordStore.resetStatistics();
|
||||
MemoryRecordStore.setCollectStatistics(true);
|
||||
@@ -321,11 +355,7 @@ class TagLookupStatementBudgetTest extends PersonalServiceFixture
|
||||
List<AbstractActionInput> inputs = MemoryRecordStore.getActionInputs().get(QueryInput.class);
|
||||
for(AbstractActionInput input : inputs == null ? List.<AbstractActionInput>of() : inputs)
|
||||
{
|
||||
String table = ((QueryInput) input).getTableName();
|
||||
if(table.startsWith(CollaborationQBitConfig.TABLE_NAME_PREFIX))
|
||||
{
|
||||
result.merge(table, 1L, Long::sum);
|
||||
}
|
||||
result.merge(((QueryInput) input).getTableName(), 1L, Long::sum);
|
||||
}
|
||||
return (result);
|
||||
}
|
||||
|
||||
Reference in new issue
Block a user