From 59942a23b6928d9551a89dd5e1acf1a5f54ad9d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jaros=C5=82aw=20Sadzi=C5=84ski?= Date: Wed, 30 Nov 2022 23:10:07 +0100 Subject: [PATCH] (core) Limiting doc remove permission to owners. Summary: Guest editors added to a document were able to remove it. This limits this permission by allowing only owners of a doc to delete it. Test Plan: Updated Reviewers: paulfitz Reviewed By: paulfitz Subscribers: dsagal, anaisconce Differential Revision: https://phab.getgrist.com/D3708 --- app/client/ui/DocMenu.ts | 6 +++--- app/gen-server/lib/HomeDBManager.ts | 6 +++--- test/gen-server/testUtils.ts | 13 +++++++++++++ test/server/lib/DocApi.ts | 22 +++++++++++++++++++--- 4 files changed, 38 insertions(+), 9 deletions(-) diff --git a/app/client/ui/DocMenu.ts b/app/client/ui/DocMenu.ts index a97bcc9b..9b5d09ec 100644 --- a/app/client/ui/DocMenu.ts +++ b/app/client/ui/DocMenu.ts @@ -488,7 +488,7 @@ export function makeDocOptionsMenu(home: HomeModel, doc: Document, renaming: Obs testId('move-doc') ), menuItem(deleteDoc, t('Remove'), - dom.cls('disabled', !roles.canDelete(doc.access)), + dom.cls('disabled', !roles.isOwner(doc)), testId('delete-doc') ), menuItem(() => home.pinUnpinDoc(doc.id, !doc.isPinned).catch(reportError), @@ -511,11 +511,11 @@ export function makeRemovedDocOptionsMenu(home: HomeModel, doc: Document, worksp return [ menuItem(() => home.restoreDoc(doc), t('Restore'), - dom.cls('disabled', !roles.canDelete(doc.access) || !!workspace.removedAt), + dom.cls('disabled', !roles.isOwner(doc) || !!workspace.removedAt), testId('doc-restore') ), menuItem(hardDeleteDoc, t('DeleteForever'), - dom.cls('disabled', !roles.canDelete(doc.access)), + dom.cls('disabled', !roles.isOwner(doc)), testId('doc-delete-forever') ), (workspace.removedAt ? diff --git a/app/gen-server/lib/HomeDBManager.ts b/app/gen-server/lib/HomeDBManager.ts index 6e0eeb52..f22e4cac 100644 --- a/app/gen-server/lib/HomeDBManager.ts +++ b/app/gen-server/lib/HomeDBManager.ts @@ -1885,7 +1885,7 @@ export class HomeDBManager extends EventEmitter { return await this._connection.transaction(async manager => { const docQuery = this._doc(scope, { manager, - markPermissions: Permissions.REMOVE, + markPermissions: Permissions.REMOVE | Permissions.SCHEMA_EDIT, allowSpecialPermit: true }) // Join the docs's ACLs and groups so we can remove them. @@ -3396,7 +3396,7 @@ export class HomeDBManager extends EventEmitter { effectiveUserId = this.getPreviewerUserId(); threshold = Permissions.VIEW; } - // Compute whether we have access to the doc + // Compute whether we have access to the ws query = query.addSelect( this._markIsPermitted('workspaces', effectiveUserId, 'open', threshold), 'is_permitted' @@ -4298,7 +4298,7 @@ export class HomeDBManager extends EventEmitter { return this._connection.transaction(async manager => { let docQuery = this._doc({...scope, showAll: true}, { manager, - markPermissions: Permissions.REMOVE, + markPermissions: Permissions.SCHEMA_EDIT | Permissions.REMOVE, allowSpecialPermit: true }); if (!removedAt) { diff --git a/test/gen-server/testUtils.ts b/test/gen-server/testUtils.ts index 827a0c19..68b37c3f 100644 --- a/test/gen-server/testUtils.ts +++ b/test/gen-server/testUtils.ts @@ -26,6 +26,19 @@ export function configForUser(username: string): AxiosRequestConfig { return config; } +/** + * Appends a permit key to the given config. Creates a new config object. + */ +export function configWithPermit(config: AxiosRequestConfig, permitKey: string): AxiosRequestConfig { + return { + ...config, + headers: { + ...config.headers, + Permit: permitKey + } + }; +} + /** * Create a new user and return their personal org. */ diff --git a/test/server/lib/DocApi.ts b/test/server/lib/DocApi.ts index e4e9a753..1e7c8ea6 100644 --- a/test/server/lib/DocApi.ts +++ b/test/server/lib/DocApi.ts @@ -198,6 +198,21 @@ describe('DocApi', function() { // Contains the tests. This is where you want to add more test. function testDocApi() { + it("should allow only owners to remove a document", async () => { + const ws1 = (await userApi.getOrgWorkspaces('current'))[0].id; + const doc1 = await userApi.newDoc({name: 'testdeleteme1'}, ws1); + const kiwiApi = makeUserApi(ORG_NAME, 'kiwi'); + + // Kiwi is editor of the document, so he can't delete it. + await userApi.updateDocPermissions(doc1, {users: {'kiwi@getgrist.com': 'editors'}}); + await assert.isRejected(kiwiApi.softDeleteDoc(doc1), /Forbidden/); + await assert.isRejected(kiwiApi.deleteDoc(doc1), /Forbidden/); + + // Kiwi is owner of the document - now he can delete it. + await userApi.updateDocPermissions(doc1, {users: {'kiwi@getgrist.com': 'owners'}}); + await assert.isFulfilled(kiwiApi.softDeleteDoc(doc1)); + await assert.isFulfilled(kiwiApi.deleteDoc(doc1)); + }); it("guesses types of new columns", async () => { const userActions = [ @@ -3151,6 +3166,7 @@ interface WebhookRequests { "add,update": object[][]; } +const ORG_NAME = 'docs-1'; function setup(name: string, cb: () => Promise) { let api: UserAPIImpl; @@ -3162,7 +3178,7 @@ function setup(name: string, cb: () => Promise) { await cb(); // create TestDoc as an empty doc into Private workspace - userApi = api = makeUserApi('docs-1'); + userApi = api = makeUserApi(ORG_NAME); const wid = await getWorkspaceId(api, 'Private'); docIds.TestDoc = await api.newDoc({name: 'TestDoc'}, wid); }); @@ -3178,9 +3194,9 @@ function setup(name: string, cb: () => Promise) { }); } -function makeUserApi(org: string) { +function makeUserApi(org: string, user?: string) { return new UserAPIImpl(`${home.serverUrl}/o/${org}`, { - headers: {Authorization: 'Bearer api_key_for_chimpy'}, + headers: {Authorization: `Bearer api_key_for_${user || 'chimpy'}`}, fetch: fetch as any, newFormData: () => new FormData() as any, logger: log