From a6063f570afb0c0a142fd85f42d543d03016d72d Mon Sep 17 00:00:00 2001 From: George Gevoian Date: Wed, 18 May 2022 21:51:48 -0700 Subject: [PATCH] (core) Polish Access Details Summary: Instead of showing a blank dialog for users whose access is limited (e.g. public members), we now show the user's role and a mention of whether their access is public. Test Plan: Browser tests. Reviewers: paulfitz Reviewed By: paulfitz Differential Revision: https://phab.getgrist.com/D3431 --- app/client/models/UserManagerModel.ts | 34 +++- app/client/ui/AccountWidget.ts | 2 +- app/client/ui/AppHeader.ts | 2 +- app/client/ui/DocMenu.ts | 2 +- app/client/ui/HomeLeftPane.ts | 2 +- app/client/ui/ShareMenu.ts | 2 +- app/client/ui/UserImage.ts | 2 +- app/client/ui/UserManager.ts | 254 ++++++++++++++++++++------ app/common/UserAPI.ts | 5 +- app/gen-server/lib/HomeDBManager.ts | 34 ++-- 10 files changed, 250 insertions(+), 89 deletions(-) diff --git a/app/client/models/UserManagerModel.ts b/app/client/models/UserManagerModel.ts index 0e3268e8..fe356fda 100644 --- a/app/client/models/UserManagerModel.ts +++ b/app/client/models/UserManagerModel.ts @@ -5,18 +5,19 @@ import {ShareAnnotations, ShareAnnotator} from 'app/common/ShareAnnotator'; import {normalizeEmail} from 'app/common/emails'; import {GristLoadConfig} from 'app/common/gristUrls'; import * as roles from 'app/common/roles'; -import {ANONYMOUS_USER_EMAIL, Document, EVERYONE_EMAIL, Organization, +import {ANONYMOUS_USER_EMAIL, Document, EVERYONE_EMAIL, FullUser, getRealAccess, Organization, PermissionData, PermissionDelta, UserAPI, Workspace} from 'app/common/UserAPI'; -import {getRealAccess} from 'app/common/UserAPI'; import {computed, Computed, Disposable, obsArray, ObsArray, observable, Observable} from 'grainjs'; import some = require('lodash/some'); export interface UserManagerModel { initData: PermissionData; // PermissionData used to initialize the UserManager - resourceType: ResourceType; // String representing the access resource + resource: Resource|null; // The access resource. + resourceType: ResourceType; // String representing the access resource type. userSelectOptions: IMemberSelectOption[]; // Select options for each user's role dropdown orgUserSelectOptions: IOrgMemberSelectOption[]; // Select options for each user's role dropdown on the org inheritSelectOptions: IMemberSelectOption[]; // Select options for the maxInheritedRole dropdown + publicUserSelectOptions: IMemberSelectOption[]; // Select options for the public member's role dropdown maxInheritedRole: Observable; // Current unsaved maxInheritedRole setting membersEdited: ObsArray; // Current unsaved editable array of members publicMember: IEditableMember|null; // Member whose access (VIEWER or null) represents that of @@ -26,7 +27,9 @@ export interface UserManagerModel { isOrg: boolean; // Indicates if the UserManager is for an org annotations: Observable; // More information about shares, keyed by email. isPersonal: boolean; // If user access info/control is restricted to self. + isPublicMember: boolean; // Indicates if current user is a public member. + activeUser: FullUser|null; // Populated if current user is logged in. gristDoc: GristDoc|null; // Populated if there is an open document. // Resets all unsaved changes @@ -108,6 +111,15 @@ export class UserManagerModelImpl extends Disposable implements UserManagerModel { value: roles.VIEWER, label: 'View Only' }, { value: null, label: 'None' } ]; + // Select options for the public member's role dropdown. + public readonly publicUserSelectOptions: IMemberSelectOption[] = [ + { value: roles.EDITOR, label: 'Editor' }, + { value: roles.VIEWER, label: 'Viewer' }, + ]; + + public activeUser: FullUser|null = this._options.activeUser ?? null; + + public resource: Resource|null = this._options.resource ?? null; public maxInheritedRole: Observable = observable(this.initData.maxInheritedRole || null); @@ -121,11 +133,13 @@ export class UserManagerModelImpl extends Disposable implements UserManagerModel public annotations = this.autoDispose(observable({users: new Map()})); - public isPersonal = this.initData.personal || false; + public isPersonal = this.initData.personal ?? false; + + public isPublicMember = this.initData.public ?? false; public isOrg: boolean = this.resourceType === 'organization'; - public gristDoc: GristDoc|null; + public gristDoc: GristDoc|null = this._options.docPageModel?.gristDoc.get() ?? null; // Checks if any members were added/removed/changed, if the max inherited role changed or if the // anonymous access setting changed to enable the confirm button to write changes to the server. @@ -139,7 +153,7 @@ export class UserManagerModelImpl extends Disposable implements UserManagerModel // Check if the current user is being removed. public readonly isSelfRemoved: Computed = this.autoDispose(computed((use) => { - return some(use(this.membersEdited), m => m.isRemoved && m.email ===this._options.activeEmail); + return some(use(this.membersEdited), m => m.isRemoved && m.email === this.activeUser?.email); })); private _shareAnnotator?: ShareAnnotator; @@ -148,14 +162,14 @@ export class UserManagerModelImpl extends Disposable implements UserManagerModel public initData: PermissionData, public resourceType: ResourceType, private _options: { - activeEmail?: string|null, + activeUser?: FullUser|null, reload?: () => Promise, docPageModel?: DocPageModel, appModel?: AppModel, + resource?: Resource, } ) { super(); - this.gristDoc = this._options.docPageModel?.gristDoc.get() ?? null; if (this._options.appModel) { const features = this._options.appModel.currentFeatures; this._shareAnnotator = new ShareAnnotator(features, initData); @@ -236,7 +250,7 @@ export class UserManagerModelImpl extends Disposable implements UserManagerModel } public isActiveUser(member: IEditableMember): boolean { - return member.email === this._options.activeEmail; + return member.email === this.activeUser?.email; } // Analyze the relation that users have to the resource or site. @@ -322,7 +336,7 @@ export class UserManagerModelImpl extends Disposable implements UserManagerModel const access = Observable.create(this, member.access); let inheritedAccess: Computed; - if (member.email === this._options.activeEmail) { + if (member.email === this.activeUser?.email) { // Note that we currently prevent the active user's role from changing to prevent users from // locking themselves out of resources. We ensure that by setting inheritedAccess to the // active user's initial access level, which is OWNER normally. (It's sometimes possible to diff --git a/app/client/ui/AccountWidget.ts b/app/client/ui/AccountWidget.ts index 6559e17b..9bffce15 100644 --- a/app/client/ui/AccountWidget.ts +++ b/app/client/ui/AccountWidget.ts @@ -53,7 +53,7 @@ export class AccountWidget extends Disposable { const api = this._appModel.api; (await loadUserManager()).showUserManagerModal(api, { permissionData: api.getOrgAccess(org.id), - activeEmail: user ? user.email : null, + activeUser: user, resourceType: 'organization', resourceId: org.id, resource: org, diff --git a/app/client/ui/AppHeader.ts b/app/client/ui/AppHeader.ts index 0024b0c2..07d07a8b 100644 --- a/app/client/ui/AppHeader.ts +++ b/app/client/ui/AppHeader.ts @@ -34,7 +34,7 @@ export class AppHeader extends Disposable { const api = this._appModel.api; (await loadUserManager()).showUserManagerModal(api, { permissionData: api.getOrgAccess(org.id), - activeEmail: user ? user.email : null, + activeUser: user, resourceType: 'organization', resourceId: org.id, resource: org diff --git a/app/client/ui/DocMenu.ts b/app/client/ui/DocMenu.ts index 23f0e8bf..d86c8770 100644 --- a/app/client/ui/DocMenu.ts +++ b/app/client/ui/DocMenu.ts @@ -429,7 +429,7 @@ export function makeDocOptionsMenu(home: HomeModel, doc: Document, renaming: Obs const user = home.app.currentUser; (await loadUserManager()).showUserManagerModal(api, { permissionData: api.getDocAccess(doc.id), - activeEmail: user ? user.email : null, + activeUser: user, resourceType: 'document', resourceId: doc.id, resource: doc, diff --git a/app/client/ui/HomeLeftPane.ts b/app/client/ui/HomeLeftPane.ts index dfd55ce2..d13326cf 100644 --- a/app/client/ui/HomeLeftPane.ts +++ b/app/client/ui/HomeLeftPane.ts @@ -202,7 +202,7 @@ function workspaceMenu(home: HomeModel, ws: Workspace, renaming: Observable; - activeEmail: string|null; + activeUser: FullUser|null; resourceType: ResourceType; resourceId: string|number; resource?: Resource; @@ -59,8 +60,10 @@ export interface IUserManagerOptions { // required properties of the options. async function getModel(options: IUserManagerOptions): Promise { const permissionData = await options.permissionData; - return new UserManagerModelImpl(permissionData, options.resourceType, - pick(options, ['activeEmail', 'reload', 'appModel', 'docPageModel'])); + return new UserManagerModelImpl( + permissionData, options.resourceType, + pick(options, ['activeUser', 'reload', 'appModel', 'docPageModel', 'resource']) + ); } /** @@ -107,55 +110,73 @@ export function showUserManagerModal(userApi: UserAPI, options: IUserManagerOpti } } - const personal = !roles.canEditAccess(options.resource?.access || null); // Get the model and assign it to the observable. Report errors to the app. getModel(options) .then(model => modelObs.set(model)) .catch(reportError); - modal(ctl => [ + + return buildUserManagerModal(modelObs, onConfirm, options); +} + +function buildUserManagerModal( + modelObs: Observable, + onConfirm: (ctl: IModalControl) => Promise, + options: IUserManagerOptions +) { + return modal(ctl => [ // We set the padding to 0 since the body scroll shadows extend to the edge of the modal. { style: 'padding: 0;' }, options.showAnimation ? dom.cls(cssAnimatedModal.className) : null, - cssModalTitle( - { style: 'margin: 40px 64px 0 64px;' }, - renderTitle(options.resourceType, options.resource, personal), - ((options.resourceType === 'document' && !personal) ? - makeCopyBtn(options.linkToCopy, cssCopyBtn.cls('-header')) : null), - testId('um-header') - ), + dom.maybe(modelObs, model => cssTitle( + renderTitle(options.resourceType, options.resource, model.isPersonal), + (options.resourceType === 'document' && (!model.isPersonal || model.isPublicMember) + ? makeCopyBtn(options.linkToCopy, cssCopyBtn.cls('-header')) + : null + ), + testId('um-header'), + )), + dom.domComputed(modelObs, model => { + if (!model) { return cssSpinner(loadingSpinner()); } - cssModalBody( - cssUserManagerBody( - // TODO: Show a loading indicator before the model is loaded. - dom.maybe(modelObs, model => new UserManager( - model, pick(options, 'linkToCopy', 'docPageModel', 'appModel', 'prompt') - ).buildDom()), - ), - ), - cssModalButtons( - { style: 'margin: 32px 64px; display: flex;' }, - bigPrimaryButton('Confirm', - dom.boolAttr('disabled', (use) => !use(modelObs) || !use(use(modelObs)!.isAnythingChanged)), - dom.on('click', () => onConfirm(ctl)), - testId('um-confirm') - ), - bigBasicButton('Cancel', - dom.on('click', () => ctl.close()), - testId('um-cancel') - ), - dom.maybe(use => use(modelObs)?.resourceType === 'document' && use(modelObs)?.gristDoc && !personal, () => - cssAccessLink({href: urlState().makeUrl({docPage: 'acl'})}, - dom.text(use => (use(modelObs) && use(use(modelObs)!.isAnythingChanged)) ? 'Save & ' : ''), - 'Open Access Rules', - dom.on('click', (ev) => { - ev.preventDefault(); - return onConfirm(ctl).then(() => urlState().pushUrl({docPage: 'acl'})); - }), - testId('um-open-access-rules') + const cssBody = model.isPersonal ? cssAccessDetailsBody : cssUserManagerBody; + return [ + cssModalBody( + cssBody( + new UserManager( + model, pick(options, 'linkToCopy', 'docPageModel', 'appModel', 'prompt', 'resource') + ).buildDom() + ), + ), + cssModalButtons( + { style: 'margin: 32px 64px; display: flex;' }, + (model.isPublicMember ? null : + bigPrimaryButton('Confirm', + dom.boolAttr('disabled', (use) => !use(model.isAnythingChanged)), + dom.on('click', () => onConfirm(ctl)), + testId('um-confirm') + ) + ), + bigBasicButton( + model.isPublicMember ? 'Close' : 'Cancel', + dom.on('click', () => ctl.close()), + testId('um-cancel') + ), + (model.resourceType === 'document' && model.gristDoc && !model.isPersonal + ? cssAccessLink({href: urlState().makeUrl({docPage: 'acl'})}, + dom.text(use => use(model.isAnythingChanged) ? 'Save & ' : ''), + 'Open Access Rules', + dom.on('click', (ev) => { + ev.preventDefault(); + return onConfirm(ctl).then(() => urlState().pushUrl({docPage: 'acl'})); + }), + testId('um-open-access-rules') + ) + : null + ), + testId('um-buttons'), ) - ), - testId('um-buttons'), - ) + ]; + }) ]); } @@ -172,7 +193,8 @@ export class UserManager extends Disposable { linkToCopy?: string, docPageModel?: DocPageModel, appModel?: AppModel, - prompt?: {email: string} + prompt?: {email: string}, + resource?: Resource, }) { super(); } @@ -180,9 +202,17 @@ export class UserManager extends Disposable { public buildDom() { const memberEmail = this.autoDispose(new MemberEmail(this._onAdd.bind(this), this._options.prompt)); + if (this._model.isPublicMember) { + return this._buildSelfPublicAccessDom(); + } + + if (this._model.isPersonal) { + return this._buildSelfAccessDom(); + } + return [ - this._model.isPersonal ? null : memberEmail.buildDom(), - this._model.isPersonal ? null : this._buildOptionsDom(), + memberEmail.buildDom(), + this._buildOptionsDom(), this._dom = shadowScroll( testId('um-members'), this._buildPublicAccessMember(), @@ -245,9 +275,21 @@ export class UserManager extends Disposable { createUserImage(getFullUser(member), 'large') ), cssMemberText( - cssMemberPrimary(member.name || dom('span', member.email, dom.cls('member-email'), testId('um-email'))), - member.name ? cssMemberSecondary(member.email, dom.cls('member-email'), testId('um-email')) : null, - this._buildAnnotationDom(member), + cssMemberPrimary( + member.name || member.email, + member.email ? dom.cls('member-email') : null, + testId('um-member-name'), + ), + !member.name ? null : cssMemberSecondary( + member.email, dom.cls('member-email'), testId('um-member-email') + ), + dom('span', + (this._model.isPersonal + ? this._buildSelfAnnotationDom(member) + : this._buildAnnotationDom(member) + ), + testId('um-member-annotation'), + ), ), member.isRemoved ? null : this._memberRoleSelector(member.effectiveAccess, member.inheritedAccess, this._model.isActiveUser(member)), @@ -273,7 +315,7 @@ export class UserManager extends Disposable { ); } - // Build an annotation for a single member. + // Build an annotation for a single member in the Manage Users dialog. private _buildAnnotationDom(member: IEditableMember) { return dom.domComputed(this._model.annotations, (annotations) => { const annotation = annotations.users.get(member.email); @@ -316,6 +358,24 @@ export class UserManager extends Disposable { }); } + // Build an annotation for the current user in the Access Details dialog. + private _buildSelfAnnotationDom(user: IEditableMember) { + return dom.domComputed(this._model.annotations, (annotations) => { + const annotation = annotations.users.get(user.email); + if (!annotation) { return null; } + + if (annotation.isSupport) { + return cssMemberType('Grist support'); + } else if (annotation.isMember && annotations.hasTeam) { + return cssMemberType('Team member'); + } else if (annotations.hasTeam) { + return cssMemberType('Outside collaborator'); + } else { + return cssMemberType('Collaborator'); + } + }); + } + private _buildPublicAccessMember() { const publicMember = this._model.publicMember; if (!publicMember) { return null; } @@ -328,8 +388,7 @@ export class UserManager extends Disposable { cssMemberSecondary('Anyone with link ', makeCopyBtn(this._options.linkToCopy)), ), this._memberRoleSelector(publicMember.effectiveAccess, publicMember.inheritedAccess, false, - // Only show the Editor and Viewer options for the role of the "Public Access" member. - this._model.userSelectOptions.filter(opt => [roles.EDITOR, roles.VIEWER].includes(opt.value!)) + this._model.publicUserSelectOptions ), cssMemberBtn( cssRemoveIcon('Remove', testId('um-member-delete')), @@ -341,6 +400,48 @@ export class UserManager extends Disposable { ); } + private _buildSelfPublicAccessDom() { + const accessValue = this._options.resource?.access; + const accessLabel = this._model.publicUserSelectOptions + .find(opt => opt.value === accessValue)?.label; + const activeUser = this._model.activeUser; + const name = activeUser?.name ?? 'Anonymous'; + + return dom('div', + cssMemberListItem( + (!activeUser + ? cssPublicMemberIcon('PublicFilled') + : cssMemberImage(createUserImage(activeUser, 'large')) + ), + cssMemberText( + cssMemberPrimary(name, testId('um-member-name')), + activeUser?.email ? cssMemberSecondary(activeUser.email) : null, + cssMemberPublicAccess( + dom('span', 'Public access', testId('um-member-annotation')), + cssPublicAccessIcon('PublicFilled'), + ), + ), + cssRoleBtn( + accessLabel ?? 'Guest', + cssCollapseIcon('Collapse'), + dom.cls('disabled'), + testId('um-member-role'), + ), + testId('um-member'), + ), + testId('um-members'), + ); + } + + private _buildSelfAccessDom() { + return dom('div', + dom.domComputed(this._model.membersEdited, members => + members[0] ? this._buildMemberDom(members[0]) : null + ), + testId('um-members'), + ); + } + // Returns a div containing a button that opens a menu to choose between roles. private _memberRoleSelector( role: Observable, @@ -351,7 +452,8 @@ export class UserManager extends Disposable { const allRoles = allRolesOverride || (this._model.isOrg ? this._model.orgUserSelectOptions : this._model.userSelectOptions); return cssRoleBtn( - menu(() => [ + // Don't include the menu if we're only showing access details for the current user. + this._model.isPersonal ? null : menu(() => [ dom.forEach(allRoles, _role => // The active user should be prevented from changing their own role. menuItem(() => isActiveUser || role.set(_role.value), _role.label, @@ -384,6 +486,7 @@ export class UserManager extends Disposable { return activeRole ? activeRole.label : "Guest"; }), cssCollapseIcon('Collapse'), + this._model.isPersonal ? dom.cls('disabled') : null, testId('um-member-role') ); } @@ -535,7 +638,7 @@ async function manageTeam(appModel: AppModel, const api = appModel.api; showUserManagerModal(api, { permissionData: api.getOrgAccess(currentOrg.id), - activeEmail: user ? user.email : null, + activeUser: user, resourceType: 'organization', resourceId: currentOrg.id, resource: currentOrg, @@ -546,13 +649,23 @@ async function manageTeam(appModel: AppModel, } } -const cssUserManagerBody = styled('div', ` +const cssAccessDetailsBody = styled('div', ` display: flex; flex-direction: column; width: 600px; + font-size: ${vars.mediumFontSize}; +`); + +const cssUserManagerBody = styled(cssAccessDetailsBody, ` height: 374px; border-bottom: 1px solid ${colors.darkGrey}; - font-size: ${vars.mediumFontSize}; +`); + +const cssSpinner = styled('div', ` + display: flex; + align-items: center; + justify-content: center; + margin: 32px; `); const cssCopyBtn = styled(basicButton, ` @@ -583,9 +696,13 @@ const cssOptionBtn = styled('span', ` `); const cssPublicMemberIcon = styled(icon, ` - width: 32px; - height: 32px; - margin: 4px 8px; + width: 40px; + height: 40px; + margin: 0 4px; + --icon-color: ${colors.lightGreen}; +`); + +const cssPublicAccessIcon = styled(icon, ` --icon-color: ${colors.lightGreen}; `); @@ -602,6 +719,7 @@ const cssRoleBtn = styled('div', ` cursor: pointer; &.disabled { + opacity: 0.5; cursor: default; } `); @@ -654,6 +772,22 @@ const cssAnimatedModal = styled('div', ` position: relative; `); +const cssTitle = styled(cssModalTitle, ` + margin: 40px 64px 0 64px; + + @media ${mediaXSmall} { + & { + margin: 16px; + } + } +`); + +const cssMemberPublicAccess = styled(cssMemberSecondary, ` + display: flex; + align-items: center; + gap: 8px; +`); + // Render the UserManager title for `resourceType` (e.g. org as "team site"). function renderTitle(resourceType: ResourceType, resource?: Resource, personal?: boolean) { switch (resourceType) { diff --git a/app/common/UserAPI.ts b/app/common/UserAPI.ts index 189a91d5..fdd3d4cd 100644 --- a/app/common/UserAPI.ts +++ b/app/common/UserAPI.ts @@ -152,7 +152,10 @@ export interface PermissionDelta { } export interface PermissionData { - personal?: boolean; + // True if permission data is restricted to current user. + personal?: true; + // True if current user is a public member. + public?: boolean; maxInheritedRole?: roles.BasicRole|null; users: UserAccessData[]; } diff --git a/app/gen-server/lib/HomeDBManager.ts b/app/gen-server/lib/HomeDBManager.ts index 54ae4f27..d89d44d4 100644 --- a/app/gen-server/lib/HomeDBManager.ts +++ b/app/gen-server/lib/HomeDBManager.ts @@ -4271,19 +4271,29 @@ export class HomeDBManager extends EventEmitter { }); } - private _filterAccessData(scope: Scope, users: UserAccessData[], - maxInheritedRole: roles.BasicRole|null, docId?: string): { personal: true}|undefined { + private _filterAccessData( + scope: Scope, + users: UserAccessData[], + maxInheritedRole: roles.BasicRole|null, + docId?: string + ): {personal: true, public: boolean}|undefined { if (scope.userId === this.getPreviewerUserId()) { return; } - // Unless we have special access to the resource, or are an owner, - // limit user information returned to being about the current user. - const thisUser = users.find(user => user.id === scope.userId); - if ((scope.specialPermit?.docId !== docId || !docId) && - (!thisUser || getRealAccess(thisUser, { maxInheritedRole, users }) !== 'owners')) { - // If not an owner, don't return information about other users. - users.length = 0; - if (thisUser) { users.push(thisUser); } - return { personal: true }; - } + + // If we have special access to the resource, don't filter user information. + if (scope.specialPermit?.docId === docId && docId) { return; } + + const thisUser = this.getAnonymousUserId() === scope.userId + ? null + : users.find(user => user.id === scope.userId); + const realAccess = thisUser ? getRealAccess(thisUser, { maxInheritedRole, users }) : null; + + // If we are an owner, don't filter user information. + if (thisUser && realAccess === 'owners') { return; } + + // Limit user information returned to being about the current user. + users.length = 0; + if (thisUser) { users.push(thisUser); } + return { personal: true, public: !realAccess }; } }