diff --git a/app/client/aclui/AccessRules.ts b/app/client/aclui/AccessRules.ts index 4c578023..81709ee5 100644 --- a/app/client/aclui/AccessRules.ts +++ b/app/client/aclui/AccessRules.ts @@ -15,8 +15,8 @@ import {colors, testId} from 'app/client/ui2018/cssVars'; import {textInput} from 'app/client/ui2018/editableLabel'; import {cssIconButton, icon} from 'app/client/ui2018/icons'; import {IOptionFull, menu, menuItemAsync} from 'app/client/ui2018/menus'; -import {emptyPermissionSet} from 'app/common/ACLPermissions'; -import {PartialPermissionSet, permissionSetToText} from 'app/common/ACLPermissions'; +import {emptyPermissionSet, MixedPermissionValue} from 'app/common/ACLPermissions'; +import {PartialPermissionSet, permissionSetToText, summarizePermissions, summarizePermissionSet} from 'app/common/ACLPermissions'; import {ACLRuleCollection} from 'app/common/ACLRuleCollection'; import {BulkColValues, RowRecord, UserAction} from 'app/common/DocActions'; import {RulePart, RuleSet, UserAttributeRule} from 'app/common/GranularAccessClause'; @@ -490,17 +490,40 @@ class TableRules extends Disposable { */ public getResources(): ResourceRec[] { // Check that the colIds are valid. - const seen = new Set(); + const seen = { + allow: new Set(), // columns mentioned in rules that only have 'allow's. + deny: new Set(), // columns mentioned in rules that only have 'deny's. + mixed: new Set() // columns mentioned in any rules. + }; for (const ruleSet of this._columnRuleSets.get()) { + const sign = ruleSet.summarizePermissions(); + const counterSign = sign === 'mixed' ? 'mixed' : (sign === 'allow' ? 'deny' : 'allow'); const colIds = ruleSet.getColIdList(); if (colIds.length === 0) { throw new UserError(`No columns listed in a column rule for table ${this.tableId}`); } for (const colId of colIds) { - if (seen.has(colId)) { - throw new UserError(`Column ${colId} appears in multiple rules for table ${this.tableId}`); + if (seen[counterSign].has(colId)) { + // There may be an order dependency between rules. We've done a little analysis, to + // allow the useful pattern of forbidding all access to columns, and then adding back + // access to different sets for different teams/conditions (or allowing all access + // by default, and then forbidding different sets). But if there's a mix of + // allows and denies, then we throw up our hands. + // TODO: could analyze more deeply. An easy step would be to analyze per permission bit. + // Could also allow order dependency and provide a way to control the order. + // TODO: could be worth also flagging multiple rulesets with the same columns as + // undesirable. + throw new UserError(`Column ${colId} appears in multiple rules for table ${this.tableId}` + + ` that might be order-dependent. Try splitting rules up differently?`); + } + if (sign === 'mixed') { + seen.allow.add(colId); + seen.deny.add(colId); + seen.mixed.add(colId); + } else { + seen[sign].add(colId); + seen.mixed.add(colId); } - seen.add(colId); } } @@ -585,6 +608,15 @@ abstract class ObsRuleSet extends Disposable { return '*'; } + /** + * Check if RuleSet may only add permissions, only remove permissions, or may do either. + * A rule that neither adds nor removes permissions is treated as mixed for simplicity, + * though this would be suboptimal if this were a useful case to support. + */ + public summarizePermissions(): MixedPermissionValue { + return summarizePermissions(this._body.get().map(p => p.summarizePermissions())); + } + public abstract buildResourceDom(): DomElementArg; public buildRuleSetDom() { @@ -891,6 +923,15 @@ class ObsRulePart extends Disposable { }; } + /** + * Check if RulePart may only add permissions, only remove permissions, or may do either. + * A rule that neither adds nor removes permissions is treated as mixed for simplicity, + * though this would be suboptimal if this were a useful case to support. + */ + public summarizePermissions(): MixedPermissionValue { + return summarizePermissionSet(this._permissions.get()); + } + public buildRulePartDom() { return cssColumnGroup( cssCellIcon( diff --git a/app/common/ACLPermissions.ts b/app/common/ACLPermissions.ts index 96e8f234..503a2596 100644 --- a/app/common/ACLPermissions.ts +++ b/app/common/ACLPermissions.ts @@ -161,3 +161,32 @@ export function mergePermissions(psets: Array>, combine: export function toMixed(pset: PartialPermissionSet): MixedPermissionSet { return mergePermissions([pset], ([bit]) => (bit === 'allow' || bit === 'mixed' ? bit : 'deny')); } + +/** + * Check if PermissionSet may only add permissions, only remove permissions, or may do either. + * A rule that neither adds nor removes permissions is treated as mixed. + */ +export function summarizePermissionSet(pset: PartialPermissionSet): MixedPermissionValue { + let sign = ''; + for (const key of Object.keys(pset) as Array) { + const pWithSome = pset[key]; + // "Some" postfix is not significant for summarization. + const p = pWithSome === 'allowSome' ? 'allow' : (pWithSome === 'denySome' ? 'deny' : pWithSome) + if (!p || p === sign) { continue; } + if (!sign) { + sign = p; + continue; + } + sign = 'mixed'; + } + return (sign === 'allow' || sign === 'deny') ? sign : 'mixed'; +} + +/** + * Summarize whether a set of permissions are all 'allow', all 'deny', or other ('mixed'). + */ +export function summarizePermissions(perms: MixedPermissionValue[]): MixedPermissionValue { + if (perms.length === 0) { return 'mixed'; } + const perm = perms[0]; + return perms.some(p => p !== perm) ? 'mixed' : perm; +}