- Notifications
You must be signed in to change notification settings - Fork1.2k
refactor(dbauthz): add authz for system-level functions#6513
Conversation
- Introduces rbac.ResourceSystem- Grants system.* to system and provisionerd rbac subjects
…st user, and when registering InMemoryProvisionerd
…functions as this breaks external provisionerds
| @@ -282,11 +282,6 @@ func (s *MethodTestSuite) TestProvsionerJob() { | |||
| check.Args(database.UpdateProvisionerJobWithCancelByIDParams{ID: j.ID}). | |||
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.
Note: most of the changes here are just moving the respective tests tosystem_test.go to keep things consistent.
| return fetch(q.log, q.auth, q.db.GetUserByID)(ctx, id) | ||
| } | ||
| // GetUsersByIDs is only used for usernames on workspace return data. |
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.
Note: Moved this to querier from system and set a simple authz check here. I can move it back to system but it's probably better to use rbac.ResourceUser here.
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.
Fair 👍.
| codersdk.FeatureExternalProvisionerDaemons: 1, | ||
| }, | ||
| }) | ||
| srv, err := client.ServeProvisionerDaemon(context.Background(), user.OrganizationID, []codersdk.ProvisionerType{ |
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.
note: updated this test to use a separate user from owner
Uh oh!
There was an error while loading.Please reload this page.
mafredri left a comment
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. Only one observation which seems fairly minor for now anyway.
Uh oh!
There was an error while loading.Please reload this page.
Emyrk left a comment
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.
LG, glad we protect these now.
| return fetch(q.log, q.auth, q.db.GetUserByID)(ctx, id) | ||
| } | ||
| // GetUsersByIDs is only used for usernames on workspace return data. |
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.
Fair 👍.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Note: I'm skipping provisionerd and provisionerjob-related functions; we need to add RBAC resources for these. Will create a follow-up PR for this.