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 <florent.fayolle@beta.gouv.fr>

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<QueryResult<PermissionData>> {
-    const orgQuery = this.org(scope, orgKey, {
+    const queryResult = await this._getOrgWithACLRules(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);
     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<QueryResult<PermissionData>> {
-    const wsQuery = this._workspace(scope, wsId, {
+    // Run the query for the workspace and org in a transaction. This brings some isolation protection
-      markPermissions: Permissions.VIEW
+    // against changes to the workspace or org while we are querying.
-    })
+    const { workspace, org, queryFailure } = await this._connection.transaction(async manager => {
-    // Join the workspace's ACL rules (with 1st level groups/users listed).
+      const wsQueryResult = await this._getWorkspaceWithACLRules(scope, wsId, { manager });
-    .leftJoinAndSelect('workspaces.aclRules', 'acl_rules')
+      if (wsQueryResult.status !== 200) {
-    .leftJoinAndSelect('acl_rules.group', 'workspace_groups')
+        // If the query for the workspace failed, return the failure result.
-    .leftJoinAndSelect('workspace_groups.memberUsers', 'workspace_group_users')
+        return { queryFailure: wsQueryResult };
-    .leftJoinAndSelect('workspace_groups.memberGroups', 'workspace_group_groups')
+      }
-    .leftJoinAndSelect('workspace_group_users.logins', 'workspace_user_logins')
+
-    // Join the org and groups/users.
+      const orgQuery = this._buildOrgWithACLRulesQuery(scope, wsQueryResult.data.org.id, { manager });
-    .leftJoinAndSelect('workspaces.org', 'org')
+      const orgQueryResult = await verifyEntity(orgQuery, { skipPermissionCheck: true });
-    .leftJoinAndSelect('org.aclRules', 'org_acl_rules')
+      if (orgQueryResult.status !== 200) {
-    .leftJoinAndSelect('org_acl_rules.group', 'org_groups')
+        // If the query for the org failed, return the failure result.
-    .leftJoinAndSelect('org_groups.memberUsers', 'org_group_users')
+        return { queryFailure: orgQueryResult };
-    .leftJoinAndSelect('org_group_users.logins', 'org_user_logins');
+      }
-    const queryResult = await verifyIsPermitted(wsQuery);
+
-    if (queryResult.status !== 200) {
+      return {
-      // If the query for the doc failed, return the failure result.
+        workspace: wsQueryResult.data,
-      return queryResult;
+        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 orgMap = getMemberUserRoles(org, this.defaultBasicGroupNames);
-    const orgMapWithMembership = getMemberUserRoles(workspace.org, this.defaultGroupNames);
+    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<QueryOptions> = {}) {
+    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<QueryOptions> = {}) {
+    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(
+async function verifyEntity(
-  queryBuilder: SelectQueryBuilder<any>
+  queryBuilder: SelectQueryBuilder<any>,
+  options: { skipPermissionCheck?: boolean } = {}
 ): Promise<QueryResult<any>> {
   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"

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.

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<string> {
     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();