From de35be6b0a51d31d01bfb4057c271de2044e4c5d Mon Sep 17 00:00:00 2001 From: Dmitry S Date: Mon, 14 Dec 2020 23:19:38 -0500 Subject: [PATCH] (core) Checks that an ACL formula can be parsed, and prevent saving unparsable ACL rules. Summary: - Fix error-handling in bundleActions(), and wait for the full bundle to complete. (The omissions here were making it impossibly to react to errors from inside bundleActions()) - Catch problematic rules early enough to undo them, by trying out ruleCollection.update() on updated rules before the updates are applied. - Added checkAclFormula() call to DocComm that checks parsing and compiling formula, and reports errors. - In UI, prevent saving if any aclFormulas are invalid, or while waiting for the to get checked. - Also fixed some lint errors Test Plan: Added a test case of error reporting in ACL formulas. Reviewers: paulfitz Reviewed By: paulfitz Differential Revision: https://phab.getgrist.com/D2689 --- app/client/components/DocComm.ts | 1 + app/client/models/DocData.ts | 15 +- app/client/ui/AccessRules.ts | 233 +++++++++++++++++++++---------- app/common/ACLRuleCollection.ts | 10 +- app/common/ActiveDocAPI.ts | 5 + app/server/lib/ActiveDoc.ts | 19 +++ app/server/lib/DocWorker.ts | 1 + app/server/lib/GranularAccess.ts | 52 +++++-- app/server/lib/Sharing.ts | 2 +- sandbox/grist/main.py | 2 + 10 files changed, 241 insertions(+), 99 deletions(-) diff --git a/app/client/components/DocComm.ts b/app/client/components/DocComm.ts index 64d25921..9ff2797b 100644 --- a/app/client/components/DocComm.ts +++ b/app/client/components/DocComm.ts @@ -53,6 +53,7 @@ export class DocComm extends Disposable implements ActiveDocAPI { public reloadPlugins = this._wrapMethod("reloadPlugins"); public reloadDoc = this._wrapMethod("reloadDoc"); public fork = this._wrapMethod("fork"); + public checkAclFormula = this._wrapMethod("checkAclFormula"); public changeUrlIdEmitter = this.autoDispose(new Emitter()); diff --git a/app/client/models/DocData.ts b/app/client/models/DocData.ts index a409696e..b1fb06d2 100644 --- a/app/client/models/DocData.ts +++ b/app/client/models/DocData.ts @@ -83,7 +83,7 @@ export class DocData extends BaseDocData { this._bundlesPending++; // Promise to allow waiting for the result of prepare() callback before it's even called. - let prepareResolve!: (value: T) => void; + let prepareResolve!: (value: T|Promise) => void; const preparePromise = new Promise(resolve => { prepareResolve = resolve; }); // Manually-triggered promise for when finalize() should be called. It's triggered by user, @@ -100,8 +100,7 @@ export class DocData extends BaseDocData { this._nextDesc = options.description; this._lastActionNum = null; this._triggerBundleFinalize = triggerFinalize; - const value = await options.prepare(); - prepareResolve(value); + await prepareResolve(options.prepare()); this._shouldIncludeInBundle = options.shouldIncludeInBundle; await triggerFinalizePromise; @@ -123,8 +122,8 @@ export class DocData extends BaseDocData { } }; - this._lastBundlePromise = doBundleActions(); - return {preparePromise, triggerFinalize}; + const completionPromise = this._lastBundlePromise = doBundleActions(); + return {preparePromise, triggerFinalize, completionPromise}; } // Execute a callback that may send multiple actions, and bundle those actions together. The @@ -146,6 +145,7 @@ export class DocData extends BaseDocData { return await bundlingInfo.preparePromise; } finally { bundlingInfo.triggerFinalize(); + await bundlingInfo.completionPromise; } } @@ -261,7 +261,7 @@ export interface BundlingOptions { /** * Result of startBundlingActions(), to allow waiting for prepare() to complete, and to trigger - * finalize() manually. + * finalize() manually, and to wait for the full bundle to complete. */ export interface BundlingInfo { // Promise for when the prepare() has completed. Note that sometimes it's delayed until the @@ -270,4 +270,7 @@ export interface BundlingInfo { // Ask DocData to call the finalize callback immediately. triggerFinalize: () => void; + + // Promise for when the bundle has been finalized. + completionPromise: Promise; } diff --git a/app/client/ui/AccessRules.ts b/app/client/ui/AccessRules.ts index 439b0dc1..40001110 100644 --- a/app/client/ui/AccessRules.ts +++ b/app/client/ui/AccessRules.ts @@ -29,14 +29,23 @@ type RuleRec = Partial & {id?: number, resourceR type UseCB = (obs: BaseObservable) => T; +// Status of rules, which determines whether the "Save" button is enabled. The order of the values +// matters, as we take the max of all the parts to determine the ultimate status. +enum RuleStatus { + Unchanged, + ChangedValid, + Invalid, + CheckPending, +} + /** * Top-most container managing state and dom-building for the ACL rule UI. */ export class AccessRules extends Disposable { // Whether anything has changed, i.e. whether to show a "Save" button. - public isAnythingChanged: Computed; + private _ruleStatus: Computed; - // Parsed rules obtained from DocData during last call to update(). Used for isAnythingChanged. + // Parsed rules obtained from DocData during last call to update(). Used for _ruleStatus. private _ruleCollection = new ACLRuleCollection(); // Array of all per-table rules. @@ -51,20 +60,28 @@ export class AccessRules extends Disposable { // Array of all UserAttribute rules. private _userAttrRules = this.autoDispose(obsArray()); + // Whether the save button should be enabled. + private _savingEnabled: Computed; + constructor(private _gristDoc: GristDoc) { super(); - this.isAnythingChanged = Computed.create(this, (use) => { + this._ruleStatus = Computed.create(this, (use) => { const defRuleSet = use(this._docDefaultRuleSet); const tableRules = use(this._tableRules); const userAttr = use(this._userAttrRules); - return (defRuleSet && use(defRuleSet.isChanged)) || - // If any table was changed or added, some t.isChanged will be set. If there were only - // removals, then tableRules.length will have changed. - tableRules.length !== this._ruleCollection.getAllTableIds().length || - tableRules.some(t => use(t.isChanged)) || - userAttr.length !== this._ruleCollection.getUserAttributeRules().size || - userAttr.some(u => use(u.isChanged)); + return Math.max( + defRuleSet ? use(defRuleSet.ruleStatus) : RuleStatus.Unchanged, + // If any tables/userAttrs were changed or added, they will be considered changed. If + // there were only removals, then length will be reduced. + getChangedStatus(tableRules.length < this._ruleCollection.getAllTableIds().length), + getChangedStatus(userAttr.length < this._ruleCollection.getUserAttributeRules().size), + ...tableRules.map(t => use(t.ruleStatus)), + ...userAttr.map(u => use(u.ruleStatus)), + ); }); + + this._savingEnabled = Computed.create(this, this._ruleStatus, (use, s) => (s === RuleStatus.ChangedValid)); + this.update().catch(reportError); } @@ -78,7 +95,7 @@ export class AccessRules extends Disposable { rules.getAllTableIds().map(tableId => TableRules.create(this._tableRules, tableId, this, rules.getAllColumnRuleSets(tableId), rules.getTableDefaultRuleSet(tableId))) ); - DefaultObsRuleSet.create(this._docDefaultRuleSet, null, undefined, rules.getDocDefaultRuleSet()); + DefaultObsRuleSet.create(this._docDefaultRuleSet, this, null, undefined, rules.getDocDefaultRuleSet()); this._userAttrRules.set( Array.from(rules.getUserAttributeRules().values(), userAttr => ObsUserAttributeRule.create(this._userAttrRules, this, userAttr)) @@ -89,7 +106,7 @@ export class AccessRules extends Disposable { * Collect the internal state into records and sync them to the document. */ public async save(): Promise { - if (!this.isAnythingChanged.get()) { return; } + if (!this._savingEnabled.get()) { return; } // Note that if anything has changed, we apply changes relative to the current state of the // ACL tables (they may have changed by other users). So our changes will win. @@ -169,6 +186,11 @@ export class AccessRules extends Disposable { } // Finally we can sync the records. await syncRecords(rulesTable, newRules); + }).catch(e => { + // Report the error, but go on to update the rules. The user may lose their entries, but + // will see what's in the document. To preserve entries and show what's wrong, we try to + // catch errors earlier. + reportError(e); }); // Re-populate the state from DocData once the records are synced. @@ -178,12 +200,19 @@ export class AccessRules extends Disposable { public buildDom() { return [ cssAddTableRow( - bigBasicButton('Saved', {disabled: true}, dom.hide(this.isAnythingChanged)), - bigPrimaryButton('Save', dom.show(this.isAnythingChanged), + bigBasicButton({disabled: true}, dom.hide(this._savingEnabled), + dom.text((use) => { + const s = use(this._ruleStatus); + return s === RuleStatus.CheckPending ? 'Checking...' : + s === RuleStatus.Invalid ? 'Invalid' : 'Saved'; + }), + testId('rules-non-save') + ), + bigPrimaryButton('Save', dom.show(this._savingEnabled), dom.on('click', () => this.save()), testId('rules-save'), ), - bigBasicButton('Revert', dom.show(this.isAnythingChanged), + bigBasicButton('Revert', dom.show(this._savingEnabled), dom.on('click', () => this.update()), testId('rules-revert'), ), @@ -240,6 +269,12 @@ export class AccessRules extends Disposable { removeItem(this._userAttrRules, userAttr); } + public async checkAclFormula(text: string): Promise { + if (text) { + return this._gristDoc.docComm.checkAclFormula(text); + } + } + private _addTableRules(tableId: string) { if (this._tableRules.get().some(t => t.tableId === tableId)) { throw new Error(`Trying to add TableRules for existing table ${tableId}`); @@ -255,8 +290,8 @@ export class AccessRules extends Disposable { // Represents all rules for a table. class TableRules extends Disposable { - // Whether any table rules changed. Always true if this._colRuleSets is undefined. - public isChanged: Computed; + // Whether any table rules changed, and if they are valid. + public ruleStatus: Computed; // The column-specific rule sets. private _columnRuleSets = this.autoDispose(obsArray()); @@ -267,29 +302,32 @@ class TableRules extends Disposable { // The default rule set (for columns '*'), if one is set. private _defaultRuleSet = Observable.create(this, null); - constructor(public readonly tableId: string, private _accessRules: AccessRules, + constructor(public readonly tableId: string, public _accessRules: AccessRules, private _colRuleSets?: RuleSet[], private _defRuleSet?: RuleSet) { super(); this._columnRuleSets.set(this._colRuleSets?.map(rs => - ColumnObsRuleSet.create(this._columnRuleSets, this, rs, rs.colIds === '*' ? [] : rs.colIds)) || []); + ColumnObsRuleSet.create(this._columnRuleSets, this._accessRules, this, rs, + rs.colIds === '*' ? [] : rs.colIds)) || []); if (!this._colRuleSets) { // Must be a newly-created TableRules object. Just create a default RuleSet (for tableId:*) - DefaultObsRuleSet.create(this._defaultRuleSet, this, this._haveColumnRules); + DefaultObsRuleSet.create(this._defaultRuleSet, this._accessRules, this, this._haveColumnRules); } else if (this._defRuleSet) { - DefaultObsRuleSet.create(this._defaultRuleSet, this, this._haveColumnRules, this._defRuleSet); + DefaultObsRuleSet.create(this._defaultRuleSet, this._accessRules, this, this._haveColumnRules, + this._defRuleSet); } - this.isChanged = Computed.create(this, (use) => { - if (!this._colRuleSets) { return true; } // This TableRules object must be newly-added + this.ruleStatus = Computed.create(this, (use) => { const columnRuleSets = use(this._columnRuleSets); const d = use(this._defaultRuleSet); - return ( - Boolean(d) !== Boolean(this._defRuleSet) || // Default rule set got added or removed - (d && use(d.isChanged)) || // Or changed - columnRuleSets.length < this._colRuleSets.length || // There was a removal - columnRuleSets.some(rs => use(rs.isChanged)) // There was an addition or a change. - ); + return Math.max( + getChangedStatus( + !this._colRuleSets || // This TableRules object must be newly-added + Boolean(d) !== Boolean(this._defRuleSet) || // Default rule set got added or removed + columnRuleSets.length < this._colRuleSets.length // There was a removal + ), + d ? use(d.ruleStatus) : RuleStatus.Unchanged, // Default rule set got changed. + ...columnRuleSets.map(rs => use(rs.ruleStatus))); // Column rule set was added or changed. }); } @@ -364,12 +402,12 @@ class TableRules extends Disposable { } private _addColumnRuleSet() { - this._columnRuleSets.push(ColumnObsRuleSet.create(this._columnRuleSets, this, undefined, [])); + this._columnRuleSets.push(ColumnObsRuleSet.create(this._columnRuleSets, this._accessRules, this, undefined, [])); } private _addDefaultRuleSet() { if (!this._defaultRuleSet.get()) { - DefaultObsRuleSet.create(this._defaultRuleSet, this, this._haveColumnRules); + DefaultObsRuleSet.create(this._defaultRuleSet, this._accessRules, this, this._haveColumnRules); } } } @@ -377,8 +415,8 @@ class TableRules extends Disposable { // Represents one RuleSet, for a combination of columns in one table, or the default RuleSet for // all remaining columns in a table. abstract class ObsRuleSet extends Disposable { - // Whether rules changed. Always true if this._ruleSet is undefined. - public isChanged: Computed; + // Whether rules changed, and if they are valid. Never unchanged if this._ruleSet is undefined. + public ruleStatus: Computed; // Whether the rule set includes any conditions besides the default rule. public haveConditions: Computed; @@ -388,7 +426,7 @@ abstract class ObsRuleSet extends Disposable { private _body = this.autoDispose(obsArray()); // ruleSet is omitted for a new ObsRuleSet added by the user. - constructor(private _tableRules: TableRules|null, private _ruleSet?: RuleSet) { + constructor(public accessRules: AccessRules, private _tableRules: TableRules|null, private _ruleSet?: RuleSet) { super(); if (this._ruleSet) { this._body.set(this._ruleSet.body.map(part => ObsRulePart.create(this._body, this, part))); @@ -397,11 +435,12 @@ abstract class ObsRuleSet extends Disposable { this._body.set([ObsRulePart.create(this._body, this, undefined, true)]); } - this.isChanged = Computed.create(this, this._body, (use, body) => { - // If anything was changed or added, some part.isChanged will be set. If there were only - // removals, then body.length will have changed. - return (body.length !== (this._ruleSet?.body?.length || 0) || - body.some(part => use(part.isChanged))); + this.ruleStatus = Computed.create(this, this._body, (use, body) => { + // If anything was changed or added, some part.ruleStatus will be other than Unchanged. If + // there were only removals, then body.length will have changed. + return Math.max( + getChangedStatus(body.length < (this._ruleSet?.body?.length || 0)), + ...body.map(part => use(part.ruleStatus))); }); this.haveConditions = Computed.create(this, this._body, (use, body) => body.some(p => !p.isDefault)); @@ -469,11 +508,14 @@ class ColumnObsRuleSet extends ObsRuleSet { private _colIds = Observable.create(this, this._initialColIds); private _colIdStr = Computed.create(this, (use) => use(this._colIds).join(", ")); - constructor(tableRules: TableRules, ruleSet: RuleSet|undefined, private _initialColIds: string[]) { - super(tableRules, ruleSet); - const baseIsChanged = this.isChanged; - this.isChanged = Computed.create(this, (use) => - !isEqual(use(this._colIds), this._initialColIds) || use(baseIsChanged)); + constructor(accessRules: AccessRules, tableRules: TableRules, ruleSet: RuleSet|undefined, + private _initialColIds: string[]) { + super(accessRules, tableRules, ruleSet); + const baseRuleStatus = this.ruleStatus; + this.ruleStatus = Computed.create(this, (use) => Math.max( + getChangedStatus(!isEqual(use(this._colIds), this._initialColIds)), + use(baseRuleStatus) + )); } public buildDom() { @@ -499,8 +541,9 @@ class ColumnObsRuleSet extends ObsRuleSet { } class DefaultObsRuleSet extends ObsRuleSet { - constructor(tableRules: TableRules|null, private _haveColumnRules?: Observable, ruleSet?: RuleSet) { - super(tableRules, ruleSet); + constructor(accessRules: AccessRules, tableRules: TableRules|null, + private _haveColumnRules?: Observable, ruleSet?: RuleSet) { + super(accessRules, tableRules, ruleSet); } public buildDom() { return cssRuleSet( @@ -515,7 +558,7 @@ class DefaultObsRuleSet extends ObsRuleSet { } class ObsUserAttributeRule extends Disposable { - public isChanged: Computed; + public ruleStatus: Computed; private _name = Observable.create(this, this._userAttr?.name || ''); private _tableId = Observable.create(this, this._userAttr?.tableId || ''); @@ -524,12 +567,13 @@ class ObsUserAttributeRule extends Disposable { constructor(private _accessRules: AccessRules, private _userAttr?: UserAttributeRule) { super(); - this.isChanged = Computed.create(this, use => ( - use(this._name) !== this._userAttr?.name || - use(this._tableId) !== this._userAttr?.tableId || - use(this._lookupColId) !== this._userAttr?.lookupColId || - use(this._charId) !== this._userAttr?.charId - )); + this.ruleStatus = Computed.create(this, use => + getChangedStatus( + use(this._name) !== this._userAttr?.name || + use(this._tableId) !== this._userAttr?.tableId || + use(this._lookupColId) !== this._userAttr?.lookupColId || + use(this._charId) !== this._userAttr?.charId + )); } public buildDom() { @@ -570,8 +614,8 @@ class ObsUserAttributeRule extends Disposable { // Represents one line of a RuleSet, a combination of an aclFormula and permissions to apply to // requests that match it. class ObsRulePart extends Disposable { - // Whether rules changed. Always true if this._rulePart is undefined. - public isChanged: Computed; + // Whether the rule part, and if it's valid or being checked. + public ruleStatus: Computed; // Formula to show in the "advanced" UI. private _aclFormula = Observable.create(this, this._rulePart?.aclFormula || ""); @@ -582,13 +626,23 @@ class ObsRulePart extends Disposable { private _permissionsText = Computed.create(this, this._permissions, (use, p) => permissionSetToText(p)); + // Whether the rule is being checked after a change. Saving will wait for such checks to finish. + private _checkPending = Observable.create(this, false); + + // If the formula failed validation, the error message to show. Blank if valid. + private _formulaError = Observable.create(this, ''); + // rulePart is omitted for a new ObsRulePart added by the user. constructor(private _ruleSet: ObsRuleSet, private _rulePart?: RulePart, public readonly isDefault: boolean = (_rulePart?.aclFormula === '')) { super(); - this.isChanged = Computed.create(this, (use) => { - return (use(this._aclFormula) !== this._rulePart?.aclFormula || - !isEqual(use(this._permissions), this._rulePart?.permissions)); + this.ruleStatus = Computed.create(this, (use) => { + if (use(this._formulaError)) { return RuleStatus.Invalid; } + if (use(this._checkPending)) { return RuleStatus.CheckPending; } + return getChangedStatus( + use(this._aclFormula) !== this._rulePart?.aclFormula || + !isEqual(use(this._permissions), this._rulePart?.permissions) + ); }); } @@ -612,17 +666,20 @@ class ObsRulePart extends Disposable { testId('rule-add'), ) ), - cssConditionInput( - this._aclFormula, async (text) => this._aclFormula.set(text), - dom.prop('disabled', this.isBuiltIn()), - dom.prop('placeholder', (use) => { - return ( - this._ruleSet.isSoleCondition(use, this) ? 'Everyone' : - this._ruleSet.isLastCondition(use, this) ? 'Everyone Else' : - 'Enter Condition' - ); - }), - testId('rule-acl-formula'), + cssCondition( + cssConditionInput( + this._aclFormula, this._setAclFormula.bind(this), + dom.prop('disabled', this.isBuiltIn()), + dom.prop('placeholder', (use) => { + return ( + this._ruleSet.isSoleCondition(use, this) ? 'Everyone' : + this._ruleSet.isLastCondition(use, this) ? 'Everyone Else' : + 'Enter Condition' + ); + }), + testId('rule-acl-formula'), + ), + dom.maybe(this._formulaError, (msg) => cssConditionError(msg, testId('rule-error'))), ), cssPermissionsInput( this._permissionsText, async (p) => this._permissions.set(parsePermissions(p)), @@ -647,6 +704,19 @@ class ObsRulePart extends Disposable { private _isNonFirstBuiltIn(): boolean { return this.isBuiltIn() && this._ruleSet.getFirstBuiltIn() !== this; } + + private async _setAclFormula(text: string) { + this._aclFormula.set(text); + this._checkPending.set(true); + this._formulaError.set(''); + try { + await this._ruleSet.accessRules.checkAclFormula(text); + } catch (e) { + this._formulaError.set(e.message); + } finally { + this._checkPending.set(false); + } + } } @@ -760,6 +830,10 @@ function removeItem(observableArray: MutableObsArray, item: T): boolean { return false; } +function getChangedStatus(value: boolean): RuleStatus { + return value ? RuleStatus.ChangedValid : RuleStatus.Unchanged; +} + const cssAddTableRow = styled('div', ` margin: 16px 64px 0 64px; display: flex; @@ -815,24 +889,28 @@ const cssRuleSetBody = styled('div', ` const cssRulePart = styled('div', ` display: flex; - align-items: center; + align-items: start; margin: 4px 0; - &-default { - margin-top: auto; - } +`); + +const cssCondition = styled('div', ` + min-width: 0; + flex: 1; + display: flex; + flex-direction: column; `); const cssConditionInput = styled(textInput, ` - display: flex; - min-width: 0; - flex: 1; - &[disabled] { background-color: ${colors.mediumGreyOpaque}; color: ${colors.dark}; } `); +const cssConditionError = styled('div', ` + color: ${colors.error}; +`); + const cssPermissionsInput = styled(cssConditionInput, ` margin-left: 8px; width: 64px; @@ -843,6 +921,7 @@ const cssIconSpace = styled('div', ` flex: none; height: 24px; width: 24px; + margin: 2px; `); const cssIconButton = styled(cssIconSpace, ` diff --git a/app/common/ACLRuleCollection.ts b/app/common/ACLRuleCollection.ts index 1255ebce..0d080803 100644 --- a/app/common/ACLRuleCollection.ts +++ b/app/common/ACLRuleCollection.ts @@ -49,6 +49,10 @@ const EMERGENCY_RULE_SET: RuleSet = { }; export class ACLRuleCollection { + // Store error if one occurs while reading rules. Rules are replaced with emergency rules + // in this case. + public ruleError: Error|undefined; + // In the absence of rules, some checks are skipped. For now this is important to maintain all // existing behavior. TODO should make sure checking access against default rules is equivalent // and efficient. @@ -72,10 +76,6 @@ export class ACLRuleCollection { // Maps name to the corresponding UserAttributeRule. private _userAttributeRules = new Map(); - // Store error if one occurs while reading rules. Rules are replaced with emergency rules - // in this case. - public ruleError: Error|undefined; - // Whether there are ANY user-defined rules. public haveRules(): boolean { return this._haveRules; @@ -170,7 +170,7 @@ export class ACLRuleCollection { try { this.ruleError = undefined; return readAclRules(docData, options); - } catch(e) { + } catch (e) { this.ruleError = e; // Report the error indirectly. return {ruleSets: [EMERGENCY_RULE_SET], userAttributes: []}; } diff --git a/app/common/ActiveDocAPI.ts b/app/common/ActiveDocAPI.ts index b7c29401..1184b30c 100644 --- a/app/common/ActiveDocAPI.ts +++ b/app/common/ActiveDocAPI.ts @@ -222,4 +222,9 @@ export interface ActiveDocAPI { * Prepare a fork of the document, and return the id(s) of the fork. */ fork(): Promise; + + /** + * Check if an ACL formula is valid. If not, will throw an error with an explanation. + */ + checkAclFormula(text: string): Promise; } diff --git a/app/server/lib/ActiveDoc.ts b/app/server/lib/ActiveDoc.ts index 1771ba86..831fc31b 100644 --- a/app/server/lib/ActiveDoc.ts +++ b/app/server/lib/ActiveDoc.ts @@ -36,6 +36,7 @@ import {UploadResult} from 'app/common/uploads'; import {DocReplacementOptions, DocState} from 'app/common/UserAPI'; import {ParseOptions} from 'app/plugin/FileParserAPI'; import {GristDocAPI} from 'app/plugin/GristAPI'; +import {compileAclFormula} from 'app/server/lib/ACLFormula'; import {Authorizer} from 'app/server/lib/Authorizer'; import {checksumFile} from 'app/server/lib/checksumFile'; import {Client} from 'app/server/lib/Client'; @@ -898,6 +899,24 @@ export class ActiveDoc extends EventEmitter { return makeForkIds({userId, isAnonymous, trunkDocId, trunkUrlId}); } + /** + * Check if an ACL formula is valid. If not, will throw an error with an explanation. + */ + public async checkAclFormula(docSession: DocSession, text: string): Promise { + // Checks can leak names of tables and columns. + if (!await this._granularAccess.canReadEverything(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. + } catch (e) { + e.message = e.message?.replace('[Sandbox] ', ''); + throw e; + } + } + public getGristDocAPI(): GristDocAPI { return this.docPluginManager.gristDocAPI; } diff --git a/app/server/lib/DocWorker.ts b/app/server/lib/DocWorker.ts index 11150094..0976f090 100644 --- a/app/server/lib/DocWorker.ts +++ b/app/server/lib/DocWorker.ts @@ -110,6 +110,7 @@ export class DocWorker { getActionSummaries: activeDocMethod.bind(null, 'viewers', 'getActionSummaries'), reloadDoc: activeDocMethod.bind(null, 'editors', 'reloadDoc'), fork: activeDocMethod.bind(null, 'viewers', 'fork'), + checkAclFormula: activeDocMethod.bind(null, 'viewers', 'checkAclFormula'), }); } diff --git a/app/server/lib/GranularAccess.ts b/app/server/lib/GranularAccess.ts index 33826848..48f5112e 100644 --- a/app/server/lib/GranularAccess.ts +++ b/app/server/lib/GranularAccess.ts @@ -5,9 +5,11 @@ import { ACLRuleCollection } from 'app/common/ACLRuleCollection'; import { ActionGroup } from 'app/common/ActionGroup'; import { createEmptyActionSummary } from 'app/common/ActionSummary'; import { Query } from 'app/common/ActiveDocAPI'; +import { ApiError } from 'app/common/ApiError'; import { AsyncCreate } from 'app/common/AsyncCreate'; -import { AddRecord, BulkAddRecord, BulkColValues, BulkRemoveRecord, BulkUpdateRecord, CellValue, - ColValues, DocAction, getTableId, isSchemaAction, RemoveRecord, ReplaceTableData, UpdateRecord } from 'app/common/DocActions'; +import { AddRecord, BulkAddRecord, BulkColValues, BulkRemoveRecord, BulkUpdateRecord } from 'app/common/DocActions'; +import { RemoveRecord, ReplaceTableData, UpdateRecord } from 'app/common/DocActions'; +import { CellValue, ColValues, DocAction, getTableId, isSchemaAction } from 'app/common/DocActions'; import { TableDataAction, UserAction } from 'app/common/DocActions'; import { DocData } from 'app/common/DocData'; import { ErrorWithCode } from 'app/common/ErrorWithCode'; @@ -109,7 +111,10 @@ export class GranularAccess { // Flag tracking whether a set of actions have been applied to the database or not. private _applied: boolean = false; - public constructor(private _docData: DocData, private _fetchQueryFromDB: (query: Query) => Promise, private _recoveryMode: boolean) { + public constructor( + private _docData: DocData, + private _fetchQueryFromDB: (query: Query) => Promise, + private _recoveryMode: boolean) { } /** @@ -145,10 +150,32 @@ export class GranularAccess { */ public async canApplyDocActions(docSession: OptDocSession, docActions: DocAction[], undo: DocAction[]) { this._applied = false; - if (!this._ruleCollection.haveRules()) { return; } - this._prepareRowSnapshots(docActions, undo); - await Promise.all( - docActions.map((action, idx) => this._checkIncomingDocAction(docSession, action, idx))); + if (this._ruleCollection.haveRules()) { + this._prepareRowSnapshots(docActions, undo); + await Promise.all( + docActions.map((action, idx) => this._checkIncomingDocAction(docSession, action, idx))); + } + + // If the actions change any rules, verify that we'll be able to handle the changed rules. If + // they are to cause an error, reject the action to avoid forcing user into recovery mode. + if (docActions.some(docAction => ['_grist_ACLRules', '_grist_Resources'].includes(getTableId(docAction)))) { + // Create a tmpDocData with just the tables we care about, then update docActions to it. + const tmpDocData: DocData = new DocData( + (tableId) => { throw new Error("Unexpected DocData fetch"); }, { + _grist_ACLResources: this._docData.getTable('_grist_ACLResources')!.getTableDataAction(), + _grist_ACLRules: this._docData.getTable('_grist_ACLRules')!.getTableDataAction(), + }); + for (const da of docActions) { + tmpDocData.receiveAction(da); + } + + // Use the post-actions data to process the rules collection, and throw error if that fails. + const ruleCollection = new ACLRuleCollection(); + await ruleCollection.update(tmpDocData, {log, compile: compileAclFormula}); + if (ruleCollection.ruleError && !this._recoveryMode) { + throw new ApiError(ruleCollection.ruleError.message, 400); + } + } } /** @@ -164,7 +191,8 @@ export class GranularAccess { this._applied = true; // If there is a rule change, redo from scratch for now. // TODO: this is placeholder code. Should deal with connected clients. - if (docActions.some(docAction => getTableId(docAction) === '_grist_ACLRules' || getTableId(docAction) === '_grist_Resources')) { + if (docActions.some(docAction => getTableId(docAction) === '_grist_ACLRules' || + getTableId(docAction) === '_grist_Resources')) { await this.update(); return; } @@ -656,7 +684,8 @@ export class GranularAccess { } // Compute which of the row ids supplied are for rows forbidden for this session. - private async _getForbiddenRows(docSession: OptDocSession, data: TableDataAction, ids: Set): Promise { + private async _getForbiddenRows(docSession: OptDocSession, data: TableDataAction, ids: Set): + Promise { const rec = new RecordView(data, undefined); const input: AclMatchInput = {user: await this._getUser(docSession), rec}; @@ -795,7 +824,10 @@ export class GranularAccess { try { // Use lodash's get() that supports paths, e.g. charId of 'a.b' would look up `user.a.b`. // TODO: add indexes to db. - rows = await this._fetchQueryFromDB({tableId: clause.tableId, filters: { [clause.lookupColId]: [get(user, clause.charId)] }}); + rows = await this._fetchQueryFromDB({ + tableId: clause.tableId, + filters: { [clause.lookupColId]: [get(user, clause.charId)] } + }); } catch (e) { log.warn(`User attribute ${clause.name} failed`, e); } diff --git a/app/server/lib/Sharing.ts b/app/server/lib/Sharing.ts index 1cc1d22e..fa30a1e7 100644 --- a/app/server/lib/Sharing.ts +++ b/app/server/lib/Sharing.ts @@ -19,7 +19,7 @@ import {WorkCoordinator} from './WorkCoordinator'; // processing hub actions or rebasing. interface UserRequest { action: UserActionBundle; - docSession: OptDocSession|null, + docSession: OptDocSession|null; resolve(result: UserResult): void; reject(err: Error): void; } diff --git a/sandbox/grist/main.py b/sandbox/grist/main.py index 16742c18..9a01652d 100644 --- a/sandbox/grist/main.py +++ b/sandbox/grist/main.py @@ -9,6 +9,7 @@ sys.path.append('thirdparty') import marshal import functools +from acl_formula import parse_acl_formula import actions import sandbox import engine @@ -110,6 +111,7 @@ def main(): def get_formula_error(table_id, col_id, row_id): return objtypes.encode_object(eng.get_formula_error(table_id, col_id, row_id)) + export(parse_acl_formula) export(eng.load_empty) export(eng.load_done)