From c37a04c5784de04debadf2bc53de6dddf144c038 Mon Sep 17 00:00:00 2001 From: Paul Fitzpatrick Date: Tue, 2 Mar 2021 16:11:37 -0500 Subject: [PATCH] (core) freshen "view as user" behavior Summary: Now as the user an owner might choose to view their document as is likely to not have access to rules, it is better to start viewing on the default document page rather than /p/acl. The "Access Rules" link is grayed out when in "view as" mode for now (improvements are planned). Test Plan: updated test Reviewers: dsagal Reviewed By: dsagal Differential Revision: https://phab.getgrist.com/D2743 --- app/client/aclui/ACLUsers.ts | 2 +- app/client/ui/Tools.ts | 8 ++++--- app/server/lib/ActiveDoc.ts | 10 ++++---- app/server/lib/DocManager.ts | 2 +- app/server/lib/GranularAccess.ts | 41 +++++++++++++++++++++----------- 5 files changed, 39 insertions(+), 24 deletions(-) diff --git a/app/client/aclui/ACLUsers.ts b/app/client/aclui/ACLUsers.ts index f821c0f3..c0e5bac8 100644 --- a/app/client/aclui/ACLUsers.ts +++ b/app/client/aclui/ACLUsers.ts @@ -40,7 +40,7 @@ function buildUserRow(user: UserAccessData, currentUser: FullUser|null, ctl: IOp testId('acl-user-view-as'), icon('FieldLink'), 'View As', { href: urlState().makeUrl( - merge({}, urlState().state.get(), {params: {linkParameters: {aclAsUser: user.email}}})), + merge({}, urlState().state.get(), {docPage: '', params: {linkParameters: {aclAsUser: user.email}}})), }), testId('acl-user-item'), ); diff --git a/app/client/ui/Tools.ts b/app/client/ui/Tools.ts index 356f23b3..fb198dd2 100644 --- a/app/client/ui/Tools.ts +++ b/app/client/ui/Tools.ts @@ -13,6 +13,8 @@ const testId = makeTestId('test-tools-'); export function tools(owner: Disposable, gristDoc: GristDoc, leftPanelOpen: Observable): Element { 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; return cssTools( cssTools.cls('-collapsed', (use) => !use(leftPanelOpen)), cssSectionHeader("TOOLS"), @@ -20,10 +22,10 @@ export function tools(owner: Disposable, gristDoc: GristDoc, leftPanelOpen: Obse (aclUIEnabled ? cssPageEntry( cssPageEntry.cls('-selected', (use) => use(gristDoc.activeViewId) === 'acl'), - cssPageEntry.cls('-disabled', !isOwner), - (isOwner ? cssPageLink : cssPageDisabledLink)(cssPageIcon('EyeShow'), + cssPageEntry.cls('-disabled', !canUseAccessRules), + (canUseAccessRules ? cssPageLink : cssPageDisabledLink)(cssPageIcon('EyeShow'), cssLinkText('Access Rules'), - isOwner ? urlState().setLinkUrl({docPage: 'acl'}) : null + canUseAccessRules ? urlState().setLinkUrl({docPage: 'acl'}) : null ), testId('access-rules'), ) : diff --git a/app/server/lib/ActiveDoc.ts b/app/server/lib/ActiveDoc.ts index b0443a00..89285e7d 100644 --- a/app/server/lib/ActiveDoc.ts +++ b/app/server/lib/ActiveDoc.ts @@ -845,8 +845,8 @@ export class ActiveDoc extends EventEmitter { return fetchURL(url, this.makeAccessId(docSession.authorizer.getUserId())); } - public forwardPluginRpc(docSession: DocSession, pluginId: string, msg: IMessage): Promise { - if (this._granularAccess.hasNuancedAccess(docSession)) { + public async forwardPluginRpc(docSession: DocSession, pluginId: string, msg: IMessage): Promise { + if (await this._granularAccess.hasNuancedAccess(docSession)) { throw new Error('cannot confirm access to plugin'); } const pluginRpc = this.docPluginManager.plugins[pluginId].rpc; @@ -876,7 +876,7 @@ export class ActiveDoc extends EventEmitter { return this.shutdown(); } - public isOwner(docSession: OptDocSession): boolean { + public isOwner(docSession: OptDocSession): Promise { return this._granularAccess.isOwner(docSession); } @@ -1036,14 +1036,14 @@ export class ActiveDoc extends EventEmitter { } public async removeSnapshots(docSession: OptDocSession, snapshotIds: string[]): Promise { - if (!this.isOwner(docSession)) { + if (!await this.isOwner(docSession)) { throw new Error('cannot remove snapshots, access denied'); } return this._docManager.storageManager.removeSnapshots(this.docName, snapshotIds); } public async deleteActions(docSession: OptDocSession, keepN: number): Promise { - if (!this.isOwner(docSession)) { + if (!await this.isOwner(docSession)) { throw new Error('cannot delete actions, access denied'); } await this._actionHistory.deleteActions(keepN); diff --git a/app/server/lib/DocManager.ts b/app/server/lib/DocManager.ts index e5564b9a..e6b143d8 100644 --- a/app/server/lib/DocManager.ts +++ b/app/server/lib/DocManager.ts @@ -382,7 +382,7 @@ export class DocManager extends EventEmitter { for (;;) { if (this._activeDocs.has(docName) && wantRecoveryMode !== undefined) { const activeDoc = await this._activeDocs.get(docName); - if (activeDoc && activeDoc.recoveryMode !== wantRecoveryMode && activeDoc.isOwner(docSession)) { + if (activeDoc && activeDoc.recoveryMode !== wantRecoveryMode && await activeDoc.isOwner(docSession)) { // shutting doc down to have a chance to re-open in the correct mode. // TODO: there could be a battle with other users opening it in a different mode. await activeDoc.shutdown(); diff --git a/app/server/lib/GranularAccess.ts b/app/server/lib/GranularAccess.ts index 929b5d73..87f072fc 100644 --- a/app/server/lib/GranularAccess.ts +++ b/app/server/lib/GranularAccess.ts @@ -206,7 +206,7 @@ export class GranularAccess implements GranularAccessForBundle { public async canApplyBundle() { if (!this._activeBundle) { throw new Error('no active bundle'); } const {docActions, docSession} = this._activeBundle; - if (this._activeBundle.hasDeliberateRuleChange && !this.isOwner(docSession)) { + if (this._activeBundle.hasDeliberateRuleChange && !await this.isOwner(docSession)) { throw new ErrorWithCode('ACL_DENY', 'Only owners can modify access rules'); } if (this._ruler.haveRules()) { @@ -360,13 +360,13 @@ export class GranularAccess implements GranularAccessForBundle { const name = a[0] as string; if (OK_ACTIONS.has(name)) { return true; } if (SPECIAL_ACTIONS.has(name)) { - if (this.hasNuancedAccess(docSession)) { + if (await this.hasNuancedAccess(docSession)) { throw new ErrorWithCode('ACL_DENY', `Blocked by access rules: '${name}' actions need uncomplicated access`); } return true; } if (SURPRISING_ACTIONS.has(name)) { - if (!this.hasFullAccess(docSession)) { + if (!await this.hasFullAccess(docSession)) { throw new ErrorWithCode('ACL_DENY', `Blocked by access rules: '${name}' actions need full access`); } return true; @@ -394,9 +394,9 @@ export class GranularAccess implements GranularAccessForBundle { * worked through. Currently if there are no owner-only tables, then everyone's * access is simple and without nuance. */ - public hasNuancedAccess(docSession: OptDocSession): boolean { + public async hasNuancedAccess(docSession: OptDocSession): Promise { if (!this._ruler.haveRules()) { return false; } - return !this.hasFullAccess(docSession); + return !await this.hasFullAccess(docSession); } /** @@ -404,7 +404,7 @@ export class GranularAccess implements GranularAccessForBundle { * permissions. */ public async canReadEverything(docSession: OptDocSession): Promise { - const access = getDocSessionAccess(docSession); + const access = await this._getNominalAccess(docSession); if (!canView(access)) { return false; } const permInfo = await this._getAccess(docSession); return this.getReadPermission(permInfo.getFullAccess()) === 'allow'; @@ -421,7 +421,7 @@ export class GranularAccess implements GranularAccessForBundle { * just a bit inconsistent. */ public async canCopyEverything(docSession: OptDocSession): Promise { - return this.isOwner(docSession) || this.canReadEverything(docSession); + return (await this.isOwner(docSession)) || (await this.canReadEverything(docSession)); } /** @@ -430,15 +430,15 @@ export class GranularAccess implements GranularAccessForBundle { * TODO: uses of this method should be checked to see if they can be fleshed out * now we have more of the ACL implementation done. */ - public hasFullAccess(docSession: OptDocSession): boolean { + public hasFullAccess(docSession: OptDocSession): Promise { return this.isOwner(docSession); } /** * Check whether user has owner-level access to the document. */ - public isOwner(docSession: OptDocSession): boolean { - const access = getDocSessionAccess(docSession); + public async isOwner(docSession: OptDocSession): Promise { + const access = await this._getNominalAccess(docSession); return access === 'owners'; } @@ -468,7 +468,7 @@ export class GranularAccess implements GranularAccessForBundle { const permInfo = await this._getAccess(docSession); const censor = new CensorshipInfo(permInfo, this._ruler.ruleCollection, tables, - this.isOwner(docSession)); + await this.isOwner(docSession)); for (const tableId of STRUCTURAL_TABLES) { censor.apply(tables[tableId]); @@ -528,6 +528,19 @@ export class GranularAccess implements GranularAccessForBundle { (docSession) => this._filterDocUpdate(docSession, message)); } + /** + * Get the role the session user has for this document. User may be overridden, + * in which case the role of the override is returned. + */ + private async _getNominalAccess(docSession: OptDocSession): Promise { + const linkParameters = docSession.authorizer?.getLinkParameters() || {}; + if (linkParameters.aclAsUserId || linkParameters.aclAsUser) { + const info = await this._getUser(docSession); + return info.Access as Role; + } + return getDocSessionAccess(docSession); + } + /** * This filters a message being broadcast to all clients to be appropriate for one * particular client, if that client may need some material filtered out. @@ -896,7 +909,7 @@ export class GranularAccess implements GranularAccessForBundle { // If aclAsUserId/aclAsUser is set, then override user for acl purposes. if (linkParameters.aclAsUserId || linkParameters.aclAsUser) { - if (!this.isOwner(docSession)) { throw new Error('only an owner can override user'); } + if (access !== 'owners') { throw new Error('only an owner can override user'); } if (attrs.override) { // Used cached properties. access = attrs.override.access; @@ -1167,7 +1180,7 @@ export class GranularAccess implements GranularAccessForBundle { const censor = new CensorshipInfo(permissionInfo, ruler.ruleCollection, step.metaAfter, - this.isOwner(cursor.docSession)); + await this.isOwner(cursor.docSession)); if (censor.apply(act)) { results.push(act); } @@ -1179,7 +1192,7 @@ export class GranularAccess implements GranularAccessForBundle { const censorBefore = new CensorshipInfo(permissionInfo, ruler.ruleCollection, step.metaBefore, - this.isOwner(cursor.docSession)); + await this.isOwner(cursor.docSession)); // For all views previously censored, if they are now uncensored, // add an UpdateRecord to expose them. for (const v of censorBefore.censoredViews) {