fix: allow group members to read group information #14200

hugodutka · 2024-08-07T12:58:55Z

Fixes #13988. See the issue for background information.

This PR:

allows Group members to see they are part of the group
allows Group members to see the total count of that group's members even if they are not permitted to read detailed information about them, like username or email
exposes a new endpoint under /organizations/{organization}/users/{user}/reduced-groups that returns an array of ReducedGroup objects. These objects contain basic information about the group and the total member count.
makes the account page (/settings/account) use this endpoint to display information about groups

I asked questions about permission scope in the issue, but didn't get an answer in time, so I assumed that by default:

group members should be able to see they are part of the group
they should be able to read the total group members count
they shouldn't be able to access detailed information about the group members

Let me know if these assumptions are correct.

This is my first PR to Coder, and I'm not that familiar with the codebase, so I'm very much open to feedback and suggestions. :)

github-actions · 2024-08-07T12:59:11Z

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

hugodutka · 2024-08-07T13:01:33Z

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

Emyrk

This has made me realize that groups and users are pretty intertwined, but we should probably have a GroupMember resource that helps handle some of these permission edge cases.

they shouldn't be able to access detailed information about the group members

This is true, and maybe we should just handle this all the way at the database layer. I know originally we returned database.User because the table group_members has almost no information and is basically useless without more info. So we returned the user.

But we could do something like we do for templates.

Basically, all template queries use the view templates_with_names to join in relevant username, icon, etc information.

This ends up being your ReducedGroup, and optionally you could do the TotalMemberCount there 🤷‍♂️.

SQLc config to make the template view the primary model: https://github.com/coder/coder/blob/main/coderd/database/sqlc.yaml#L77-L78

PR example of updating said view: https://github.com/coder/coder/pull/13858/files

P.S: This is touching many parts of the code with some weak documentation and examples. There is an effort to document some of the RBAC stuff here: #14065

This PR is solving some fundamental problems with groups that should be fixed, and really happy to see this PR coming in 👍

coderd/database/modelmethods.go

codersdk/groups.go

enterprise/coderd/coderd.go

enterprise/coderd/groups.go

hugodutka · 2024-08-08T12:38:07Z

Thank you for the review @Emyrk. Couple of questions:

GroupMember resource

we should probably have a GroupMember resource

I considered this before and found some issues:

We don't have much control over custom roles in customer deployments. Any role with Group read permissions would need to be updated with the GroupMember permissions to keep on working as before. I'm not sure if "custom roles" is a widely used feature, but customers would probably need to be informed about this change.
We probably should rework the authz checks on InsertGroupMember, DeleteGroupMemberFromGroup, making this a bigger PR

Do you think we should add the new resource given these problems?

GroupMember DB View

I like this idea. It would also simplify querying for members of the Everyone group.

This ends up being your ReducedGroup, and optionally you could do the TotalMemberCount there 🤷‍♂️.

If we add the TotalMemberCount field to codersdk.Group as you suggested then we don't need the ReducedGroup type at all, so I don't think we need the column in the view.

Emyrk · 2024-08-08T14:07:13Z

Thank you for the review @Emyrk. Couple of questions:

GroupMember resource

we should probably have a GroupMember resource

I considered this before and found some issues:

We don't have much control over custom roles in customer deployments. Any role with Group read permissions would need to be updated with the GroupMember permissions to keep on working as before. I'm not sure if "custom roles" is a widely used feature, but customers would probably need to be informed about this change.

Custom roles is not yet released, it's in experimental. So there are currently no custom_roles in the wild.

You are correct, adding related resources causes an issue when making roles. As granting 1 half of the relation doesn't really make much sense, and there is no obvious linkage between the two.

But this is a flaw of our current RBAC, and it already exists in a few places. The extra resource is easier to understand then the ACL workaround you have in place imo.

The good news of the custom_roles though is that we control the interface to making custom roles. Currently, it's basically a 1:1 with permissions, but we could implement some nicer UI and auto create these linkages. So the custom_roles endpoint or frontend could assume group_member.read if group.read is assigned for example.

So tl;dr, this issue exists for other resources today, and I think we can solve this somewhere else.

We probably should rework the authz checks on InsertGroupMember, DeleteGroupMemberFromGroup, making this a bigger PR

Actually I do not think we do. When you insert/delete a member from the group, you are not accessing the group member, you are mutating the ResourceGroup still imo. A Group is a set of users, if you are mutating the set, then we should use ResourceGroup.

If you are reading/updating an individual member, then it makes sense to use ResourceGroupMember.

A final note on all of this. If we model this as a ResourceGroupMember, we can actually end up doing the authz check in SQL like we do for users.

As deployments grow, we could easily be fetching a few thousand users, and doing 1k authz checks isn't very efficient (they are in memory and can be ~1ms).

So if we model things as we have been discussing, it's a fast follow up to optimize into a SQL query later.

hugodutka · 2024-08-09T17:48:06Z

@Emyrk, I adjusted the PR according to your feedback.

List of changes:

created a group_members_expanded database view
configured sqlc to use the view as the source of the GroupMember object
rewrote group member queries to use the view
created the RBAC resource ResourceGroupMember and added it to roles which had Group permissions
rewrote dbauthz queries to use the new RBAC object
refactored code which uses the queries to work with the GroupMember return type (instead of User)
added TotalMemberCount to codersdk.Group
removed the /organizations/{organization}/users/{user}/reduced-groups route
rolled back frontend changes to use the previous groups by organization endpoint

Let me know if this matches what you had in mind. I'll fix the tests then.

Emyrk

This is overall looking good 👍

I created a test to mess around with some of this in an easier setting:
#14223

If I get a review of it into main, it'll be easier to use for your testing. Right now I just copy pasted it in to check out the code paths.

Emyrk · 2024-08-09T18:58:59Z

coderd/database/migrations/000242_group_members_view.up.sql

+CREATE VIEW
+    group_members_expanded
+AS
+-- If the group is a user made group, then we need to check the group_members table.
+-- If it is the "Everyone" group, then we need to check the organization_members table.
+WITH all_members AS (
+    SELECT user_id, group_id FROM group_members
+    UNION
+    SELECT user_id, organization_id AS group_id FROM organization_members
+)
+SELECT
+    users.id AS user_id,
+    users.email AS user_email,
+    users.username AS user_username,
+    users.hashed_password AS user_hashed_password,
+    users.created_at AS user_created_at,
+    users.updated_at AS user_updated_at,
+    users.status AS user_status,
+    users.rbac_roles AS user_rbac_roles,
+    users.login_type AS user_login_type,
+    users.avatar_url AS user_avatar_url,
+    users.deleted AS user_deleted,
+    users.last_seen_at AS user_last_seen_at,
+    users.quiet_hours_schedule AS user_quiet_hours_schedule,
+    users.theme_preference AS user_theme_preference,
+    users.name AS user_name,
+    users.github_com_user_id AS user_github_com_user_id,
+    groups.organization_id AS organization_id,
+    groups.name AS group_name,
+    all_members.group_id AS group_id
+FROM
+    all_members
+JOIN
+    users ON users.id = all_members.user_id
+JOIN
+    groups ON groups.id = all_members.group_id
+WHERE
+    users.deleted = 'false';
+
+COMMENT ON VIEW group_members_expanded IS 'Joins group members with user information, organization ID, group name. Includes both regular group members and organization members (as part of the "Everyone" group).';


This is exactly what I had in mind. Ideally we trim even more of the excess imo.

I know the UI currently takes a codersdk.User, so let's not fix this in this PR. But a followup action we can take is start removing a lot of the excess fields returned here, and make a codersdk.GroupMember type.

coderd/database/modelmethods.go

Emyrk · 2024-08-09T20:33:01Z

coderd/database/dbmem/dbmem.go

@@ -724,17 +724,40 @@ func (q *FakeQuerier) getOrganizationMemberNoLock(orgID uuid.UUID) []database.Or
 }

 // getEveryoneGroupMembersNoLock fetches all the users in an organization.
-func (q *FakeQuerier) getEveryoneGroupMembersNoLock(orgID uuid.UUID) []database.User {
+func (q *FakeQuerier) getEveryoneGroupMembersNoLock(orgID uuid.UUID) []database.GroupMember {


The FakeQuerier has a data field groupMembers []database.GroupMember. This should now be groupMembers []database.GroupMemberTable

Doing that will review some queries that need to be updated to include the joined information.

Emyrk · 2024-08-12T13:37:08Z

My unit test was merged.

You will have to just fixup these test assertions:

coder/coderd/database/dbauthz/groupsauth_test.go

Lines 114 to 130 in 8af8c77

    
           { 
        
           	Name: "GroupMember", 
        
           	Subject: rbac.Subject{ 
        
           		ID:    users[0].ID.String(), 
        
           		Roles: rbac.Roles(must(rbac.RoleIdentifiers{rbac.ScopedRoleOrgMember(org.ID)}.Expand())), 
        
           		Groups: []string{ 
        
           			group.Name, 
        
           		}, 
        
           		Scope: rbac.ExpandableScope(rbac.ScopeAll), 
        
           	}, 
        
           	// TODO: currently group members cannot see their own groups. 
        
           	//	If this is fixed, these booleans should be flipped to true. 
        
           	ReadGroup:   false, 
        
           	ReadMembers: false, 
        
           	// TODO: If fixed, they should only be able to see themselves 
        
           	// MembersExpected: 1, 
        
           },

Just wanted to get an actual breaking test from these changes 👍

- allow group members to see they are part of the group, but not see that information about other members - add a GetGroupMembersCountByGroupID SQL query, which allows group members to see members count without revealing other information about the members - expose a reducedGroupsByUserAndOrganization API endpoint

- fix type issues coming from replacing User with GroupMember in group member queries

… by org endpoint

… work on empty groups

…on the account page

hugodutka · 2024-08-12T20:24:58Z

@Emyrk I think I updated all the tests. I rebased onto main and make test, make lint pass locally.

I updated groupsauth_test.go to work with the new permission system. I saw that you assumed that if a user didn't have GroupMember read permission then GetGroupMembersByGroupID would return an error, but my implementation returns an empty list instead. I updated the test to match this behavior. Hope that's ok.

Emyrk

@hugodutka this is great 👍

I updated groupsauth_test.go to work with the new permission system. I saw that you assumed that if a user didn't have GroupMember read permission then GetGroupMembersByGroupID would return an error, but my implementation returns an empty list instead. I updated the test to match this behavior. Hope that's ok.

👍 That is fine

My only blocking request is the dbgen change mentioned in one of my comments. If you change that, the rest LGTM and I'll merge it in. Appreciate the contribution 🥳 .

Just leaving notes for myself, I'll open some issues for some follow ups after this:

Reduce the number of fields returned in group_members_expanded. Requires plumbing through the apis and UI. An excess number of fields is currently returned.
- Ideally we can replace codersdk.ReducedUser in codersdk.Group with this database.GroupMember type?
/groups api endpoint exists in /{organization}. Given Account settings page currently lists the current groups a user is in. Should either replace, or include which organizations #14213, we should reconsider a single /groups endpoint with filters, or add additional endpoints for fetching groups.

coderd/database/dbgen/dbgen.go

coderd/database/dbmem/dbmem.go

Emyrk · 2024-08-13T00:05:43Z

coderd/database/modelmethods.go

+func (gm GroupMember) RBACObject() rbac.Object {
+	return rbac.ResourceGroupMember.WithID(gm.UserID).InOrg(gm.OrganizationID).WithOwner(gm.UserID.String())


Emyrk · 2024-08-13T00:06:41Z

coderd/rbac/roles_test.go

@@ -112,6 +112,7 @@ func TestRolePermissions(t *testing.T) {
 	// Subjects to user
 	memberMe := authSubject{Name: "member_me", Actor: rbac.Subject{ID: currentUser.String(), Roles: rbac.RoleIdentifiers{rbac.RoleMember()}}}
 	orgMemberMe := authSubject{Name: "org_member_me", Actor: rbac.Subject{ID: currentUser.String(), Roles: rbac.RoleIdentifiers{rbac.RoleMember(), rbac.ScopedRoleOrgMember(orgID)}}}
+	groupMemberMe := authSubject{Name: "group_member_me", Actor: rbac.Subject{ID: currentUser.String(), Roles: rbac.RoleIdentifiers{rbac.RoleMember(), rbac.ScopedRoleOrgMember(orgID)}, Groups: []string{groupID.String()}}}


This is great 👍

…xist

hugodutka · 2024-08-13T08:44:30Z

@Emyrk I made the changes you requested. Thank you for all the reviews! Contributing this PR was fun.

Emyrk

👍 Approved! I'll merge it in. The weekly-docs CI job is failing because docker changed a link 😢

cdr-bot bot added the community Pull Requests and issues created by the community. label Aug 7, 2024

github-actions bot assigned hugodutka Aug 7, 2024

cdrcommunity added a commit to coder/cla that referenced this pull request Aug 7, 2024

@hugodutka has signed the CLA in coder/coder#14200

6cfda9d

Emyrk reviewed Aug 7, 2024

View reviewed changes

coderd/database/modelmethods.go Outdated Show resolved Hide resolved

codersdk/groups.go Outdated Show resolved Hide resolved

enterprise/coderd/coderd.go Outdated Show resolved Hide resolved

johnstcn reviewed Aug 8, 2024

View reviewed changes

enterprise/coderd/groups.go Outdated Show resolved Hide resolved

hugodutka force-pushed the hugodutka/13988-group-member-read-permissions branch 3 times, most recently from 3930c6b to e07e1d9 Compare August 9, 2024 17:38

Emyrk reviewed Aug 9, 2024

View reviewed changes

hugodutka force-pushed the hugodutka/13988-group-member-read-permissions branch from e07e1d9 to 49297b2 Compare August 12, 2024 11:44

Emyrk mentioned this pull request Aug 12, 2024

Account settings page currently lists the current groups a user is in. Should either replace, or include which organizations #14213

Closed

3 tasks

hugodutka added 13 commits August 12, 2024 19:34

rename ReducedGroup.member_count to ReducedGroup.total_member_count

2409072

use the reduced groups for user endpoint on the AccountPage

5391c15

rename UserWithGroupAndOrgID to GroupMemberRBACHelper

5fcc218

add the group_members_expanded db view

2878c21

add comment on migration

3aa2a23

rewrite group member queries to use the group_members_expanded view

e428d40

add the RBAC ResourceGroupMember and add it to relevant roles

13ca3a9

rewrite GetGroupMembersByGroupID permission checks

1027d9f

- make the GroupMember type contain all user fields

0aea7f8

- fix type issues coming from replacing User with GroupMember in group member queries

simplify RBAC check on group member count

e5c5f3f

add the MemberTotalCount field to codersdk.Group

16e95d0

remove the reduced groups endpoint, revert frontend to use the groups…

d3b4d7c

… by org endpoint

hugodutka added 7 commits August 12, 2024 19:34

revert "simplify RBAC check on group member count" - new query didn't…

b06f943

… work on empty groups

display group.total_member_count instead of group.members.length …

4a65874

…on the account page

adjust total_member_count on MockGroup

1f4dcc7

fixes after rebase

f1513e0

simplify RBAC check on GetGroupMembersCountByGroupID

9ef0e0d

fix tests

7627933

resolve lint error

1080b29

hugodutka force-pushed the hugodutka/13988-group-member-read-permissions branch from 6d469d5 to 1080b29 Compare August 12, 2024 19:34

hugodutka added 2 commits August 12, 2024 20:11

update the groupsauth test to work with new group member permissions

19486da

fix a mistake in test

5373dd7

Emyrk reviewed Aug 13, 2024

View reviewed changes

hugodutka added 2 commits August 13, 2024 08:39

make ErrUserDeleted private

08646da

dbgen.GroupMember now fails when a supplied user or group doesn't e…

4ec6adb

…xist

Emyrk approved these changes Aug 13, 2024

View reviewed changes

Emyrk merged commit 6f9b1a3 into coder:main Aug 13, 2024
30 of 32 checks passed

github-actions bot locked and limited conversation to collaborators Aug 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: allow group members to read group information #14200

fix: allow group members to read group information #14200

hugodutka commented Aug 7, 2024

github-actions bot commented Aug 7, 2024 •

edited

Loading

hugodutka commented Aug 7, 2024

Emyrk left a comment

hugodutka commented Aug 8, 2024

Emyrk commented Aug 8, 2024

GroupMember resource

hugodutka commented Aug 9, 2024 •

edited

Loading

Emyrk left a comment

Emyrk Aug 9, 2024

Emyrk Aug 9, 2024

Emyrk commented Aug 12, 2024

hugodutka commented Aug 12, 2024

Emyrk left a comment •

edited

Loading

Emyrk Aug 13, 2024

Emyrk Aug 13, 2024

hugodutka commented Aug 13, 2024

Emyrk left a comment

		func (gm GroupMember) RBACObject() rbac.Object {
		return rbac.ResourceGroupMember.WithID(gm.UserID).InOrg(gm.OrganizationID).WithOwner(gm.UserID.String())

fix: allow group members to read group information #14200

fix: allow group members to read group information #14200

Conversation

hugodutka commented Aug 7, 2024

github-actions bot commented Aug 7, 2024 • edited Loading

hugodutka commented Aug 7, 2024

Emyrk left a comment

Choose a reason for hiding this comment

hugodutka commented Aug 8, 2024

GroupMember resource

GroupMember DB View

Emyrk commented Aug 8, 2024

GroupMember resource

hugodutka commented Aug 9, 2024 • edited Loading

Emyrk left a comment

Choose a reason for hiding this comment

Emyrk Aug 9, 2024

Choose a reason for hiding this comment

Emyrk Aug 9, 2024

Choose a reason for hiding this comment

Emyrk commented Aug 12, 2024

hugodutka commented Aug 12, 2024

Emyrk left a comment • edited Loading

Choose a reason for hiding this comment

Emyrk Aug 13, 2024

Choose a reason for hiding this comment

Emyrk Aug 13, 2024

Choose a reason for hiding this comment

hugodutka commented Aug 13, 2024

Emyrk left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 7, 2024 •

edited

Loading

hugodutka commented Aug 9, 2024 •

edited

Loading

Emyrk left a comment •

edited

Loading