From 866ec66096b1ea71cc63cd2b2ab244ff29042fb6 Mon Sep 17 00:00:00 2001 From: Florent Date: Wed, 31 Jan 2024 20:04:22 +0100 Subject: [PATCH] Optimize sql query for workspace acl (#824) Without this optimization, we fetched loads of entries from the database, which led to database and nodejs overloads. We could go further, this is a modest patch towards better performance. We use two queries: one fetches the workspaces, the second the organization that the workspace belongs to. --------- Co-authored-by: Florent FAYOLLE --- app/gen-server/lib/HomeDBManager.ts | 155 ++++++++++++++++++---------- test/gen-server/ApiServerAccess.ts | 22 ++++ test/gen-server/apiUtils.ts | 3 +- 3 files changed, 122 insertions(+), 58 deletions(-) diff --git a/app/gen-server/lib/HomeDBManager.ts b/app/gen-server/lib/HomeDBManager.ts index 1e82318f..8b081982 100644 --- a/app/gen-server/lib/HomeDBManager.ts +++ b/app/gen-server/lib/HomeDBManager.ts @@ -813,7 +813,7 @@ export class HomeDBManager extends EventEmitter { .leftJoinAndSelect('org_groups.memberUsers', 'org_member_users'); const result = await orgQuery.getRawAndEntities(); if (result.entities.length === 0) { - // If the query for the doc failed, return the failure result. + // If the query for the org failed, return the failure result. throw new ApiError('org not found', 404); } org = result.entities[0]; @@ -1073,7 +1073,7 @@ export class HomeDBManager extends EventEmitter { needRealOrg: true }); orgQuery = this._addFeatures(orgQuery); - const orgQueryResult = await verifyIsPermitted(orgQuery); + const orgQueryResult = await verifyEntity(orgQuery); const org: Organization = this.unwrapQueryResult(orgQueryResult); const productFeatures = org.billingAccount.product.features; @@ -1589,7 +1589,7 @@ export class HomeDBManager extends EventEmitter { markPermissions, needRealOrg: true }); - const queryResult = await verifyIsPermitted(orgQuery); + const queryResult = await verifyEntity(orgQuery); if (queryResult.status !== 200) { // If the query for the workspace failed, return the failure result. return queryResult; @@ -1657,7 +1657,7 @@ export class HomeDBManager extends EventEmitter { .leftJoinAndSelect('docs.aclRules', 'doc_acl_rules') .leftJoinAndSelect('doc_acl_rules.group', 'doc_group') .leftJoinAndSelect('orgs.billingAccount', 'billing_accounts'); - const queryResult = await verifyIsPermitted(orgQuery); + const queryResult = await verifyEntity(orgQuery); if (queryResult.status !== 200) { // If the query for the org failed, return the failure result. return queryResult; @@ -1710,7 +1710,7 @@ export class HomeDBManager extends EventEmitter { .leftJoinAndSelect('acl_rules.group', 'org_group') .leftJoinAndSelect('orgs.workspaces', 'workspaces'); // we may want to count workspaces. orgQuery = this._addFeatures(orgQuery); // add features to access optional workspace limit. - const queryResult = await verifyIsPermitted(orgQuery); + const queryResult = await verifyEntity(orgQuery); if (queryResult.status !== 200) { // If the query for the organization failed, return the failure result. return queryResult; @@ -1750,7 +1750,7 @@ export class HomeDBManager extends EventEmitter { manager, markPermissions: Permissions.UPDATE }); - const queryResult = await verifyIsPermitted(wsQuery); + const queryResult = await verifyEntity(wsQuery); if (queryResult.status !== 200) { // If the query for the workspace failed, return the failure result. return queryResult; @@ -1782,7 +1782,7 @@ export class HomeDBManager extends EventEmitter { .leftJoinAndSelect('docs.aclRules', 'doc_acl_rules') .leftJoinAndSelect('doc_acl_rules.group', 'doc_groups') .leftJoinAndSelect('workspaces.org', 'orgs'); - const queryResult = await verifyIsPermitted(wsQuery); + const queryResult = await verifyEntity(wsQuery); if (queryResult.status !== 200) { // If the query for the workspace failed, return the failure result. return queryResult; @@ -1835,7 +1835,7 @@ export class HomeDBManager extends EventEmitter { .leftJoinAndSelect('workspaces.aclRules', 'acl_rules') .leftJoinAndSelect('acl_rules.group', 'workspace_group'); wsQuery = this._addFeatures(wsQuery); - const queryResult = await verifyIsPermitted(wsQuery); + const queryResult = await verifyEntity(wsQuery); if (queryResult.status !== 200) { // If the query for the organization failed, return the failure result. return queryResult; @@ -2009,7 +2009,7 @@ export class HomeDBManager extends EventEmitter { markPermissions, }); } - const queryResult = await verifyIsPermitted(query); + const queryResult = await verifyEntity(query); if (queryResult.status !== 200) { // If the query for the doc or fork failed, return the failure result. return queryResult; @@ -2062,7 +2062,7 @@ export class HomeDBManager extends EventEmitter { manager, allowSpecialPermit: true, }); - const queryResult = await verifyIsPermitted(forkQuery); + const queryResult = await verifyEntity(forkQuery); if (queryResult.status !== 200) { // If the query for the fork failed, return the failure result. return queryResult; @@ -2080,7 +2080,7 @@ export class HomeDBManager extends EventEmitter { // Join the workspace and org to get their ids. .leftJoinAndSelect('docs.aclRules', 'acl_rules') .leftJoinAndSelect('acl_rules.group', 'groups'); - const queryResult = await verifyIsPermitted(docQuery); + const queryResult = await verifyEntity(docQuery); if (queryResult.status !== 200) { // If the query for the doc failed, return the failure result. return queryResult; @@ -2218,7 +2218,7 @@ export class HomeDBManager extends EventEmitter { .leftJoinAndSelect('org_groups.memberUsers', 'org_member_users'); orgQuery = this._addFeatures(orgQuery); orgQuery = this._withAccess(orgQuery, userId, 'orgs'); - const queryResult = await verifyIsPermitted(orgQuery); + const queryResult = await verifyEntity(orgQuery); if (queryResult.status !== 200) { // If the query for the organization failed, return the failure result. return queryResult; @@ -2279,7 +2279,7 @@ export class HomeDBManager extends EventEmitter { .leftJoinAndSelect('org_groups.memberUsers', 'org_users'); wsQuery = this._addFeatures(wsQuery, 'org'); wsQuery = this._withAccess(wsQuery, userId, 'workspaces'); - const queryResult = await verifyIsPermitted(wsQuery); + const queryResult = await verifyEntity(wsQuery); if (queryResult.status !== 200) { // If the query for the workspace failed, return the failure result. return queryResult; @@ -2380,17 +2380,7 @@ export class HomeDBManager extends EventEmitter { // Returns UserAccessData for all users with any permissions on the org. public async getOrgAccess(scope: Scope, orgKey: string|number): Promise> { - const orgQuery = this.org(scope, orgKey, { - markPermissions: Permissions.VIEW, - needRealOrg: true, - allowSpecialPermit: true - }) - // Join the org's ACL rules (with 1st level groups/users listed). - .leftJoinAndSelect('orgs.aclRules', 'acl_rules') - .leftJoinAndSelect('acl_rules.group', 'org_groups') - .leftJoinAndSelect('org_groups.memberUsers', 'org_member_users') - .leftJoinAndSelect('org_member_users.logins', 'user_logins'); - const queryResult = await verifyIsPermitted(orgQuery); + const queryResult = await this._getOrgWithACLRules(scope, orgKey); if (queryResult.status !== 200) { // If the query for the doc failed, return the failure result. return queryResult; @@ -2419,33 +2409,41 @@ export class HomeDBManager extends EventEmitter { // maxInheritedRole set on the workspace. Note that information for all users in the org // is given to indicate which users have access to the org but not to this particular workspace. public async getWorkspaceAccess(scope: Scope, wsId: number): Promise> { - const wsQuery = this._workspace(scope, wsId, { - markPermissions: Permissions.VIEW - }) - // Join the workspace's ACL rules (with 1st level groups/users listed). - .leftJoinAndSelect('workspaces.aclRules', 'acl_rules') - .leftJoinAndSelect('acl_rules.group', 'workspace_groups') - .leftJoinAndSelect('workspace_groups.memberUsers', 'workspace_group_users') - .leftJoinAndSelect('workspace_groups.memberGroups', 'workspace_group_groups') - .leftJoinAndSelect('workspace_group_users.logins', 'workspace_user_logins') - // Join the org and groups/users. - .leftJoinAndSelect('workspaces.org', 'org') - .leftJoinAndSelect('org.aclRules', 'org_acl_rules') - .leftJoinAndSelect('org_acl_rules.group', 'org_groups') - .leftJoinAndSelect('org_groups.memberUsers', 'org_group_users') - .leftJoinAndSelect('org_group_users.logins', 'org_user_logins'); - const queryResult = await verifyIsPermitted(wsQuery); - if (queryResult.status !== 200) { - // If the query for the doc failed, return the failure result. - return queryResult; + // Run the query for the workspace and org in a transaction. This brings some isolation protection + // against changes to the workspace or org while we are querying. + const { workspace, org, queryFailure } = await this._connection.transaction(async manager => { + const wsQueryResult = await this._getWorkspaceWithACLRules(scope, wsId, { manager }); + if (wsQueryResult.status !== 200) { + // If the query for the workspace failed, return the failure result. + return { queryFailure: wsQueryResult }; + } + + const orgQuery = this._buildOrgWithACLRulesQuery(scope, wsQueryResult.data.org.id, { manager }); + const orgQueryResult = await verifyEntity(orgQuery, { skipPermissionCheck: true }); + if (orgQueryResult.status !== 200) { + // If the query for the org failed, return the failure result. + return { queryFailure: orgQueryResult }; + } + + return { + workspace: wsQueryResult.data, + org: orgQueryResult.data + }; + }); + if (queryFailure) { + return queryFailure; } - const workspace: Workspace = queryResult.data; + const wsMap = getMemberUserRoles(workspace, this.defaultCommonGroupNames); + + // Also fetch the organization ACLs so we can determine inherited rights. + // The orgMap gives the org access inherited by each user. - const orgMap = getMemberUserRoles(workspace.org, this.defaultBasicGroupNames); - const orgMapWithMembership = getMemberUserRoles(workspace.org, this.defaultGroupNames); + const orgMap = getMemberUserRoles(org, this.defaultBasicGroupNames); + const orgMapWithMembership = getMemberUserRoles(org, this.defaultGroupNames); // Iterate through the org since all users will be in the org. - const users: UserAccessData[] = getResourceUsers([workspace, workspace.org]).map(u => { + + const users: UserAccessData[] = getResourceUsers([workspace, org]).map(u => { const orgAccess = orgMapWithMembership[u.id] || null; return { ...this.makeFullUser(u), @@ -2576,7 +2574,7 @@ export class HomeDBManager extends EventEmitter { .leftJoinAndSelect('orgs.aclRules', 'org_acl_rules') .leftJoinAndSelect('org_acl_rules.group', 'org_groups') .leftJoinAndSelect('org_groups.memberUsers', 'org_users'); - const docQueryResult = await verifyIsPermitted(docQuery); + const docQueryResult = await verifyEntity(docQuery); if (docQueryResult.status !== 200) { // If the query for the doc failed, return the failure result. return docQueryResult; @@ -2603,7 +2601,7 @@ export class HomeDBManager extends EventEmitter { .leftJoinAndSelect('org_acl_rules.group', 'org_groups') .leftJoinAndSelect('org_groups.memberUsers', 'org_users'); wsQuery = this._addFeatures(wsQuery); - const wsQueryResult = await verifyIsPermitted(wsQuery); + const wsQueryResult = await verifyEntity(wsQuery); if (wsQueryResult.status !== 200) { // If the query for the organization failed, return the failure result. return wsQueryResult; @@ -2681,7 +2679,7 @@ export class HomeDBManager extends EventEmitter { manager }) .addSelect(this._markIsPermitted('orgs', scope.userId, 'open', permissions), 'is_permitted'); - const docQueryResult = await verifyIsPermitted(docQuery); + const docQueryResult = await verifyEntity(docQuery); if (docQueryResult.status !== 200) { // If the query for the doc failed, return the failure result. return docQueryResult; @@ -4669,7 +4667,7 @@ export class HomeDBManager extends EventEmitter { // SQL results are flattened, and multiplying the number of rows we have already // by the number of workspace users could get excessive. .leftJoinAndSelect('docs.workspace', 'workspace'); - const queryResult = await verifyIsPermitted(docQuery); + const queryResult = await verifyEntity(docQuery); const doc: Document = this.unwrapQueryResult(queryResult); // Load the workspace's member groups/users. @@ -4777,7 +4775,7 @@ export class HomeDBManager extends EventEmitter { manager, markPermissions: Permissions.REMOVE }); - const workspace: Workspace = this.unwrapQueryResult(await verifyIsPermitted(wsQuery)); + const workspace: Workspace = this.unwrapQueryResult(await verifyEntity(wsQuery)); await manager.createQueryBuilder() .update(Workspace).set({removedAt}).where({id: workspace.id}) .execute(); @@ -4795,7 +4793,7 @@ export class HomeDBManager extends EventEmitter { if (!removedAt) { docQuery = this._addFeatures(docQuery); // pull in billing information for doc count limits } - const doc: Document = this.unwrapQueryResult(await verifyIsPermitted(docQuery)); + const doc: Document = this.unwrapQueryResult(await verifyEntity(docQuery)); if (!removedAt) { await this._checkRoomForAnotherDoc(doc.workspace, manager); } @@ -4829,16 +4827,59 @@ export class HomeDBManager extends EventEmitter { if (thisUser) { users.push(thisUser); } return { personal: true, public: !realAccess }; } + + private _getWorkspaceWithACLRules(scope: Scope, wsId: number, options: Partial = {}) { + const query = this._workspace(scope, wsId, { + markPermissions: Permissions.VIEW, + ...options + }) + // Join the workspace's ACL rules (with 1st level groups/users listed). + .leftJoinAndSelect('workspaces.aclRules', 'acl_rules') + .leftJoinAndSelect('acl_rules.group', 'workspace_groups') + .leftJoinAndSelect('workspace_groups.memberUsers', 'workspace_group_users') + .leftJoinAndSelect('workspace_groups.memberGroups', 'workspace_group_groups') + .leftJoinAndSelect('workspace_group_users.logins', 'workspace_user_logins') + .leftJoinAndSelect('workspaces.org', 'org'); + return verifyEntity(query); + } + + private _buildOrgWithACLRulesQuery(scope: Scope, org: number|string, opts: Partial = {}) { + return this.org(scope, org, { + needRealOrg: true, + ...opts + }) + // Join the org's ACL rules (with 1st level groups/users listed). + .leftJoinAndSelect('orgs.aclRules', 'acl_rules') + .leftJoinAndSelect('acl_rules.group', 'org_groups') + .leftJoinAndSelect('org_groups.memberUsers', 'org_member_users') + .leftJoinAndSelect('org_member_users.logins', 'user_logins'); + } + + private _getOrgWithACLRules(scope: Scope, org: number|string) { + const orgQuery = this._buildOrgWithACLRulesQuery(scope, org, { + markPermissions: Permissions.VIEW, + allowSpecialPermit: true, + }); + return verifyEntity(orgQuery); + } + } // Return a QueryResult reflecting the output of a query builder. // Checks on the "is_permitted" field which select queries set on resources to // indicate whether the user has access. +// // If the output is empty, we signal that the desired resource does not exist. -// If the "is_permitted" field is falsy, we signal that the resource is forbidden. +// +// If we retrieve more than 1 entity, we signal that the request is ambiguous. +// +// If the "is_permitted" field is falsy, we signal that the resource is forbidden, +// unless skipPermissionCheck is set. +// // Returns the resource fetched by the queryBuilder. -async function verifyIsPermitted( - queryBuilder: SelectQueryBuilder +async function verifyEntity( + queryBuilder: SelectQueryBuilder, + options: { skipPermissionCheck?: boolean } = {} ): Promise> { const results = await queryBuilder.getRawAndEntities(); if (results.entities.length === 0) { @@ -4851,7 +4892,7 @@ async function verifyIsPermitted( status: 400, errMessage: `ambiguous ${getFrom(queryBuilder)} request` }; - } else if (!results.raw[0].is_permitted) { + } else if (!options.skipPermissionCheck && !results.raw[0].is_permitted) { return { status: 403, errMessage: "access denied" diff --git a/test/gen-server/ApiServerAccess.ts b/test/gen-server/ApiServerAccess.ts index 987b3880..2742fed5 100644 --- a/test/gen-server/ApiServerAccess.ts +++ b/test/gen-server/ApiServerAccess.ts @@ -924,6 +924,21 @@ describe('ApiServerAccess', function() { isMember: true, }] }); + + const deltaOrg = { + users: { + [kiwiEmail]: "owners", + } + }; + const respDeltaOrg = await axios.patch(`${homeUrl}/api/orgs/${oid}/access`, {delta: deltaOrg}, chimpy); + assert.equal(respDeltaOrg.status, 200); + + const resp3 = await axios.get(`${homeUrl}/api/workspaces/${wid}/access`, chimpy); + assert.include(resp3.data.users.find((user: any) => user.email === kiwiEmail), { + access: "editors", + parentAccess: "owners" + }); + // Reset the access settings const resetDelta = { maxInheritedRole: "owners", @@ -933,6 +948,13 @@ describe('ApiServerAccess', function() { }; const resetResp = await axios.patch(`${homeUrl}/api/workspaces/${wid}/access`, {delta: resetDelta}, chimpy); assert.equal(resetResp.status, 200); + const resetOrgDelta = { + users: { + [kiwiEmail]: "members", + } + }; + const resetOrgResp = await axios.patch(`${homeUrl}/api/orgs/${oid}/access`, {delta: resetOrgDelta}, chimpy); + assert.equal(resetOrgResp.status, 200); // Assert that ws guests are properly displayed. // Tests a minor bug that showed ws guests as having null access. diff --git a/test/gen-server/apiUtils.ts b/test/gen-server/apiUtils.ts index 33976052..f269be63 100644 --- a/test/gen-server/apiUtils.ts +++ b/test/gen-server/apiUtils.ts @@ -22,6 +22,7 @@ import * as path from 'path'; import {createInitialDb, removeConnection, setUpDB} from 'test/gen-server/seed'; import {setPlan} from 'test/gen-server/testUtils'; import {fixturesRoot} from 'test/server/testUtils'; +import {isAffirmative} from 'app/common/gutil'; export class TestServer { public serverUrl: string; @@ -36,7 +37,7 @@ export class TestServer { public async start(servers: ServerType[] = ["home"], options: FlexServerOptions = {}): Promise { await createInitialDb(); - this.server = await mergedServerMain(0, servers, {logToConsole: false, + this.server = await mergedServerMain(0, servers, {logToConsole: isAffirmative(process.env.DEBUG), externalStorage: false, ...options}); this.serverUrl = this.server.getOwnUrl();