(core) Fix summary table titles and linking when source table is hidden by ACL

Summary:
Two summary table widgets that share a source table and have compatible groupby columns can be filter linked. This diff fixes a bug where this linking is broken when the source table is hidden by access rules. The source table data isn't needed for the linking, but its metadata is, and that metadata is censored by GranularAccess. To deal with this:

- `LinkConfig._assertValid` allows blank `tableId`s specifically for linking two summary tables.
- `LinkingState.filterColValues` gets the `colId`s of groupby columns from the summary table columns rather than the source table.

A closely related problem is that the titles of summary tables are incomplete when the source table is hidden, e.g. they just say `[by A]` instead of `Table1 [by A]`. To fix this, the raw view sections of source tables are 'uncensored' in GranularAccess.

Initially I also planned to uncensor the tableId of the source table, which seemed like a better and more general fix for the blank tableId problem. But several parts of client code use blank tableIds to know that a table should be hidden, so they were left as is.

Test Plan: Added an nbrowser test for summary table linking, and a server test for uncensoring the raw view section in GranularAccess.

Reviewers: georgegevoian, paulfitz

Reviewed By: georgegevoian, paulfitz

Subscribers: paulfitz

Differential Revision: https://phab.getgrist.com/D3608
This commit is contained in:
Alex Hall 2022-08-30 22:13:57 +02:00
parent 193d5360ad
commit ecf2fdf71a
4 changed files with 42 additions and 5 deletions

View File

@ -119,12 +119,11 @@ export class LinkingState extends Disposable {
} }
const srcRowId = srcSection.activeRowId(); const srcRowId = srcSection.activeRowId();
for (const c of srcSection.table().groupByColumns()) { for (const c of srcSection.table().groupByColumns()) {
const col = c.summarySource(); const colId = c.colId();
const colId = col.colId();
const srcValue = srcTableData.getValue(srcRowId as number, colId); const srcValue = srcTableData.getValue(srcRowId as number, colId);
result.filters[colId] = [srcValue]; result.filters[colId] = [srcValue];
result.operations[colId] = 'in'; result.operations[colId] = 'in';
if (isDirectSummary && isListType(col.type())) { if (isDirectSummary && isListType(c.summarySource().type())) {
// If the source groupby column is a ChoiceList or RefList, then null or '' in the summary table // If the source groupby column is a ChoiceList or RefList, then null or '' in the summary table
// should match against an empty list in the source table. // should match against an empty list in the source table.
result.operations[colId] = srcValue ? 'intersects' : 'empty'; result.operations[colId] = srcValue ? 'intersects' : 'empty';

View File

@ -499,10 +499,15 @@ export function createViewSectionRec(this: ViewSectionRec, docModel: DocModel):
this._linkingState = Holder.create(this); this._linkingState = Holder.create(this);
this.linkingState = this.autoDispose(ko.pureComputed(() => { this.linkingState = this.autoDispose(ko.pureComputed(() => {
if (!this.activeLinkSrcSectionRef()) {
// This view section isn't selecting by anything.
return null;
}
try { try {
const config = new LinkConfig(this); const config = new LinkConfig(this);
return LinkingState.create(this._linkingState, docModel, config); return LinkingState.create(this._linkingState, docModel, config);
} catch (err) { } catch (err) {
console.warn(err);
// Dispose old LinkingState in case creating the new one failed. // Dispose old LinkingState in case creating the new one failed.
this._linkingState.dispose(); this._linkingState.dispose();
return null; return null;

View File

@ -332,13 +332,21 @@ export class LinkConfig {
this.srcSection.table().primaryTableId()); this.srcSection.table().primaryTableId());
const tgtTableId = (tgtCol ? getReferencedTableId(tgtCol.type()) : const tgtTableId = (tgtCol ? getReferencedTableId(tgtCol.type()) :
this.tgtSection.table().primaryTableId()); this.tgtSection.table().primaryTableId());
const srcTableSummarySourceTable = this.srcSection.table().summarySourceTable();
const tgtTableSummarySourceTable = this.tgtSection.table().summarySourceTable();
try { try {
assert(Boolean(this.srcSection.getRowId()), "srcSection was disposed"); assert(Boolean(this.srcSection.getRowId()), "srcSection was disposed");
assert(!tgtCol || tgtCol.parentId() === this.tgtSection.tableRef(), "tgtCol belongs to wrong table"); assert(!tgtCol || tgtCol.parentId() === this.tgtSection.tableRef(), "tgtCol belongs to wrong table");
assert(!srcCol || srcCol.parentId() === this.srcSection.tableRef(), "srcCol belongs to wrong table"); assert(!srcCol || srcCol.parentId() === this.srcSection.tableRef(), "srcCol belongs to wrong table");
assert(this.srcSection.getRowId() !== this.tgtSection.getRowId(), "srcSection links to itself"); assert(this.srcSection.getRowId() !== this.tgtSection.getRowId(), "srcSection links to itself");
assert(tgtTableId, "tgtCol not a valid reference");
assert(srcTableId, "srcCol not a valid reference"); // We usually expect srcTableId and tgtTableId to be non-empty, but there's one exception:
// when linking two summary tables that share a source table (which we can check directly)
// and the source table is hidden by ACL, so its tableId is empty from our perspective.
if (!(srcTableSummarySourceTable !== 0 && srcTableSummarySourceTable === tgtTableSummarySourceTable)) {
assert(tgtTableId, "tgtCol not a valid reference");
assert(srcTableId, "srcCol not a valid reference");
}
assert(srcTableId === tgtTableId, "mismatched tableIds"); assert(srcTableId === tgtTableId, "mismatched tableIds");
} catch (e) { } catch (e) {
throw new Error(`LinkConfig invalid: ` + throw new Error(`LinkConfig invalid: ` +

View File

@ -2193,6 +2193,7 @@ export class CensorshipInfo {
const columnCode = (tableRef: number, colId: string) => `${tableRef} ${colId}`; const columnCode = (tableRef: number, colId: string) => `${tableRef} ${colId}`;
const censoredColumnCodes: Set<string> = new Set(); const censoredColumnCodes: Set<string> = new Set();
const tableRefToTableId: Map<number, string> = new Map(); const tableRefToTableId: Map<number, string> = new Map();
const tableRefToIndex: Map<number, number> = new Map();
const uncensoredTables: Set<number> = new Set(); const uncensoredTables: Set<number> = new Set();
// Scan for forbidden tables. // Scan for forbidden tables.
let rec = new RecordView(tables._grist_Tables, undefined); let rec = new RecordView(tables._grist_Tables, undefined);
@ -2202,6 +2203,7 @@ export class CensorshipInfo {
const tableId = rec.get('tableId') as string; const tableId = rec.get('tableId') as string;
const tableRef = ids[idx]; const tableRef = ids[idx];
tableRefToTableId.set(tableRef, tableId); tableRefToTableId.set(tableRef, tableId);
tableRefToIndex.set(tableRef, idx);
const tableAccess = permInfo.getTableAccess(tableId); const tableAccess = permInfo.getTableAccess(tableId);
if (tableAccess.perms.read === 'deny') { if (tableAccess.perms.read === 'deny') {
this.censoredTables.add(tableRef); this.censoredTables.add(tableRef);
@ -2254,6 +2256,29 @@ export class CensorshipInfo {
!this.censoredColumns.has(rec.get('colRef') as number)) { continue; } !this.censoredColumns.has(rec.get('colRef') as number)) { continue; }
this.censoredFields.add(ids[idx]); this.censoredFields.add(ids[idx]);
} }
// Now undo some of the above...
// Specifically, when a summary table is not censored, uncensor the source table's raw view section,
// so that the user can see the source table's title,
// which is used to construct the summary table's title. The section's fields remain censored.
// This would also be a sensible place to uncensor the source tableId, but that causes other problems.
rec = new RecordView(tables._grist_Tables, undefined);
ids = getRowIdsFromDocAction(tables._grist_Tables);
for (let idx = 0; idx < ids.length; idx++) {
rec.index = idx;
const tableRef = ids[idx];
const sourceTableRef = rec.get('summarySourceTable') as number;
const sourceTableIndex = tableRefToIndex.get(sourceTableRef);
if (
this.censoredTables.has(tableRef) ||
!sourceTableRef ||
sourceTableIndex === undefined ||
!this.censoredTables.has(sourceTableRef)
) { continue; }
rec.index = sourceTableIndex;
const rawViewSectionRef = rec.get('rawViewSectionRef') as number;
this.censoredSections.delete(rawViewSectionRef);
}
} }
public apply(a: DataAction) { public apply(a: DataAction) {