- Notifications
You must be signed in to change notification settings - Fork1k
feat: add endpoint for retrieving workspace acl#19375
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
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 see we do not return the workspace ACL as apart of the workspace get, and instead require a new query for it.
I am guessing this is because of permissions. I do not think it's crazy to say if you canread a workspace, you can also read the list of people it is shared with.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| groups:=make([]codersdk.WorkspaceGroup,0,len(dbGroups)) | ||
| for_,it:=rangedbGroups { | ||
| varmembers []database.GroupMember | ||
| // For context see https://github.com/coder/coder/pull/19375 | ||
| // nolint:gocritic | ||
| members,err=api.Database.GetGroupMembersByGroupID(dbauthz.AsSystemRestricted(ctx), database.GetGroupMembersByGroupIDParams{ | ||
| GroupID:it.Group.ID, | ||
| IncludeSystem:false, | ||
| }) | ||
| iferr!=nil { | ||
| httpapi.InternalServerError(rw,err) | ||
| return | ||
| } | ||
| groups=append(groups, codersdk.WorkspaceGroup{ | ||
| Group:db2sdk.Group(database.GetGroupsRow{ | ||
| Group:it.Group, | ||
| OrganizationName:it.OrganizationName, | ||
| OrganizationDisplayName:it.OrganizationDisplayName, | ||
| },members,len(members)), | ||
| Role:convertToWorkspaceRole(workspaceACL.Groups[it.Group.ID.String()].Permissions), | ||
| }) | ||
| } |
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.
Same as above. Can we just make someMinimialGroup type that does not require fetching all members in the group?
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 was thinking this could be nice for showing the users that are in a group on the sharing page, but maybe we could require a click for that and have it be a separate query.
I agree, but I think we need to actually accommodate that in the permissions users are given, rather than hack around it. While we are hacking around it, I think it needs to be kept to an endpoint that's only available with the experiment. Since it's not stable, we can decide to remove this endpoint later if it seems redundant when the usual workspace endpoints get sharing info. |
Ah this is a great point 👍 |
aslilac commentedAug 22, 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.
I kinda wanna punt on the groups issue. I think the extra info could be useful for the frontend (tho it does seem that the templates permissions page underutilizes the extra data). I can even make an issue to re-evaluate when we pick back up on shared workspaces. |
@Emyrk I would appreciate if this one got merged too 😄 there will be tacos in it for you |
d7ee101 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Implements
/acl [get]for workspaces, with tests.One big concern this PR is starting to bring up: users that have
workspace.sharewill also needorg_member.readandgroup.readwithin their organization, or they won't actually be able to share with anyone. However, giving everyoneorg_member.readandgroup.readwould probably agitate some of our more security conscious customers and not giving it to anyone would either just be lying about our security model or would make the feature useless.This is probably going to have to be solved by reimplementing the default "organization member" role as a custom role, which we were already planning on doing anyway. Just something important to keep in mind when we get to that point.
It's worth noting that all of this applies to template ACL as well, but in that case we kind of got away with it because usually only admins can see/edit template ACLs. For workspaces, that is very muchnot the case. Still, we might actually want to go back and fix this same issue for templates.