From 887cd388c095ac581575d2873d5edf37d5e79c68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jaros=C5=82aw=20Sadzi=C5=84ski?= Date: Mon, 9 Jan 2023 09:53:41 +0100 Subject: [PATCH] (core) Adding creator as an owner for a new doc and ws Summary: By default editor inherits permission for a new document or workspace. Now editor is added explicitly as an owner of a new doc or workspace. Test Plan: Updated Reviewers: georgegevoian, paulfitz Reviewed By: georgegevoian, paulfitz Subscribers: dsagal, paulfitz Differential Revision: https://phab.getgrist.com/D3734 --- app/gen-server/lib/HomeDBManager.ts | 47 ++++++++++++++++++++++++----- test/server/lib/DocApi.ts | 45 ++++++++++++++++++++++++++- 2 files changed, 84 insertions(+), 8 deletions(-) diff --git a/app/gen-server/lib/HomeDBManager.ts b/app/gen-server/lib/HomeDBManager.ts index f737350b..2ea076ea 100644 --- a/app/gen-server/lib/HomeDBManager.ts +++ b/app/gen-server/lib/HomeDBManager.ts @@ -251,6 +251,12 @@ export interface DocumentMetadata { usage?: DocumentUsage|null; } +interface CreateWorkspaceOptions { + org: Organization, + props: Partial, + ownerId?: number +} + /** * HomeDBManager handles interaction between the ApiServer and the Home database, * encapsulating the typeorm logic. @@ -1418,7 +1424,7 @@ export class HomeDBManager extends EventEmitter { } // Add a starter workspace to the org. Any limits on org workspace // count are not checked, this will succeed unconditionally. - await this._doAddWorkspace(savedOrg, {name: 'Home'}, manager); + await this._doAddWorkspace({org: savedOrg, props: {name: 'Home'}}, manager); if (!options.setUserAsOwner) { // This user just made a team site (once this transaction is applied). @@ -1619,7 +1625,7 @@ export class HomeDBManager extends EventEmitter { }); } } - const workspace = await this._doAddWorkspace(org, props, manager); + const workspace = await this._doAddWorkspace({org, props, ownerId: scope.userId}, manager); return { status: 200, data: workspace.id @@ -1766,7 +1772,7 @@ export class HomeDBManager extends EventEmitter { } doc.workspace = workspace; // Create the special initial permission groups for the new workspace. - const groupMap = this._createGroups(workspace); + const groupMap = this._createGroups(workspace, scope.userId); doc.aclRules = this.defaultCommonGroups.map(_grpDesc => { // Get the special group with the name needed for this ACL Rule const group = groupMap[_grpDesc.name]; @@ -1780,6 +1786,15 @@ export class HomeDBManager extends EventEmitter { // Saves the document as well as its new ACL Rules and Group. const groups = doc.aclRules.map(rule => rule.group); const result = await manager.save([doc, ...doc.aclRules, ...doc.aliases, ...groups]); + // Ensure that the creator is in the ws and org's guests group. Creator already has + // access to the workspace (he is at least an editor), but we need to be sure that + // even if he is removed from the workspace, he will still have access to this doc. + // Guest groups are updated after any access is changed, so even if we won't add creator + // now, he will be added later. NOTE: those functions would normally fail in transaction + // as those groups might by already fixed (when there is another doc created in the same + // time), but they are ignoring any unique constraints errors. + await this._repairWorkspaceGuests(scope, workspace.id, manager); + await this._repairOrgGuests(scope, workspace.org.id, manager); return { status: 200, data: (result[0] as Document).id @@ -2939,6 +2954,9 @@ export class HomeDBManager extends EventEmitter { if (toAdd.length > 0) { await manager.createQueryBuilder() .insert() + // Since we are adding new records in group_users, we may get a duplicate key error if two documents + // are added at the same time (even in transaction, since we are not blocking the whole table). + .orIgnore() .into('group_users') .values(toAdd.map(id => ({user_id: id, group_id: groupId}))) .execute(); @@ -2968,8 +2986,10 @@ export class HomeDBManager extends EventEmitter { * Creates, initializes and saves a workspace in the given org with the given properties. * Product limits on number of workspaces allowed in org are not checked. */ - private async _doAddWorkspace(org: Organization, props: Partial, - transaction?: EntityManager): Promise { + private async _doAddWorkspace( + {org, props, ownerId}: CreateWorkspaceOptions, + transaction?: EntityManager + ): Promise { if (!props.name) { throw new ApiError('Bad request: name required', 400); } return await this._runInTransaction(transaction, async manager => { // Create a new workspace. @@ -2978,7 +2998,8 @@ export class HomeDBManager extends EventEmitter { workspace.updateFromProperties(props); workspace.org = org; // Create the special initial permission groups for the new workspace. - const groupMap = this._createGroups(org); + // Optionally add the owner to the workspace. + const groupMap = this._createGroups(org, ownerId); workspace.aclRules = this.defaultCommonGroups.map(_grpDesc => { // Get the special group with the name needed for this ACL Rule const group = groupMap[_grpDesc.name]; @@ -2992,6 +3013,11 @@ export class HomeDBManager extends EventEmitter { // Saves the workspace as well as its new ACL Rules and Group. const groups = workspace.aclRules.map(rule => rule.group); const result = await manager.save([workspace, ...workspace.aclRules, ...groups]); + if (ownerId) { + // If we modified direct access to the workspace, we need to update the + // guest group to include the owner. + await this._repairOrgGuests({userId: ownerId}, org.id, manager); + } return result[0]; }); } @@ -3586,7 +3612,7 @@ export class HomeDBManager extends EventEmitter { * Returns a name to group mapping for the standard groups. Useful when adding a new child * entity. Finds and includes the correct parent groups as member groups. */ - private _createGroups(inherit?: Organization|Workspace): {[name: string]: Group} { + private _createGroups(inherit?: Organization|Workspace, ownerId?: number): {[name: string]: Group} { const groupMap: {[name: string]: Group} = {}; this.defaultGroups.forEach(groupProps => { if (!groupProps.orgOnly || !inherit) { @@ -3599,6 +3625,13 @@ export class HomeDBManager extends EventEmitter { groupMap[groupProps.name] = group; } }); + // Add the owner explicitly to the owner group. + if (ownerId) { + const ownerGroup = groupMap[roles.OWNER]; + const user = new User(); + user.id = ownerId; + ownerGroup.memberUsers = [user]; + } return groupMap; } diff --git a/test/server/lib/DocApi.ts b/test/server/lib/DocApi.ts index f32388d0..0d4bcd26 100644 --- a/test/server/lib/DocApi.ts +++ b/test/server/lib/DocApi.ts @@ -198,6 +198,47 @@ describe('DocApi', function() { // Contains the tests. This is where you want to add more test. function testDocApi() { + it("creator should be owner of a created ws", async () => { + const kiwiEmail = 'kiwi@getgrist.com'; + const ws1 = (await userApi.getOrgWorkspaces('current'))[0].id; + // Make sure kiwi isn't allowed here. + await userApi.updateOrgPermissions(ORG_NAME, {users: {[kiwiEmail]: null}}); + const kiwiApi = makeUserApi(ORG_NAME, 'kiwi'); + await assert.isRejected(kiwiApi.getWorkspaceAccess(ws1), /Forbidden/); + // Add kiwi as an editor for the org. + await assert.isRejected(kiwiApi.getOrgAccess(ORG_NAME), /Forbidden/); + await userApi.updateOrgPermissions(ORG_NAME, {users: {[kiwiEmail]: 'editors'}}); + // Make a workspace as Kiwi, he should be owner of it. + const kiwiWs = await kiwiApi.newWorkspace({name: 'kiwiWs'}, ORG_NAME); + const kiwiWsAccess = await kiwiApi.getWorkspaceAccess(kiwiWs); + assert.equal(kiwiWsAccess.users.find(u => u.email === kiwiEmail)?.access, 'owners'); + // Delete workspace. + await kiwiApi.deleteWorkspace(kiwiWs); + // Remove kiwi from the org. + await userApi.updateOrgPermissions(ORG_NAME, {users: {[kiwiEmail]: null}}); + }); + + it("creator should be owner of a created doc", async () => { + const kiwiEmail = 'kiwi@getgrist.com'; + const ws1 = (await userApi.getOrgWorkspaces('current'))[0].id; + await userApi.updateOrgPermissions(ORG_NAME, {users: {[kiwiEmail]: null}}); + // Make sure kiwi isn't allowed here. + const kiwiApi = makeUserApi(ORG_NAME, 'kiwi'); + await assert.isRejected(kiwiApi.getWorkspaceAccess(ws1), /Forbidden/); + // Add kiwi as an editor of this workspace. + await userApi.updateWorkspacePermissions(ws1, {users: {[kiwiEmail]: 'editors'}}); + await assert.isFulfilled(kiwiApi.getWorkspaceAccess(ws1)); + // Create a document as kiwi. + const kiwiDoc = await kiwiApi.newDoc({name: 'kiwiDoc'}, ws1); + // Make sure kiwi is an owner of the document. + const kiwiDocAccess = await kiwiApi.getDocAccess(kiwiDoc); + assert.equal(kiwiDocAccess.users.find(u => u.email === kiwiEmail)?.access, 'owners'); + await kiwiApi.deleteDoc(kiwiDoc); + // Remove kiwi from the workspace. + await userApi.updateWorkspacePermissions(ws1, {users: {[kiwiEmail]: null}}); + await assert.isRejected(kiwiApi.getWorkspaceAccess(ws1), /Forbidden/); + }); + 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); @@ -226,6 +267,8 @@ function testDocApi() { // Kiwi is owner of the document - now he can rename it. await userApi.updateDocPermissions(doc1, {users: {'kiwi@getgrist.com': 'owners'}}); await assert.isFulfilled(kiwiApi.renameDoc(doc1, "testrenameme2")); + + await userApi.deleteDoc(doc1); }); it("guesses types of new columns", async () => { @@ -3551,8 +3594,8 @@ function testDocApi() { assert.isAtLeast(stats[0].usage?.updatedTime ?? 0, now); assert.isNull(stats[0].usage?.lastErrorMessage); assert.isNull(stats[0].usage?.lastFailureTime); - assert.isAtLeast(stats[0].usage?.lastSuccessTime ?? 0, now); assert.equal(stats[0].usage?.lastHttpStatus, 200); + assert.isAtLeast(stats[0].usage?.lastSuccessTime ?? 0, now); assert.deepEqual(stats[0].usage?.lastEventBatch, { status: 'success', attempts: 1,