NotificationsYou must be signed in to change notification settings
Fork1.1k
Star11.6k

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

Jump to bottom

Merged

Emyrk merged 1 commit intocoder:mainfromnxf5025:provisioner-terraform-workspace-owner-rbac-roles

Mar 2, 2025

Merged

feat(provisioner): add support for workspace_owner_rbac_roles#16407

Emyrk merged 1 commit intocoder:mainfromnxf5025:provisioner-terraform-workspace-owner-rbac-roles

Mar 2, 2025

Conversation

Copy link

Contributor

nxf5025 commentedFeb 3, 2025

Part ofcoder/terraform-provider-coder#330

Adds support for the coder_workspace_owner.rbac_roles attribute

cdr-botbot added the communityPull Requests and issues created by the community. label

Feb 3, 2025

github-actionsbot assignednxf5025

Feb 3, 2025

Copy link

github-actionsbot commentedFeb 3, 2025•
edited
Loading

All contributors have signed the CLA ✍️ ✅
_{Posted by theCLA Assistant Lite bot.}

Copy link

ContributorAuthor

nxf5025 commentedFeb 3, 2025

I have read the CLA Document and I hereby sign the CLA

cdrci2 added a commit to coder/cla that referenced this pull request

Feb 3, 2025

@nxf5025has signed the CLA incoder/coder#16407

7909b57

nxf5025 mentioned this pull request

Feb 3, 2025

feat: addrbac_roles tocoder_workspace_owner data sourcecoder/terraform-provider-coder#330

Merged

matifali requested a review fromEmyrk

February 3, 2025 18:23

nxf5025 force-pushed theprovisioner-terraform-workspace-owner-rbac-roles branch frombf30eee toada6342Compare

February 3, 2025 19:12

Emyrk reviewed

Feb 6, 2025

View reviewed changes

Copy link

Member

Emyrk left a 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 👍

coderd/provisionerdserver/provisionerdserver.go Outdated

Comment on lines 597 to 608

		ownerRbacRoles:= []string{}
		for_,role:=rangeowner.RBACRoles {
		ownerRbacRoles=append(ownerRbacRoles,role)
		}

Copy link

Member

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:

Suggested change

	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

Copy link

ContributorAuthor

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

nxf5025 force-pushed theprovisioner-terraform-workspace-owner-rbac-roles branch fromada6342 tof70ff35Compare

February 7, 2025 16:51

Emyrk reviewed

Feb 10, 2025

View reviewed changes

Copy link

Member

Emyrk left a 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.

coderd/provisionerdserver/provisionerdserver.go Outdated

		}
		ownerRbacRoles:= []*sdkproto.OwnerRbacRoles{}
		for_,role:=rangeroles.Roles {
		ownerRbacRoles=append(ownerRbacRoles,&sdkproto.OwnerRbacRoles{Name:role,OrgId:s.OrganizationID.String()})

Copy link

Member

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"

Suggested change

	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()})

coderd/provisionerdserver/provisionerdserver.go Outdated

		})
		}

		roles,err:=s.Database.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx),owner.ID)

Copy link

Member

EmyrkFeb 10, 2025

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

Suggested change

	roles,err:=s.Database.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx),owner.ID)
	roles,err:=s.Database.GetAuthorizationUserRoles(ctx,owner.ID)

Emyrk reviewed

Feb 10, 2025

View reviewed changes

provisionersdk/proto/provisioner.proto Outdated

		DESTROY=2;
		}

		messageOwnerRbacRoles {

Copy link

Member

EmyrkFeb 10, 2025

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.

Suggested change

	messageOwnerRbacRoles {
	messageRole {

nxf5025 force-pushed theprovisioner-terraform-workspace-owner-rbac-roles branch fromf70ff35 tob960125Compare

February 13, 2025 14:00

Copy link

ContributorAuthor

nxf5025 commentedFeb 13, 2025

Updated PR based on suggestions

Emyrk approved these changes

Feb 13, 2025

View reviewed changes

Copy link

Member

Emyrk left a 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

provisioner/terraform/provision_test.go Outdated

		Value:"member",
		}, {
		Key:"rbac_roles_org_id",
		Value:"00000000-0000-0000-0000-000000000000",

Copy link

Member

EmyrkFeb 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Suggested change

	Value:"00000000-0000-0000-0000-000000000000",
	Value:"",

provisioner/terraform/provision_test.go Outdated

		},
		Request:&proto.PlanRequest{
		Metadata:&proto.Metadata{
		WorkspaceOwnerRbacRoles: []*proto.Role{{Name:"member",OrgId:"00000000-0000-0000-0000-000000000000"}},

Copy link

Member

EmyrkFeb 13, 2025

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?

Suggested change

	WorkspaceOwnerRbacRoles: []*proto.Role{{Name:"member",OrgId:"00000000-0000-0000-0000-000000000000"}},
	WorkspaceOwnerRbacRoles: []*proto.Role{{Name:"member",OrgId:""}},