Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 18 additions & 8 deletions packages/cubejs-api-gateway/src/gateway.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -1195,7 +1196,15 @@ class ApiGateway {
currentQuery = this.parseMemberExpressionsInQuery(currentQuery);
}

const normalizedQuery = normalizeQuery(currentQuery, persistent);
let normalizedQuery = normalizeQuery(currentQuery, persistent);

if (hasExpressionsInQuery) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, add comment about why early/second call is necessary. I'll probably forget about this whole issue next week 😅

// 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,
Expand All @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've not yet checked yet what can applyRowLevelSecurity introduce

But parseMemberExpressionsInQuery is not enough anyway.
There's three states of member expressions:

  1. MemberExpression
    Object with expression method, that takes cubes as symbols and returns SQL

  2. ParsedMemberExpression
    Object with expression as array of strings. Essentially JSON-compatible way of storing MemberExpression

  3. Just a string with JSON-serialzed ParsedMemberExpression.

Schema compiler expects only MemberExpression.
parseMemberExpressionsInQuery turns strings in proper places in query to ParsedMemberExpression.
evalMemberExpressionsInQuery turns ParsedMemberExpression to MemberExpression in those places.

So, depending on what applyRowLevelSecurity can add either only evalMemberExpressionsInQuery, or both of parseMemberExpressionsInQuery + evalMemberExpressionsInQuery are necessary.

Copy link
Contributor

@mcheshkov mcheshkov Dec 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expression: () => '1 = 0',
That's MemberExpression, with expression as method, returning SQL. For this case no need to add any additional calls after applyRowLevelSecurity

String member expression would look like something this: query.segments.push('{"cubeName":"foo","expr":"1=0"}');
This would require to add both calls to parse and eval

Copy link
Member Author

@bsod90 bsod90 Dec 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I've removed all my previous comments and just added the eval. Looks like it should be fine now

rewrittenQuery = this.evalMemberExpressionsInQuery(rewrittenQuery);
}

return normalizeQuery(
rewrittenQuery,
persistent,
);
return normalizeQuery(rewrittenQuery, persistent);
}
)
);
Expand Down
21 changes: 21 additions & 0 deletions packages/cubejs-testing/birdbox-fixtures/rbac/cube.js
Original file line number Diff line number Diff line change
Expand Up @@ -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`);
}
};
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 []`;
18 changes: 18 additions & 0 deletions packages/cubejs-testing/test/smoke-rbac.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,24 @@
});
});

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;
Expand Down
Loading