From 45c7602f499a0dfb58178716af6047309461d94e Mon Sep 17 00:00:00 2001 From: Dmitry S Date: Wed, 18 Jan 2023 10:36:11 -0500 Subject: [PATCH] (core) Implement checkbox for SchemaEdit permission in Access Rules UI. Summary: - Introduces a fictitious "*SPECIAL:SchemaEdit" resource in UI only. - Hides "S" bit for the default rule section. - Shows a checkbox UI similar to other checkboxes, with an additional dismissable warning. Test Plan: Added a browser test Reviewers: paulfitz, georgegevoian Reviewed By: paulfitz, georgegevoian Differential Revision: https://phab.getgrist.com/D3765 --- app/client/aclui/AccessRules.ts | 186 +++++++++++++++++++++++++------- app/common/ACLPermissions.ts | 22 ++++ app/common/ACLRuleCollection.ts | 87 ++++++++++++++- test/nbrowser/gristUtils.ts | 8 +- 4 files changed, 257 insertions(+), 46 deletions(-) diff --git a/app/client/aclui/AccessRules.ts b/app/client/aclui/AccessRules.ts index 2021c909..d78fb25c 100644 --- a/app/client/aclui/AccessRules.ts +++ b/app/client/aclui/AccessRules.ts @@ -26,7 +26,7 @@ import { summarizePermissions, summarizePermissionSet } from 'app/common/ACLPermissions'; -import {ACLRuleCollection, SPECIAL_RULES_TABLE_ID} from 'app/common/ACLRuleCollection'; +import {ACLRuleCollection, isSchemaEditResource, SPECIAL_RULES_TABLE_ID} from 'app/common/ACLRuleCollection'; import {AclRuleProblem, AclTableDescription, getTableTitle} from 'app/common/ActiveDocAPI'; import {BulkColValues, getColValues, RowRecord, UserAction} from 'app/common/DocActions'; import { @@ -45,6 +45,7 @@ import { Computed, Disposable, dom, + DomContents, DomElementArg, IDisposableOwner, MutableObsArray, @@ -201,7 +202,7 @@ export class AccessRules extends Disposable { const rules = this._ruleCollection; const [ , , aclResources] = await Promise.all([ - rules.update(this._gristDoc.docData, {log: console}), + rules.update(this._gristDoc.docData, {log: console, pullOutSchemaEdit: true}), this._updateDocAccessData(), this._gristDoc.docComm.getAclResources(), ]); @@ -217,7 +218,7 @@ export class AccessRules extends Disposable { ); const withDefaultRules = ['SeedRule']; - const separateRules = ['FullCopies', 'AccessRules']; + const separateRules = ['SchemaEdit', 'FullCopies', 'AccessRules']; SpecialRules.create( this._specialRulesWithDefault, SPECIAL_RULES_TABLE_ID, this, @@ -252,12 +253,20 @@ export class AccessRules extends Disposable { [{tableId: '*', colIds: '*'}], this._specialRulesWithDefault.get()?.getResources() || [], this._specialRulesSeparate.get()?.getResources() || [], - ...this._tableRules.get().map(tr => tr.getResources())) - .map(r => ({id: -1, ...r})); + ...this._tableRules.get().map(tr => tr.getResources()) + ) + // Skip the fake "*SPECIAL:SchemaEdit" resource (frontend-specific); these rules are saved to the default resource. + .filter(resource => !isSchemaEditResource(resource)) + .map(r => ({id: -1, ...r})); // Prepare userActions and a mapping of serializedResource to rowIds. const resourceSync = syncRecords(resourcesTable, newResources, serializeResource); + const defaultResourceRowId = resourceSync.rowIdMap.get(serializeResource({id: -1, tableId: '*', colIds: '*'})); + if (!defaultResourceRowId) { + throw new Error('Default resource missing in resource map'); + } + // For syncing rules, we'll go by rowId that we store with each RulePart and with the RuleSet. const newRules: RowRecord[] = []; for (const rule of this.getRules()) { @@ -267,10 +276,16 @@ export class AccessRules extends Disposable { } // Look up the rowId for the resource. - const resourceKey = serializeResource(rule.resourceRec as RowRecord); - const resourceRowId = resourceSync.rowIdMap.get(resourceKey); - if (!resourceRowId) { - throw new Error(`Resource missing in resource map: ${resourceKey}`); + let resourceRowId: number|undefined; + // Assign the rules for the fake "*SPECIAL:SchemaEdit" resource to the default resource where they belong. + if (isSchemaEditResource(rule.resourceRec!)) { + resourceRowId = defaultResourceRowId; + } else { + const resourceKey = serializeResource(rule.resourceRec as RowRecord); + resourceRowId = resourceSync.rowIdMap.get(resourceKey); + if (!resourceRowId) { + throw new Error(`Resource missing in resource map: ${resourceKey}`); + } } newRules.push({ id: rule.id || -1, @@ -283,10 +298,6 @@ export class AccessRules extends Disposable { } // UserAttribute rules are listed in the same rulesTable. - const defaultResourceRowId = resourceSync.rowIdMap.get(serializeResource({id: -1, tableId: '*', colIds: '*'})); - if (!defaultResourceRowId) { - throw new Error('Default resource missing in resource map'); - } for (const userAttr of this._userAttrRules.get()) { const rule = userAttr.getRule(); newRules.push({ @@ -829,7 +840,12 @@ class SpecialRules extends TableRules { owner: IDisposableOwner, accessRules: AccessRules, tableRules: TableRules, ruleSet: RuleSet|undefined, initialColIds: string[], ): ColumnObsRuleSet { - return SpecialObsRuleSet.create(owner, accessRules, tableRules, ruleSet, initialColIds); + if (isEqual(ruleSet?.colIds, ['SchemaEdit'])) { + // The special rule for "schemaEdit" permissions. + return SpecialSchemaObsRuleSet.create(owner, accessRules, tableRules, ruleSet, initialColIds); + } else { + return SpecialObsRuleSet.create(owner, accessRules, tableRules, ruleSet, initialColIds); + } } } @@ -1014,12 +1030,7 @@ abstract class ObsRuleSet extends Disposable { * Which permission bits to allow the user to set. */ public getAvailableBits(): PermissionKey[] { - if (this._tableRules) { - return ['read', 'update', 'create', 'delete']; - } else { - // For the doc-wide rule set, expose the schemaEdit bit too. - return ['read', 'update', 'create', 'delete', 'schemaEdit']; - } + return ['read', 'update', 'create', 'delete']; } /** @@ -1112,18 +1123,32 @@ class DefaultObsRuleSet extends ObsRuleSet { } } +interface SpecialRuleBody { + permissions: string; + formula: string; +} + /** * Properties we need to know about how a special rule should function and * be rendered. */ -interface SpecialRuleProperties { +interface SpecialRuleProperties extends SpecialRuleBody { description: string; name: string; availableBits: PermissionKey[]; - permissions: string; - formula: string; } +const schemaEditRules: {[key: string]: SpecialRuleBody} = { + allowEditors: { + permissions: '+S', + formula: 'user.Access == EDITOR', + }, + denyEditors: { + permissions: '-S', + formula: 'user.Access != OWNER', + }, +}; + const specialRuleProperties: Record = { AccessRules: { name: t('Permission to view Access Rules'), @@ -1147,6 +1172,13 @@ Useful for examples and templates, but not for sensitive data.`), permissions: '+CRUD', formula: 'user.Access in [OWNER]', }, + SchemaEdit: { + name: t("Permission to edit document structure"), + description: t("Allow editors to edit structure (e.g. modify and delete tables, columns, " + + "layouts), and to write formulas, which give access to all data regardless of read restrictions."), + availableBits: ['schemaEdit'], + ...schemaEditRules.denyEditors, + }, }; function getSpecialRuleProperties(name: string): SpecialRuleProperties { @@ -1165,32 +1197,29 @@ class SpecialObsRuleSet extends ColumnObsRuleSet { } public buildRuleSetDom() { - const isNonStandard: Observable = Computed.create(null, this._body, (use, body) => - !body.every(rule => rule.isBuiltInOrEmpty(use) || rule.matches(use, this.props.formula, this.props.permissions))); - - const allowEveryone: Observable = Computed.create(null, this._body, - (use, body) => !use(isNonStandard) && !body.every(rule => rule.isBuiltInOrEmpty(use))) - .onWrite(val => this._allowEveryone(val)); - + const isNonStandard = this._createIsNonStandardObs(); + const isChecked = this._createIsCheckedObs(isNonStandard); if (isNonStandard.get()) { this._isExpanded.set(true); } return dom('div', - dom.autoDispose(allowEveryone), + dom.autoDispose(isChecked), + dom.autoDispose(isNonStandard), cssRuleDescription( - {style: 'white-space: pre-line;'}, // preserve line breaks in long descriptions cssIconButton(icon('Expand'), dom.style('transform', (use) => use(this._isExpanded) ? 'rotate(90deg)' : ''), dom.on('click', () => this._isExpanded.set(!this._isExpanded.get())), testId('rule-special-expand'), + {style: 'margin: -4px'}, // subtract padding to align better. ), - squareCheckbox(allowEveryone, + cssCheckbox(isChecked, dom.prop('disabled', isNonStandard), testId('rule-special-checkbox'), ), this.props.description, ), + this._buildDomWarning(), dom.maybe(this._isExpanded, () => cssTableRounded( {style: 'margin-left: 56px'}, @@ -1238,14 +1267,29 @@ class SpecialObsRuleSet extends ColumnObsRuleSet { } } + protected _buildDomWarning(): DomContents { + return null; + } + + // Observable for whether this ruleSet is "standard", i.e. checked or unchecked state, without + // any strange rules that need to be shown expanded with the checkbox greyed out. + protected _createIsNonStandardObs(): Observable { + return Computed.create(null, this._body, (use, body) => + !body.every(rule => rule.isBuiltInOrEmpty(use) || rule.matches(use, this.props.formula, this.props.permissions))); + } + + // Observable for whether the checkbox should be shown as checked. Writing to it will update + // rules so as to toggle the checkbox. + protected _createIsCheckedObs(isNonStandard: Observable): Observable { + return Computed.create(null, this._body, + (use, body) => !use(isNonStandard) && !body.every(rule => rule.isBuiltInOrEmpty(use))) + .onWrite(val => this._allowEveryone(val)); + } + private _allowEveryone(value: boolean) { const builtInRules = this._body.get().filter(r => r.isBuiltIn()); if (value) { - const rulePart: RulePart = { - aclFormula: this.props.formula, - permissionsText: this.props.permissions, - permissions: parsePermissions(this.props.permissions), - }; + const rulePart = makeRulePart(this.props); this._body.set([ObsRulePart.create(this._body, this, rulePart, true), ...builtInRules]); } else { this._body.set(builtInRules); @@ -1256,6 +1300,63 @@ class SpecialObsRuleSet extends ColumnObsRuleSet { } } +function makeRulePart({permissions, formula}: SpecialRuleBody): RulePart { + const rulePart: RulePart = { + aclFormula: formula, + permissionsText: permissions, + permissions: parsePermissions(permissions), + }; + return rulePart; +} + +/** + * SchemaEdit permissions are moved out to a special fake resource "*SPECIAL:SchemaEdit" in the + * frontend, to be presented under their own checkbox option. Its behaviors are a bit different + * from other checkbox options; the differences are in the overridden methods here. + */ +class SpecialSchemaObsRuleSet extends SpecialObsRuleSet { + protected _buildDomWarning(): DomContents { + return dom.maybe( + (use) => use(this._body).every(rule => rule.isBuiltInOrEmpty(use)), + () => cssConditionError({style: 'margin-left: 56px; margin-bottom: 8px;'}, + "This default should be changed if editors' access is to be limited. ", + dom('a', {style: 'color: inherit; text-decoration: underline'}, + 'Dismiss', dom.on('click', () => this._allowEditors('confirm'))), + testId('rule-schema-edit-warning'), + ) + ); + } + + // SchemaEdit rules support an extra "standard" state, where a no-op rule exists (explicit rule + // allowing EDITORs SchemaEdit permission), in which case we don't show a warning. + protected _createIsNonStandardObs(): Observable { + return Computed.create(null, this._body, (use, body) => + !body.every(rule => rule.isBuiltInOrEmpty(use) || rule.matches(use, this.props.formula, this.props.permissions) + || rule.matches(use, schemaEditRules.allowEditors.formula, schemaEditRules.allowEditors.permissions))); + } + + protected _createIsCheckedObs(isNonStandard: Observable): Observable { + return Computed.create(null, this._body, + (use, body) => body.every(rule => rule.isBuiltInOrEmpty(use) + || rule.matches(use, schemaEditRules.allowEditors.formula, schemaEditRules.allowEditors.permissions))) + .onWrite(val => this._allowEditors(val)); + } + + // The third "confirm" option is used by the "Dismiss" link in the warning. + private _allowEditors(value: boolean|'confirm') { + const builtInRules = this._body.get().filter(r => r.isBuiltIn()); + if (value === 'confirm') { + const rulePart = makeRulePart(schemaEditRules.allowEditors); + this._body.set([ObsRulePart.create(this._body, this, rulePart, true), ...builtInRules]); + } else if (!value) { + const rulePart = makeRulePart(schemaEditRules.denyEditors); + this._body.set([ObsRulePart.create(this._body, this, rulePart, true), ...builtInRules]); + } else { + this._body.set(builtInRules); + } + } +} + class ObsUserAttributeRule extends Disposable { public ruleStatus: Computed; @@ -1943,9 +2044,14 @@ const cssRuleBody = styled('div', ` const cssRuleDescription = styled('div', ` color: ${theme.text}; display: flex; - align-items: center; + align-items: top; margin: 16px 0 8px 0; - gap: 8px; + gap: 12px; + white-space: pre-line; /* preserve line breaks in long descriptions */ +`); + +const cssCheckbox = styled(squareCheckbox, ` + flex: none; `); const cssCellContent = styled('div', ` diff --git a/app/common/ACLPermissions.ts b/app/common/ACLPermissions.ts index 008cfc83..617aae00 100644 --- a/app/common/ACLPermissions.ts +++ b/app/common/ACLPermissions.ts @@ -190,3 +190,25 @@ export function summarizePermissions(perms: MixedPermissionValue[]): MixedPermis const perm = perms[0]; return perms.some(p => p !== perm) ? 'mixed' : perm; } + + +function isEmpty(permissions: PartialPermissionSet): boolean { + return Object.values(permissions).every(v => v === ""); +} + + +/** + * Divide up a PartialPermissionSet into two: one containing only the 'schemaEdit' permission bit, + * and the other containing everything else. Empty parts will be returned as undefined, except + * when both are empty, in which case nonSchemaEdit will be returned as an empty permission set. + */ +export function splitSchemaEditPermissionSet(permissions: PartialPermissionSet): + {schemaEdit?: PartialPermissionSet, nonSchemaEdit?: PartialPermissionSet} { + + const schemaEdit = {...emptyPermissionSet(), schemaEdit: permissions.schemaEdit}; + const nonSchemaEdit: PartialPermissionSet = {...permissions, schemaEdit: ""}; + return { + schemaEdit: !isEmpty(schemaEdit) ? schemaEdit : undefined, + nonSchemaEdit: !isEmpty(nonSchemaEdit) || isEmpty(schemaEdit) ? nonSchemaEdit : undefined, + }; +} diff --git a/app/common/ACLRuleCollection.ts b/app/common/ACLRuleCollection.ts index 0274174c..63428559 100644 --- a/app/common/ACLRuleCollection.ts +++ b/app/common/ACLRuleCollection.ts @@ -1,9 +1,9 @@ -import {parsePermissions} from 'app/common/ACLPermissions'; +import {parsePermissions, permissionSetToText, splitSchemaEditPermissionSet} 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'; -import {getSetMapValue} from 'app/common/gutil'; +import {getSetMapValue, isNonNullish} from 'app/common/gutil'; import {MetaRowRecord} from 'app/common/TableData'; import {decodeObject} from 'app/plugin/objtypes'; import sortBy = require('lodash/sortBy'); @@ -34,7 +34,28 @@ const DEFAULT_RULE_SET: RuleSet = { }], }; +// Check if the given resource is the special "SchemaEdit" resource, which only exists as a +// frontend representation. +export function isSchemaEditResource(resource: {tableId: string, colIds: string}): boolean { + return resource.tableId === SPECIAL_RULES_TABLE_ID && resource.colIds === 'SchemaEdit'; +} + const SPECIAL_RULE_SETS: Record = { + SchemaEdit: { + tableId: SPECIAL_RULES_TABLE_ID, + colIds: ['SchemaEdit'], + body: [{ + aclFormula: "user.Access in [EDITOR, OWNER]", + matchFunc: (input) => ['editors', 'owners'].includes(String(input.user.Access)), + permissions: parsePermissions('+S'), + permissionsText: '+S', + }, { + aclFormula: "", + matchFunc: defaultMatchFunc, + permissions: parsePermissions('-S'), + permissionsText: '-S', + }], + }, AccessRules: { tableId: SPECIAL_RULES_TABLE_ID, colIds: ['AccessRules'], @@ -191,6 +212,21 @@ export class ACLRuleCollection { } else { specialRuleSets.set(specialType, {...ruleSet, body: [...ruleSet.body, ...specialDefault.body]}); } + } else if (options.pullOutSchemaEdit && ruleSet.tableId === '*' && ruleSet.colIds === '*') { + // If pullOutSchemaEdit is requested, we move out rules with SchemaEdit permissions from + // the default resource into the ficticious "*SPECIAL:SchemaEdit" resource. This is used + // in the frontend only, to present those rules in a separate section. + const schemaParts = ruleSet.body.map(part => splitSchemaEditRulePart(part).schemaEdit).filter(isNonNullish); + + if (schemaParts.length > 0) { + const specialType = 'SchemaEdit'; + const specialDefault = specialRuleSets.get(specialType)!; + specialRuleSets.set(specialType, { + tableId: SPECIAL_RULES_TABLE_ID, + colIds: ['SchemaEdit'], + body: [...schemaParts, ...specialDefault.body] + }); + } } } @@ -203,9 +239,15 @@ export class ACLRuleCollection { for (const ruleSet of ruleSets) { if (ruleSet.tableId === '*') { if (ruleSet.colIds === '*') { + // If pullOutSchemaEdit is requested, skip the SchemaEdit rules for the default resource; + // those got pulled out earlier into the fictitious "*SPECIAL:SchemaEdit" resource. + const body = options.pullOutSchemaEdit ? + ruleSet.body.map(part => splitSchemaEditRulePart(part).nonSchemaEdit).filter(isNonNullish) : + ruleSet.body; + defaultRuleSet = { ...ruleSet, - body: [...ruleSet.body, ...DEFAULT_RULE_SET.body], + body: [...body, ...DEFAULT_RULE_SET.body], }; } else { // tableId of '*' cannot list particular columns. @@ -341,6 +383,11 @@ export interface ReadAclOptions { // 1. They would show in the UI // 2. They would be saved back after editing, causing them to accumulate includeHelperCols?: boolean; + + // If true, rules with 'schemaEdit' permission are moved out of the '*:*' resource into a + // fictitious '*SPECIAL:SchemaEdit' resource. This is used only on the client, to present + // schemaEdit as a separate checkbox. Such rules are saved back to the '*:*' resource. + pullOutSchemaEdit?: boolean; } export interface ReadAclResults { @@ -474,3 +521,37 @@ function readAclRules(docData: DocData, {log, compile, includeHelperCols}: ReadA } return {ruleSets, userAttributes}; } + + +/** + * In the UI, we present SchemaEdit rules in a separate section, even though in reality they live + * as schemaEdit permission bits among the rules for the default resource. This function splits a + * RulePart into two: one containing the schemaEdit permission bit, and the other containing the + * other bits. If either part is empty, it will be returned as undefined, but if both are empty, + * nonSchemaEdit will be included as a rule with empty permission bits. + * + * It's possible for both parts to be non-empty (for rules created before the updated UI), in + * which case the schemaEdit one will have a fake origRecord, to cause it to be saved as a new + * record when saving. + */ +function splitSchemaEditRulePart(rulePart: RulePart): {schemaEdit?: RulePart, nonSchemaEdit?: RulePart} { + const p = splitSchemaEditPermissionSet(rulePart.permissions); + let schemaEdit: RulePart|undefined; + let nonSchemaEdit: RulePart|undefined; + if (p.schemaEdit) { + schemaEdit = {...rulePart, + permissions: p.schemaEdit, + permissionsText: permissionSetToText(p.schemaEdit), + }; + } + if (p.nonSchemaEdit) { + nonSchemaEdit = {...rulePart, + permissions: p.nonSchemaEdit, + permissionsText: permissionSetToText(p.nonSchemaEdit), + }; + } + if (schemaEdit && nonSchemaEdit) { + schemaEdit.origRecord = {id: -1} as MetaRowRecord<'_grist_ACLRules'>; + } + return {schemaEdit, nonSchemaEdit}; +} diff --git a/test/nbrowser/gristUtils.ts b/test/nbrowser/gristUtils.ts index 26e412e5..d8d07f14 100644 --- a/test/nbrowser/gristUtils.ts +++ b/test/nbrowser/gristUtils.ts @@ -121,10 +121,12 @@ export function startsWith(value: string): RegExp { } /** - * Helper to scroll an element into view. + * Helper to scroll an element into view. Returns the passed-in element. */ -export function scrollIntoView(elem: WebElement): Promise { - return driver.executeScript((el: any) => el.scrollIntoView({behavior: 'auto'}), elem); +export function scrollIntoView(elem: WebElement): WebElementPromise { + return new WebElementPromise(driver, + driver.executeScript((el: any) => el.scrollIntoView({behavior: 'auto'}), elem) + .then(() => elem)); } /**