diff --git a/app/client/components/Comm.ts b/app/client/components/Comm.ts index a81d71f0..1e3783a8 100644 --- a/app/client/components/Comm.ts +++ b/app/client/components/Comm.ts @@ -156,6 +156,9 @@ export interface CommResponseError { error: string; errorCode: string; shouldFork?: boolean; // if set, the server suggests forking the document. + details?: any; // if set, error has extra details available. TODO - the treatment of + // details could do with some harmonisation between rest API and ws API, + // and between front-end and back-end types. } function isCommResponseError(msg: CommResponse | CommResponseError): msg is CommResponseError { @@ -460,6 +463,9 @@ export class Comm extends dispose.Disposable implements GristServerAPI, DocListA code = ` [${message.errorCode}]`; err.code = message.errorCode; } + if (message.details) { + err.details = message.details; + } err.shouldFork = message.shouldFork; console.log(`Comm response #${reqId} ${r.methodName} ERROR:${code} ${message.error}` + (message.shouldFork ? ` (should fork)` : '')); diff --git a/app/client/models/NotifyModel.ts b/app/client/models/NotifyModel.ts index 1b5f085b..8451a83a 100644 --- a/app/client/models/NotifyModel.ts +++ b/app/client/models/NotifyModel.ts @@ -7,6 +7,7 @@ import {timeFormat} from 'app/common/timeFormat'; import {bundleChanges, Disposable, Holder, IDisposable, IDisposableOwner } from 'grainjs'; import {Computed, dom, DomElementArg, MutableObsArray, obsArray, Observable} from 'grainjs'; import clamp = require('lodash/clamp'); +import defaults = require('lodash/defaults'); // When rendering app errors, we'll only show the last few. const maxAppErrors = 5; @@ -45,6 +46,8 @@ export interface INotifyOptions { expireSec?: number; badgeCounter?: boolean; + memos?: string[]; // A list of relevant notes. + // cssToastAction class from NotifyUI will be applied automatically to action elements. actions?: NotifyAction[]; @@ -87,12 +90,13 @@ export class Notification extends Expirable implements INotification { expireSec: 0, canUserClose: false, actions: [], + memos: [], key: null, }; constructor(_opts: INotifyOptions) { super(); - Object.assign(this.options, _opts); + this.options = defaults({}, _opts, this.options) if (this.options.expireSec > 0) { const expireTimer = setTimeout(() => this.expire(), 1000 * this.options.expireSec); diff --git a/app/client/models/errors.ts b/app/client/models/errors.ts index a7c6ba99..cc0f0c31 100644 --- a/app/client/models/errors.ts +++ b/app/client/models/errors.ts @@ -84,7 +84,7 @@ export function reportError(err: Error|string): void { } else if (err.name === 'NeedUpgradeError') { _notifier.createUserError(err.message, {actions: ['upgrade'], key: 'NEED_UPGRADE'}); } else if (code === 'AUTH_NO_EDIT' || code === 'ACL_DENY') { - _notifier.createUserError(message, {key: code}); + _notifier.createUserError(err.message, {key: code, memos: details?.memos}); } else { // If we don't recognize it, consider it an application error (bug) that the user should be // able to report. diff --git a/app/client/ui/NotifyUI.ts b/app/client/ui/NotifyUI.ts index 000fc654..33f44a50 100644 --- a/app/client/ui/NotifyUI.ts +++ b/app/client/ui/NotifyUI.ts @@ -57,6 +57,9 @@ function buildNotificationDom(item: Notification, options: IBeaconOpenOptions) { item.options.actions.length ? cssToastActions( item.options.actions.map((action) => buildAction(action, item, options)) ) : null, + item.options.memos.length ? cssToastMemos( + item.options.memos.map(memo => cssToastMemo(memo)) + ) : null, ), dom.maybe(item.options.canUserClose, () => cssToastClose(testId('toast-close'), @@ -300,6 +303,19 @@ const cssToastAction = styled('div', ` } `); +const cssToastMemos = styled('div', ` + margin-top: 16px; + display: flex; + flex-direction: column; +`); + +const cssToastMemo = styled('div', ` + margin: 3px; + color: ${colors.dark}; + background: ${colors.light}; + padding: 3px; +`); + const cssProgressBarWrapper = styled('div', ` margin-top: 18px; margin-bottom: 11px; diff --git a/app/common/ACLRuleCollection.ts b/app/common/ACLRuleCollection.ts index 2ad19b19..b94a4070 100644 --- a/app/common/ACLRuleCollection.ts +++ b/app/common/ACLRuleCollection.ts @@ -272,7 +272,6 @@ function readAclRules(docData: DocData, {log, compile}: ReadAclOptions): ReadAcl if (rule.userAttributes) { if (tableId !== '*' || colIds !== '*') { throw new Error(`ACLRule ${rule.id} invalid; user attributes must be on the default resource`); - continue; } const parsed = JSON.parse(String(rule.userAttributes)); // TODO: could perhaps use ts-interface-checker here. @@ -280,7 +279,6 @@ function readAclRules(docData: DocData, {log, compile}: ReadAclOptions): ReadAcl [parsed.name, parsed.tableId, parsed.lookupColId, parsed.charId] .every(p => p && typeof p === 'string'))) { throw new Error(`User attribute rule ${rule.id} is invalid`); - continue; } parsed.origRecord = rule; userAttributes.push(parsed as UserAttributeRule); @@ -289,10 +287,12 @@ function readAclRules(docData: DocData, {log, compile}: ReadAclOptions): ReadAcl } else if (rule.aclFormula && !rule.aclFormulaParsed) { throw new Error(`ACLRule ${rule.id} invalid because missing its parsed formula`); } else { + const aclFormulaParsed = rule.aclFormula && JSON.parse(String(rule.aclFormulaParsed)); body.push({ origRecord: rule, aclFormula: String(rule.aclFormula), - matchFunc: rule.aclFormula ? compile?.(JSON.parse(String(rule.aclFormulaParsed))) : defaultMatchFunc, + matchFunc: rule.aclFormula ? compile?.(aclFormulaParsed) : defaultMatchFunc, + memo: aclFormulaParsed && aclFormulaParsed[0] === 'Comment' && aclFormulaParsed[2], permissions: parsePermissions(String(rule.permissionsText)), permissionsText: String(rule.permissionsText), }); diff --git a/app/common/ApiError.ts b/app/common/ApiError.ts index 0dbfd581..3ff4adbc 100644 --- a/app/common/ApiError.ts +++ b/app/common/ApiError.ts @@ -28,6 +28,8 @@ export interface ApiErrorDetails { // If set, contains suggestions for fixing a problem. tips?: ApiTip[]; + + memos?: string[]; } /** diff --git a/app/common/BaseAPI.ts b/app/common/BaseAPI.ts index 83100ebe..063355ae 100644 --- a/app/common/BaseAPI.ts +++ b/app/common/BaseAPI.ts @@ -136,6 +136,9 @@ function throwApiError(url: string, resp: Response | AxiosResponse, body: any) { if (body.error) { details.userError = body.error; } + if (body.memos) { + details.memos = body.memos; + } throw new ApiError(`Request to ${url} failed with status ${resp.status}: ` + `${resp.statusText} (${body.error || 'unknown cause'})`, resp.status, details); } diff --git a/app/common/ErrorWithCode.ts b/app/common/ErrorWithCode.ts index 0fecba47..5044da84 100644 --- a/app/common/ErrorWithCode.ts +++ b/app/common/ErrorWithCode.ts @@ -3,6 +3,7 @@ import {OpenDocMode} from 'app/common/DocListAPI'; interface ErrorDetails { status?: number; accessMode?: OpenDocMode; + memos?: string[]; } /** @@ -13,11 +14,9 @@ interface ErrorDetails { * */ export class ErrorWithCode extends Error { - public accessMode?: OpenDocMode; - public status?: number; - constructor(public code: string, message: string, details: ErrorDetails = {}) { + constructor(public code: string, message: string, public details: ErrorDetails = {}) { super(message); - this.status = details.status; - this.accessMode = details.accessMode; } + public get accessMode() { return this.details?.accessMode; } + public get status() { return this.details?.status; } } diff --git a/app/common/GranularAccessClause.ts b/app/common/GranularAccessClause.ts index d1696b76..4287d4b9 100644 --- a/app/common/GranularAccessClause.ts +++ b/app/common/GranularAccessClause.ts @@ -17,6 +17,9 @@ export interface RulePart { // Compiled version of aclFormula. matchFunc?: AclMatchFunc; + + // Optional memo, currently extracted from comment in formula. + memo?: string; } // Light wrapper around characteristics or records. diff --git a/app/server/lib/ACLFormula.ts b/app/server/lib/ACLFormula.ts index 34f50824..d170df81 100644 --- a/app/server/lib/ACLFormula.ts +++ b/app/server/lib/ACLFormula.ts @@ -64,6 +64,7 @@ function _compileNode(parsedAclFormula: ParsedAclFormula): AclEvalFunc { const attrName = rawArgs[1] as string; return _compileAndCombine([args[0]], ([value]) => getAttr(value, attrName, args[0])); } + case 'Comment': return _compileNode(args[0]); } throw new Error(`Unknown node type '${parsedAclFormula[0]}'`); } diff --git a/app/server/lib/ActiveDoc.ts b/app/server/lib/ActiveDoc.ts index 93e650fd..98cf856c 100644 --- a/app/server/lib/ActiveDoc.ts +++ b/app/server/lib/ActiveDoc.ts @@ -29,7 +29,6 @@ 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 {ErrorWithCode} from 'app/common/ErrorWithCode'; import {byteString, countIf} from 'app/common/gutil'; import {InactivityTimer} from 'app/common/InactivityTimer'; import * as marshal from 'app/common/marshal'; @@ -604,9 +603,8 @@ export class ActiveDoc extends EventEmitter { // If user does not have rights to access what this query is asking for, fail. const tableAccess = await this._granularAccess.getTableAccess(docSession, query.tableId); - if (tableAccess.read === 'deny') { - throw new Error('not authorized to read table'); - } + + this._granularAccess.assertCanRead(tableAccess); // Some tests read _grist_ tables via the api. The _fetchQueryFromDB method // currently cannot read those tables, so we load them from the data engine @@ -614,7 +612,7 @@ export class ActiveDoc extends EventEmitter { // Also, if row-level access is being controlled, we wait for formula columns // to be populated. const wantFull = waitForFormulas || query.tableId.startsWith('_grist_') || - tableAccess.read === 'mixed'; + this._granularAccess.getReadPermission(tableAccess) === 'mixed'; const onDemand = this._onDemandActions.isOnDemand(query.tableId); this.logInfo(docSession, "fetchQuery %s %s", JSON.stringify(query), onDemand ? "(onDemand)" : "(regular)"); @@ -636,7 +634,7 @@ export class ActiveDoc extends EventEmitter { } // If row-level access is being controlled, filter the data appropriately. // Likewise if column-level access is being controlled. - if (tableAccess.read !== 'allow') { + if (this._granularAccess.getReadPermission(tableAccess) !== 'allow') { await this._granularAccess.filterData(docSession, data!); } this.logInfo(docSession, "fetchQuery -> %d rows, cols: %s", @@ -1181,14 +1179,12 @@ export class ActiveDoc extends EventEmitter { protected async _applyUserActions(docSession: OptDocSession, actions: UserAction[], options: ApplyUAOptions = {}): Promise { - if (!await this._granularAccess.canMaybeApplyUserActions(docSession, actions)) { - throw new ErrorWithCode('ACL_DENY', 'Action blocked by access rules'); - } - const client = docSession.client; this.logDebug(docSession, "_applyUserActions(%s, %s)", client, shortDesc(actions)); this._inactivityTimer.ping(); // The doc is in active use; ping it to stay open longer. + await this._granularAccess.assertCanMaybeApplyUserActions(docSession, actions); + const user = docSession.mode === 'system' ? 'grist' : (client && client.session ? (await client.session.getEmail()) : ""); diff --git a/app/server/lib/Client.ts b/app/server/lib/Client.ts index cfdce688..3d68b51c 100644 --- a/app/server/lib/Client.ts +++ b/app/server/lib/Client.ts @@ -276,6 +276,9 @@ export class Client { if (err.code) { response.errorCode = err.code; } + if (err.details) { + response.details = err.details; + } if (typeof code === 'string' && code === 'AUTH_NO_EDIT' && err.accessMode === 'fork') { response.shouldFork = true; } diff --git a/app/server/lib/GranularAccess.ts b/app/server/lib/GranularAccess.ts index 59c22a88..ecf13f78 100644 --- a/app/server/lib/GranularAccess.ts +++ b/app/server/lib/GranularAccess.ts @@ -1,6 +1,4 @@ -import { MixedPermissionSet, PartialPermissionSet, PermissionSet, TablePermissionSet } from 'app/common/ACLPermissions'; -import { makePartialPermissions, mergePartialPermissions, mergePermissions } from 'app/common/ACLPermissions'; -import { emptyPermissionSet, toMixed } from 'app/common/ACLPermissions'; +import { ALL_PERMISSION_PROPS } from 'app/common/ACLPermissions'; import { ACLRuleCollection } from 'app/common/ACLRuleCollection'; import { ActionGroup } from 'app/common/ActionGroup'; import { createEmptyActionSummary } from 'app/common/ActionSummary'; @@ -15,7 +13,7 @@ import { DocData } from 'app/common/DocData'; import { UserOverride } from 'app/common/DocListAPI'; import { ErrorWithCode } from 'app/common/ErrorWithCode'; import { AclMatchInput, InfoView } from 'app/common/GranularAccessClause'; -import { RuleSet, UserInfo } from 'app/common/GranularAccessClause'; +import { UserInfo } from 'app/common/GranularAccessClause'; import { getSetMapValue, isObject } from 'app/common/gutil'; import { canView, Role } from 'app/common/roles'; import { FullUser } from 'app/common/UserAPI'; @@ -23,9 +21,11 @@ import { HomeDBManager } from 'app/gen-server/lib/HomeDBManager'; import { compileAclFormula } from 'app/server/lib/ACLFormula'; import { getDocSessionAccess, getDocSessionUser, OptDocSession } from 'app/server/lib/DocSession'; import * as log from 'app/server/lib/log'; +import { PermissionInfo, PermissionSetWithContext, TablePermissionSetWithContext } from 'app/server/lib/PermissionInfo'; import { integerParam } from 'app/server/lib/requestUtils'; import { getRelatedRows, getRowIdsFromDocAction } from 'app/server/lib/RowAccess'; import cloneDeep = require('lodash/cloneDeep'); +import fromPairs = require('lodash/fromPairs'); import get = require('lodash/get'); import pullAt = require('lodash/pullAt'); @@ -85,7 +85,7 @@ const OK_ACTIONS = new Set(['Calculate', 'AddEmptyTable']); * steps and clients. We expect modifications to be serialized, and the following * pattern of calls for modifications: * - * - canMaybeApplyUserActions(), called with UserActions for an initial access check. + * - assertCanMaybeApplyUserActions(), called with UserActions for an initial access check. * Since not all checks can be done without analyzing UserActions into DocActions, * it is ok for this call to pass even if a more definitive test later will fail. * - canApplyDocActions(), called when DocActions have been produced from UserActions, @@ -146,7 +146,7 @@ export class GranularAccess { */ public async hasTableAccess(docSession: OptDocSession, tableId: string) { const pset = await this.getTableAccess(docSession, tableId); - return pset.read !== 'deny'; + return this.getReadPermission(pset) !== 'deny'; } /** @@ -280,9 +280,11 @@ export class GranularAccess { * TODO: not smart about intermediate states, if there is a table or column rename it will * have trouble, and might forbid something that should be allowed. */ - public async canMaybeApplyUserActions(docSession: OptDocSession, actions: UserAction[]): Promise { + public async assertCanMaybeApplyUserActions(docSession: OptDocSession, actions: UserAction[]): Promise { for (const action of actions) { - if (!await this.canMaybeApplyUserAction(docSession, action)) { return false; } + if (!await this.assertCanMaybeApplyUserAction(docSession, action)) { + throw new ErrorWithCode('ACL_DENY', 'Action blocked by access rules'); + } } return true; } @@ -290,7 +292,7 @@ export class GranularAccess { /** * Check if user can apply a given action to the document. */ - public async canMaybeApplyUserAction(docSession: OptDocSession, a: UserAction|DocAction): Promise { + public async assertCanMaybeApplyUserAction(docSession: OptDocSession, a: UserAction|DocAction): Promise { const name = a[0] as string; if (OK_ACTIONS.has(name)) { return true; } if (SPECIAL_ACTIONS.has(name)) { @@ -300,9 +302,9 @@ export class GranularAccess { return this.hasFullAccess(docSession); } if (a[0] === 'ApplyUndoActions') { - return this.canMaybeApplyUserActions(docSession, a[1] as UserAction[]); + return this.assertCanMaybeApplyUserActions(docSession, a[1] as UserAction[]); } else if (a[0] === 'ApplyDocActions') { - return this.canMaybeApplyUserActions(docSession, a[1] as UserAction[]); + return this.assertCanMaybeApplyUserActions(docSession, a[1] as UserAction[]); } else if (isTableAction(a)) { const tableId = getTableId(a); // If there are any access control nuances, deny _grist_* tables. @@ -311,8 +313,8 @@ export class GranularAccess { return !this.hasNuancedAccess(docSession); } const tableAccess = await this.getTableAccess(docSession, tableId); - const accessFn = getAccessForActionType(a); - const access = accessFn(tableAccess); + const accessCheck = getAccessForActionType(a, 'fatal'); + const access = accessCheck.get(tableAccess); // if access is mixed, leave this to be checked in detail later. return access === 'allow' || access === 'mixed' || access === 'mixedColumns'; } @@ -337,7 +339,7 @@ export class GranularAccess { const access = getDocSessionAccess(docSession); if (!canView(access)) { return false; } const permInfo = await this._getAccess(docSession); - return permInfo.getFullAccess().read === 'allow'; + return this.getReadPermission(permInfo.getFullAccess()) === 'allow'; } /** @@ -399,7 +401,7 @@ export class GranularAccess { const tableId = rec.tableId as string; const tableRef = rec.id; const tableAccess = permInfo.getTableAccess(tableId); - if (tableAccess.read === 'deny') { + if (this.getReadPermission(tableAccess) === 'deny') { censoredTables.add(tableRef); } // TODO If some columns are allowed and the rest (*) are denied, we need to be able to @@ -407,7 +409,7 @@ export class GranularAccess { for (const ruleSet of this._ruleCollection.getAllColumnRuleSets(tableId)) { if (Array.isArray(ruleSet.colIds)) { for (const colId of ruleSet.colIds) { - if (permInfo.getColumnAccess(tableId, colId).read === 'deny') { + if (this.getReadPermission(permInfo.getColumnAccess(tableId, colId)) === 'deny') { censoredColumnCodes.add(columnCode(tableRef, colId)); } } @@ -475,7 +477,7 @@ export class GranularAccess { * Distill the clauses for the given session and table, to figure out the * access level and any row-level access functions needed. */ - public async getTableAccess(docSession: OptDocSession, tableId: string): Promise { + public async getTableAccess(docSession: OptDocSession, tableId: string): Promise { return (await this._getAccess(docSession)).getTableAccess(tableId); } @@ -486,12 +488,14 @@ export class GranularAccess { public async filterData(docSession: OptDocSession, data: TableDataAction) { const permInfo = await this._getAccess(docSession); const tableId = getTableId(data); - if (permInfo.getTableAccess(tableId).read === 'mixed') { - await this._filterRowsAndCells(docSession, data, data, data, canRead); + if (this.getReadPermission(permInfo.getTableAccess(tableId)) === 'mixed') { + await this._filterRowsAndCells(docSession, data, data, data, readAccessCheck); } // Filter columns, omitting any to which the user has no access, regardless of rows. - this._filterColumns(data[3], (colId) => permInfo.getColumnAccess(tableId, colId).read !== 'deny'); + this._filterColumns( + data[3], + (colId) => this.getReadPermission(permInfo.getColumnAccess(tableId, colId)) !== 'deny'); } public async getUserOverride(docSession: OptDocSession): Promise { @@ -499,25 +503,33 @@ export class GranularAccess { return this._getUserAttributes(docSession).override; } + public getReadPermission(ps: PermissionSetWithContext) { + return ps.perms.read; + } + + public assertCanRead(ps: PermissionSetWithContext) { + readAccessCheck.throwIfDenied(ps); + } + /** * Strip out any denied columns from an action. Returns null if nothing is left. - * accessFn may throw if denials are fatal. + * accessCheck may throw if denials are fatal. */ private _pruneColumns(a: DocAction, permInfo: PermissionInfo, tableId: string, - accessFn: AccessFn): DocAction|null { + accessCheck: AccessCheck): DocAction|null { if (a[0] === 'RemoveRecord' || a[0] === 'BulkRemoveRecord') { return a; } else if (a[0] === 'AddRecord' || a[0] === 'BulkAddRecord' || a[0] === 'UpdateRecord' || a[0] === 'BulkUpdateRecord' || a[0] === 'ReplaceTableData' || a[0] === 'TableData') { const na = cloneDeep(a); - this._filterColumns(na[3], (colId) => accessFn(permInfo.getColumnAccess(tableId, colId)) !== 'deny'); + this._filterColumns(na[3], (colId) => accessCheck.get(permInfo.getColumnAccess(tableId, colId)) !== 'deny'); if (Object.keys(na[3]).length === 0) { return null; } return na; } else if (a[0] === 'AddColumn' || a[0] === 'RemoveColumn' || a[0] === 'RenameColumn' || a[0] === 'ModifyColumn') { const na = cloneDeep(a); const colId: string = na[2]; - if (accessFn(permInfo.getColumnAccess(tableId, colId)) === 'deny') { return null; } + if (accessCheck.get(permInfo.getColumnAccess(tableId, colId)) === 'deny') { return null; } throw new ErrorWithCode('NEED_RELOAD', 'document needs reload'); } else { // Remaining cases of AddTable, RemoveTable, RenameTable should have @@ -614,22 +626,22 @@ export class GranularAccess { // Return the results, also applying any cell-level access control. for (const docAction of revisedDocActions) { - await this._filterRowsAndCells(docSession, rowsAfter, rowsAfter, docAction, canRead); + await this._filterRowsAndCells(docSession, rowsAfter, rowsAfter, docAction, readAccessCheck); } return revisedDocActions; } /** * Like _pruneRows, but fails immediately if access to any row is forbidden. - * The accessFn supplied should throw an error on denial. + * The accessCheck supplied should throw an error on denial. */ private async _checkRows(docSession: OptDocSession, a: DocAction, idx: number, - accessFn: AccessFn): Promise { + accessCheck: AccessCheck): Promise { // For the moment, only deal with Record-related actions. // TODO: process table/column schema changes more carefully. if (isSchemaAction(a)) { return; } const {rowsBefore, rowsAfter} = await this._getRowsBeforeAndAfter(idx); - await this._filterRowsAndCells(docSession, rowsBefore, rowsAfter, a, accessFn); + await this._filterRowsAndCells(docSession, rowsBefore, rowsAfter, a, accessCheck); } private async _getRowsBeforeAndAfter(idx: number) { @@ -656,7 +668,7 @@ export class GranularAccess { * Modify action in place, scrubbing any rows and cells to which access is not granted. */ private async _filterRowsAndCells(docSession: OptDocSession, rowsBefore: TableDataAction, rowsAfter: TableDataAction, - docAction: DocAction, accessFn: AccessFn) { + docAction: DocAction, accessCheck: AccessCheck) { if (docAction && isSchemaAction(docAction)) { // TODO should filter out metadata about an unavailable column, probably. return []; @@ -696,14 +708,14 @@ export class GranularAccess { const rowPermInfo = new PermissionInfo(this._ruleCollection, input); // getTableAccess() evaluates all column rules for THIS record. So it's really rowAccess. const rowAccess = rowPermInfo.getTableAccess(tableId); - const access = accessFn(rowAccess); + const access = accessCheck.get(rowAccess); if (access === 'deny') { toRemove.push(idx); } else if (access !== 'allow' && colValues) { // Go over column rules. for (const colId of Object.keys(colValues)) { const colAccess = rowPermInfo.getColumnAccess(tableId, colId); - if (accessFn(colAccess) === 'deny') { + if (accessCheck.get(colAccess) === 'deny') { censorAt(colId, idx); } } @@ -737,7 +749,7 @@ export class GranularAccess { const rowPermInfo = new PermissionInfo(this._ruleCollection, input); // getTableAccess() evaluates all column rules for THIS record. So it's really rowAccess. const rowAccess = rowPermInfo.getTableAccess(tableId); - if (canRead(rowAccess) === 'deny') { + if (this.getReadPermission(rowAccess) === 'deny') { toRemove.push(rowIds[idx]); } } @@ -1003,134 +1015,36 @@ export class GranularAccess { const tableId = getTableId(a); const permInfo = await this._getAccess(docSession); const tableAccess = permInfo.getTableAccess(tableId); - const access = tableAccess.read; + const access = this.getReadPermission(tableAccess); if (access === 'deny') { return []; } if (access === 'allow') { return [a]; } if (access === 'mixedColumns') { - return [this._pruneColumns(a, permInfo, tableId, canRead)].filter(isObject); + return [this._pruneColumns(a, permInfo, tableId, readAccessCheck)].filter(isObject); } // The remainder is the mixed condition. const revisedDocActions = await this._pruneRows(docSession, a, idx); const result = revisedDocActions.map(na => this._pruneColumns(na, permInfo, tableId, - canRead)).filter(isObject); + readAccessCheck)).filter(isObject); return result; } private async _checkIncomingDocAction(docSession: OptDocSession, a: DocAction, idx: number): Promise { - const accessFn = denyIsFatal(getAccessForActionType(a)); + const accessCheck = getAccessForActionType(a, 'fatal'); const tableId = getTableId(a); const permInfo = await this._getAccess(docSession); const tableAccess = permInfo.getTableAccess(tableId); - const access = accessFn(tableAccess); + const access = accessCheck.get(tableAccess); if (access === 'allow') { return; } if (access === 'mixedColumns') { // Somewhat abusing prune method by calling it with an access function that // throws on denial. - this._pruneColumns(a, permInfo, tableId, accessFn); + this._pruneColumns(a, permInfo, tableId, accessCheck); } // The remainder is the mixed condition. - await this._checkRows(docSession, a, idx, accessFn); + await this._checkRows(docSession, a, idx, accessCheck); // Somewhat abusing prune method by calling it with an access function that // throws on denial. - this._pruneColumns(a, permInfo, tableId, accessFn); - } -} - -/** - * Evaluate a RuleSet on a given input (user and optionally record). If a record is needed but not - * included, the result may include permission values like 'allowSome', 'denySome'. - */ -function evaluateRule(ruleSet: RuleSet, input: AclMatchInput): PartialPermissionSet { - let pset: PartialPermissionSet = emptyPermissionSet(); - for (const rule of ruleSet.body) { - try { - if (rule.matchFunc!(input)) { - pset = mergePartialPermissions(pset, rule.permissions); - } - } catch (e) { - if (e.code === 'NEED_ROW_DATA') { - pset = mergePartialPermissions(pset, makePartialPermissions(rule.permissions)); - } else { - // For other errors, assume the rule is invalid, and treat as a non-match. - // TODO An appropriate user should be alerted that a clause is not being honored. - log.warn("ACLRule for %s failed: %s", ruleSet.tableId, e.message); - } - } - } - return pset; -} - -/** - * Helper for evaluating rules given a particular user and optionally a record. It evaluates rules - * for a column, table, or document, with caching to avoid evaluating the same rule multiple times. - */ -class PermissionInfo { - private _ruleResults = new Map(); - - // Construct a PermissionInfo for a particular input, which is a combination of user and - // optionally a record. - constructor(private _acls: ACLRuleCollection, private _input: AclMatchInput) {} - - // Get permissions for "tableId:colId", defaulting to "tableId:*" and "*:*" as needed. - // If 'mixed' is returned, different rows may have different permissions. It should never return - // 'mixed' if the input includes `rec`. - public getColumnAccess(tableId: string, colId: string): MixedPermissionSet { - const ruleSet: RuleSet|undefined = this._acls.getColumnRuleSet(tableId, colId); - return ruleSet ? this._processColumnRule(ruleSet) : this._getTableDefaultAccess(tableId); - } - - // Combine permissions from all rules for the given table. - // If 'mixedColumns' is returned, different columns have different permissions, but they do NOT - // depend on rows. If 'mixed' is returned, some permissions depend on rows. - public getTableAccess(tableId: string): TablePermissionSet { - const columnAccess = this._acls.getAllColumnRuleSets(tableId).map(rs => this._processColumnRule(rs)); - columnAccess.push(this._getTableDefaultAccess(tableId)); - - return mergePermissions(columnAccess, (bits) => ( - bits.every(b => b === 'allow') ? 'allow' : - bits.every(b => b === 'deny') ? 'deny' : - bits.every(b => b === 'allow' || b === 'deny') ? 'mixedColumns' : - 'mixed' - )); - } - - // Combine permissions from all rules throughout. - // If 'mixed' is returned, then different tables, rows, or columns have different permissions. - public getFullAccess(): MixedPermissionSet { - const tableAccess = this._acls.getAllTableIds().map(tableId => this.getTableAccess(tableId)); - tableAccess.push(this._getDocDefaultAccess()); - - return mergePermissions(tableAccess, (bits) => ( - bits.every(b => b === 'allow') ? 'allow' : - bits.every(b => b === 'deny') ? 'deny' : - 'mixed' - )); - } - - // Get permissions for "tableId:*", defaulting to "*:*" as needed. - // If 'mixed' is returned, different rows may have different permissions. - private _getTableDefaultAccess(tableId: string): MixedPermissionSet { - const ruleSet: RuleSet|undefined = this._acls.getTableDefaultRuleSet(tableId); - return ruleSet ? this._processRule(ruleSet, () => this._getDocDefaultAccess()) : - this._getDocDefaultAccess(); - } - - // Get permissions for "*:*". - private _getDocDefaultAccess(): MixedPermissionSet { - return this._processRule(this._acls.getDocDefaultRuleSet()); - } - - // Evaluate and cache the given column rule, falling back to the corresponding table default. - private _processColumnRule(ruleSet: RuleSet): MixedPermissionSet { - return this._processRule(ruleSet, () => this._getTableDefaultAccess(ruleSet.tableId)); - } - - // Evaluate the given rule, with the default fallback, and cache the result. - private _processRule(ruleSet: RuleSet, defaultAccess?: () => MixedPermissionSet): MixedPermissionSet { - return getSetMapValue(this._ruleResults, ruleSet, () => { - const pset = evaluateRule(ruleSet, this._input); - return toMixed(defaultAccess ? mergePartialPermissions(pset, defaultAccess()) : pset); - }); + this._pruneColumns(a, permInfo, tableId, accessCheck); } } @@ -1173,34 +1087,49 @@ class UserAttributes { public override?: UserOverride; } -// A function for extracting one of the create/read/update/delete/schemaEdit permissions -// from a permission set. -type AccessFn = (ps: PermissionSet) => string; -// Get an AccessFn appropriate for the specific action. +class AccessCheck { + constructor(public access: 'update'|'delete'|'create'|'schemaEdit'|'read', + public severity: 'check'|'fatal') { + } + + public get(ps: PermissionSetWithContext): string { + const result = ps.perms[this.access]; + if (result !== 'deny' || this.severity !== 'fatal') { return result; } + this.throwIfDenied(ps); + return result; + } + + public throwIfDenied(ps: PermissionSetWithContext): void { + const result = ps.perms[this.access]; + if (result !== 'deny') { return; } + const memos = ps.getMemos()[this.access]; + throw new ErrorWithCode('ACL_DENY', `Blocked by ${ps.ruleType} access rules`, { + memos, + status: 403 + }); + } +} + +export const accessChecks = { + check: fromPairs(ALL_PERMISSION_PROPS.map(prop => [prop, new AccessCheck(prop, 'check')])), + fatal: fromPairs(ALL_PERMISSION_PROPS.map(prop => [prop, new AccessCheck(prop, 'fatal')])), +}; + + +// The AccessCheck for the "read" permission is used enough to merit a shortcut. +const readAccessCheck = accessChecks.check.read; + +// Get an AccessCheck appropriate for the specific action. // TODO: deal with ReplaceTableData, which both deletes and creates rows. -function getAccessForActionType(a: DocAction): AccessFn { +function getAccessForActionType(a: DocAction, severity: 'check'|'fatal'): AccessCheck { if (a[0] === 'UpdateRecord' || a[0] === 'BulkUpdateRecord') { - return (ps) => ps.update; + return accessChecks[severity].update; } else if (a[0] === 'RemoveRecord' || a[0] === 'BulkRemoveRecord') { - return (ps) => ps.delete; + return accessChecks[severity].delete; } else if (a[0] === 'AddRecord' || a[0] === 'BulkAddRecord') { - return (ps) => ps.create; + return accessChecks[severity].create; } else { - return (ps) => ps.schemaEdit; + return accessChecks[severity].schemaEdit; } } - -// Tweak an AccessFn so that it throws an exception if access is denied. -function denyIsFatal(fn: AccessFn): AccessFn { - return (ps) => { - const result = fn(ps); - if (result === 'deny') { throw new ErrorWithCode('ACL_DENY', 'Blocked by access rules'); } - return result; - }; -} - -// A simple access function that returns the "read" permission. -function canRead(ps: PermissionSet) { - return ps.read; -} diff --git a/app/server/lib/PermissionInfo.ts b/app/server/lib/PermissionInfo.ts new file mode 100644 index 00000000..029c80a2 --- /dev/null +++ b/app/server/lib/PermissionInfo.ts @@ -0,0 +1,239 @@ +import { ALL_PERMISSION_PROPS, emptyPermissionSet, + makePartialPermissions, mergePartialPermissions, mergePermissions, + MixedPermissionSet, PartialPermissionSet, PermissionSet, TablePermissionSet, + toMixed } from 'app/common/ACLPermissions'; +import { ACLRuleCollection } from 'app/common/ACLRuleCollection'; +import { AclMatchInput, RuleSet } from 'app/common/GranularAccessClause'; +import { getSetMapValue } from 'app/common/gutil'; +import * as log from 'app/server/lib/log'; +import { mapValues } from 'lodash'; + +/** + * A PermissionSet with context about how it was created. Allows us to produce more + * informative error messages. + */ +export interface PermissionSetWithContextOf { + perms: T; + ruleType: 'full'|'table'|'column'; + getMemos: () => MemoSet; +} + +export type MixedPermissionSetWithContext = PermissionSetWithContextOf; +export type TablePermissionSetWithContext = PermissionSetWithContextOf; +export type PermissionSetWithContext = PermissionSetWithContextOf>; + +// Accumulator for memos of relevant rules. +export type MemoSet = PermissionSet; + +// Merge MemoSets straightforwardly, by concatenation. +export function mergeMemoSets(psets: MemoSet[]): MemoSet { + const result: Partial = {}; + for (const prop of ALL_PERMISSION_PROPS) { + result[prop] = ([] as string[]).concat(...psets.map(p => p[prop])); + } + return result as MemoSet; +} + +export function emptyMemoSet(): MemoSet { + return { + read: [], + create: [], + update: [], + delete: [], + schemaEdit: [], + }; +} + +/** + * Abstract base class for processing rules given a particular input. + * Main use of this class will be to calculate permissions, but will also + * be used to calculate metadata about permissions. + */ +abstract class RuleInfo { + + // Construct a RuleInfo for a particular input, which is a combination of user and + // optionally a record. + constructor(protected _acls: ACLRuleCollection, protected _input: AclMatchInput) {} + + public getColumnAspect(tableId: string, colId: string): MixedT { + const ruleSet: RuleSet|undefined = this._acls.getColumnRuleSet(tableId, colId); + return ruleSet ? this._processColumnRule(ruleSet) : this._getTableDefaultAspect(tableId); + } + + public getTableAspect(tableId: string): TableT { + const columnAccess = this._acls.getAllColumnRuleSets(tableId).map(rs => this._processColumnRule(rs)); + columnAccess.push(this._getTableDefaultAspect(tableId)); + return this._mergeTableAccess(columnAccess); + } + + public getFullAspect(): MixedT { + const tableAccess = this._acls.getAllTableIds().map(tableId => this.getTableAspect(tableId)); + tableAccess.push(this._getDocDefaultAspect()); + + return this._mergeFullAccess(tableAccess); + } + + protected abstract _processRule(ruleSet: RuleSet, defaultAccess?: () => MixedT): MixedT; + protected abstract _mergeTableAccess(access: MixedT[]): TableT; + protected abstract _mergeFullAccess(access: TableT[]): MixedT; + + private _getTableDefaultAspect(tableId: string): MixedT { + const ruleSet: RuleSet|undefined = this._acls.getTableDefaultRuleSet(tableId); + return ruleSet ? this._processRule(ruleSet, () => this._getDocDefaultAspect()) : + this._getDocDefaultAspect(); + } + + private _getDocDefaultAspect(): MixedT { + return this._processRule(this._acls.getDocDefaultRuleSet()); + } + + private _processColumnRule(ruleSet: RuleSet): MixedT { + return this._processRule(ruleSet, () => this._getTableDefaultAspect(ruleSet.tableId)); + } +} + +/** + * Pool memos from rules, on the assumption that access has been denied and we are looking + * for possible explanations to offer the user. + */ +export class MemoInfo extends RuleInfo { + protected _processRule(ruleSet: RuleSet, defaultAccess?: () => MemoSet): MemoSet { + const pset = extractMemos(ruleSet, this._input); + return defaultAccess ? mergeMemoSets([pset, defaultAccess()]) : pset; + } + + protected _mergeTableAccess(access: MemoSet[]): MemoSet { + return mergeMemoSets(access); + } + + protected _mergeFullAccess(access: MemoSet[]): MemoSet { + return mergeMemoSets(access); + } +} + +/** + * Helper for evaluating rules given a particular user and optionally a record. It evaluates rules + * for a column, table, or document, with caching to avoid evaluating the same rule multiple times. + */ +export class PermissionInfo extends RuleInfo { + private _ruleResults = new Map(); + + // Get permissions for "tableId:colId", defaulting to "tableId:*" and "*:*" as needed. + // If 'mixed' is returned, different rows may have different permissions. It should never return + // 'mixed' if the input includes `rec`. + // Wrap permissions with information about how they were computed. This allows + // us to issue more informative error messages. + public getColumnAccess(tableId: string, colId: string): MixedPermissionSetWithContext { + return { + perms: this.getColumnAspect(tableId, colId), + ruleType: 'column', + getMemos: () => new MemoInfo(this._acls, this._input).getColumnAspect(tableId, colId) + }; + } + + // Combine permissions from all rules for the given table. + // If 'mixedColumns' is returned, different columns have different permissions, but they do NOT + // depend on rows. If 'mixed' is returned, some permissions depend on rows. + // Wrap permission sets for better error messages. + public getTableAccess(tableId: string): TablePermissionSetWithContext { + return { + perms: this.getTableAspect(tableId), + ruleType: 'table', + getMemos: () => new MemoInfo(this._acls, this._input).getTableAspect(tableId) + }; + } + + // Combine permissions from all rules throughout. + // If 'mixed' is returned, then different tables, rows, or columns have different permissions. + // Wrap permission sets for better error messages. + public getFullAccess(): MixedPermissionSetWithContext { + return { + perms: this.getFullAspect(), + ruleType: 'full', + getMemos: () => new MemoInfo(this._acls, this._input).getFullAspect() + }; + } + + protected _processRule(ruleSet: RuleSet, defaultAccess?: () => MixedPermissionSet): MixedPermissionSet { + return getSetMapValue(this._ruleResults, ruleSet, () => { + const pset = evaluateRule(ruleSet, this._input); + return toMixed(defaultAccess ? mergePartialPermissions(pset, defaultAccess()) : pset); + }); + } + + protected _mergeTableAccess(access: MixedPermissionSet[]): TablePermissionSet { + return mergePermissions(access, (bits) => ( + bits.every(b => b === 'allow') ? 'allow' : + bits.every(b => b === 'deny') ? 'deny' : + bits.every(b => b === 'allow' || b === 'deny') ? 'mixedColumns' : + 'mixed' + )); + } + + protected _mergeFullAccess(access: TablePermissionSet[]): MixedPermissionSet { + return mergePermissions(access, (bits) => ( + bits.every(b => b === 'allow') ? 'allow' : + bits.every(b => b === 'deny') ? 'deny' : + 'mixed' + )); + } +} + +/** + * Evaluate a RuleSet on a given input (user and optionally record). If a record is needed but not + * included, the result may include permission values like 'allowSome', 'denySome'. + */ +function evaluateRule(ruleSet: RuleSet, input: AclMatchInput): PartialPermissionSet { + let pset: PartialPermissionSet = emptyPermissionSet(); + for (const rule of ruleSet.body) { + try { + if (rule.matchFunc!(input)) { + pset = mergePartialPermissions(pset, rule.permissions); + } + } catch (e) { + if (e.code === 'NEED_ROW_DATA') { + pset = mergePartialPermissions(pset, makePartialPermissions(rule.permissions)); + } else { + // Unexpected error. Interpret rule pessimistically. + // Anything it would explicitly allow, no longer allow through this rule. + // Anything it would explicitly deny, go ahead and deny. + pset = mergePartialPermissions(pset, mapValues(rule.permissions, val => (val === 'allow' ? "" : val))); + log.warn("ACLRule for %s failed: %s", ruleSet.tableId, e.message); + } + } + } + return pset; +} + +/** + * If a rule has a memo, and passes, add that memo for all permissions it denies. + * If a rule has a memo, and fails, add that memo for all permissions it allows. + */ +function extractMemos(ruleSet: RuleSet, input: AclMatchInput): MemoSet { + const pset = emptyMemoSet(); + for (const rule of ruleSet.body) { + try { + const passing = rule.matchFunc!(input); + for (const prop of ALL_PERMISSION_PROPS) { + const p = rule.permissions[prop]; + const memos: string[] = pset[prop]; + if (rule.memo) { + if (passing && p === 'deny') { + memos.push(rule.memo); + } else if (!passing && p === 'allow') { + memos.push(rule.memo); + } + } + } + } catch (e) { + if (e.code !== 'NEED_ROW_DATA') { + // If a rule is failing unexpectedly, give some information via memos. + // TODO: Could give a more structured result. + for (const prop of ALL_PERMISSION_PROPS) { + pset[prop].push(`Rule [${rule.aclFormula}] for ${ruleSet.tableId} has an error: ${e.message}`); + } + } + } + } + return pset; +} diff --git a/app/server/lib/expressWrap.ts b/app/server/lib/expressWrap.ts index c332a5fe..1bbbd813 100644 --- a/app/server/lib/expressWrap.ts +++ b/app/server/lib/expressWrap.ts @@ -24,8 +24,15 @@ export const jsonErrorHandler: express.ErrorRequestHandler = (err, req, res, nex log.warn("Error during api call to %s: (%s) user %d params %s body %s", req.path, err.message, mreq.userId, JSON.stringify(req.params), JSON.stringify(req.body)); - res.status(err.status || 500).json({error: err.message || 'internal error', - details: err.details}); + let details = err.details && {...err.details}; + const status = details?.status || err.status || 500; + if (details) { + // Remove some details exposed for websocket API only. + delete details.accessMode; + delete details.status; // TODO: reconcile err.status and details.status, no need for both. + if (Object.keys(details).length === 0) { details = undefined; } + } + res.status(status).json({error: err.message || 'internal error', details}); }; /** diff --git a/sandbox/grist/acl_formula.py b/sandbox/grist/acl_formula.py index 0db91dd4..77c6f466 100644 --- a/sandbox/grist/acl_formula.py +++ b/sandbox/grist/acl_formula.py @@ -1,5 +1,7 @@ import ast +import io import json +import tokenize from collections import namedtuple import asttokens @@ -22,10 +24,16 @@ def parse_acl_formula(acl_formula): Const value (number, string, bool) Name name (string) Attr node, attr_name + Comment node, comment """ try: tree = ast.parse(acl_formula, mode='eval') - return _TreeConverter().visit(tree) + result = _TreeConverter().visit(tree) + for part in tokenize.generate_tokens(io.StringIO(acl_formula.decode('utf-8')).readline): + if part[0] == tokenize.COMMENT and part[1].startswith('#'): + result = ['Comment', result, part[1][1:].strip()] + break + return result except SyntaxError as err: # In case of an error, include line and offset. raise SyntaxError("%s on line %s col %s" % (err.args[0], err.lineno, err.offset)) diff --git a/sandbox/grist/test_acl_formula.py b/sandbox/grist/test_acl_formula.py index b077e01f..3095975f 100644 --- a/sandbox/grist/test_acl_formula.py +++ b/sandbox/grist/test_acl_formula.py @@ -79,6 +79,24 @@ class TestACLFormula(unittest.TestCase): ['Attr', ['Attr', ['Name', 'user'], 'Status'], 'IsActive'] ]) + self.assertEqual(parse_acl_formula( + "True # Comment! "), + ['Comment', ['Const', True], 'Comment!']) + + self.assertEqual(parse_acl_formula( + "\"#x\" == \" # Not a comment \"#Comment!"), + ['Comment', + ['Eq', ['Const', '#x'], ['Const', ' # Not a comment ']], + 'Comment!' + ]) + + self.assertEqual(parse_acl_formula( + "# Allow owners\nuser.Access == 'owners' # ignored\n# comment ignored"), + ['Comment', + ['Eq', ['Attr', ['Name', 'user'], 'Access'], ['Const', 'owners']], + 'Allow owners' + ]) + def test_unsupported(self): # Test a few constructs we expect to fail # Not an expression