- Notifications
You must be signed in to change notification settings - Fork928
feat: Convert rego queries into SQL clauses#4225
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
coderd/database/custom_queries.go Outdated
| ) | ||
| // AuthorizedGetWorkspaces returns all workspaces that the user is authorized to access. | ||
| func (q *sqlQuerier) AuthorizedGetWorkspaces(ctx context.Context, arg GetWorkspacesParams, authorizedFilter rbac.AuthorizeFilter) ([]Workspace, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I wish we could add hooks into SQLc to do this. Kinda sucks to have to copy the original function and call it like this.
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This looks good to me, but I'll defer to other folks who are working on v2.
This solves it without proper types in our AST.Might bite the bullet and implement some better types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
These tests are surprisingly simple to follow, or at least much simpler than I would have expected interfacing with Rego to be.
My primary concern is the query separation between generated and non-generated. I'd love if we could have a single query (maybe even for just getting a single?) that used the same authorization flow, but maybe that's just unreasonable.
eg.
GetWorkspaceByIDGetWorkspacesGetWorkspaceByOwnerAndName
Could all be replaced by this?
coderd/database/custom_queries.go Outdated
| // AuthorizedGetWorkspaces returns all workspaces that the user is authorized to access. | ||
| // This code is copied from `GetWorkspaces` and adds the authorized filter WHERE | ||
| // clause. | ||
| func (q *sqlQuerier) AuthorizedGetWorkspaces(ctx context.Context, arg GetWorkspacesParams, authorizedFilter rbac.AuthorizeFilter) ([]Workspace, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I might change the wording of this. Prefixing withAuthorized makes me think that I'm authorized to get workspaces, not that the workspaces returned I'm authorized to access.
Thoughts onGetAuthorizedWorkspaces instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is it possible for us to get rid ofGetWorkspaces in its entirety? There doesn't seem much of a point in getting workspaces that the user doesn't have access to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@kylecarbs that is a good question. For system level things there might be a need? But we can always add back "GetWorkspaces" later.
I was using SQLc's generated code to make this function. To get rid ofGetWorkspaces would be to remove all those SQLc calls. I guess that is ok? It is nice to use it to generate most of this code, it just can't support the dynamic parts of the query.
This change is only made for "listing" objects, for singular objects running the rego on the returned object is easier. So if we implemented this for the singular too, I would not convert to SQL, I would just call the sqlc code and run I wish there was a way to integrate this into SQLc, as it is a great tool, it just doesn't support this use case. My goal for this PR was to implement this functionality to make this possible. We next implement it for the 2/3 list queries that are "slow" with the |
Uh oh!
There was an error while loading.Please reload this page.
What this does
Currently
Filterworks by querying all resources of a given type, then callingAuthorize()on each element in the list. This would not work with pagination.This feature takes the queries resulting from partial execution, and returns a SQL clause that can be added into a
WHEREstatement of a SQL query. The results are equivalent to all authorized objects in the originalFiltercall.This PR only implements this for listing workspaces. This technique can be copied easily to other listing (templates, users, etc). I just started with the one with the largest lists and the one we will likely paginate first.
Example
Partial execution returns these queries. If either is true, the user in question can read the object.
This code converts these rego queries into the following SQL:
Long Rego Query Example
Future Work
Implement this technique for other list calls.