- Notifications
You must be signed in to change notification settings - Fork928
feat: add template RBAC#4125
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
This reverts commit1f4ceee.
| @@ -0,0 +1,12 @@ | |||
| BEGIN; | |||
| ALTER TABLE templates ADD COLUMN user_acl jsonb NOT NULL default '{}'; | |||
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.
Why store this as a massivejsonb blob instead of a table? How do we plan on efficiently getting a list of all templates the user has 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.
It's basically the same way everything else is done. You get a list of all the templates and then you filter it through the auth filter for the subset of templates that you have access to. Just wrote a test for thishere
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.
For workspaces, itkinda does this, but we still query by the owner so it's lessened significantly.
I guess the use-case I have in mind is 10 templates with 10 users on each... for every HTTP request we'll load 100 entries into memory and check against them? That sounds like a lot of excess when we could (I don't see why not, but maybe there's a reason) have a table that just has the user ID indexed...
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.
The memory overhead is unfortunate but this way plays better with rego is the short answer. We can look into rewriting it if you think it's a showstopper, I'm not sold onjsonb but joining tables doesn't play well with sqlc either so the maintainability of the code might suffer as a result of trying to glue everything together.
Is the frontend going to be in a separate PR? |
@bpmct no@BrunoQuaresma is going to push it here |
| BEGIN; | ||
| ALTER TABLE templates ADD COLUMN user_acl jsonb NOT NULL default '{}'; | ||
| ALTER TABLE templates ADD COLUMN is_private boolean NOT NULL default 'false'; |
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_ isn't necessary here
| TemplateRoleAdmin TemplateRole = "admin" | ||
| TemplateRoleWrite TemplateRole = "write" | ||
| TemplateRoleRead TemplateRole = "read" | ||
| TemplateRoleDeleted TemplateRole = "" |
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.
Deleted should beDelete in this name pattern
| UPDATE | ||
| templates | ||
| SET | ||
| user_acl = $2 | ||
| WHERE | ||
| id = $1` |
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.
Why can't this be done withsqlc 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.
because it exposes ajson.RawMessage through the API. I wanted to preserve type safety as much as possible since we can't enforce the jsonb structure in the DB
| } | ||
| func (q *sqlQuerier) GetTemplateUserRoles(ctx context.Context, id uuid.UUID) ([]TemplateUser, error) { | ||
| const query = ` |
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'm not convinced we should escape sqlc here... this function is only used once, so why not just do the struct conversion where it's queried from 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.
there's unfortunately multiple problems when I tried to write this withsqlc. One is that it didn't recognize the intermediatevalue column in the subquery. When I tried to jank around that the resulting return type was wildly different than what I tried to express 😞
No description provided.