- Notifications
You must be signed in to change notification settings - Fork1k
refactor: consolidate template and workspace acl validation#19192
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
Conversation
I think this should finally address all your feedback on the first PR (except for the addition of an "update ACL" verb). sorry for the delay! |
aslilac commentedAug 6, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
jk, I just realized it was only yelling about a file that I can edit, not the fully generated files |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
I would like us to test sending in deleted users to theValidate. Idk if we should auto delete or what.
Can also make an issue and follow up 👍
| typeUpdateValidator[Role codersdk.WorkspaceRole| codersdk.TemplateRole]interface { | ||
| // Users should return a map from user UUIDs (as strings) to the role they | ||
| // are being assigned. Additionally, it should return a string that will be | ||
| // used as the field name for the ValidationErrors returned from Validate. | ||
| Users() (map[string]Role,string) | ||
| // Groups should return a map from group UUIDs (as strings) to the role they | ||
| // are being assigned. Additionally, it should return a string that will be | ||
| // used as the field name for the ValidationErrors returned from Validate. | ||
| Groups() (map[string]Role,string) | ||
| // ValidateRole should return an error that will be used in the | ||
| // ValidationError if the role is invalid for the corresponding resource type. | ||
| ValidateRole(roleRole)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.
❤️
Uh oh!
There was an error while loading.Please reload this page.
I tested a deleted user in one of the template tests at least: coder/enterprise/coderd/templates_test.go Line 1505 in48dd824
I'm working on another pr right now to add the other workspace ACL endpoints and a bunch of tests for all of them |
26458cd intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
ValidateUserIDsquery andValidateGroupIDsqueriesGetGroups, but that has a join in it and would send more data over the wire. Plus it's just nice for these two queries to have parity and there isn't a good equivalent for users already. I hope we can find more places that these will be useful.EXPLAINand this one should be faster (tho not exactly fast, it could certainly be optimized more)ACLUpdateValidatorinterface that lets us create little adapters type for resources that implement an/acl [patch]endpoint like templates and workspaces. It handles validating UUIDs, validating role names, and ensuring that the IDs given actually correspond to IDs in the database.