From ca8ac806db96e207a397aa8c91c57c0653dec380 Mon Sep 17 00:00:00 2001 From: Dmitry S Date: Sun, 3 Mar 2024 22:41:48 -0500 Subject: [PATCH] (core) Trim unapplicable permissions bits for column rules, both at parse time, and in UI Summary: - UI now trims column rules before saving. - When rules are loaded, bits that aren't applicable to a resource get ignored. This should fix the incorrect behavior in existing docs without a migration. Test Plan: - Added test of UI, that it now sends trimmed rules - Added a unitteset of new trimPermissions() function - Add test of fixed interpretation of existing rules: now only permission bits applicable to a resource get respected. I.e. create/delete/schemaEdit are ignored in column rules, and schemaEdit is also ignored in table rules. - Note that DuplicateTest was affected: updated on the assumption that schemaEdit still can't actually apply at a table level. Reviewers: georgegevoian, paulfitz Reviewed By: paulfitz Differential Revision: https://phab.getgrist.com/D4205 --- app/client/aclui/AccessRules.ts | 24 +++++++++++++++++------- app/client/aclui/PermissionsWidget.ts | 5 +---- app/common/ACLPermissions.ts | 22 +++++++++++++++++++++- app/common/ACLRuleCollection.ts | 10 ++++++++-- test/chai-as-promised.js | 3 +++ test/common/ACLPermissions.ts | 15 +++++++++++++-- 6 files changed, 63 insertions(+), 16 deletions(-) diff --git a/app/client/aclui/AccessRules.ts b/app/client/aclui/AccessRules.ts index 4a3a208a..010d05b1 100644 --- a/app/client/aclui/AccessRules.ts +++ b/app/client/aclui/AccessRules.ts @@ -6,7 +6,7 @@ import {aclFormulaEditor} from 'app/client/aclui/ACLFormulaEditor'; import {aclMemoEditor} from 'app/client/aclui/ACLMemoEditor'; import {aclSelect} from 'app/client/aclui/ACLSelect'; import {ACLUsersPopup} from 'app/client/aclui/ACLUsers'; -import {PermissionKey, permissionsWidget} from 'app/client/aclui/PermissionsWidget'; +import {permissionsWidget} from 'app/client/aclui/PermissionsWidget'; import {GristDoc} from 'app/client/components/GristDoc'; import {logTelemetryEvent} from 'app/client/lib/telemetry'; import {reportError, UserError} from 'app/client/models/errors'; @@ -20,13 +20,17 @@ import {textInput} from 'app/client/ui2018/editableLabel'; import {cssIconButton, icon} from 'app/client/ui2018/icons'; import {menu, menuItemAsync} from 'app/client/ui2018/menus'; import { + AVAILABLE_BITS_COLUMNS, + AVAILABLE_BITS_TABLES, emptyPermissionSet, MixedPermissionValue, parsePermissions, PartialPermissionSet, + PermissionKey, permissionSetToText, summarizePermissions, - summarizePermissionSet + summarizePermissionSet, + trimPermissions } from 'app/common/ACLPermissions'; import {ACLRuleCollection, isSchemaEditResource, SPECIAL_RULES_TABLE_ID} from 'app/common/ACLRuleCollection'; import {AclRuleProblem, AclTableDescription, getTableTitle} from 'app/common/ActiveDocAPI'; @@ -990,12 +994,19 @@ abstract class ObsRuleSet extends Disposable { // Should not happen. continue; } + + // Include only the permissions for the bits that this RuleSet supports. E.g. this matters + // for seed rules, which may include create/delete bits which shouldn't apply to columns. + const origPermissions = parsePermissions(permissionsText); + const trimmedPermissions = trimPermissions(origPermissions, this.getAvailableBits()); + const trimmedPermissionsText = permissionSetToText(trimmedPermissions); + this.addRulePart( this.getFirst() || null, { aclFormula, - permissionsText, - permissions: parsePermissions(permissionsText), + permissionsText: trimmedPermissionsText, + permissions: trimmedPermissions, memo, }, true, @@ -1048,7 +1059,7 @@ abstract class ObsRuleSet extends Disposable { * Which permission bits to allow the user to set. */ public getAvailableBits(): PermissionKey[] { - return ['read', 'update', 'create', 'delete']; + return AVAILABLE_BITS_TABLES; } /** @@ -1117,8 +1128,7 @@ class ColumnObsRuleSet extends ObsRuleSet { } public getAvailableBits(): PermissionKey[] { - // Create/Delete bits can't be set on a column-specific rule. - return ['read', 'update']; + return AVAILABLE_BITS_COLUMNS; } public hasColumns() { diff --git a/app/client/aclui/PermissionsWidget.ts b/app/client/aclui/PermissionsWidget.ts index 0f4e379e..d5eb7ff5 100644 --- a/app/client/aclui/PermissionsWidget.ts +++ b/app/client/aclui/PermissionsWidget.ts @@ -6,15 +6,12 @@ import {colors, testId, theme} from 'app/client/ui2018/cssVars'; import {cssIconButton, icon} from 'app/client/ui2018/icons'; import {menu, menuIcon, menuItem} from 'app/client/ui2018/menus'; import {PartialPermissionSet, PartialPermissionValue} from 'app/common/ACLPermissions'; -import {ALL_PERMISSION_PROPS, emptyPermissionSet} from 'app/common/ACLPermissions'; +import {ALL_PERMISSION_PROPS, emptyPermissionSet, PermissionKey} from 'app/common/ACLPermissions'; import {capitalize} from 'app/common/gutil'; import {dom, DomElementArg, Observable, styled} from 'grainjs'; import isEqual = require('lodash/isEqual'); import {makeT} from 'app/client/lib/localization'; -// One of the strings 'read', 'update', etc. -export type PermissionKey = keyof PartialPermissionSet; - // Canonical order of permission bits when rendered in a permissionsWidget. const PERMISSION_BIT_ORDER = 'RUCDS'; diff --git a/app/common/ACLPermissions.ts b/app/common/ACLPermissions.ts index 617aae00..22e6a3e0 100644 --- a/app/common/ACLPermissions.ts +++ b/app/common/ACLPermissions.ts @@ -41,7 +41,10 @@ export type PartialPermissionSet = PermissionSet; export type MixedPermissionSet = PermissionSet; export type TablePermissionSet = PermissionSet; -const PERMISSION_BITS: {[letter: string]: keyof PermissionSet} = { +// One of the strings 'read', 'update', etc. +export type PermissionKey = keyof PermissionSet; + +const PERMISSION_BITS: {[letter: string]: PermissionKey} = { R: 'read', C: 'create', U: 'update', @@ -60,6 +63,9 @@ const ALIASES: {[key: string]: string} = { }; const REVERSE_ALIASES = fromPairs(Object.entries(ALIASES).map(([alias, value]) => [value, alias])); +export const AVAILABLE_BITS_TABLES: PermissionKey[] = ['read', 'update', 'create', 'delete']; +export const AVAILABLE_BITS_COLUMNS: PermissionKey[] = ['read', 'update']; + // Comes in useful for initializing unset PermissionSets. export function emptyPermissionSet(): PartialPermissionSet { return {read: "", create: "", update: "", delete: "", schemaEdit: ""}; @@ -141,6 +147,20 @@ export function mergePartialPermissions(a: PartialPermissionSet, b: PartialPermi return mergePermissions([a, b], ([_a, _b]) => combinePartialPermission(_a, _b)); } +/** + * Returns permissions trimmed to include only the available bits, and empty for any other bits. + */ +export function trimPermissions( + permissions: PartialPermissionSet, availableBits: PermissionKey[] +): PartialPermissionSet { + const trimmed = emptyPermissionSet(); + for (const bit of availableBits) { + trimmed[bit] = permissions[bit]; + } + return trimmed; +} + + /** * Merge a list of PermissionSets by combining individual bits. */ diff --git a/app/common/ACLRuleCollection.ts b/app/common/ACLRuleCollection.ts index 92375530..434dc151 100644 --- a/app/common/ACLRuleCollection.ts +++ b/app/common/ACLRuleCollection.ts @@ -1,4 +1,5 @@ import {parsePermissions, permissionSetToText, splitSchemaEditPermissionSet} from 'app/common/ACLPermissions'; +import {AVAILABLE_BITS_COLUMNS, AVAILABLE_BITS_TABLES, trimPermissions} from 'app/common/ACLPermissions'; import {ACLShareRules, TableWithOverlay} from 'app/common/ACLShareRules'; import {AclRuleProblem} from 'app/common/ActiveDocAPI'; import {DocData} from 'app/common/DocData'; @@ -537,13 +538,18 @@ function readAclRules(docData: DocData, {log, compile, enrichRulesForImplementat if (hasShares && rule.id >= 0) { aclFormulaParsed = shareRules.transformNonShareRules({rule, aclFormulaParsed}); } + let permissions = parsePermissions(String(rule.permissionsText)); + if (tableId !== '*' && tableId !== SPECIAL_RULES_TABLE_ID) { + const availableBits = (colIds === '*') ? AVAILABLE_BITS_TABLES : AVAILABLE_BITS_COLUMNS; + permissions = trimPermissions(permissions, availableBits); + } body.push({ origRecord: rule, aclFormula: String(rule.aclFormula), matchFunc: rule.aclFormula ? compile?.(aclFormulaParsed) : defaultMatchFunc, memo: rule.memo, - permissions: parsePermissions(String(rule.permissionsText)), - permissionsText: String(rule.permissionsText), + permissions, + permissionsText: permissionSetToText(permissions) }); } } diff --git a/test/chai-as-promised.js b/test/chai-as-promised.js index 03f5e170..b22c652e 100644 --- a/test/chai-as-promised.js +++ b/test/chai-as-promised.js @@ -2,3 +2,6 @@ const chai = require('chai'); const chaiAsPromised = require('chai-as-promised'); chai.use(chaiAsPromised); + +// By default this is false, which affects asserts like isRejected and isFulfilled. +chai.config.includeStack = true; diff --git a/test/common/ACLPermissions.ts b/test/common/ACLPermissions.ts index a74f3351..081ec5d9 100644 --- a/test/common/ACLPermissions.ts +++ b/test/common/ACLPermissions.ts @@ -1,7 +1,7 @@ -import {emptyPermissionSet, PartialPermissionSet, +import {emptyPermissionSet, PartialPermissionSet, PermissionKey, summarizePermissions, summarizePermissionSet} from 'app/common/ACLPermissions'; import {makePartialPermissions, parsePermissions, permissionSetToText} from 'app/common/ACLPermissions'; -import {mergePartialPermissions, mergePermissions} from 'app/common/ACLPermissions'; +import {mergePartialPermissions, mergePermissions, trimPermissions} from 'app/common/ACLPermissions'; import {assert} from 'chai'; describe("ACLPermissions", function() { @@ -112,6 +112,17 @@ describe("ACLPermissions", function() { ); }); + it('should support trimPermissions', function() { + const trim = (permissionsText: string, availableBits: PermissionKey[]) => + permissionSetToText(trimPermissions(parsePermissions(permissionsText), availableBits)); + assert.deepEqual(trim("+CRUD", ["read", "update"]), "+RU"); + assert.deepEqual(trim("all", ["read", "update"]), "+RU"); + assert.deepEqual(trim("-C+R-U+D-S", ["update", "read"]), "+R-U"); + assert.deepEqual(trim("none", ["read", "update", "create", "delete", "schemaEdit"]), "none"); + assert.deepEqual(trim("none", ["read", "update", "create", "delete"]), "-CRUD"); + assert.deepEqual(trim("none", ["read"]), "-R"); + }); + it ('should allow summarization of permission sets', function() { assert.deepEqual(summarizePermissionSet(parsePermissions("+U-D")), 'mixed'); assert.deepEqual(summarizePermissionSet(parsePermissions("+U+D")), 'allow');