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

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

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 onInsertGroupMember,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 theEveryone group.

This ends up being yourReducedGroup, and optionally you could do theTotalMemberCount there 🤷‍♂️.

If we add theTotalMemberCount field tocodersdk.Group as you suggested then we don't need theReducedGroup type at all, so I don't think we need the column in the view.

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 assumegroup_member.read ifgroup.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 onInsertGroupMember,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 theResourceGroup still imo. A Group is a set of users, if you are mutating the set, then we should useResourceGroup.

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

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

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.

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

List of changes:

• created agroup_members_expanded database view
• configured sqlc to use the view as the source of theGroupMember object
• rewrote group member queries to use the view
• created the RBAC resourceResourceGroupMember and added it to roles which hadGroup permissions
• rewrote dbauthz queries to use the new RBAC object
• refactored code which uses the queries to work with theGroupMember return type (instead ofUser)
• addedTotalMemberCount tocodersdk.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.

My unit test was merged.

You will have to just fixup these test assertions:

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

@Emyrk I think I updated all the tests. I rebased ontomain andmake test,make lint pass locally.

I updatedgroupsauth_test.go to work with the new permission system. I saw that you assumed that if a user didn't haveGroupMemberread permission thenGetGroupMembersByGroupID 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 I made the changes you requested. Thank you for all the reviews! Contributing this PR was fun.