From 9d1bc5a518af4ff31f81046b39ba01515291eadd Mon Sep 17 00:00:00 2001 From: Paul Fitzpatrick Date: Thu, 25 Mar 2021 13:37:09 -0400 Subject: [PATCH] (core) make AccessRules and FullCopies effective Summary: This allows `*SPECIAL:AccessRules` to give read access to the access rules to more users, and `*SPECIAL:FullCopies` to grant download/copy rights to more users. This diff also changes forks to be owned by the user who forked them (previously they were an editor), since that feels more natural. Test Plan: Added and updated tests. Reviewers: dsagal Reviewed By: dsagal Differential Revision: https://phab.getgrist.com/D2760 --- app/client/aclui/ACLUsers.ts | 6 +++- app/client/aclui/AccessRules.ts | 7 ++-- app/client/models/DocModel.ts | 2 ++ app/client/models/entities/ACLRuleRec.ts | 7 ++++ app/client/ui/ShareMenu.ts | 2 +- app/client/ui/Tools.ts | 25 +++++++++----- app/common/ACLRuleCollection.ts | 13 +++++--- app/gen-server/lib/HomeDBManager.ts | 7 ++-- app/server/lib/ActiveDoc.ts | 11 +++---- app/server/lib/GranularAccess.ts | 42 +++++++++++++++++++----- 10 files changed, 86 insertions(+), 36 deletions(-) create mode 100644 app/client/models/entities/ACLRuleRec.ts diff --git a/app/client/aclui/ACLUsers.ts b/app/client/aclui/ACLUsers.ts index e7875319..c6803a76 100644 --- a/app/client/aclui/ACLUsers.ts +++ b/app/client/aclui/ACLUsers.ts @@ -57,7 +57,11 @@ export class ACLUsersPopup extends Disposable { public async init(pageModel: DocPageModel) { this._currentUser = pageModel.userOverride.get()?.user || pageModel.appModel.currentValidUser; const doc = pageModel.currentDoc.get(); - if (doc) { + // Disabling "View as user" for forks for the moment. The getDocAccess endpoint + // only succeeds for documents that exist in the DB currently. + // TODO: modify the getDocAccess endpoint to accept forks, through the kind of + // manipulation that getDoc does. Then we can enable this button for forks. + if (doc && !doc.isFork) { const permissionData = await pageModel.appModel.api.getDocAccess(doc.id); if (this.isDisposed()) { return; } this._usersInDoc = permissionData.users.map(user => ({ diff --git a/app/client/aclui/AccessRules.ts b/app/client/aclui/AccessRules.ts index 5183d0e5..34b5452f 100644 --- a/app/client/aclui/AccessRules.ts +++ b/app/client/aclui/AccessRules.ts @@ -318,9 +318,10 @@ export class AccessRules extends Disposable { ), ), bigBasicButton('Add User Attributes', dom.on('click', () => this._addUserAttributes())), - bigBasicButton('Users', cssDropdownIcon('Dropdown'), elem => this._aclUsersPopup.attachPopup(elem), - dom.style('visibility', use => use(this._aclUsersPopup.isInitialized) ? '' : 'hidden'), - ), + !this._gristDoc.docPageModel.isFork.get() ? + bigBasicButton('Users', cssDropdownIcon('Dropdown'), elem => this._aclUsersPopup.attachPopup(elem), + dom.style('visibility', use => use(this._aclUsersPopup.isInitialized) ? '' : 'hidden'), + ) : null, ), cssConditionError(dom.text(this._errorMessage), {style: 'margin-left: 16px'}, testId('access-rules-error') diff --git a/app/client/models/DocModel.ts b/app/client/models/DocModel.ts index ebaeab51..c74cb747 100644 --- a/app/client/models/DocModel.ts +++ b/app/client/models/DocModel.ts @@ -24,6 +24,7 @@ import * as rowset from 'app/client/models/rowset'; import {RowId} from 'app/client/models/rowset'; import {schema, SchemaTypes} from 'app/common/schema'; +import {ACLRuleRec, createACLRuleRec} from 'app/client/models/entities/ACLRuleRec'; import {ColumnRec, createColumnRec} from 'app/client/models/entities/ColumnRec'; import {createDocInfoRec, DocInfoRec} from 'app/client/models/entities/DocInfoRec'; import {createPageRec, PageRec} from 'app/client/models/entities/PageRec'; @@ -111,6 +112,7 @@ export class DocModel { public validations: MTM = this._metaTableModel("_grist_Validations", createValidationRec); public replHist: MTM = this._metaTableModel("_grist_REPL_Hist", createREPLRec); public pages: MTM = this._metaTableModel("_grist_Pages", createPageRec); + public rules: MTM = this._metaTableModel("_grist_ACLRules", createACLRuleRec); public allTables: KoArray; public allTableIds: KoArray; diff --git a/app/client/models/entities/ACLRuleRec.ts b/app/client/models/entities/ACLRuleRec.ts new file mode 100644 index 00000000..f3ebc797 --- /dev/null +++ b/app/client/models/entities/ACLRuleRec.ts @@ -0,0 +1,7 @@ +import {DocModel, IRowModel} from 'app/client/models/DocModel'; + +export type ACLRuleRec = IRowModel<"_grist_ACLRules">; + +export function createACLRuleRec(this: ACLRuleRec, docModel: DocModel): void { + // currently don't care much about content. +} diff --git a/app/client/ui/ShareMenu.ts b/app/client/ui/ShareMenu.ts index cd96f6af..3c68e3b3 100644 --- a/app/client/ui/ShareMenu.ts +++ b/app/client/ui/ShareMenu.ts @@ -123,7 +123,7 @@ function shareButton(buttonText: string|null, menuCreateFunc: MenuCreateFunc, function menuManageUsers(doc: DocInfo, pageModel: DocPageModel) { return [ menuItem(() => manageUsers(doc, pageModel), 'Manage Users', - dom.cls('disabled', !roles.canEditAccess(doc.access)), + dom.cls('disabled', !roles.canEditAccess(doc.access) || doc.isFork), testId('tb-share-option') ), menuDivider(), diff --git a/app/client/ui/Tools.ts b/app/client/ui/Tools.ts index a1b93d05..4a90af7e 100644 --- a/app/client/ui/Tools.ts +++ b/app/client/ui/Tools.ts @@ -9,7 +9,7 @@ import { colors } from 'app/client/ui2018/cssVars'; import { icon } from 'app/client/ui2018/icons'; import { cssLink } from 'app/client/ui2018/links'; import { userOverrideParams } from 'app/common/gristUrls'; -import { Disposable, dom, makeTestId, Observable, styled } from "grainjs"; +import { Disposable, dom, makeTestId, Observable, observable, styled } from "grainjs"; const testId = makeTestId('test-tools-'); @@ -17,7 +17,13 @@ export function tools(owner: Disposable, gristDoc: GristDoc, leftPanelOpen: Obse const aclUIEnabled = Boolean(urlState().state.get().params?.aclUI); const isOwner = gristDoc.docPageModel.currentDoc.get()?.access === 'owners'; const isOverridden = Boolean(gristDoc.docPageModel.userOverride.get()); - const canUseAccessRules = isOwner && !isOverridden; + const canViewAccessRules = observable(false); + function updateCanViewAccessRules() { + canViewAccessRules.set((isOwner && !isOverridden) || + gristDoc.docModel.rules.getNumRows() > 0); + } + owner.autoDispose(gristDoc.docModel.rules.tableData.tableActionEmitter.addListener(updateCanViewAccessRules)); + updateCanViewAccessRules(); return cssTools( cssTools.cls('-collapsed', (use) => !use(leftPanelOpen)), cssSectionHeader("TOOLS"), @@ -25,12 +31,15 @@ export function tools(owner: Disposable, gristDoc: GristDoc, leftPanelOpen: Obse (aclUIEnabled ? cssPageEntry( cssPageEntry.cls('-selected', (use) => use(gristDoc.activeViewId) === 'acl'), - cssPageEntry.cls('-disabled', !isOwner), - cssPageLink(cssPageIcon('EyeShow'), - cssLinkText('Access Rules'), - canUseAccessRules ? urlState().setLinkUrl({docPage: 'acl'}) : null, - isOverridden ? addRevertViewAsUI() : null, - ), + cssPageEntry.cls('-disabled', (use) => !use(canViewAccessRules)), + dom.domComputed(canViewAccessRules, (_canViewAccessRules) => { + return cssPageLink( + cssPageIcon('EyeShow'), + cssLinkText('Access Rules'), + _canViewAccessRules ? urlState().setLinkUrl({docPage: 'acl'}) : null, + isOverridden ? addRevertViewAsUI() : null, + ); + }), testId('access-rules'), ) : null diff --git a/app/common/ACLRuleCollection.ts b/app/common/ACLRuleCollection.ts index d72da5a1..c66b8e68 100644 --- a/app/common/ACLRuleCollection.ts +++ b/app/common/ACLRuleCollection.ts @@ -44,8 +44,8 @@ const SPECIAL_RULE_SETS: Record = { }, { aclFormula: "", matchFunc: defaultMatchFunc, - permissions: parsePermissions('none'), - permissionsText: 'none', + permissions: parsePermissions('-R'), + permissionsText: '-R', }], }, FullCopies: { @@ -59,8 +59,8 @@ const SPECIAL_RULE_SETS: Record = { }, { aclFormula: "", matchFunc: defaultMatchFunc, - permissions: parsePermissions('none'), - permissionsText: 'none', + permissions: parsePermissions('-R'), + permissionsText: '-R', }], } }; @@ -100,6 +100,9 @@ export class ACLRuleCollection { // Maps 'tableId:colId' to one of the RuleSets in the list _columnRuleSets.get(tableId). private _tableColumnMap = new Map(); + // Rules for SPECIAL_RULES_TABLE_ID "columns". + private _specialRuleSets = new Map(); + // Map of tableId to the single default RuleSet for the table (colIds of '*') private _tableRuleSets = new Map(); @@ -119,6 +122,7 @@ export class ACLRuleCollection { // Return the RuleSet for "tableId:colId", or undefined if there isn't one for this column. public getColumnRuleSet(tableId: string, colId: string): RuleSet|undefined { + if (tableId === SPECIAL_RULES_TABLE_ID) { return this._specialRuleSets.get(colId); } return this._tableColumnMap.get(`${tableId}:${colId}`); } @@ -220,6 +224,7 @@ export class ACLRuleCollection { this._defaultRuleSet = defaultRuleSet; this._tableIds = [...tableIds]; this._userAttributeRules = userAttributeMap; + this._specialRuleSets = specialRuleSets; } /** diff --git a/app/gen-server/lib/HomeDBManager.ts b/app/gen-server/lib/HomeDBManager.ts index 500c1865..b8065d50 100644 --- a/app/gen-server/lib/HomeDBManager.ts +++ b/app/gen-server/lib/HomeDBManager.ts @@ -983,19 +983,16 @@ export class HomeDBManager extends EventEmitter { doc.trunkAccess = doc.access; // Forks without a user id are editable by anyone with view access to the trunk. - if (forkUserId === undefined && doc.access === 'viewers') { doc.access = 'editors'; } + if (forkUserId === undefined && roles.canView(doc.access)) { doc.access = 'owners'; } if (forkUserId !== undefined) { // A fork user id is known, so only that user should get to edit the fork. if (userId === forkUserId) { - // Promote to editor if just a viewer of the trunk. - if (doc.access === 'viewers') { doc.access = 'editors'; } + if (roles.canView(doc.access)) { doc.access = 'owners'; } } else { // reduce to viewer if not already viewer doc.access = roles.getWeakestRole('viewers', doc.access); } } - // No-one may be an owner of a fork, since there's no way to set up ACLs for it. - if (doc.access === 'owners') { doc.access = 'editors'; } // Finally, if we are viewing a snapshot, we can't edit it. if (snapshotId) { diff --git a/app/server/lib/ActiveDoc.ts b/app/server/lib/ActiveDoc.ts index 2e63968d..d438e06c 100644 --- a/app/server/lib/ActiveDoc.ts +++ b/app/server/lib/ActiveDoc.ts @@ -697,9 +697,8 @@ export class ActiveDoc extends EventEmitter { */ public async findColFromValues(docSession: DocSession, values: any[], n: number, optTableId?: string): Promise { - // This could leak information about private tables, so if user cannot read entire - // document, do nothing. - if (!await this._granularAccess.canReadEverything(docSession)) { return []; } + // This could leak information about private tables, so check for permission. + if (!await this._granularAccess.canScanData(docSession)) { return []; } this.logInfo(docSession, "findColFromValues(%s, %s, %s)", docSession, values, n); await this.waitForInitialization(); return this._pyCall('find_col_from_values', values, n, optTableId); @@ -843,7 +842,7 @@ export class ActiveDoc extends EventEmitter { public async autocomplete(docSession: DocSession, txt: string, tableId: string): Promise { // Autocompletion can leak names of tables and columns. - if (!await this._granularAccess.canReadEverything(docSession)) { return []; } + if (!await this._granularAccess.canScanData(docSession)) { return []; } await this.waitForInitialization(); return this._pyCall('autocomplete', txt, tableId); } @@ -895,7 +894,7 @@ export class ActiveDoc extends EventEmitter { const user = getDocSessionUser(docSession); // For now, fork only if user can read everything (or is owner). // TODO: allow forks with partial content. - if (!user || !await this._granularAccess.canCopyEverything(docSession)) { + if (!user || !await this.canDownload(docSession)) { throw new ApiError('Insufficient access to document to copy it entirely', 403); } const userId = user.id; @@ -960,7 +959,7 @@ export class ActiveDoc extends EventEmitter { * regardless of rules that may block access to them. */ public async getAclResources(docSession: DocSession): Promise<{[tableId: string]: string[]}> { - if (await this._granularAccess.hasNuancedAccess(docSession) || !this.docData) { + if (!this.docData || !await this._granularAccess.hasAccessRulesPermission(docSession)) { throw new Error('Cannot list ACL resources'); } const result: {[tableId: string]: string[]} = {}; diff --git a/app/server/lib/GranularAccess.ts b/app/server/lib/GranularAccess.ts index 3d20e6c3..88ec4fdb 100644 --- a/app/server/lib/GranularAccess.ts +++ b/app/server/lib/GranularAccess.ts @@ -1,5 +1,5 @@ import { ALL_PERMISSION_PROPS } from 'app/common/ACLPermissions'; -import { ACLRuleCollection } from 'app/common/ACLRuleCollection'; +import { ACLRuleCollection, SPECIAL_RULES_TABLE_ID } from 'app/common/ACLRuleCollection'; import { ActionGroup } from 'app/common/ActionGroup'; import { createEmptyActionSummary } from 'app/common/ActionSummary'; import { Query } from 'app/common/ActiveDocAPI'; @@ -400,6 +400,23 @@ export class GranularAccess implements GranularAccessForBundle { return !await this.hasFullAccess(docSession); } + /** + * Check if user is explicitly permitted to download/copy document. + * They may be allowed to download in any case, see canCopyEverything. + */ + public async hasFullCopiesPermission(docSession: OptDocSession): Promise { + const permInfo = await this._getAccess(docSession); + return permInfo.getColumnAccess(SPECIAL_RULES_TABLE_ID, 'FullCopies').perms.read === 'allow'; + } + + /** + * Check if user may view Access Rules. + */ + public async hasAccessRulesPermission(docSession: OptDocSession): Promise { + const permInfo = await this._getAccess(docSession); + return permInfo.getColumnAccess(SPECIAL_RULES_TABLE_ID, 'AccessRules').perms.read === 'allow'; + } + /** * Check whether user can read everything in document. Checks both home-level and doc-level * permissions. @@ -411,6 +428,14 @@ export class GranularAccess implements GranularAccessForBundle { return this.getReadPermission(permInfo.getFullAccess()) === 'allow'; } + /** + * An odd little right for findColFromValues and autocomplete. Allow if user can read + * all data, or is an owner. Might be worth making a special permission. + */ + public async canScanData(docSession: OptDocSession): Promise { + return await this.isOwner(docSession) || await this.canReadEverything(docSession); + } + /** * Check whether user can copy everything in document. Owners can always copy * everything, even if there are rules that specify they cannot. @@ -422,7 +447,8 @@ export class GranularAccess implements GranularAccessForBundle { * just a bit inconsistent. */ public async canCopyEverything(docSession: OptDocSession): Promise { - return (await this.isOwner(docSession)) || (await this.canReadEverything(docSession)); + return await this.hasFullCopiesPermission(docSession) || + await this.canReadEverything(docSession); } /** @@ -469,7 +495,7 @@ export class GranularAccess implements GranularAccessForBundle { const permInfo = await this._getAccess(docSession); const censor = new CensorshipInfo(permInfo, this._ruler.ruleCollection, tables, - await this.isOwner(docSession)); + await this.hasAccessRulesPermission(docSession)); for (const tableId of STRUCTURAL_TABLES) { censor.apply(tables[tableId]); @@ -1181,7 +1207,7 @@ export class GranularAccess implements GranularAccessForBundle { const censor = new CensorshipInfo(permissionInfo, ruler.ruleCollection, step.metaAfter, - await this.isOwner(cursor.docSession)); + await this.hasAccessRulesPermission(cursor.docSession)); if (censor.apply(act)) { results.push(act); } @@ -1193,7 +1219,7 @@ export class GranularAccess implements GranularAccessForBundle { const censorBefore = new CensorshipInfo(permissionInfo, ruler.ruleCollection, step.metaBefore, - await this.isOwner(cursor.docSession)); + await this.hasAccessRulesPermission(cursor.docSession)); // For all views previously censored, if they are now uncensored, // add an UpdateRecord to expose them. for (const v of censorBefore.censoredViews) { @@ -1478,7 +1504,7 @@ export class CensorshipInfo { public constructor(permInfo: PermissionInfo, ruleCollection: ACLRuleCollection, tables: {[key: string]: TableDataAction}, - private _isOwner: boolean) { + private _canViewACLs: boolean) { // Collect a list of censored columns (by " "). const columnCode = (tableRef: number, colId: string) => `${tableRef} ${colId}`; const censoredColumnCodes: Set = new Set(); @@ -1550,11 +1576,11 @@ export class CensorshipInfo { const ids = getRowIdsFromDocAction(a); if (!STRUCTURAL_TABLES.has(tableId)) { return true; } if (!(tableId in this.censored)) { - if (!this._isOwner && a[0] === 'TableData') { + if (!this._canViewACLs && a[0] === 'TableData') { a[2] = []; a[3] = {}; } - return this._isOwner; + return this._canViewACLs; } const rec = new RecordEditor(a, undefined, true); const method = getCensorMethod(getTableId(a));