From a1a84d99c0e06ed2eb52b67cf56d808fa6d9dc9e Mon Sep 17 00:00:00 2001 From: Paul Fitzpatrick Date: Wed, 10 Mar 2021 09:08:46 -0500 Subject: [PATCH] (core) alert user if they try to use rec in a column rule controlling read permission Summary: This particular combination of features is not built out - data will be censored but changes to data will not. So the user will now get an error if they try to do it. Existing rules of this kind will continue to operate as before, and can be set via the api. Test Plan: added test Reviewers: dsagal Reviewed By: dsagal Differential Revision: https://phab.getgrist.com/D2751 --- app/client/aclui/AccessRules.ts | 48 ++++++++++++++++++++++++--- app/client/aclui/PermissionsWidget.ts | 16 +++++---- app/common/ActiveDocAPI.ts | 3 +- app/common/GranularAccessClause.ts | 30 +++++++++++++++++ app/server/lib/ActiveDoc.ts | 6 ++-- 5 files changed, 90 insertions(+), 13 deletions(-) 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;