feat(provisioner): add support for workspace_owner_rbac_roles #16407
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
bf30eee to
ada6342
Compare
| ownerRbacRoles := []string{} | ||
| for _, role := range owner.RBACRoles { | ||
| ownerRbacRoles = append(ownerRbacRoles, role) | ||
| } |
There was a problem hiding this comment.
Unfortunately this is not sufficient.
This is the code we use to fetch all the user's roles from the database:
https://github.com/coder/coder/blob/main/coderd/httpmw/apikey.go#L447-L475
So we should change this to:
| ownerRbacRoles := []string{} | |
| for _, role := range owner.RBACRoles { | |
| ownerRbacRoles = append(ownerRbacRoles, role) | |
| } | |
| roles, err := s.Database.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx), owner.ID) | |
| if err != nil { | |
| // Should we fail the job? | |
| } | |
| ownerRbacRoles := []string{} | |
| for _, role := range roles.Roles { | |
| ownerRbacRoles = append(ownerRbacRoles, role) | |
| } |
This will include all org roles as <org-id>:<org-role>. Would it be possible to have the role datatype be a struct, instead of list(string)?
role {
name string
org-id string
}This would be preferred, as roles are uniquely identified by the pair of (org-id, rolename). And roles at the site wide level just have the uuid of 000...000
There was a problem hiding this comment.
Makes perfect sense to me! Just updated the PR
ada6342 to
f70ff35
Compare
| } | ||
| ownerRbacRoles := []*sdkproto.OwnerRbacRoles{} | ||
| for _, role := range roles.Roles { | ||
| ownerRbacRoles = append(ownerRbacRoles, &sdkproto.OwnerRbacRoles{Name: role, OrgId: s.OrganizationID.String()}) |
There was a problem hiding this comment.
I think we should convert the nil uuid to the empty string. Since we losing the parsed uuid, determining a nil uuid is trickier on the other side of the communication. Just pass the empty string to indicate "no org id"
| ownerRbacRoles = append(ownerRbacRoles, &sdkproto.OwnerRbacRoles{Name: role, OrgId: s.OrganizationID.String()}) | |
| if s.OrganizationID == uuid.Nil { | |
| ownerRbacRoles = append(ownerRbacRoles, &sdkproto.OwnerRbacRoles{Name: role, OrgId: ""}) | |
| continue | |
| } | |
| ownerRbacRoles = append(ownerRbacRoles, &sdkproto.OwnerRbacRoles{Name: role, OrgId: s.OrganizationID.String()}) |
| @@ -594,6 +594,15 @@ func (s *server) acquireProtoJob(ctx context.Context, job database.ProvisionerJo | |||
| }) | |||
| } | |||
|
|
|||
| roles, err := s.Database.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx), owner.ID) | |||
There was a problem hiding this comment.
The ctx should already have the right perms. If it does not, we can add what it needs to here: https://github.com/coder/coder/blob/main/coderd/database/dbauthz/dbauthz.go#L164-L193
| roles, err := s.Database.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx), owner.ID) | |
| roles, err := s.Database.GetAuthorizationUserRoles(ctx, owner.ID) |
| @@ -244,6 +244,11 @@ enum WorkspaceTransition { | |||
| DESTROY = 2; | |||
| } | |||
|
|
|||
| message OwnerRbacRoles { | |||
There was a problem hiding this comment.
Role is descriptive enough imo.
| message OwnerRbacRoles { | |
| message Role { |
f70ff35 to
b960125
Compare
|
Updated PR based on suggestions |
| Value: "member", | ||
| }, { | ||
| Key: "rbac_roles_org_id", | ||
| Value: "00000000-0000-0000-0000-000000000000", |
There was a problem hiding this comment.
| Value: "00000000-0000-0000-0000-000000000000", | |
| Value: "", |
| }, | ||
| Request: &proto.PlanRequest{ | ||
| Metadata: &proto.Metadata{ | ||
| WorkspaceOwnerRbacRoles: []*proto.Role{{Name: "member", OrgId: "00000000-0000-0000-0000-000000000000"}}, |
There was a problem hiding this comment.
I think the test should be an empty string now?
| WorkspaceOwnerRbacRoles: []*proto.Role{{Name: "member", OrgId: "00000000-0000-0000-0000-000000000000"}}, | |
| WorkspaceOwnerRbacRoles: []*proto.Role{{Name: "member", OrgId: ""}}, |
|
@nxf5025 can you make the test changes and merge/rebase? I will then merge |
b960125 to
bd9d6c8
Compare
|
Hey @Emyrk sorry for the delay. Got hit with the flu. I just rebased and updated the test. |
|
@nxf5025 np, thank you for getting this in! |
|
@Emyrk - I see this went stale and got closed. Anything you need from me to get this in? |
|
@nxf5025 merged! |
Part of coder/terraform-provider-coder#330
Adds support for the coder_workspace_owner.rbac_roles attribute