diff --git a/app/client/aclui/AccessRules.ts b/app/client/aclui/AccessRules.ts index 81709ee5..3078a79c 100644 --- a/app/client/aclui/AccessRules.ts +++ b/app/client/aclui/AccessRules.ts @@ -18,8 +18,9 @@ import {IOptionFull, menu, menuItemAsync} from 'app/client/ui2018/menus'; import {emptyPermissionSet, MixedPermissionValue} from 'app/common/ACLPermissions'; import {PartialPermissionSet, permissionSetToText, summarizePermissions, summarizePermissionSet} from 'app/common/ACLPermissions'; import {ACLRuleCollection} from 'app/common/ACLRuleCollection'; +import { ApiError } from 'app/common/ApiError'; import {BulkColValues, RowRecord, UserAction} from 'app/common/DocActions'; -import {RulePart, RuleSet, UserAttributeRule} from 'app/common/GranularAccessClause'; +import {FormulaProperties, getFormulaProperties, RulePart, RuleSet, UserAttributeRule} from 'app/common/GranularAccessClause'; import {isHiddenCol} from 'app/common/gristTypes'; import {isObject} from 'app/common/gutil'; import {SchemaTypes} from 'app/common/schema'; @@ -367,10 +368,11 @@ export class AccessRules extends Disposable { removeItem(this._userAttrRules, userAttr); } - public async checkAclFormula(text: string): Promise { + public async checkAclFormula(text: string): Promise { if (text) { return this._gristDoc.docComm.checkAclFormula(text); } + return {}; } // Check if the given tableId, and optionally a list of colIds, are present in this document. @@ -688,6 +690,13 @@ abstract class ObsRuleSet extends Disposable { const tableId = this._tableRules?.tableId; return (tableId && this.accessRules.getValidColIds(tableId)) || []; } + + /** + * Check if this rule set is limited to a set of columns. + */ + public hasColumns() { + return false; + } } class ColumnObsRuleSet extends ObsRuleSet { @@ -730,6 +739,10 @@ class ColumnObsRuleSet extends ObsRuleSet { // Create/Delete bits can't be set on a column-specific rule. return ['read', 'update']; } + + public hasColumns() { + return true; + } } class DefaultObsRuleSet extends ObsRuleSet { @@ -887,6 +900,8 @@ class ObsRulePart extends Disposable { // If the formula failed validation, the error message to show. Blank if valid. private _formulaError = Observable.create(this, ''); + private _formulaProperties = Observable.create(this, getAclFormulaProperties(this._rulePart)); + // Error message if any validation failed. private _error: Computed; @@ -932,6 +947,25 @@ class ObsRulePart extends Disposable { return summarizePermissionSet(this._permissions.get()); } + /** + * Verify that the rule is in a good state, optionally given a proposed permission change. + */ + public sanityCheck(pset?: PartialPermissionSet) { + if (this._ruleSet.hasColumns() && + (pset || this._permissions.get()).read && + this._formulaProperties.get().hasRecOrNewRec) { + if (pset && pset.read && this._permissions.get().read) { + // We don't want users to set either allow or deny read permissions in + // row-dependent rules, but if such a permission is set, allow it to be + // toggled to enable the deny->allow->unset cycling in the UI. + return; + } + throw new ApiError('Control of the read permission for column rules is unavailable ' + + 'when the formula uses the rec variable. ' + + 'Sorry! We will get to it, promise.', 400); + } + } + public buildRulePartDom() { return cssColumnGroup( cssCellIcon( @@ -961,7 +995,7 @@ class ObsRulePart extends Disposable { ), cssCell1(cssCell.cls('-stretch'), permissionsWidget(this._ruleSet.getAvailableBits(), this._permissions, - {disabled: this.isBuiltIn()}, + {disabled: this.isBuiltIn(), sanityCheck: (pset) => this.sanityCheck(pset)}, testId('rule-permissions') ), ), @@ -993,7 +1027,8 @@ class ObsRulePart extends Disposable { this._checkPending.set(true); this._formulaError.set(''); try { - await this._ruleSet.accessRules.checkAclFormula(text); + this._formulaProperties.set(await this._ruleSet.accessRules.checkAclFormula(text)); + this.sanityCheck(); } catch (e) { this._formulaError.set(e.message); } finally { @@ -1117,6 +1152,11 @@ function getChangedStatus(value: boolean): RuleStatus { return value ? RuleStatus.ChangedValid : RuleStatus.Unchanged; } +function getAclFormulaProperties(part?: RulePart): FormulaProperties { + const aclFormulaParsed = part?.origRecord?.aclFormulaParsed; + return aclFormulaParsed ? getFormulaProperties(JSON.parse(String(aclFormulaParsed))) : {}; +} + const cssOuter = styled('div', ` flex: auto; height: 100%; diff --git a/app/client/aclui/PermissionsWidget.ts b/app/client/aclui/PermissionsWidget.ts index 4cb157a4..88dbe2e8 100644 --- a/app/client/aclui/PermissionsWidget.ts +++ b/app/client/aclui/PermissionsWidget.ts @@ -20,7 +20,7 @@ export type PermissionKey = keyof PartialPermissionSet; export function permissionsWidget( availableBits: PermissionKey[], pset: Observable, - options: {disabled: boolean}, + options: {disabled: boolean, sanityCheck?: (p: PartialPermissionSet) => void}, ...args: DomElementArg[] ) { // These are the permission sets available to set via the dropdown. @@ -28,6 +28,10 @@ export function permissionsWidget( const allowAll: PartialPermissionSet = makePermissionSet(availableBits, () => 'allow'); const denyAll: PartialPermissionSet = makePermissionSet(availableBits, () => 'deny'); const readOnly: PartialPermissionSet = makePermissionSet(availableBits, (b) => b === 'read' ? 'allow' : 'deny'); + const setPermissions = (p: PartialPermissionSet) => { + options.sanityCheck?.(p); + pset.set(p); + }; return cssPermissions( dom.forEach(availableBits, (bit) => { @@ -38,7 +42,7 @@ export function permissionsWidget( dom.cls('disabled', options.disabled), // Cycle the bit's value on click, unless disabled. (options.disabled ? null : - dom.on('click', () => pset.set({...pset.get(), [bit]: next(pset.get()[bit])})) + dom.on('click', () => setPermissions({...pset.get(), [bit]: next(pset.get()[bit])})) ) ); }), @@ -57,16 +61,16 @@ export function permissionsWidget( null ), // If the set matches any recognized pattern, mark that item with a tick (checkmark). - cssMenuItem(() => pset.set(allowAll), tick(isEqual(pset.get(), allowAll)), 'Allow All', + cssMenuItem(() => setPermissions(allowAll), tick(isEqual(pset.get(), allowAll)), 'Allow All', dom.cls('disabled', options.disabled) ), - cssMenuItem(() => pset.set(denyAll), tick(isEqual(pset.get(), denyAll)), 'Deny All', + cssMenuItem(() => setPermissions(denyAll), tick(isEqual(pset.get(), denyAll)), 'Deny All', dom.cls('disabled', options.disabled) ), - cssMenuItem(() => pset.set(readOnly), tick(isEqual(pset.get(), readOnly)), 'Read Only', + cssMenuItem(() => setPermissions(readOnly), tick(isEqual(pset.get(), readOnly)), 'Read Only', dom.cls('disabled', options.disabled) ), - cssMenuItem(() => pset.set(empty), + cssMenuItem(() => setPermissions(empty), // For the empty permission set, it seems clearer to describe it as "No Effect", but to // all it "Clear" when offering to the user as the action. isEqual(pset.get(), empty) ? [tick(true), 'No Effect'] : [tick(false), 'Clear'], diff --git a/app/common/ActiveDocAPI.ts b/app/common/ActiveDocAPI.ts index cbc3b740..2fb484cb 100644 --- a/app/common/ActiveDocAPI.ts +++ b/app/common/ActiveDocAPI.ts @@ -1,5 +1,6 @@ import {ActionGroup} from 'app/common/ActionGroup'; import {CellValue, TableDataAction, UserAction} from 'app/common/DocActions'; +import {FormulaProperties} from 'app/common/GranularAccessClause'; import {Peer} from 'app/common/sharing'; import {UploadResult} from 'app/common/uploads'; import {ParseOptions} from 'app/plugin/FileParserAPI'; @@ -226,7 +227,7 @@ export interface ActiveDocAPI { /** * Check if an ACL formula is valid. If not, will throw an error with an explanation. */ - checkAclFormula(text: string): Promise; + checkAclFormula(text: string): Promise; /** * Returns the full set of tableIds, with the list of colIds for each table. This is intended diff --git a/app/common/GranularAccessClause.ts b/app/common/GranularAccessClause.ts index 4e320fe3..cb4e1f87 100644 --- a/app/common/GranularAccessClause.ts +++ b/app/common/GranularAccessClause.ts @@ -57,6 +57,13 @@ export type AclMatchFunc = (input: AclMatchInput) => boolean; */ export type ParsedAclFormula = [string, ...Array]; +/** + * Observations about a formula. + */ +export interface FormulaProperties { + hasRecOrNewRec?: boolean; +} + export interface UserAttributeRule { origRecord?: RowRecord; // Original record used to create this UserAttributeRule. name: string; // Should be unique among UserAttributeRules. @@ -64,3 +71,26 @@ export interface UserAttributeRule { lookupColId: string; // Column in tableId in which to do the lookup. charId: string; // Attribute to look up, possibly a path. E.g. 'Email' or 'office.city'. } + +/** + * Check some key facts about the formula. + */ +export function getFormulaProperties(formula: ParsedAclFormula) { + const result: FormulaProperties = {} + if (usesRec(formula)) { result.hasRecOrNewRec = true; } + return result; +} + +/** + * Check whether a formula mentions `rec` or `newRec`. + */ +export function usesRec(formula: ParsedAclFormula): boolean { + if (!Array.isArray(formula)) { throw new Error('expected a list'); } + if (formula[0] === 'Name' && (formula[1] === 'rec' || formula[1] === 'newRec')) { + return true; + } + return formula.some(el => { + if (!Array.isArray(el)) { return false; } + return usesRec(el as any); + }); +} diff --git a/app/server/lib/ActiveDoc.ts b/app/server/lib/ActiveDoc.ts index 9cb51601..e70edd46 100644 --- a/app/server/lib/ActiveDoc.ts +++ b/app/server/lib/ActiveDoc.ts @@ -30,6 +30,7 @@ import {toTableDataAction} from 'app/common/DocActions'; import {DocData} from 'app/common/DocData'; import {DocSnapshots} from 'app/common/DocSnapshot'; import {EncActionBundleFromHub} from 'app/common/EncActionBundle'; +import {FormulaProperties, getFormulaProperties} from 'app/common/GranularAccessClause'; import {byteString, countIf} from 'app/common/gutil'; import {InactivityTimer} from 'app/common/InactivityTimer'; import * as marshal from 'app/common/marshal'; @@ -933,15 +934,16 @@ export class ActiveDoc extends EventEmitter { /** * Check if an ACL formula is valid. If not, will throw an error with an explanation. */ - public async checkAclFormula(docSession: DocSession, text: string): Promise { + public async checkAclFormula(docSession: DocSession, text: string): Promise { // Checks can leak names of tables and columns. - if (await this._granularAccess.hasNuancedAccess(docSession)) { return; } + if (await this._granularAccess.hasNuancedAccess(docSession)) { return {}; } await this.waitForInitialization(); try { const parsedAclFormula = await this._pyCall('parse_acl_formula', text); compileAclFormula(parsedAclFormula); // TODO We also need to check the validity of attributes, and of tables and columns // mentioned in resources and userAttribute rules. + return getFormulaProperties(parsedAclFormula); } catch (e) { e.message = e.message?.replace('[Sandbox] ', ''); throw e;