(core) Disallow the combination of Public Edit access and granular ACLs.

Summary:
- When Public Edit access is enabled, Access Rules page shows a warning and
  prevents saving rules.
- When any ACL rules are present, attempts to set Public access to Editor role
  get downgraded to Viewer role, with a warning notification.
- No checks are made on the server side, so the combination may be achieved via
  the API (but we may block it in the future).

Test Plan: Added a test case.

Reviewers: paulfitz

Reviewed By: paulfitz

Differential Revision: https://phab.getgrist.com/D2767

app/client/aclui/ACLUsers.ts

@@ -10,7 +10,8 @@ import {menuCssClass} from 'app/client/ui2018/menus';
 import {userOverrideParams} from 'app/common/gristUrls';
 import {FullUser} from 'app/common/LoginSessionAPI';
 import * as roles from 'app/common/roles';
-import {ANONYMOUS_USER_EMAIL, EVERYONE_EMAIL, getRealAccess, UserAccessData} from 'app/common/UserAPI';
+import {ANONYMOUS_USER_EMAIL, EVERYONE_EMAIL} from 'app/common/UserAPI';
+import {getRealAccess, PermissionData, UserAccessData} from 'app/common/UserAPI';
 import {Disposable, dom, Observable, styled} from 'grainjs';
 import {cssMenu, cssMenuWrap, defaultMenuOptions, IOpenController, setPopupToCreateDom} from 'popweasel';
 
@@ -54,16 +55,9 @@ export class ACLUsersPopup extends Disposable {
   private _usersInDoc: UserAccessData[] = [];
   private _currentUser: FullUser|null = null;
 
-  public async init(pageModel: DocPageModel) {
+  public init(pageModel: DocPageModel, permissionData: PermissionData|null) {
     this._currentUser = pageModel.userOverride.get()?.user || pageModel.appModel.currentValidUser;
-    const doc = pageModel.currentDoc.get();
+    if (permissionData) {
-    // 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 => ({
         ...user,
         access: getRealAccess(user, permissionData),

app/client/aclui/AccessRules.ts

@@ -26,7 +26,9 @@ import {FormulaProperties, RulePart, RuleSet, UserAttributeRule} from 'app/commo
 import {getFormulaProperties} from 'app/common/GranularAccessClause';
 import {isHiddenCol} from 'app/common/gristTypes';
 import {isObject} from 'app/common/gutil';
+import * as roles from 'app/common/roles';
 import {SchemaTypes} from 'app/common/schema';
+import {ANONYMOUS_USER_EMAIL, EVERYONE_EMAIL, getRealAccess} from 'app/common/UserAPI';
 import {BaseObservable, Computed, Disposable, MutableObsArray, obsArray, Observable} from 'grainjs';
 import {dom, DomElementArg, IDisposableOwner, styled} from 'grainjs';
 import isEqual = require('lodash/isEqual');
@@ -91,6 +93,8 @@ export class AccessRules extends Disposable {
 
   private _aclUsersPopup = ACLUsersPopup.create(this);
 
+  private _publicEditAccess = Observable.create(this, false);
+
   constructor(private _gristDoc: GristDoc) {
     super();
     this._ruleStatus = Computed.create(this, (use) => {
@@ -110,7 +114,8 @@ export class AccessRules extends Disposable {
       );
     });
 
-    this._savingEnabled = Computed.create(this, this._ruleStatus, (use, s) => (s === RuleStatus.ChangedValid));
+    this._savingEnabled = Computed.create(this, this._ruleStatus, (use, s) =>
+      (s === RuleStatus.ChangedValid) && !use(this._publicEditAccess));
 
     this._userAttrChoices = Computed.create(this, this._userAttrRules, (use, rules) => {
       const result: IAttrOption[] = [
@@ -137,22 +142,12 @@ export class AccessRules extends Disposable {
     for (const tableId of ['_grist_ACLResources', '_grist_ACLRules']) {
       const tableData = this._gristDoc.docData.getTable(tableId)!;
       this.autoDispose(tableData.tableActionEmitter.addListener(this._onChange, this));
+      this.autoDispose(this._gristDoc.docPageModel.currentDoc.addListener(this._updateDocAccessData, this));
     }
 
     this.update().catch((e) => this._errorMessage.set(e.message));
   }
 
-  public _onChange() {
-    if (this._ruleStatus.get() === RuleStatus.Unchanged) {
-      // If no changes, it's safe to just reload the rules from docData.
-      this.update().catch((e) => this._errorMessage.set(e.message));
-    } else {
-      this._errorMessage.set(
-        'Access rules have changed. Click Reset to revert your changes and refresh the rules.'
-      );
-    }
-  }
-
   public get allTableIds() { return Object.keys(this._aclResources).sort(); }
   public get userAttrRules() { return this._userAttrRules; }
   public get userAttrChoices() { return this._userAttrChoices; }
@@ -164,11 +159,13 @@ export class AccessRules extends Disposable {
     if (this.isDisposed()) { return; }
     this._errorMessage.set('');
     const rules = this._ruleCollection;
+
     [ , , this._aclResources] = await Promise.all([
       rules.update(this._gristDoc.docData, {log: console}),
-      this._aclUsersPopup.init(this._gristDoc.docPageModel),
+      this._updateDocAccessData(),
       this._gristDoc.docComm.getAclResources(),
     ]);
+    if (this.isDisposed()) { return; }
 
     this._tableRules.set(
       rules.getAllTableIds()
@@ -292,7 +289,7 @@ export class AccessRules extends Disposable {
           dom.text((use) => {
             const s = use(this._ruleStatus);
             return s === RuleStatus.CheckPending ? 'Checking...' :
-              s === RuleStatus.Invalid ? 'Invalid' : 'Saved';
+              s === RuleStatus.Unchanged ? 'Saved' : 'Invalid';
           }),
           testId('rules-non-save')
         ),
@@ -318,12 +315,19 @@ export class AccessRules extends Disposable {
           ),
         ),
         bigBasicButton('Add User Attributes', dom.on('click', () => this._addUserAttributes())),
+        // Disabling "View as user" for forks for the moment. TODO Modify getDocAccess endpoint
+        // to accept forks, through the kind of manipulation that getDoc does; then can enable.
         !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'},
+      cssConditionError({style: 'margin-left: 16px'},
+        dom.maybe(this._publicEditAccess, () => dom('div',
+          'Public "Editor" access is incompatible with Access Rules. ' +
+          'To set rules, remove it or reduce to "Viewer".'
+        )),
+        dom.text(this._errorMessage),
         testId('access-rules-error')
       ),
       shadowScroll(
@@ -427,6 +431,38 @@ export class AccessRules extends Disposable {
   private _addUserAttributes() {
     this._userAttrRules.push(ObsUserAttributeRule.create(this._userAttrRules, this, undefined, {focus: true}));
   }
+
+  private _onChange() {
+    if (this._ruleStatus.get() === RuleStatus.Unchanged) {
+      // If no changes, it's safe to just reload the rules from docData.
+      this.update().catch((e) => this._errorMessage.set(e.message));
+    } else {
+      this._errorMessage.set(
+        'Access rules have changed. Click Reset to revert your changes and refresh the rules.'
+      );
+    }
+  }
+
+  private async _updateDocAccessData() {
+    const pageModel = this._gristDoc.docPageModel;
+    const doc = pageModel.currentDoc.get();
+
+    // Note that the getDocAccess endpoint does not succeed for forks currently.
+    const permissionData = doc && !doc.isFork ? await pageModel.appModel.api.getDocAccess(doc.id) : null;
+    if (this.isDisposed()) { return; }
+
+    this._aclUsersPopup.init(pageModel, permissionData);
+
+    // We do not allow Public Editor access in combination with Granular ACL rules. When
+    // _publicEditAccess is on, we show a warning and prevent saving rules.
+    if (permissionData) {
+      const publicEditAccess = permissionData.users.some(user => (
+        (user.email === EVERYONE_EMAIL || user.email === ANONYMOUS_USER_EMAIL) &&
+        roles.canEdit(getRealAccess(user, permissionData))
+      ));
+      this._publicEditAccess.set(publicEditAccess);
+    }
+  }
 }
 
 // Represents all rules for a table.

app/client/components/GristDoc.ts

@@ -522,6 +522,14 @@ export class GristDoc extends DisposableWithEvents {
     });
   }
 
+  public hasGranularAccessRules(): boolean {
+    const rulesTable = this.docData.getTable('_grist_ACLRules')!;
+    // To check if there are rules, ignore the default no-op rule created for an older incarnation
+    // of ACLs. It exists in older documents, and is still created for new ones. We detect it by
+    // the use of the deprecated 'permissions' field, and not the new 'permissionsText' field.
+    return rulesTable.numRecords() > rulesTable.filterRowIds({permissionsText: '', permissions: 63}).length;
+  }
+
   private _getToolContent(tool: typeof RightPanelTool.type): IExtraTool|null {
     switch (tool) {
       case 'docHistory': {

app/client/models/NotifyModel.ts

@@ -96,7 +96,7 @@ export class Notification extends Expirable implements INotification {
 
   constructor(_opts: INotifyOptions) {
     super();
-    this.options = defaults({}, _opts, this.options)
+    this.options = defaults({}, _opts, this.options);
 
     if (this.options.expireSec > 0) {
       const expireTimer = setTimeout(() => this.expire(), 1000 * this.options.expireSec);

app/client/models/UserManagerModel.ts

@@ -1,3 +1,5 @@
+import {DocPageModel} from 'app/client/models/DocPageModel';
+import {reportError, UserError} from 'app/client/models/errors';
 import {normalizeEmail} from 'app/common/emails';
 import {GristLoadConfig} from 'app/common/gristUrls';
 import * as roles from 'app/common/roles';
@@ -120,7 +122,8 @@ export class UserManagerModelImpl extends Disposable implements UserManagerModel
   constructor(
     public initData: PermissionData,
     public resourceType: ResourceType,
-    private _activeUserEmail: string|null
+    private _activeUserEmail: string|null,
+    private _docPageModel?: DocPageModel,
   ) {
     super();
   }
@@ -194,13 +197,20 @@ export class UserManagerModelImpl extends Disposable implements UserManagerModel
         delta.maxInheritedRole = maxInheritedRole;
       }
     }
-    // Looping through the members has the side effect of updating the delta.
     const members = [...this.membersEdited.get()];
     if (this.publicMember) {
       members.push(this.publicMember);
     }
-    members.forEach((m, i) => {
+    // Loop through the members and update the delta.
-      const access = m.access.get();
+    for (const m of members) {
+      let access = m.access.get();
+      if (m === this.publicMember && access === roles.EDITOR &&
+          this._docPageModel?.gristDoc.get()?.hasGranularAccessRules()) {
+        access = roles.VIEWER;
+        reportError(new UserError(
+          'Public "Editor" access is incompatible with Access Rules. Reduced to "Viewer".'
+        ));
+      }
       if (!roles.isValidRole(access)) {
         throw new Error(`Cannot update user to invalid role ${access}`);
       }
@@ -208,7 +218,7 @@ export class UserManagerModelImpl extends Disposable implements UserManagerModel
         // Add users whose access changed.
         delta.users![m.email] = m.isRemoved ? null : access as roles.NonGuestRole;
       }
-    });
+    }
     return delta;
   }
 

app/client/ui/UserManager.ts

@@ -45,7 +45,8 @@ export interface IUserManagerOptions {
 // required properties of the options.
 async function getModel(options: IUserManagerOptions): Promise<UserManagerModelImpl> {
   const permissionData = await options.permissionData;
-  return new UserManagerModelImpl(permissionData, options.resourceType, options.activeEmail);
+  return new UserManagerModelImpl(permissionData, options.resourceType, options.activeEmail,
+    options.docPageModel);
 }
 
 /**