diff --git a/app/client/aclui/AccessRules.ts b/app/client/aclui/AccessRules.ts index 212edc7f..dda0b575 100644 --- a/app/client/aclui/AccessRules.ts +++ b/app/client/aclui/AccessRules.ts @@ -26,7 +26,7 @@ import { summarizePermissionSet } from 'app/common/ACLPermissions'; import {ACLRuleCollection, SPECIAL_RULES_TABLE_ID} from 'app/common/ACLRuleCollection'; -import {AclTableDescription, getTableTitle} from 'app/common/ActiveDocAPI'; +import {AclRuleProblem, AclTableDescription, getTableTitle} from 'app/common/ActiveDocAPI'; import {BulkColValues, getColValues, RowRecord, UserAction} from 'app/common/DocActions'; import { FormulaProperties, @@ -112,6 +112,9 @@ export class AccessRules extends Disposable { // Error or warning message to show next to Save/Reset buttons if non-empty. private _errorMessage = Observable.create(this, ''); + // Details of rule problems, for offering solutions to the user. + private _ruleProblems = this.autoDispose(obsArray()); + // Map of tableId to basic metadata for all tables in the document. private _aclResources = new Map(); @@ -196,7 +199,8 @@ export class AccessRules extends Disposable { this._updateDocAccessData(), this._gristDoc.docComm.getAclResources(), ]); - this._aclResources = new Map(Object.entries(aclResources)); + this._aclResources = new Map(Object.entries(aclResources.tables)); + this._ruleProblems.set(aclResources.problems); if (this.isDisposed()) { return; } this._tableRules.set( @@ -354,6 +358,14 @@ export class AccessRules extends Disposable { dom.text(this._errorMessage), testId('access-rules-error') ), + + dom.maybe(use => { + const ruleProblems = use(this._ruleProblems); + return ruleProblems.length > 0 ? ruleProblems : null; + }, ruleProblems => + cssSection( + cssRuleProblems( + this.buildRuleProblemsDom(ruleProblems)))), shadowScroll( dom.maybe(use => use(this._userAttrRules).length, () => cssSection( @@ -398,6 +410,26 @@ export class AccessRules extends Disposable { ); } + public buildRuleProblemsDom(ruleProblems: AclRuleProblem[]) { + const buttons: Array = []; + for (const problem of ruleProblems) { + // Is the problem a missing table? + if (problem.tables) { + this._addButtonsForMissingTables(buttons, problem.tables.tableIds); + } + // Is the problem a missing column? + if (problem.columns) { + this._addButtonsForMissingColumns(buttons, problem.columns.tableId, problem.columns.colIds); + } + // Is the problem a misconfigured user attribute? + if (problem.userAttributes) { + const names = problem.userAttributes.names; + this._addButtonsForMisconfiguredUserAttributes(buttons, names); + } + } + return buttons.map(button => dom('span', button)); + } + /** * Get a list of all rule records, for saving. */ @@ -476,6 +508,63 @@ export class AccessRules extends Disposable { this._aclUsersPopup.init(pageModel, permissionData); } + + private _addButtonsForMissingTables(buttons: Array, tableIds: string[]) { + for (const tableId of tableIds) { + // We don't know what the table's name was, just its tableId. + // Hopefully, the user will understand. + const title = t('RemoveRulesMentioningTable', { tableId }); + const button = bigBasicButton(title, cssRemoveIcon('Remove'), dom.on('click', async () => { + await Promise.all(this._tableRules.get() + .filter(rules => rules.tableId === tableId) + .map(rules => rules.remove())); + button.style.display = 'none'; + })); + buttons.push(button); + } + } + + private _addButtonsForMissingColumns(buttons: Array, + tableId: string, colIds: string[]) { + const removeColRules = (rules: TableRules, colId: string) => { + for (const rule of rules.columnRuleSets.get()) { + const ruleColIds = new Set(rule.getColIdList()); + if (!ruleColIds.has(colId)) { continue; } + if (ruleColIds.size === 1) { + rule.remove(); + } else { + rule.removeColId(colId); + } + } + }; + for (const colId of colIds) { + // TODO: we could translate tableId to table name in this case. + const title = t('RemoveRulesMentioningColumn', { tableId, colId }); + const button = bigBasicButton(title, cssRemoveIcon('Remove'), dom.on('click', async () => { + await Promise.all(this._tableRules.get() + .filter(rules => rules.tableId === tableId) + .map(rules => removeColRules(rules, colId))); + button.style.display = 'none'; + })); + buttons.push(button); + } + } + + private _addButtonsForMisconfiguredUserAttributes( + buttons: Array, + names: string[] + ) { + for (const name of names) { + const title = t('RemoveUserAttribute', {name}); + const button = bigBasicButton(title, cssRemoveIcon('Remove'), dom.on('click', async () => { + await Promise.all(this._userAttrRules.get() + .filter(rule => rule.name.get() === name) + .map(rule => rule.remove())); + button.style.display = 'none'; + })); + buttons.push(button); + } + } } // Represents all rules for a table. @@ -521,6 +610,14 @@ class TableRules extends Disposable { }); } + public remove() { + this._accessRules.removeTableRules(this); + } + + public get columnRuleSets() { + return this._columnRuleSets; + } + public buildDom() { return cssSection( cssSectionHeading( @@ -530,7 +627,7 @@ class TableRules extends Disposable { menuItemAsync(() => this._addColumnRuleSet(), t('AddColumnRule')), menuItemAsync(() => this._addDefaultRuleSet(), t('AddDefaultRule'), dom.cls('disabled', use => Boolean(use(this._defaultRuleSet)))), - menuItemAsync(() => this._accessRules.removeTableRules(this), t('DeleteTableRules')), + menuItemAsync(() => this.remove(), t('DeleteTableRules')), ]), testId('rule-table-menu-btn'), ), @@ -707,6 +804,10 @@ abstract class ObsRuleSet extends Disposable { }); } + public remove() { + this._tableRules?.removeRuleSet(this); + } + public getRules(tableId: string): RuleRec[] { // Return every part in the body, tacking on resourceRec to each rule. return this._body.get().map(part => ({ @@ -864,6 +965,10 @@ class ColumnObsRuleSet extends ObsRuleSet { return this._colIds.get(); } + public removeColId(colId: string) { + this._colIds.set(this._colIds.get().filter(c => (c !== colId))); + } + public getColIds(): string { return this._colIds.get().join(","); } @@ -1031,6 +1136,10 @@ class ObsUserAttributeRule extends Disposable { }); } + public remove() { + this._accessRules.removeUserAttributes(this); + } + public get name() { return this._name; } public get tableId() { return this._tableId; } @@ -1440,6 +1549,10 @@ const cssDropdownIcon = styled(icon, ` margin: -2px -2px 0 4px; `); +const cssRemoveIcon = styled(icon, ` + margin: -2px -2px 0 4px; +`); + const cssSection = styled('div', ` margin: 16px 16px 24px 16px; `); @@ -1581,3 +1694,13 @@ const cssDefaultLabel = styled('div', ` color: ${theme.accessRulesTableBodyFg}; font-weight: bold; `); + +const cssRuleProblems = styled('div', ` + flex: auto; + height: 100%; + width: 100%; + display: flex; + flex-direction: row; + flex-wrap: wrap; + gap: 8px; +`); diff --git a/app/common/ACLRuleCollection.ts b/app/common/ACLRuleCollection.ts index 1fc1d525..62a1d0f4 100644 --- a/app/common/ACLRuleCollection.ts +++ b/app/common/ACLRuleCollection.ts @@ -1,4 +1,5 @@ import {parsePermissions} from 'app/common/ACLPermissions'; +import {AclRuleProblem} from 'app/common/ActiveDocAPI'; import {ILogger} from 'app/common/BaseAPI'; import {DocData} from 'app/common/DocData'; import {AclMatchFunc, ParsedAclFormula, RulePart, RuleSet, UserAttributeRule} from 'app/common/GranularAccessClause'; @@ -232,6 +233,20 @@ export class ACLRuleCollection { * Check that all references to table and column IDs in ACL rules are valid. */ public checkDocEntities(docData: DocData) { + const problems = this.findRuleProblems(docData); + if (problems.length === 0) { return; } + throw new Error(problems[0].comment); + } + + /** + * Enumerate rule problems caused by table and column IDs that are not valid. + * Problems include: + * - Rules for a table that does not exist + * - Rules for columns that include a column that does not exist + * - User attributes links to a column that does not exist + */ + public findRuleProblems(docData: DocData): AclRuleProblem[] { + const problems: AclRuleProblem[] = []; const tablesTable = docData.getMetaTable('_grist_Tables'); const columnsTable = docData.getMetaTable('_grist_Tables_column'); @@ -239,7 +254,12 @@ export class ACLRuleCollection { const validTableIds = new Set(tablesTable.getColValues('tableId')); const invalidTables = this.getAllTableIds().filter(t => !validTableIds.has(t)); if (invalidTables.length > 0) { - throw new Error(`Invalid tables in rules: ${invalidTables.join(', ')}`); + problems.push({ + tables: { + tableIds: invalidTables, + }, + comment: `Invalid tables in rules: ${invalidTables.join(', ')}`, + }); } // Collect valid columns, grouped by tableRef (rowId of table record). @@ -249,15 +269,22 @@ export class ACLRuleCollection { getSetMapValue(validColumns, colTableRefs[i], () => new Set()).add(colId); } - // For each table, check that any explicitly mentioned columns are valid. + // For each valid table, check that any explicitly mentioned columns are valid. for (const tableId of this.getAllTableIds()) { + if (!validTableIds.has(tableId)) { continue; } const tableRef = tablesTable.findRow('tableId', tableId); const validTableCols = validColumns.get(tableRef); for (const ruleSet of this.getAllColumnRuleSets(tableId)) { if (Array.isArray(ruleSet.colIds)) { const invalidColIds = ruleSet.colIds.filter(c => !validTableCols?.has(c)); if (invalidColIds.length > 0) { - throw new Error(`Invalid columns in rules for table ${tableId}: ${invalidColIds.join(', ')}`); + problems.push({ + columns: { + tableId, + colIds: invalidColIds, + }, + comment: `Invalid columns in rules for table ${tableId}: ${invalidColIds.join(', ')}`, + }); } } } @@ -265,16 +292,25 @@ export class ACLRuleCollection { // Check for valid tableId/lookupColId combinations in UserAttribute rules. const invalidUAColumns: string[] = []; + const names: string[] = []; for (const rule of this.getUserAttributeRules().values()) { const tableRef = tablesTable.findRow('tableId', rule.tableId); const colRef = columnsTable.findMatchingRowId({parentId: tableRef, colId: rule.lookupColId}); if (!colRef) { invalidUAColumns.push(`${rule.tableId}.${rule.lookupColId}`); + names.push(rule.name); } } if (invalidUAColumns.length > 0) { - throw new Error(`Invalid columns in User Attribute rules: ${invalidUAColumns.join(', ')}`); + problems.push({ + userAttributes: { + invalidUAColumns, + names, + }, + comment: `Invalid columns in User Attribute rules: ${invalidUAColumns.join(', ')}`, + }); } + return problems; } private _safeReadAclRules(docData: DocData, options: ReadAclOptions): ReadAclResults { diff --git a/app/common/ActiveDocAPI.ts b/app/common/ActiveDocAPI.ts index 1cefa165..73af4c22 100644 --- a/app/common/ActiveDocAPI.ts +++ b/app/common/ActiveDocAPI.ts @@ -176,6 +176,26 @@ export interface AclTableDescription { groupByColLabels: string[] | null; // Labels of groupby columns for summary tables, or null. } +export interface AclResources { + tables: {[tableId: string]: AclTableDescription}; + problems: AclRuleProblem[]; +} + +export interface AclRuleProblem { + tables?: { + tableIds: string[], + }; + columns?: { + tableId: string, + colIds: string[], + }; + userAttributes?: { + invalidUAColumns: string[], + names: string[], + } + comment: string; +} + export function getTableTitle(table: AclTableDescription): string { let {title} = table; if (table.groupByColLabels) { @@ -349,7 +369,7 @@ export interface ActiveDocAPI { * for editing ACLs. It is only available to users who can edit ACLs, and lists all resources * regardless of rules that may block access to them. */ - getAclResources(): Promise<{[tableId: string]: AclTableDescription}>; + getAclResources(): Promise; /** * Wait for document to finish initializing. diff --git a/app/server/lib/ActiveDoc.ts b/app/server/lib/ActiveDoc.ts index cf24c4d0..53299193 100644 --- a/app/server/lib/ActiveDoc.ts +++ b/app/server/lib/ActiveDoc.ts @@ -4,6 +4,7 @@ * change events. */ +import {ACLRuleCollection} from 'app/common/ACLRuleCollection'; import { getEnvContent, LocalActionBundle, @@ -14,6 +15,7 @@ import { import {ActionGroup, MinimalActionGroup} from 'app/common/ActionGroup'; import {ActionSummary} from "app/common/ActionSummary"; import { + AclResources, AclTableDescription, ApplyUAExtendedOptions, ApplyUAOptions, @@ -1427,8 +1429,11 @@ export class ActiveDoc extends EventEmitter { * Returns the full set of tableIds, with basic metadata for each table. This is intended * for editing ACLs. It is only available to users who can edit ACLs, and lists all resources * regardless of rules that may block access to them. + * + * Also returns information about resources mentioned in rules that no longer + * exist. */ - public async getAclResources(docSession: DocSession): Promise<{[tableId: string]: AclTableDescription}> { + public async getAclResources(docSession: DocSession): Promise { if (!this.docData || !await this._granularAccess.hasAccessRulesPermission(docSession)) { throw new Error('Cannot list ACL resources'); } @@ -1453,7 +1458,10 @@ export class ActiveDoc extends EventEmitter { result[tableId].groupByColLabels!.push(sourceCol.label); } } - return result; + const ruleCollection = new ACLRuleCollection(); + await ruleCollection.update(this.docData, {log}); + const problems = ruleCollection.findRuleProblems(this.docData); + return {tables: result, problems}; } /** diff --git a/static/locales/en.client.json b/static/locales/en.client.json index 3faeb2f3..b72d275e 100644 --- a/static/locales/en.client.json +++ b/static/locales/en.client.json @@ -584,7 +584,10 @@ "AttributeNamePlaceholder": "Attribute name", "Everyone": "Everyone", "EveryoneElse": "Everyone Else", - "EnterCondition": "Enter Condition" + "EnterCondition": "Enter Condition", + "RemoveRulesMentioningTable": "Remove {{- tableId }} rules", + "RemoveRulesMentioningColumn": "Remove column {{- colId }} from {{- tableId }} rules", + "RemoveUserAttribute": "Remove {{- name }} user attribute" }, "PermissionsWidget": { "AllowAll": "Allow All",