-
Notifications
You must be signed in to change notification settings - Fork 904
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Introduces rbac.ResourceSystem - Grants system.* to system and provisionerd rbac subjects
…st user, and when registering InMemoryProvisionerd
35341eb
to
6f40cf6
Compare
…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 to system_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
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.
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 👍.
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.