- Notifications
You must be signed in to change notification settings - Fork927
refactor(dbauthz): add authz for system-level functions#6513
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
- 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.
| @@ -963,6 +963,18 @@ func (q *querier) GetUserByID(ctx context.Context, id uuid.UUID) (database.User, | |||
| 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 👍.
| @@ -42,11 +59,16 @@ func TestProvisionerDaemonServe(t *testing.T) { | |||
| 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.
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.
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.
| @@ -963,6 +963,18 @@ func (q *querier) GetUserByID(ctx context.Context, id uuid.UUID) (database.User, | |||
| 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.