From 9696e24aac699ffdd43d7e0d1fc05a0fb5fc76e3 Mon Sep 17 00:00:00 2001 From: Paul Fitzpatrick Date: Wed, 28 Apr 2021 08:54:24 -0400 Subject: [PATCH] (core) always allow owners to edit access rules, so they don't get stuck Summary: Access to structural tables currently depends on SchemaEdit permission. We now make an exception for owner access to _grist_ACLResources and _grist_ACLRules, giving them unconditional access. It was too easy for owners to lock themselves out of editing access rules. Test Plan: added test Reviewers: dsagal Reviewed By: dsagal Differential Revision: https://phab.getgrist.com/D2790 --- app/server/lib/GranularAccess.ts | 58 ++++++++++++++++++++------------ 1 file changed, 36 insertions(+), 22 deletions(-) diff --git a/app/server/lib/GranularAccess.ts b/app/server/lib/GranularAccess.ts index 8ff9e4f6..76086358 100644 --- a/app/server/lib/GranularAccess.ts +++ b/app/server/lib/GranularAccess.ts @@ -388,7 +388,7 @@ export class GranularAccess implements GranularAccessForBundle { return false; // have to look closely } const tableAccess = await this.getTableAccess(docSession, tableId); - const accessCheck = getAccessForActionType(a, 'fatal'); + const accessCheck = await this._getAccessForActionType(docSession, a, 'fatal'); accessCheck.get(tableAccess); // will throw if access denied. return true; } else { @@ -613,7 +613,7 @@ export class GranularAccess implements GranularAccessForBundle { * accessCheck may throw if denials are fatal. */ private _pruneColumns(a: DocAction, permInfo: IPermissionInfo, tableId: string, - accessCheck: AccessCheck): DocAction|null { + accessCheck: IAccessCheck): DocAction|null { if (a[0] === 'RemoveRecord' || a[0] === 'BulkRemoveRecord') { return a; } else if (a[0] === 'AddRecord' || a[0] === 'BulkAddRecord' || a[0] === 'UpdateRecord' || @@ -759,7 +759,7 @@ export class GranularAccess implements GranularAccessForBundle { * Like _pruneRows, but fails immediately if access to any row is forbidden. * The accessCheck supplied should throw an error on denial. */ - private async _checkRows(cursor: ActionCursor, accessCheck: AccessCheck): Promise { + private async _checkRows(cursor: ActionCursor, accessCheck: IAccessCheck): Promise { const {action} = cursor; // This check applies to data changes only. if (!isDataAction(action)) { return; } @@ -808,7 +808,7 @@ export class GranularAccess implements GranularAccessForBundle { * Modify action in place, scrubbing any rows and cells to which access is not granted. */ private async _filterRowsAndCells(cursor: ActionCursor, rowsBefore: TableDataAction, rowsAfter: TableDataAction, - accessCheck: AccessCheck) { + accessCheck: IAccessCheck) { const ruler = await this._getRuler(cursor); const {docSession, action} = cursor; if (action && isSchemaAction(action)) { @@ -1329,8 +1329,8 @@ export class GranularAccess implements GranularAccessForBundle { } private async _checkIncomingDocAction(cursor: ActionCursor): Promise { - const {action} = cursor; - const accessCheck = getAccessForActionType(action, 'fatal'); + const {action, docSession} = cursor; + const accessCheck = await this._getAccessForActionType(docSession, action, 'fatal'); const tableId = getTableId(action); const permInfo = await this._getStepAccess(cursor); const tableAccess = permInfo.getTableAccess(tableId); @@ -1372,6 +1372,32 @@ export class GranularAccess implements GranularAccessForBundle { const steps = await this._getMetaSteps(); return steps[cursor.actionIdx]; } + + // Get an AccessCheck appropriate for the specific action. + // TODO: deal with ReplaceTableData, which both deletes and creates rows. + private async _getAccessForActionType(docSession: OptDocSession, a: DocAction, + severity: 'check'|'fatal'): Promise { + const tableId = getTableId(a); + if (STRUCTURAL_TABLES.has(tableId)) { + // Special case: ensure owners always have full access to ACL tables, so they + // can change rules and don't get stuck. + if (isAclTable(tableId) && await this.isOwner(docSession)) { + return { + get() { return 'allow'; } + }; + } + // Otherwise, access to structural tables currently follows the schemaEdit flag. + return accessChecks[severity].schemaEdit; + } else if (a[0] === 'UpdateRecord' || a[0] === 'BulkUpdateRecord') { + return accessChecks[severity].update; + } else if (a[0] === 'RemoveRecord' || a[0] === 'BulkRemoveRecord') { + return accessChecks[severity].delete; + } else if (a[0] === 'AddRecord' || a[0] === 'BulkAddRecord') { + return accessChecks[severity].create; + } else { + return accessChecks[severity].schemaEdit; + } + } } /** @@ -1529,8 +1555,11 @@ class UserAttributes { public override?: UserOverride; } +interface IAccessCheck { + get(ps: PermissionSetWithContext): string; +} -class AccessCheck { +class AccessCheck implements IAccessCheck { constructor(public access: 'update'|'delete'|'create'|'schemaEdit'|'read', public severity: 'check'|'fatal') { } @@ -1562,21 +1591,6 @@ export const accessChecks = { // 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, severity: 'check'|'fatal'): AccessCheck { - if (STRUCTURAL_TABLES.has(getTableId(a))) { - return accessChecks[severity].schemaEdit; - } else if (a[0] === 'UpdateRecord' || a[0] === 'BulkUpdateRecord') { - return accessChecks[severity].update; - } else if (a[0] === 'RemoveRecord' || a[0] === 'BulkRemoveRecord') { - return accessChecks[severity].delete; - } else if (a[0] === 'AddRecord' || a[0] === 'BulkAddRecord') { - return accessChecks[severity].create; - } else { - return accessChecks[severity].schemaEdit; - } -} /** * Manage censoring metadata.