- Notifications
You must be signed in to change notification settings - Fork906
feat(provisioner): add support for workspace_owner_rbac_roles#16407
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
feat(provisioner): add support for workspace_owner_rbac_roles#16407
Uh oh!
There was an error while loading.Please reload this page.
Conversation
github-actionsbot commentedFeb 3, 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.
All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
bf30eee toada6342CompareThere 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.
Sorry for responding so late. I have the time to help push this forward now 👍
| ownerRbacRoles := []string{} | ||
| for _, role := range owner.RBACRoles { | ||
| ownerRbacRoles = append(ownerRbacRoles, role) | ||
| } |
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.
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) | |
| iferr!=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 therole datatype be a struct, instead oflist(string)?
role { namestring org-idstring}
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 of000...000
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.
Makes perfect sense to me! Just updated the PR
ada6342 tof70ff35CompareThere 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 is looking good 👍. Some minor nits, and then I'll approve.
| } | ||
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think we should convert the nil uuid to the empty string. Since we losing the parseduuid, 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()}) | |
| ifs.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thectx 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Role is descriptive enough imo.
| messageOwnerRbacRoles { | |
| messageRole { |
f70ff35 tob960125CompareUpdated PR based on suggestions |
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.
Thanks for the persistence 👍
Just a nit about the unit test
| Value: "member", | ||
| }, { | ||
| Key: "rbac_roles_org_id", | ||
| Value: "00000000-0000-0000-0000-000000000000", |
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.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 tobd9d6c8CompareHey@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? |
ca23abe intocoder:mainUh oh!
There was an error while loading.Please reload this page.
@nxf5025 merged! |
Part ofcoder/terraform-provider-coder#330
Adds support for the coder_workspace_owner.rbac_roles attribute