feat(groups): Allow group membership management #2142

javirln · 2025-06-26T16:00:32Z

This patch introduces support for managing users within a group. Key changes include:

New API endpoints:
- AddMembers to add users to a group
- RemoveMembers to remove users from a group
Audit logging for membership changes
Extended test coverage to include the new endpoints
API-level refactor to use a generic IdentityReference for group-related requests
New CLI commands for group and membership management (currently hidden):
- group
- group describe
- group list
- group delete
- group member add
- group member list
- group member delete
CLI commands handle pagination when needed and confirmation prompts for deletions

Permissions matrix:

Admins and Owners have full access to group and membership operations
Only Admins, Owners, and Group Maintainers can add or remove members
Viewers and Org Members can list groups and their members

Pending:

Support for updating a user’s role within a group.
CLI support for updating groups

CLI Examples:

Creating a group

$ chainloop group create --name "Operations" --description "Focused group dedicated to day to day operations"
┌──────────────────────────────────────┬────────────┬──────────────────────────────────────────────────┬──────────────────────┬──────────────────────┐
│ ID                                   │ NAME       │ DESCRIPTION                                      │ CREATED AT           │ UPDATED AT           │
├──────────────────────────────────────┼────────────┼──────────────────────────────────────────────────┼──────────────────────┼──────────────────────┤
│ 69314171-9705-473f-988c-3a57365aa7cd │ Operations │ Focused group dedicated to day to day operations │ 2025-06-26T15:44:18Z │ 2025-06-26T15:44:18Z │
└──────────────────────────────────────┴────────────┴──────────────────────────────────────────────────┴──────────────────────┴──────────────────────┘
INF Group created successfully

Adding members to a group

$ chainloop group member add --name "Operations" --email "john@chainloop.local"
INF Member john@chainloop.local successfully added to group Operations

Listing members of a group

$ chainloop group member list --name Operations                                
┌──────────────────────────────────────────────┬────────────┬─────────────────────┐
│ EMAIL                                        │ ROLE       │ ADDED AT            │
├──────────────────────────────────────────────┼────────────┼─────────────────────┤
│ john@chainloop.local                         │ Member     │ 26 Jun 25 15:45 UTC │
│ Sarah De los Angeles <sarah@chainloop.local> │ Maintainer │ 26 Jun 25 15:44 UTC │
└──────────────────────────────────────────────┴────────────┴─────────────────────┘
INF Showing [1-2] out of 2

Removing members from a group

$ chainloop group member delete --name Operations --email "john@chainloop.local"
WRN You are about to remove the user "john@chainloop.local" from the group "Operations"

To confirm, please type "1QKW-7MAU"
1QKW-7MAU 
INF Member john@chainloop.local successfully removed from group Operations

Removing groups

$ chainloop group delete --name Operations
WRN Are you sure you want to delete the group 'Operations'?
To confirm, please type "RA72-4LQC"
RA72-4LQC
INF Group 'Operations' has been removed

migmartri · 2025-06-26T16:31:32Z

chore(groups)

maybe more like feature? :)

migmartri · 2025-06-26T21:06:08Z

Great description of the PR @javirln!

app/controlplane/internal/service/service.go

migmartri · 2025-06-27T13:49:40Z

@javirln let me know once you have fixed the conflicts

app/controlplane/pkg/authz/authz.go

Signed-off-by: Javier Rodriguez <javier@chainloop.dev>

jiparis · 2025-06-30T09:08:56Z

app/controlplane/pkg/data/group.go

 		}

+		// Update the user membership with the role of maintainer
+		_, err = tx.Membership.Create().


If I'm not wrong, we agreed on not adding the OrgAdmin as maintainer. They can declare other maintainers, but they don't need to be maintainers as they are already admins (and maintainers are not mandatory)

in GitHub, the creator of the group becomes a maintainer automatically, then they can leave though

Yes, not a big deal at all. Let's keep it.

migmartri · 2025-06-30T09:00:33Z

app/cli/cmd/group_delete.go

+
+func newGroupDeleteCmd() *cobra.Command {
+	var groupName string
+	var force bool


is force used?

I think it is from a previous implementation, removing it.

migmartri · 2025-06-30T09:01:44Z

app/cli/cmd/group_list.go

+	return cmd
+}
+
+func GroupListTableOutput(groupListResult *action.GroupListResult) error {


it might not need to be exposed

migmartri · 2025-06-30T09:01:58Z

app/cli/cmd/group_list.go

+}
+
+// Helper function to format time string
+func formatTime(timeStr string) string {


this doesn't exist already?

I haven't seen it, we parse the time individually, see the example of workflow list

chainloop/app/cli/cmd/workflow_list.go

Line 128 in e461bc7

p.CreatedAt.Format(time.RFC822),

migmartri · 2025-06-30T09:12:26Z

app/controlplane/pkg/data/group.go

 		}

+		// Update the user membership with the role of maintainer
+		_, err = tx.Membership.Create().


in GitHub, the creator of the group becomes a maintainer automatically, then they can leave though

jiparis · 2025-06-30T09:16:20Z

app/cli/cmd/group.go

+		Hidden:  true,
+	}
+
+	cmd.AddCommand(newGroupCreateCmd(), newGroupDescribeCmd(), newGroupListCmd(), newGroupDeleteCmd(), newGroupMembersCmd())


The GroupUpdate operation is implemented in backend but not exposed in the CLI. We can do it later.

jiparis · 2025-06-30T09:19:11Z

app/controlplane/pkg/biz/group.go

+		return false, NewErrValidationStr("organization ID, group ID, and user ID cannot be empty")
+	}
+
+	membership, err := uc.groupRepo.FindGroupMembershipByGroupAndID(ctx, groupID, userID)


I think here we should now use the enforcer.

jiparis · 2025-06-30T09:24:51Z

app/controlplane/internal/service/service.go

+
+	// If a specific policy was provided, check if the user's role allows that policy
+	if policy != nil && s.enforcer != nil {
+		pass, err := s.enforcer.Enforce(userRole, policy)


Here, userRole would be the Organization role, not the "maintainer" role. The idea would be to remove the above if isMaintainer check (or leave it as a double check), and look for the membership role of the user in the group. If found (it would be role:group:maintainer, we use it to enforce the permission. You can just use the current helpers:

// Allow if user has admin or owner role if userRole == string(authz.RoleAdmin) || userRole == string(authz.RoleOwner) { return nil } ... return authorizeResource(ctx, policy, authz.ResourceTypeGroup, resolvedGroupID)

This way we rely entirely on roles to authorize the operation. It might look more complex, but it's way more flexible, since we can add later other roles, or remove some permissions from the current one, etc.

Update: authorizeResource cannot be used since it only works if RBAC is enabled (Member role). You'll need to iterate through user resource memberships from the context.

jiparis · 2025-06-30T09:29:59Z

@javirln we are almost there. I found that the Role is not used at all, since it's still using the maintainer flag for everything.

Signed-off-by: Javier Rodriguez <javier@chainloop.dev>

migmartri

Thanks @javirln

jiparis · 2025-06-30T11:25:26Z

Thanks @javirln it works great!

javirln self-assigned this Jun 26, 2025

javirln requested review from jiparis and migmartri June 26, 2025 16:00

javirln marked this pull request as ready for review June 26, 2025 16:00

migmartri reviewed Jun 26, 2025

View reviewed changes

app/controlplane/internal/service/service.go Outdated Show resolved Hide resolved

javirln changed the title ~~chore(groups): Allow group membership management~~ feat(groups): Allow group membership management Jun 27, 2025

jiparis reviewed Jun 27, 2025

View reviewed changes

app/controlplane/pkg/authz/authz.go Outdated Show resolved Hide resolved

jiparis reviewed Jun 27, 2025

View reviewed changes

app/controlplane/pkg/authz/authz.go Show resolved Hide resolved

app/controlplane/pkg/authz/authz.go Outdated Show resolved Hide resolved

javirln force-pushed the feat/pfm-3208-members branch from c7a9e37 to b093750 Compare June 27, 2025 14:40

javirln added 3 commits June 30, 2025 09:28

feat(groups): Allow group membership management

e4704f8

Signed-off-by: Javier Rodriguez <javier@chainloop.dev>

make linter happy

df150d3

Signed-off-by: Javier Rodriguez <javier@chainloop.dev>

change order of checks

5c8a978

Signed-off-by: Javier Rodriguez <javier@chainloop.dev>

javirln force-pushed the feat/pfm-3208-members branch from a21a6a2 to 5c8a978 Compare June 30, 2025 07:28

jiparis reviewed Jun 30, 2025

View reviewed changes

migmartri reviewed Jun 30, 2025

View reviewed changes

jiparis reviewed Jun 30, 2025

View reviewed changes

javirln added 3 commits June 30, 2025 12:08

include update command and feedback

7d4871a

Signed-off-by: Javier Rodriguez <javier@chainloop.dev>

fix tests

9f65b4a

Signed-off-by: Javier Rodriguez <javier@chainloop.dev>

fix test

c306f15

Signed-off-by: Javier Rodriguez <javier@chainloop.dev>

migmartri approved these changes Jun 30, 2025

View reviewed changes

javirln merged commit 30ddb14 into chainloop-dev:main Jun 30, 2025
13 checks passed

BrewTestBot mentioned this pull request Jun 30, 2025

chainloop-cli 1.12.0 Homebrew/homebrew-core#228601

Merged

feat(groups): Allow group membership management #2142

feat(groups): Allow group membership management #2142

Uh oh!

Conversation

javirln commented Jun 26, 2025

Uh oh!

migmartri commented Jun 26, 2025

Uh oh!

migmartri commented Jun 26, 2025

Uh oh!

Uh oh!

migmartri commented Jun 27, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiparis commented Jun 30, 2025

Uh oh!

migmartri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jiparis commented Jun 30, 2025

Uh oh!

Uh oh!