fix(api-gateway): make sure DAP works sql pushdown

fixes an issue when processing queries with member expressions
cube-js · Dec 6, 2024 · 33c1659 · 33c1659
1 parent 2b4bc9e
commit 33c1659
Show file tree

Hide file tree

Showing 5 changed files with 70 additions and 8 deletions.
diff --git a/packages/cubejs-api-gateway/src/gateway.ts b/packages/cubejs-api-gateway/src/gateway.ts
@@ -1185,7 +1185,8 @@ class ApiGateway {
     let normalizedQueries: NormalizedQuery[] = await Promise.all(
       queries.map(
         async (currentQuery) => {
-          const hasExpressionsInQuery = this.hasExpressionsInQuery(currentQuery);
+          const hasExpressionsInQuery =
+            this.hasExpressionsInQuery(currentQuery);
 
           if (hasExpressionsInQuery) {
             if (!memberExpressions) {
@@ -1195,7 +1196,15 @@ class ApiGateway {
             currentQuery = this.parseMemberExpressionsInQuery(currentQuery);
           }
 
-          const normalizedQuery = normalizeQuery(currentQuery, persistent);
+          let normalizedQuery = normalizeQuery(currentQuery, persistent);
+
+          if (hasExpressionsInQuery) {
+            // We need to parse/eval all member expressions early as applyRowLevelSecurity
+            // needs to access the full SQL query in order to evaluate rules
+            normalizedQuery =
+              this.evalMemberExpressionsInQuery(normalizedQuery);
+          }
+
           // First apply cube/view level security policies
           const queryWithRlsFilters = await compilerApi.applyRowLevelSecurity(
             normalizedQuery,
@@ -1204,17 +1213,18 @@ class ApiGateway {
           // Then apply user-supplied queryRewrite
           let rewrittenQuery = await this.queryRewrite(
             queryWithRlsFilters,
-            context,
+            context
           );
 
-          if (hasExpressionsInQuery) {
+          // applyRowLevelSecurity may add new filters which may contain raw member expressions
+          // if that's the case, we should run an extra pass of parsing here to make sure
+          // nothing breaks down the road
+          if (this.hasExpressionsInQuery(rewrittenQuery)) {
+            rewrittenQuery = this.parseMemberExpressionsInQuery(rewrittenQuery);
             rewrittenQuery = this.evalMemberExpressionsInQuery(rewrittenQuery);
           }
 
-          return normalizeQuery(
-            rewrittenQuery,
-            persistent,
-          );
+          return normalizeQuery(rewrittenQuery, persistent);
         }
       )
     );

diff --git a/packages/cubejs-testing/birdbox-fixtures/rbac/cube.js b/packages/cubejs-testing/birdbox-fixtures/rbac/cube.js
@@ -43,6 +43,27 @@ module.exports = {
         },
       };
     }
+    if (user === 'default') {
+      if (password && password !== 'default_password') {
+        throw new Error(`Password doesn't match for ${user}`);
+      }
+      return {
+        password,
+        superuser: false,
+        securityContext: {
+          auth: {
+            username: 'default',
+            userAttributes: {
+              region: 'CA',
+              city: 'San Francisco',
+              canHaveAdmin: false,
+              minDefaultId: 20000,
+            },
+            roles: [],
+          },
+        },
+      };
+    }
     throw new Error(`User "${user}" doesn't exist`);
   }
 };
diff --git a/packages/cubejs-testing/birdbox-fixtures/rbac/model/cubes/users.yaml b/packages/cubejs-testing/birdbox-fixtures/rbac/model/cubes/users.yaml
@@ -19,6 +19,11 @@ cubes:
 
     access_policy:
       - role: "*"
+        row_level:
+          filters:
+            - member: "{CUBE}.city"
+              operator: equals
+              values: ["{ security_context.auth.userAttributes.city }"]
       - role: admin
         conditions:
           # This thing will fail if there's no auth info in the context

diff --git a/packages/cubejs-testing/test/__snapshots__/smoke-rbac.test.ts.snap b/packages/cubejs-testing/test/__snapshots__/smoke-rbac.test.ts.snap
@@ -587,4 +587,12 @@ Array [
 ]
 `;
 
+exports[`Cube RBAC Engine RBAC via SQL API default policy SELECT with member expressions: users_member_expression 1`] = `
+Array [
+  Object {
+    "count": "149",
+  },
+]
+`;
+
 exports[`Cube RBAC Engine RBAC via SQL API manager SELECT * from line_items: line_items_manager 1`] = `Array []`;
diff --git a/packages/cubejs-testing/test/smoke-rbac.test.ts b/packages/cubejs-testing/test/smoke-rbac.test.ts
@@ -165,6 +165,24 @@ describe('Cube RBAC Engine', () => {
     });
   });
 
+  describe('RBAC via SQL API default policy', () => {
+    let connection: PgClient;
+
+    beforeAll(async () => {
+      connection = await createPostgresClient('default', 'default_password');
+    });
+
+    afterAll(async () => {
+      await connection.end();
+    }, JEST_AFTER_ALL_DEFAULT_TIMEOUT);
+
+    test('SELECT with member expressions', async () => {
+      const res = await connection.query('SELECT COUNT(city) as count from "users" HAVING (COUNT(1) > 0)');
+      // Pushed SQL queries should not fail
+      expect(res.rows).toMatchSnapshot('users_member_expression');
+    });
+  });
+
   describe('RBAC via REST API', () => {
     let client: CubeApi;
     let defaultClient: CubeApi;