From ebc6c503a3e0dbcff977ac25226ee123425ddd07 Mon Sep 17 00:00:00 2001 From: Gayathri Vijayan <66356931+grvijayan@users.noreply.github.com> Date: Tue, 21 Oct 2025 11:18:21 +0200 Subject: [PATCH] feat(group): add group permissions (#10853) # Which Problems Are Solved Ensuring the user group resource is managed with appropriate permissions. # How the Problems Are Solved By configuring and checking for the relevant permissions needed to create, read, update, and delete the user groups resource. # Additional Changes N/A # Additional Context - Related to #9702 - Follow-up for PRs #10455, #10758 --- cmd/defaults.yaml | 29 + .../group/v2/integration_test/group_test.go | 182 +++++-- .../group/v2/integration_test/query_test.go | 502 +++++++++++++++++- .../group/v2/integration_test/server_test.go | 35 +- internal/api/grpc/group/v2/query.go | 4 +- internal/command/group.go | 30 +- internal/command/group_test.go | 141 ++++- internal/command/permission_checks.go | 13 + internal/domain/group.go | 27 - internal/domain/permission.go | 4 + internal/query/group.go | 87 ++- internal/query/group_test.go | 122 +++++ proto/zitadel/group/v2/group_service.proto | 34 +- 13 files changed, 1065 insertions(+), 145 deletions(-) diff --git a/cmd/defaults.yaml b/cmd/defaults.yaml index 188e75e40..e5d0f10f4 100644 --- a/cmd/defaults.yaml +++ b/cmd/defaults.yaml @@ -1379,6 +1379,10 @@ InternalAuthZ: - "userschema.read" - "userschema.write" - "userschema.delete" + - "group.create" + - "group.write" + - "group.read" + - "group.delete" - Role: "IAM_OWNER_VIEWER" Permissions: - "iam.read" @@ -1415,6 +1419,7 @@ InternalAuthZ: - "action.execution.read" - "userschema.read" - "session.read" + - "group.read" - Role: "IAM_ORG_MANAGER" Permissions: - "org.read" @@ -1474,6 +1479,10 @@ InternalAuthZ: - "project.grant.member.delete" - "session.read" - "session.delete" + - "group.create" + - "group.write" + - "group.read" + - "group.delete" - Role: "IAM_USER_MANAGER" Permissions: - "org.read" @@ -1502,6 +1511,7 @@ InternalAuthZ: - "project.grant.member.read" - "session.read" - "session.delete" + - "group.read" - Role: "IAM_ADMIN_IMPERSONATOR" Permissions: - "admin.impersonation" @@ -1566,6 +1576,10 @@ InternalAuthZ: - "project.grant.member.delete" - "session.read" - "session.delete" + - "group.create" + - "group.write" + - "group.read" + - "group.delete" - Role: "IAM_LOGIN_CLIENT" Permissions: - "iam.read" @@ -1604,6 +1618,7 @@ InternalAuthZ: - "session.link" - "session.delete" - "userschema.read" + - "group.read" - Role: "ORG_USER_MANAGER" Permissions: - "org.read" @@ -1623,6 +1638,7 @@ InternalAuthZ: - "project.role.read" - "session.read" - "session.delete" + - "group.read" - Role: "ORG_OWNER_VIEWER" Permissions: - "org.read" @@ -1644,6 +1660,7 @@ InternalAuthZ: - "project.grant.read" - "project.grant.member.read" - "project.grant.user.grant.read" + - "group.read" - Role: "ORG_SETTINGS_MANAGER" Permissions: - "org.read" @@ -1674,6 +1691,7 @@ InternalAuthZ: - "project.app.read" - "project.grant.read" - "project.grant.member.read" + - "group.read" - Role: "ORG_PROJECT_PERMISSION_EDITOR" Permissions: - "org.read" @@ -1939,6 +1957,10 @@ SystemAuthZ: - "userschema.read" - "userschema.write" - "userschema.delete" + - "group.create" + - "group.write" + - "group.read" + - "group.delete" - Role: "IAM_OWNER_VIEWER" Permissions: - "iam.read" @@ -1975,6 +1997,7 @@ SystemAuthZ: - "action.execution.read" - "userschema.read" - "session.read" + - "group.read" - Role: "IAM_ORG_MANAGER" Permissions: - "org.read" @@ -2034,6 +2057,10 @@ SystemAuthZ: - "project.grant.member.delete" - "session.read" - "session.delete" + - "group.create" + - "group.write" + - "group.read" + - "group.delete" - Role: "IAM_USER_MANAGER" Permissions: - "org.read" @@ -2062,6 +2089,7 @@ SystemAuthZ: - "project.grant.member.read" - "session.read" - "session.delete" + - "group.read" - Role: "IAM_ADMIN_IMPERSONATOR" Permissions: - "admin.impersonation" @@ -2107,6 +2135,7 @@ SystemAuthZ: - "session.link" - "session.delete" - "userschema.read" + - "group.read" # If a new projection is introduced it will be prefilled during the setup process (if enabled) # This can prevent serving outdated data after a version upgrade, but might require a longer setup / upgrade process: diff --git a/internal/api/grpc/group/v2/integration_test/group_test.go b/internal/api/grpc/group/v2/integration_test/group_test.go index 713db5aee..5196a6b24 100644 --- a/internal/api/grpc/group/v2/integration_test/group_test.go +++ b/internal/api/grpc/group/v2/integration_test/group_test.go @@ -26,14 +26,24 @@ func TestServer_CreateGroup(t *testing.T) { resp := instance.CreateGroup(iamOwnerCtx, t, orgResp.GetOrganizationId(), alreadyExistingGroupName) tests := []struct { - name string - ctx context.Context - req *group_v2.CreateGroupRequest - wantResp bool - wantGroupID string - wantErrorCode codes.Code - wantErrMsg string + name string + ctx context.Context + req *group_v2.CreateGroupRequest + wantResp bool + wantGroupID string + wantErrCode codes.Code + wantErrMsg string }{ + { + name: "unauthenticated, error", + ctx: context.Background(), + req: &group_v2.CreateGroupRequest{ + Name: integration.GroupName(), + OrganizationId: orgResp.GetOrganizationId(), + }, + wantErrCode: codes.Unauthenticated, + wantErrMsg: "auth header missing", + }, { name: "invalid name, error", ctx: iamOwnerCtx, @@ -41,8 +51,8 @@ func TestServer_CreateGroup(t *testing.T) { Name: " ", OrganizationId: "org1", }, - wantErrorCode: codes.InvalidArgument, - wantErrMsg: "Errors.Group.InvalidName (GROUP-m177lN)", + wantErrCode: codes.InvalidArgument, + wantErrMsg: "Errors.Group.InvalidName (GROUP-m177lN)", }, { name: "missing organization id, error", @@ -50,8 +60,18 @@ func TestServer_CreateGroup(t *testing.T) { req: &group_v2.CreateGroupRequest{ Name: integration.GroupName(), }, - wantErrorCode: codes.InvalidArgument, - wantErrMsg: "invalid CreateGroupRequest.OrganizationId: value length must be between 1 and 200 runes, inclusive", + wantErrCode: codes.InvalidArgument, + wantErrMsg: "invalid CreateGroupRequest.OrganizationId: value length must be between 1 and 200 runes, inclusive", + }, + { + name: "missing permission, error", + ctx: instance.WithAuthorizationToken(CTX, integration.UserTypeNoPermission), + req: &group_v2.CreateGroupRequest{ + Name: integration.GroupName(), + OrganizationId: orgResp.GetOrganizationId(), + }, + wantErrCode: codes.NotFound, + wantErrMsg: "membership not found (AUTHZ-cdgFk)", }, { name: "organization not found, error", @@ -60,32 +80,51 @@ func TestServer_CreateGroup(t *testing.T) { Name: integration.GroupName(), OrganizationId: "org1", }, - wantErrorCode: codes.FailedPrecondition, - wantErrMsg: "Organisation not found (CMDGRP-j1mH8l)", + wantErrCode: codes.FailedPrecondition, + wantErrMsg: "Organisation not found (CMDGRP-j1mH8l)", }, { - name: "already existing group (unique name constraint), error", + name: "instance owner, already existing group (unique name constraint), error", ctx: iamOwnerCtx, req: &group_v2.CreateGroupRequest{ Name: alreadyExistingGroupName, OrganizationId: orgResp.GetOrganizationId(), }, - wantErrorCode: codes.AlreadyExists, - wantErrMsg: "Errors.Group.AlreadyExists (V3-DKcYh)", + wantErrCode: codes.AlreadyExists, + wantErrMsg: "Errors.Group.AlreadyExists (V3-DKcYh)", }, { - name: "already existing group ID, error", + name: "instance owner, already existing group ID, error", ctx: iamOwnerCtx, req: &group_v2.CreateGroupRequest{ Id: gu.Ptr(resp.Id), Name: integration.GroupName(), OrganizationId: orgResp.GetOrganizationId(), }, - wantErrorCode: codes.AlreadyExists, - wantErrMsg: "Errors.Group.AlreadyExists (CMDGRP-shRut3)", + wantErrCode: codes.AlreadyExists, + wantErrMsg: "Errors.Group.AlreadyExists (CMDGRP-shRut3)", }, { - name: "create group with ID, ok", + name: "organization owner, missing permission, error", + ctx: instance.WithAuthorizationToken(CTX, integration.UserTypeOrgOwner), + req: &group_v2.CreateGroupRequest{ + Name: integration.GroupName(), + OrganizationId: orgResp.GetOrganizationId(), + }, + wantErrCode: codes.NotFound, + wantErrMsg: "membership not found (AUTHZ-cdgFk)", + }, + { + name: "organization owner, with permission, ok", + ctx: instance.WithAuthorizationToken(CTX, integration.UserTypeOrgOwner), + req: &group_v2.CreateGroupRequest{ + Name: integration.GroupName(), + OrganizationId: instance.DefaultOrg.GetId(), + }, + wantResp: true, + }, + { + name: "instance owner, create group with ID, ok", ctx: iamOwnerCtx, req: &group_v2.CreateGroupRequest{ Id: gu.Ptr("1234"), @@ -96,7 +135,7 @@ func TestServer_CreateGroup(t *testing.T) { wantGroupID: "1234", }, { - name: "create group without user provided ID, ok", + name: "instance owner, create group without user provided ID, ok", ctx: iamOwnerCtx, req: &group_v2.CreateGroupRequest{ Name: integration.GroupName(), @@ -108,11 +147,11 @@ func TestServer_CreateGroup(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { got, err := instance.Client.GroupV2.CreateGroup(tt.ctx, tt.req) - if tt.wantErrorCode != codes.OK { + if tt.wantErrCode != codes.OK { require.Error(t, err) require.Empty(t, got.GetId()) require.Empty(t, got.GetCreationDate()) - assert.Equal(t, tt.wantErrorCode, status.Code(err)) + assert.Equal(t, tt.wantErrCode, status.Code(err)) assert.Equal(t, tt.wantErrMsg, status.Convert(err).Message()) return } @@ -129,8 +168,12 @@ func TestServer_CreateGroup(t *testing.T) { func TestServer_UpdateGroup(t *testing.T) { iamOwnerCtx := instance.WithAuthorizationToken(CTX, integration.UserTypeIAMOwner) - orgResp := instance.CreateOrganization(iamOwnerCtx, integration.OrganizationName(), integration.Email()) + // create a group in the default org + groupDefOrg := instance.CreateGroup(iamOwnerCtx, t, instance.DefaultOrg.GetId(), integration.GroupName()) + // create a new org + orgResp := instance.CreateOrganization(iamOwnerCtx, integration.OrganizationName(), integration.Email()) + // create a group in the new org groupName := integration.GroupName() existingGroup := instance.CreateGroup(iamOwnerCtx, t, orgResp.GetOrganizationId(), groupName) @@ -142,6 +185,15 @@ func TestServer_UpdateGroup(t *testing.T) { wantErrCode codes.Code wantErrMsg string }{ + { + name: "unauthenticated, error", + ctx: context.Background(), + req: &group_v2.UpdateGroupRequest{ + Name: gu.Ptr(integration.GroupName()), + }, + wantErrCode: codes.Unauthenticated, + wantErrMsg: "auth header missing", + }, { name: "invalid name, error", ctx: iamOwnerCtx, @@ -153,17 +205,46 @@ func TestServer_UpdateGroup(t *testing.T) { wantErrMsg: "Errors.Group.InvalidName (GROUP-dUNd3r)", }, { - name: "group not found, error", + name: "missing permission, error", + ctx: instance.WithAuthorizationToken(CTX, integration.UserTypeNoPermission), + req: &group_v2.UpdateGroupRequest{ + Id: existingGroup.GetId(), + Name: gu.Ptr("updated group name"), + }, + wantErrCode: codes.NotFound, + wantErrMsg: "membership not found (AUTHZ-cdgFk)", + }, + { + name: "organization owner, missing permission, error", + ctx: instance.WithAuthorizationToken(CTX, integration.UserTypeOrgOwner), + req: &group_v2.UpdateGroupRequest{ + Id: existingGroup.GetId(), + Name: gu.Ptr("updated group name"), + }, + wantErrCode: codes.NotFound, + wantErrMsg: "membership not found (AUTHZ-cdgFk)", + }, + { + name: "organization owner, with permission, ok", + ctx: instance.WithAuthorizationToken(CTX, integration.UserTypeOrgOwner), + req: &group_v2.UpdateGroupRequest{ + Id: groupDefOrg.GetId(), + Name: gu.Ptr("updated group name 1"), + }, + wantChangeDate: true, + }, + { + name: "instance owner, group not found, error", ctx: iamOwnerCtx, req: &group_v2.UpdateGroupRequest{ Id: "12345", - Name: gu.Ptr("updated group name"), + Name: gu.Ptr("updated group name 2"), }, wantErrCode: codes.NotFound, wantErrMsg: "Errors.Group.NotFound (CMDGRP-b33zly)", }, { - name: "no change, ok", + name: "instance owner, no change, ok", ctx: iamOwnerCtx, req: &group_v2.UpdateGroupRequest{ Id: existingGroup.GetId(), @@ -172,7 +253,7 @@ func TestServer_UpdateGroup(t *testing.T) { wantChangeDate: true, }, { - name: "change name, ok", + name: "instance owner, change name, ok", ctx: iamOwnerCtx, req: &group_v2.UpdateGroupRequest{ Id: existingGroup.GetId(), @@ -181,7 +262,7 @@ func TestServer_UpdateGroup(t *testing.T) { wantChangeDate: true, }, { - name: "change description, ok", + name: "instance owner, change description, ok", ctx: iamOwnerCtx, req: &group_v2.UpdateGroupRequest{ Id: existingGroup.GetId(), @@ -190,7 +271,7 @@ func TestServer_UpdateGroup(t *testing.T) { wantChangeDate: true, }, { - name: "full change, ok", + name: "instance owner, full change, ok", ctx: iamOwnerCtx, req: &group_v2.UpdateGroupRequest{ Id: existingGroup.GetId(), @@ -218,11 +299,18 @@ func TestServer_UpdateGroup(t *testing.T) { func TestServer_DeleteGroup(t *testing.T) { iamOwnerCtx := instance.WithAuthorizationToken(CTX, integration.UserTypeIAMOwner) + + // create a group in the default org + groupDefOrg := instance.CreateGroup(iamOwnerCtx, t, instance.DefaultOrg.GetId(), integration.GroupName()) + + // create a new org orgResp := instance.CreateOrganization(iamOwnerCtx, integration.OrganizationName(), integration.Email()) + // create a group in the new org groupName := integration.GroupName() existingGroup := instance.CreateGroup(iamOwnerCtx, t, orgResp.GetOrganizationId(), groupName) + // create a group in the new org to be deleted before the test deleteGroupName := integration.GroupName() deleteGroup := instance.CreateGroup(iamOwnerCtx, t, orgResp.GetOrganizationId(), deleteGroupName) deleteResp := instance.DeleteGroup(iamOwnerCtx, t, deleteGroup.GetId()) @@ -235,6 +323,15 @@ func TestServer_DeleteGroup(t *testing.T) { wantErrMsg string deletionTime *timestamp.Timestamp }{ + { + name: "unauthenticated, error", + ctx: context.Background(), + req: &group_v2.DeleteGroupRequest{ + Id: "12345", + }, + wantErrCode: codes.Unauthenticated, + wantErrMsg: "auth header missing", + }, { name: "missing id, error", ctx: iamOwnerCtx, @@ -244,6 +341,31 @@ func TestServer_DeleteGroup(t *testing.T) { wantErrCode: codes.InvalidArgument, wantErrMsg: "invalid DeleteGroupRequest.Id: value length must be between 1 and 200 runes, inclusive", }, + { + name: "missing permission, error", + ctx: instance.WithAuthorizationToken(CTX, integration.UserTypeNoPermission), + req: &group_v2.DeleteGroupRequest{ + Id: existingGroup.GetId(), + }, + wantErrCode: codes.NotFound, + wantErrMsg: "membership not found (AUTHZ-cdgFk)", + }, + { + name: "organization owner, missing permission, error", + ctx: instance.WithAuthorizationToken(CTX, integration.UserTypeOrgOwner), + req: &group_v2.DeleteGroupRequest{ + Id: existingGroup.GetId(), + }, + wantErrCode: codes.NotFound, + wantErrMsg: "membership not found (AUTHZ-cdgFk)", + }, + { + name: "organization owner, with permission, ok", + ctx: instance.WithAuthorizationToken(CTX, integration.UserTypeOrgOwner), + req: &group_v2.DeleteGroupRequest{ + Id: groupDefOrg.GetId(), + }, + }, { name: "group not found, ok", ctx: iamOwnerCtx, diff --git a/internal/api/grpc/group/v2/integration_test/query_test.go b/internal/api/grpc/group/v2/integration_test/query_test.go index 926f80f9c..a31b8c016 100644 --- a/internal/api/grpc/group/v2/integration_test/query_test.go +++ b/internal/api/grpc/group/v2/integration_test/query_test.go @@ -50,7 +50,7 @@ func TestServer_GetGroup(t *testing.T) { wantErrMsg: "invalid GetGroupRequest.Id: value length must be between 1 and 200 runes, inclusive", }, { - name: "get group, not found", + name: "get group, instance owner, not found", args: args{ ctx: iamOwnerCtx, req: &group_v2.GetGroupRequest{ @@ -61,7 +61,62 @@ func TestServer_GetGroup(t *testing.T) { wantErrMsg: "Errors.Group.NotFound (QUERY-SG4WbR)", }, { - name: "get group, found", + name: "get group, missing permission, error", + args: args{ + ctx: instance.WithAuthorizationToken(CTX, integration.UserTypeNoPermission), + dep: func(req *group_v2.GetGroupRequest, resp *group_v2.GetGroupResponse) { + groupName := integration.GroupName() + group := instance.CreateGroup(iamOwnerCtx, t, instance.DefaultOrg.GetId(), groupName) + + req.Id = group.GetId() + }, + req: &group_v2.GetGroupRequest{}, + }, + wantErrCode: codes.NotFound, + wantErrMsg: "membership not found (AUTHZ-cdgFk)", + }, + { + name: "get group, organization owner, missing permission, error", + args: args{ + ctx: instance.WithAuthorizationToken(CTX, integration.UserTypeOrgOwner), + dep: func(req *group_v2.GetGroupRequest, resp *group_v2.GetGroupResponse) { + orgResp := instance.CreateOrganization(iamOwnerCtx, integration.OrganizationName(), integration.Email()) + groupName := integration.GroupName() + group := instance.CreateGroup(iamOwnerCtx, t, orgResp.GetOrganizationId(), groupName) + + req.Id = group.GetId() + }, + req: &group_v2.GetGroupRequest{}, + }, + wantErrCode: codes.NotFound, + wantErrMsg: "membership not found (AUTHZ-cdgFk)", + }, + { + name: "get group, organization owner, with permission, found", + args: args{ + ctx: instance.WithAuthorizationToken(CTX, integration.UserTypeOrgOwner), + dep: func(req *group_v2.GetGroupRequest, resp *group_v2.GetGroupResponse) { + groupName := integration.GroupName() + group := instance.CreateGroup(iamOwnerCtx, t, instance.DefaultOrg.GetId(), groupName) + + req.Id = group.GetId() + resp.Group = &group_v2.Group{ + Id: group.GetId(), + Name: groupName, + Description: "", + OrganizationId: instance.DefaultOrg.GetId(), + ChangeDate: group.GetCreationDate(), + CreationDate: group.GetCreationDate(), + } + }, + req: &group_v2.GetGroupRequest{}, + }, + want: &group_v2.GetGroupResponse{ + Group: &group_v2.Group{}, + }, + }, + { + name: "get group, instance owner, found", args: args{ ctx: iamOwnerCtx, dep: func(req *group_v2.GetGroupRequest, resp *group_v2.GetGroupResponse) { @@ -132,7 +187,97 @@ func TestServer_ListGroups(t *testing.T) { wantErrMsg: "auth header missing", }, { - name: "group ID not found", + name: "no permission, empty list, TotalResult count returned", + args: args{ + ctx: instance.WithAuthorizationToken(CTX, integration.UserTypeNoPermission), + dep: func(req *group_v2.ListGroupsRequest, resp *group_v2.ListGroupsResponse) { + orgResp := instance.CreateOrganization(iamOwnerCtx, integration.OrganizationName(), integration.Email()) + groupName := integration.GroupName() + group1 := instance.CreateGroup(iamOwnerCtx, t, orgResp.GetOrganizationId(), groupName) + + req.Filters[0].Filter = &group_v2.GroupsSearchFilter_GroupIds{ + GroupIds: &filter.InIDsFilter{ + Ids: []string{group1.GetId()}, + }, + } + }, + req: &group_v2.ListGroupsRequest{ + Filters: []*group_v2.GroupsSearchFilter{{}}, + }, + }, + want: &group_v2.ListGroupsResponse{ + Pagination: &filter.PaginationResponse{ + TotalResult: 1, + AppliedLimit: 100, + }, + Groups: []*group_v2.Group{}, + }, + }, + { + name: "org owner, missing permission, empty list, TotalResult count returned", + args: args{ + ctx: instance.WithAuthorizationToken(CTX, integration.UserTypeOrgOwner), + dep: func(req *group_v2.ListGroupsRequest, resp *group_v2.ListGroupsResponse) { + orgResp := instance.CreateOrganization(iamOwnerCtx, integration.OrganizationName(), integration.Email()) + groupName := integration.GroupName() + group1 := instance.CreateGroup(iamOwnerCtx, t, orgResp.GetOrganizationId(), groupName) + + req.Filters[0].Filter = &group_v2.GroupsSearchFilter_GroupIds{ + GroupIds: &filter.InIDsFilter{ + Ids: []string{group1.GetId()}, + }, + } + }, + req: &group_v2.ListGroupsRequest{ + Filters: []*group_v2.GroupsSearchFilter{{}}, + }, + }, + want: &group_v2.ListGroupsResponse{ + Pagination: &filter.PaginationResponse{ + TotalResult: 1, + AppliedLimit: 100, + }, + Groups: []*group_v2.Group{}, + }, + }, + { + name: "org owner, with permission, ok", + args: args{ + ctx: instance.WithAuthorizationToken(CTX, integration.UserTypeOrgOwner), + dep: func(req *group_v2.ListGroupsRequest, resp *group_v2.ListGroupsResponse) { + groupName := integration.GroupName() + group1 := instance.CreateGroup(iamOwnerCtx, t, instance.DefaultOrg.GetId(), groupName) + + req.Filters[0].Filter = &group_v2.GroupsSearchFilter_GroupIds{ + GroupIds: &filter.InIDsFilter{ + Ids: []string{group1.GetId()}, + }, + } + resp.Groups[0] = &group_v2.Group{ + Id: group1.GetId(), + Name: groupName, + Description: "", + OrganizationId: instance.DefaultOrg.GetId(), + CreationDate: group1.GetCreationDate(), + ChangeDate: group1.GetCreationDate(), + } + }, + req: &group_v2.ListGroupsRequest{ + Filters: []*group_v2.GroupsSearchFilter{{}}, + }, + }, + want: &group_v2.ListGroupsResponse{ + Pagination: &filter.PaginationResponse{ + TotalResult: 1, + AppliedLimit: 100, + }, + Groups: []*group_v2.Group{ + {}, + }, + }, + }, + { + name: "instance owner, group ID not found", args: args{ ctx: iamOwnerCtx, req: &group_v2.ListGroupsRequest{ @@ -367,3 +512,354 @@ func TestServer_ListGroups(t *testing.T) { }) } } + +func TestServer_ListGroups_WithPermissionV2(t *testing.T) { + ensureFeaturePermissionV2Enabled(t, instancePermissionV2) + iamOwnerCtx := instancePermissionV2.WithAuthorizationToken(CTX, integration.UserTypeIAMOwner) + type args struct { + ctx context.Context + req *group_v2.ListGroupsRequest + dep func(*group_v2.ListGroupsRequest, *group_v2.ListGroupsResponse) + } + tests := []struct { + name string + args args + want *group_v2.ListGroupsResponse + wantErrCode codes.Code + wantErrMsg string + }{ + { + name: "list groups, unauthenticated", + args: args{ + ctx: CTX, + req: &group_v2.ListGroupsRequest{}, + }, + wantErrCode: codes.Unauthenticated, + wantErrMsg: "auth header missing", + }, + { + name: "no permission, empty list, TotalResult count set to 0", + args: args{ + ctx: instancePermissionV2.WithAuthorizationToken(CTX, integration.UserTypeNoPermission), + dep: func(req *group_v2.ListGroupsRequest, resp *group_v2.ListGroupsResponse) { + orgResp := instancePermissionV2.CreateOrganization(iamOwnerCtx, integration.OrganizationName(), integration.Email()) + groupName := integration.GroupName() + group1 := instancePermissionV2.CreateGroup(iamOwnerCtx, t, orgResp.GetOrganizationId(), groupName) + + req.Filters[0].Filter = &group_v2.GroupsSearchFilter_GroupIds{ + GroupIds: &filter.InIDsFilter{ + Ids: []string{group1.GetId()}, + }, + } + }, + req: &group_v2.ListGroupsRequest{ + Filters: []*group_v2.GroupsSearchFilter{{}}, + }, + }, + want: &group_v2.ListGroupsResponse{ + Pagination: &filter.PaginationResponse{ + TotalResult: 0, + AppliedLimit: 100, + }, + Groups: []*group_v2.Group{}, + }, + }, + { + name: "org owner, missing permission, empty list, TotalResult count set to 0", + args: args{ + ctx: instancePermissionV2.WithAuthorizationToken(CTX, integration.UserTypeOrgOwner), + dep: func(req *group_v2.ListGroupsRequest, resp *group_v2.ListGroupsResponse) { + orgResp := instancePermissionV2.CreateOrganization(iamOwnerCtx, integration.OrganizationName(), integration.Email()) + groupName := integration.GroupName() + group1 := instancePermissionV2.CreateGroup(iamOwnerCtx, t, orgResp.GetOrganizationId(), groupName) + + req.Filters[0].Filter = &group_v2.GroupsSearchFilter_GroupIds{ + GroupIds: &filter.InIDsFilter{ + Ids: []string{group1.GetId()}, + }, + } + }, + req: &group_v2.ListGroupsRequest{ + Filters: []*group_v2.GroupsSearchFilter{{}}, + }, + }, + want: &group_v2.ListGroupsResponse{ + Pagination: &filter.PaginationResponse{ + TotalResult: 0, + AppliedLimit: 100, + }, + Groups: []*group_v2.Group{}, + }, + }, + { + name: "org owner, with permission, ok", + args: args{ + ctx: instancePermissionV2.WithAuthorizationToken(CTX, integration.UserTypeOrgOwner), + dep: func(req *group_v2.ListGroupsRequest, resp *group_v2.ListGroupsResponse) { + groupName := integration.GroupName() + group1 := instancePermissionV2.CreateGroup(iamOwnerCtx, t, instancePermissionV2.DefaultOrg.GetId(), groupName) + + req.Filters[0].Filter = &group_v2.GroupsSearchFilter_GroupIds{ + GroupIds: &filter.InIDsFilter{ + Ids: []string{group1.GetId()}, + }, + } + resp.Groups[0] = &group_v2.Group{ + Id: group1.GetId(), + Name: groupName, + Description: "", + OrganizationId: instancePermissionV2.DefaultOrg.GetId(), + CreationDate: group1.GetCreationDate(), + ChangeDate: group1.GetCreationDate(), + } + }, + req: &group_v2.ListGroupsRequest{ + Filters: []*group_v2.GroupsSearchFilter{{}}, + }, + }, + want: &group_v2.ListGroupsResponse{ + Pagination: &filter.PaginationResponse{ + TotalResult: 1, + AppliedLimit: 100, + }, + Groups: []*group_v2.Group{ + {}, + }, + }, + }, + { + name: "instance owner, group ID not found", + args: args{ + ctx: iamOwnerCtx, + req: &group_v2.ListGroupsRequest{ + Filters: []*group_v2.GroupsSearchFilter{ + { + Filter: &group_v2.GroupsSearchFilter_GroupIds{ + GroupIds: &filter.InIDsFilter{ + Ids: []string{"random-group"}, + }, + }, + }, + }, + }, + }, + want: &group_v2.ListGroupsResponse{ + Pagination: &filter.PaginationResponse{ + TotalResult: 0, + AppliedLimit: 100, + }, + }, + }, + { + name: "list single group by ID, ok", + args: args{ + ctx: iamOwnerCtx, + dep: func(req *group_v2.ListGroupsRequest, resp *group_v2.ListGroupsResponse) { + orgResp := instancePermissionV2.CreateOrganization(iamOwnerCtx, integration.OrganizationName(), integration.Email()) + groupName := integration.GroupName() + group1 := instancePermissionV2.CreateGroup(iamOwnerCtx, t, orgResp.GetOrganizationId(), groupName) + + resp.Groups[0] = &group_v2.Group{ + Id: group1.GetId(), + Name: groupName, + Description: "", + OrganizationId: orgResp.GetOrganizationId(), + CreationDate: group1.GetCreationDate(), + ChangeDate: group1.GetCreationDate(), + } + req.Filters[0].Filter = &group_v2.GroupsSearchFilter_GroupIds{ + GroupIds: &filter.InIDsFilter{ + Ids: []string{group1.GetId()}, + }, + } + }, + req: &group_v2.ListGroupsRequest{ + Filters: []*group_v2.GroupsSearchFilter{{}}, + }, + }, + want: &group_v2.ListGroupsResponse{ + Pagination: &filter.PaginationResponse{ + TotalResult: 1, + AppliedLimit: 100, + }, + Groups: []*group_v2.Group{ + {}, + }, + }, + }, + { + name: "list multiple groups by IDs, ok", + args: args{ + ctx: iamOwnerCtx, + dep: func(req *group_v2.ListGroupsRequest, resp *group_v2.ListGroupsResponse) { + orgResp := instancePermissionV2.CreateOrganization(iamOwnerCtx, integration.OrganizationName(), integration.Email()) + groupName1 := integration.GroupName() + group1 := instancePermissionV2.CreateGroup(iamOwnerCtx, t, orgResp.GetOrganizationId(), groupName1) + + resp.Groups[1] = &group_v2.Group{ + Id: group1.GetId(), + Name: groupName1, + Description: "", + OrganizationId: orgResp.GetOrganizationId(), + CreationDate: group1.GetCreationDate(), + ChangeDate: group1.GetCreationDate(), + } + groupName2 := integration.GroupName() + group2 := instancePermissionV2.CreateGroup(iamOwnerCtx, t, orgResp.GetOrganizationId(), groupName2) + + resp.Groups[0] = &group_v2.Group{ + Id: group2.GetId(), + Name: groupName2, + Description: "", + OrganizationId: orgResp.GetOrganizationId(), + CreationDate: group2.GetCreationDate(), + ChangeDate: group2.GetCreationDate(), + } + req.Filters[0].Filter = &group_v2.GroupsSearchFilter_GroupIds{ + GroupIds: &filter.InIDsFilter{ + Ids: []string{group1.GetId(), group2.GetId()}, + }, + } + }, + req: &group_v2.ListGroupsRequest{ + Filters: []*group_v2.GroupsSearchFilter{{}}, + }, + }, + want: &group_v2.ListGroupsResponse{ + Pagination: &filter.PaginationResponse{ + TotalResult: 2, + AppliedLimit: 100, + }, + Groups: []*group_v2.Group{ + {}, {}, + }, + }, + }, + { + name: "list group by name, ok", + args: args{ + ctx: iamOwnerCtx, + dep: func(req *group_v2.ListGroupsRequest, resp *group_v2.ListGroupsResponse) { + orgResp := instancePermissionV2.CreateOrganization(iamOwnerCtx, integration.OrganizationName(), integration.Email()) + groupName := integration.GroupName() + group1 := instancePermissionV2.CreateGroup(iamOwnerCtx, t, orgResp.GetOrganizationId(), groupName) + + resp.Groups[0] = &group_v2.Group{ + Id: group1.GetId(), + Name: groupName, + Description: "", + OrganizationId: orgResp.GetOrganizationId(), + CreationDate: group1.GetCreationDate(), + ChangeDate: group1.GetCreationDate(), + } + req.Filters[0].Filter = &group_v2.GroupsSearchFilter_NameFilter{ + NameFilter: &group_v2.GroupNameFilter{ + Name: groupName, + }, + } + }, + req: &group_v2.ListGroupsRequest{ + Filters: []*group_v2.GroupsSearchFilter{{}}, + }, + }, + want: &group_v2.ListGroupsResponse{ + Pagination: &filter.PaginationResponse{ + TotalResult: 1, + AppliedLimit: 100, + }, + Groups: []*group_v2.Group{ + {}, + }, + }, + }, + { + name: "list by organization ID, ok", + args: args{ + ctx: iamOwnerCtx, + dep: func(req *group_v2.ListGroupsRequest, resp *group_v2.ListGroupsResponse) { + org1 := instancePermissionV2.CreateOrganization(iamOwnerCtx, integration.OrganizationName(), integration.Email()) + groupName2 := integration.GroupName() + group2 := instancePermissionV2.CreateGroup(iamOwnerCtx, t, org1.GetOrganizationId(), groupName2) + + resp.Groups[2] = &group_v2.Group{ + Id: group2.GetId(), + Name: groupName2, + Description: "", + OrganizationId: org1.GetOrganizationId(), + CreationDate: group2.GetCreationDate(), + ChangeDate: group2.GetCreationDate(), + } + groupName1 := integration.GroupName() + group1 := instancePermissionV2.CreateGroup(iamOwnerCtx, t, org1.GetOrganizationId(), groupName1) + + resp.Groups[1] = &group_v2.Group{ + Id: group1.GetId(), + Name: groupName1, + Description: "", + OrganizationId: org1.GetOrganizationId(), + CreationDate: group1.GetCreationDate(), + ChangeDate: group1.GetCreationDate(), + } + groupName0 := integration.GroupName() + group0 := instancePermissionV2.CreateGroup(iamOwnerCtx, t, org1.GetOrganizationId(), groupName0) + + resp.Groups[0] = &group_v2.Group{ + Id: group0.GetId(), + Name: groupName0, + Description: "", + OrganizationId: org1.GetOrganizationId(), + CreationDate: group0.GetCreationDate(), + ChangeDate: group0.GetCreationDate(), + } + org2 := instancePermissionV2.CreateOrganization(iamOwnerCtx, integration.OrganizationName(), integration.Email()) + org2GroupName0 := integration.GroupName() + _ = instancePermissionV2.CreateGroup(iamOwnerCtx, t, org2.GetOrganizationId(), org2GroupName0) + + req.Filters[0].Filter = &group_v2.GroupsSearchFilter_OrganizationId{ + OrganizationId: &filter.IDFilter{ + Id: org1.GetOrganizationId(), + }, + } + }, + req: &group_v2.ListGroupsRequest{ + Filters: []*group_v2.GroupsSearchFilter{{}}, + }, + }, + want: &group_v2.ListGroupsResponse{ + Pagination: &filter.PaginationResponse{ + TotalResult: 3, + AppliedLimit: 100, + }, + Groups: []*group_v2.Group{ + {}, {}, {}, + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.args.dep != nil { + tt.args.dep(tt.args.req, tt.want) + } + + retryDuration, tick := integration.WaitForAndTickWithMaxDuration(iamOwnerCtx, time.Minute) + require.EventuallyWithT(t, func(ttt *assert.CollectT) { + got, err := instancePermissionV2.Client.GroupV2.ListGroups(tt.args.ctx, tt.args.req) + if tt.wantErrCode != codes.OK { + require.Error(ttt, err) + assert.Equal(ttt, tt.wantErrCode, status.Code(err)) + assert.Equal(ttt, tt.wantErrMsg, status.Convert(err).Message()) + return + } + require.NoError(ttt, err) + if assert.Len(ttt, got.Groups, len(tt.want.Groups)) { + for i := range got.Groups { + assert.EqualExportedValues(ttt, tt.want.Groups[i], got.Groups[i], "want: %v, got: %v", tt.want.Groups[i], got.Groups[i]) + } + } + assert.Equal(ttt, tt.want.Pagination.AppliedLimit, got.Pagination.AppliedLimit) + assert.Equal(ttt, tt.want.Pagination.TotalResult, got.Pagination.TotalResult) + }, retryDuration, tick, "timeout waiting for expected result") + }) + } +} diff --git a/internal/api/grpc/group/v2/integration_test/server_test.go b/internal/api/grpc/group/v2/integration_test/server_test.go index bef6d603c..12b6b6730 100644 --- a/internal/api/grpc/group/v2/integration_test/server_test.go +++ b/internal/api/grpc/group/v2/integration_test/server_test.go @@ -8,12 +8,18 @@ import ( "testing" "time" + "github.com/muhlemmer/gu" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/zitadel/zitadel/internal/integration" + "github.com/zitadel/zitadel/pkg/grpc/feature/v2" ) var ( - CTX context.Context - instance *integration.Instance + CTX context.Context + instance *integration.Instance + instancePermissionV2 *integration.Instance ) func TestMain(m *testing.M) { @@ -22,6 +28,31 @@ func TestMain(m *testing.M) { defer cancel() CTX = ctx instance = integration.NewInstance(ctx) + instancePermissionV2 = integration.NewInstance(ctx) return m.Run() }()) } + +func ensureFeaturePermissionV2Enabled(t *testing.T, testInstance *integration.Instance) { + ctx := testInstance.WithAuthorizationToken(context.Background(), integration.UserTypeIAMOwner) + f, err := testInstance.Client.FeatureV2.GetInstanceFeatures(ctx, &feature.GetInstanceFeaturesRequest{ + Inheritance: true, + }) + require.NoError(t, err) + + if f.PermissionCheckV2.GetEnabled() { + return + } + + _, err = testInstance.Client.FeatureV2.SetInstanceFeatures(ctx, &feature.SetInstanceFeaturesRequest{ + PermissionCheckV2: gu.Ptr(true), + }) + require.NoError(t, err) + + retryDuration, tick := integration.WaitForAndTickWithMaxDuration(ctx, 5*time.Minute) + require.EventuallyWithT(t, func(tt *assert.CollectT) { + f, err := testInstance.Client.FeatureV2.GetInstanceFeatures(ctx, &feature.GetInstanceFeaturesRequest{Inheritance: true}) + require.NoError(tt, err) + assert.True(tt, f.PermissionCheckV2.GetEnabled()) + }, retryDuration, tick, "timed out waiting for ensuring testInstance feature") +} diff --git a/internal/api/grpc/group/v2/query.go b/internal/api/grpc/group/v2/query.go index b4cbec315..b5046451f 100644 --- a/internal/api/grpc/group/v2/query.go +++ b/internal/api/grpc/group/v2/query.go @@ -15,7 +15,7 @@ import ( // GetGroup returns a group that matches the group ID in the request func (s *Server) GetGroup(ctx context.Context, req *connect.Request[group_v2.GetGroupRequest]) (*connect.Response[group_v2.GetGroupResponse], error) { - group, err := s.query.GetGroupByID(ctx, req.Msg.GetId()) + group, err := s.query.GetGroupByID(ctx, req.Msg.GetId(), s.checkPermission) if err != nil { return nil, err } @@ -30,7 +30,7 @@ func (s *Server) ListGroups(ctx context.Context, req *connect.Request[group_v2.L if err != nil { return nil, err } - resp, err := s.query.SearchGroups(ctx, queries) + resp, err := s.query.SearchGroups(ctx, queries, s.checkPermission) if err != nil { return nil, err } diff --git a/internal/command/group.go b/internal/command/group.go index 406bf5e6b..cf1593d8d 100644 --- a/internal/command/group.go +++ b/internal/command/group.go @@ -30,7 +30,13 @@ func (c *Commands) CreateGroup(ctx context.Context, group *CreateGroup) (details ctx, span := tracing.NewSpan(ctx) defer func() { span.EndWithError(err) }() - // todo: check permissions + // create a unique group ID if not provided + if group.AggregateID == "" { + group.AggregateID, err = c.idGenerator.Next() + if err != nil { + return nil, err + } + } if err = group.IsValid(); err != nil { return nil, err @@ -38,20 +44,17 @@ func (c *Commands) CreateGroup(ctx context.Context, group *CreateGroup) (details if group.ResourceOwner == "" { return nil, zerrors.ThrowInvalidArgument(nil, "CMDGRP-msc0Tt", "Errors.Group.MissingOrganizationID") } + + if err = c.checkPermissionCreateGroup(ctx, group.ResourceOwner, group.AggregateID); err != nil { + return nil, err + } + // check whether the organization where the group should be created exists err = c.checkOrgExists(ctx, group.ResourceOwner) if err != nil { return nil, zerrors.ThrowPreconditionFailed(nil, "CMDGRP-j1mH8l", "Errors.Org.NotFound") } - // create a unique group ID if not provided - if group.AggregateID == "" { - group.AggregateID, err = c.idGenerator.Next() - if err != nil { - return nil, err - } - } - // check if a group with the same ID already exists groupWriteModel, err := c.getGroupWriteModelByID(ctx, group.AggregateID, group.ResourceOwner) if err != nil { @@ -97,7 +100,6 @@ func (c *Commands) UpdateGroup(ctx context.Context, groupUpdate *UpdateGroup) (d return nil, err } - // todo: check permissions existingGroup, err := c.getGroupWriteModelByID(ctx, groupUpdate.AggregateID, groupUpdate.ResourceOwner) if err != nil { return nil, err @@ -106,6 +108,10 @@ func (c *Commands) UpdateGroup(ctx context.Context, groupUpdate *UpdateGroup) (d return nil, zerrors.ThrowNotFound(nil, "CMDGRP-b33zly", "Errors.Group.NotFound") } + if err = c.checkPermissionUpdateGroup(ctx, existingGroup.ResourceOwner, existingGroup.AggregateID); err != nil { + return nil, err + } + changedEvent := existingGroup.NewChangedEvent( ctx, GroupAggregateFromWriteModel(ctx, &existingGroup.WriteModel), @@ -138,6 +144,10 @@ func (c *Commands) DeleteGroup(ctx context.Context, groupID string) (details *do return writeModelToObjectDetails(&existingGroup.WriteModel), nil } + if err = c.checkPermissionDeleteGroup(ctx, existingGroup.ResourceOwner, existingGroup.AggregateID); err != nil { + return nil, err + } + err = c.pushAppendAndReduce(ctx, existingGroup, repo.NewGroupRemovedEvent(ctx, diff --git a/internal/command/group_test.go b/internal/command/group_test.go index 9b1758465..ec569fe1e 100644 --- a/internal/command/group_test.go +++ b/internal/command/group_test.go @@ -26,8 +26,9 @@ func TestCommands_CreateGroup(t *testing.T) { idGeneratorErr := errors.New("id generator error") type fields struct { - eventstore func(t *testing.T) *eventstore.Eventstore - idGenerator id.Generator + eventstore func(t *testing.T) *eventstore.Eventstore + idGenerator id.Generator + checkPermission domain.PermissionCheck } type args struct { ctx context.Context @@ -51,6 +52,7 @@ func TestCommands_CreateGroup(t *testing.T) { group: &CreateGroup{ ObjectRoot: models.ObjectRoot{ ResourceOwner: "org1", + AggregateID: "1234", }, Name: " ", Description: "example group", @@ -66,6 +68,9 @@ func TestCommands_CreateGroup(t *testing.T) { args: args{ ctx: context.Background(), group: &CreateGroup{ + ObjectRoot: models.ObjectRoot{ + AggregateID: "1234", + }, Name: "example", Description: "example group", }, @@ -73,17 +78,38 @@ func TestCommands_CreateGroup(t *testing.T) { wantErr: zerrors.IsErrorInvalidArgument, }, { - name: "org not found, precondition error", + name: "missing permissions, error", fields: fields{ - eventstore: expectEventstore( - expectFilter(), - ), + eventstore: expectEventstore(), + checkPermission: newMockPermissionCheckNotAllowed(), }, args: args{ ctx: context.Background(), group: &CreateGroup{ ObjectRoot: models.ObjectRoot{ ResourceOwner: "org1", + AggregateID: "1234", + }, + Name: "example", + Description: "example group", + }, + }, + wantErr: zerrors.IsPermissionDenied, + }, + { + name: "org not found, precondition error", + fields: fields{ + eventstore: expectEventstore( + expectFilter(), + ), + checkPermission: newMockPermissionCheckAllowed(), + }, + args: args{ + ctx: context.Background(), + group: &CreateGroup{ + ObjectRoot: models.ObjectRoot{ + ResourceOwner: "org1", + AggregateID: "1234", }, Name: "example", Description: "example group", @@ -94,16 +120,8 @@ func TestCommands_CreateGroup(t *testing.T) { { name: "failed to generate group id, error", fields: fields{ - eventstore: expectEventstore( - expectFilter( - eventFromEventPusher( - org.NewOrgAddedEvent(context.Background(), - &org.NewAggregate("org1").Aggregate, - "org1", - ), - ), - ), - ), + eventstore: expectEventstore(), + checkPermission: newMockPermissionCheckAllowed(), }, args: args{ ctx: context.Background(), @@ -144,6 +162,7 @@ func TestCommands_CreateGroup(t *testing.T) { ), ), ), + checkPermission: newMockPermissionCheckAllowed(), }, args: args{ ctx: context.Background(), @@ -164,12 +183,14 @@ func TestCommands_CreateGroup(t *testing.T) { eventstore: expectEventstore( expectFilterError(filterErr), ), + checkPermission: newMockPermissionCheckAllowed(), }, args: args{ ctx: context.Background(), group: &CreateGroup{ ObjectRoot: models.ObjectRoot{ ResourceOwner: "org1", + AggregateID: "1234", }, Name: "example", Description: "example group", @@ -193,6 +214,7 @@ func TestCommands_CreateGroup(t *testing.T) { ), expectFilterError(filterErr), ), + checkPermission: newMockPermissionCheckAllowed(), }, args: args{ ctx: context.Background(), @@ -233,6 +255,7 @@ func TestCommands_CreateGroup(t *testing.T) { ), ), ), + checkPermission: newMockPermissionCheckAllowed(), }, args: args{ ctx: context.Background(), @@ -272,6 +295,7 @@ func TestCommands_CreateGroup(t *testing.T) { ), ), ), + checkPermission: newMockPermissionCheckAllowed(), }, args: args{ ctx: context.Background(), @@ -312,6 +336,7 @@ func TestCommands_CreateGroup(t *testing.T) { ), ), ), + checkPermission: newMockPermissionCheckAllowed(), }, args: args{ ctx: context.Background(), @@ -347,8 +372,9 @@ func TestCommands_CreateGroup(t *testing.T) { } c := &Commands{ - eventstore: tt.fields.eventstore(t), - idGenerator: tt.fields.idGenerator, + eventstore: tt.fields.eventstore(t), + idGenerator: tt.fields.idGenerator, + checkPermission: tt.fields.checkPermission, } got, err := c.CreateGroup(tt.args.ctx, tt.args.group) @@ -370,7 +396,8 @@ func TestCommands_UpdateGroup(t *testing.T) { pushErr := errors.New("push error") type fields struct { - eventstore func(t *testing.T) *eventstore.Eventstore + eventstore func(t *testing.T) *eventstore.Eventstore + checkPermission domain.PermissionCheck } type args struct { ctx context.Context @@ -434,6 +461,33 @@ func TestCommands_UpdateGroup(t *testing.T) { return errors.Is(err, filterErr) }, }, + { + name: "missing permission, error", + fields: fields{ + eventstore: expectEventstore( + expectFilter( + eventFromEventPusher( + group.NewGroupAddedEvent(context.Background(), + &group.NewAggregate("1234", "org1").Aggregate, + "group1", + "group1 description", + ), + ), + )), + checkPermission: newMockPermissionCheckNotAllowed(), + }, + args: args{ + ctx: context.Background(), + group: &UpdateGroup{ + ObjectRoot: models.ObjectRoot{ + AggregateID: "1234", + }, + Name: gu.Ptr("updated name"), + Description: gu.Ptr("updated description"), + }, + }, + wantErr: zerrors.IsPermissionDenied, + }, { name: "failed to push group changed event, error", fields: fields{ @@ -459,6 +513,7 @@ func TestCommands_UpdateGroup(t *testing.T) { ), ), ), + checkPermission: newMockPermissionCheckAllowed(), }, args: args{ ctx: context.Background(), @@ -485,6 +540,7 @@ func TestCommands_UpdateGroup(t *testing.T) { ), ), ), + checkPermission: newMockPermissionCheckAllowed(), }, args: args{ ctx: context.Background(), @@ -524,6 +580,7 @@ func TestCommands_UpdateGroup(t *testing.T) { ), ), ), + checkPermission: newMockPermissionCheckAllowed(), }, args: args{ ctx: context.Background(), @@ -562,6 +619,7 @@ func TestCommands_UpdateGroup(t *testing.T) { ), ), ), + checkPermission: newMockPermissionCheckAllowed(), }, args: args{ ctx: context.Background(), @@ -601,6 +659,7 @@ func TestCommands_UpdateGroup(t *testing.T) { ), ), ), + checkPermission: newMockPermissionCheckAllowed(), }, args: args{ ctx: context.Background(), @@ -623,7 +682,8 @@ func TestCommands_UpdateGroup(t *testing.T) { t.Parallel() c := &Commands{ - eventstore: tt.fields.eventstore(t), + eventstore: tt.fields.eventstore(t), + checkPermission: tt.fields.checkPermission, } got, err := c.UpdateGroup(tt.args.ctx, tt.args.group) if tt.wantErr == nil { @@ -644,7 +704,8 @@ func TestCommands_DeleteGroup(t *testing.T) { pushErr := errors.New("push error") type fields struct { - eventstore func(t *testing.T) *eventstore.Eventstore + eventstore func(t *testing.T) *eventstore.Eventstore + checkPermission domain.PermissionCheck } type args struct { ctx context.Context @@ -687,6 +748,28 @@ func TestCommands_DeleteGroup(t *testing.T) { ID: "1234", }, }, + { + name: "missing permission, error", + fields: fields{ + eventstore: expectEventstore( + expectFilter( + eventFromEventPusher( + group.NewGroupAddedEvent(context.Background(), + &group.NewAggregate("1234", "org1").Aggregate, + "group1", + "group1 description", + ), + ), + ), + ), + checkPermission: newMockPermissionCheckNotAllowed(), + }, + args: args{ + ctx: context.Background(), + groupID: "1234", + }, + wantErr: zerrors.IsPermissionDenied, + }, { name: "failed to push group delete event, error", fields: fields{ @@ -694,7 +777,7 @@ func TestCommands_DeleteGroup(t *testing.T) { expectFilter( eventFromEventPusher( group.NewGroupAddedEvent(context.Background(), - &group.NewAggregate("1234", "").Aggregate, + &group.NewAggregate("1234", "org1").Aggregate, "group1", "group1 description", ), @@ -703,11 +786,12 @@ func TestCommands_DeleteGroup(t *testing.T) { expectPushFailed( pushErr, group.NewGroupRemovedEvent(context.Background(), - &group.NewAggregate("1234", "").Aggregate, + &group.NewAggregate("1234", "org1").Aggregate, "group1", ), ), ), + checkPermission: newMockPermissionCheckAllowed(), }, args: args{ ctx: context.Background(), @@ -724,7 +808,7 @@ func TestCommands_DeleteGroup(t *testing.T) { expectFilter( eventFromEventPusher( group.NewGroupAddedEvent(context.Background(), - &group.NewAggregate("1234", "").Aggregate, + &group.NewAggregate("1234", "org1").Aggregate, "group1", "group1 description", ), @@ -733,19 +817,21 @@ func TestCommands_DeleteGroup(t *testing.T) { expectPush( eventFromEventPusher( group.NewGroupRemovedEvent(context.Background(), - &group.NewAggregate("1234", "").Aggregate, + &group.NewAggregate("1234", "org1").Aggregate, "group1", ), ), ), ), + checkPermission: newMockPermissionCheckAllowed(), }, args: args{ ctx: context.Background(), groupID: "1234", }, want: &domain.ObjectDetails{ - ID: "1234", + ID: "1234", + ResourceOwner: "org1", }, }, } @@ -755,7 +841,8 @@ func TestCommands_DeleteGroup(t *testing.T) { t.Parallel() c := &Commands{ - eventstore: tt.fields.eventstore(t), + eventstore: tt.fields.eventstore(t), + checkPermission: tt.fields.checkPermission, } got, err := c.DeleteGroup(tt.args.ctx, tt.args.groupID) if tt.wantErr == nil { diff --git a/internal/command/permission_checks.go b/internal/command/permission_checks.go index 65ab3fa01..75ffdcf16 100644 --- a/internal/command/permission_checks.go +++ b/internal/command/permission_checks.go @@ -6,6 +6,7 @@ import ( "github.com/zitadel/zitadel/internal/api/authz" "github.com/zitadel/zitadel/internal/domain" "github.com/zitadel/zitadel/internal/eventstore" + "github.com/zitadel/zitadel/internal/repository/group" "github.com/zitadel/zitadel/internal/repository/instance" "github.com/zitadel/zitadel/internal/repository/org" "github.com/zitadel/zitadel/internal/repository/project" @@ -159,3 +160,15 @@ func (c *Commands) NewPermissionCheckUserGrantWrite(ctx context.Context) UserGra func (c *Commands) NewPermissionCheckUserGrantDelete(ctx context.Context) UserGrantPermissionCheck { return c.newUserGrantPermissionCheck(ctx, domain.PermissionUserGrantDelete) } + +func (c *Commands) checkPermissionCreateGroup(ctx context.Context, resourceOwner, groupID string) error { + return c.newPermissionCheck(ctx, domain.PermissionGroupCreate, group.AggregateType)(resourceOwner, groupID) +} + +func (c *Commands) checkPermissionUpdateGroup(ctx context.Context, resourceOwner, groupID string) error { + return c.newPermissionCheck(ctx, domain.PermissionGroupWrite, group.AggregateType)(resourceOwner, groupID) +} + +func (c *Commands) checkPermissionDeleteGroup(ctx context.Context, resourceOwner, groupID string) error { + return c.newPermissionCheck(ctx, domain.PermissionGroupDelete, group.AggregateType)(resourceOwner, groupID) +} diff --git a/internal/domain/group.go b/internal/domain/group.go index a23e2b58d..90ce6477a 100644 --- a/internal/domain/group.go +++ b/internal/domain/group.go @@ -1,32 +1,5 @@ package domain -import ( - "strings" - - "github.com/zitadel/zitadel/internal/eventstore/v1/models" - "github.com/zitadel/zitadel/internal/zerrors" -) - -const ( - maxGroupNameLen = 200 -) - -// Group represents a user group in an organization -type Group struct { - models.ObjectRoot - - Name string - Description string -} - -func (g *Group) IsValid() error { - groupName := strings.TrimSpace(g.Name) - if groupName == "" || len(groupName) > maxGroupNameLen { - return zerrors.ThrowInvalidArgument(nil, "GROUP-m177lN", "Errors.Group.InvalidName") - } - return nil -} - type GroupState int32 const ( diff --git a/internal/domain/permission.go b/internal/domain/permission.go index ff21530af..2d8bda192 100644 --- a/internal/domain/permission.go +++ b/internal/domain/permission.go @@ -70,6 +70,10 @@ const ( PermissionIAMPolicyWrite = "iam.policy.write" PermissionIAMPolicyDelete = "iam.policy.delete" PermissionPolicyRead = "policy.read" + PermissionGroupCreate = "group.create" + PermissionGroupWrite = "group.write" + PermissionGroupRead = "group.read" + PermissionGroupDelete = "group.delete" ) // ProjectPermissionCheck is used as a check for preconditions dependent on application, project, user resourceowner and usergrants. diff --git a/internal/query/group.go b/internal/query/group.go index 493f33433..3ce333436 100644 --- a/internal/query/group.go +++ b/internal/query/group.go @@ -4,6 +4,7 @@ import ( "context" "database/sql" "errors" + "slices" "time" sq "github.com/Masterminds/squirrel" @@ -81,41 +82,34 @@ type GroupSearchQuery struct { Queries []SearchQuery } -func (q *Queries) GetGroupByID(ctx context.Context, id string) (group *Group, err error) { +func (q *Queries) GetGroupByID(ctx context.Context, id string, permissionCheck domain.PermissionCheck) (group *Group, err error) { ctx, span := tracing.NewSpan(ctx) defer func() { span.EndWithError(err) }() - // todo: add permission check - - stmt, scan := prepareGroupQuery() - eq := sq.Eq{ - GroupColumnID.identifier(): id, - GroupColumnInstanceID.identifier(): authz.GetInstance(ctx).InstanceID(), - } - query, args, err := stmt.Where(eq).ToSql() + group, err = q.getGroupByID(ctx, id, group) if err != nil { - return nil, zerrors.ThrowInternal(err, "QUERY-8bde1", "Errors.Query.SQLStatement") + return nil, err } - - err = q.client.QueryRowContext(ctx, func(row *sql.Row) error { - group, err = scan(row) - return err - }, query, args...) - return group, err - + if err = groupCheckPermission(ctx, group.ResourceOwner, group.ID, permissionCheck); err != nil { + return nil, err + } + return group, nil } // SearchGroups returns the list of groups that match the search criteria -func (q *Queries) SearchGroups(ctx context.Context, queries *GroupSearchQuery) (_ *Groups, err error) { +func (q *Queries) SearchGroups(ctx context.Context, queries *GroupSearchQuery, permissionCheck domain.PermissionCheck) (_ *Groups, err error) { ctx, span := tracing.NewSpan(ctx) defer func() { span.EndWithError(err) }() - // todo: add permission check + permissionCheckV2 := PermissionV2(ctx, permissionCheck) - groups, err := q.searchGroups(ctx, queries) + groups, err := q.searchGroups(ctx, queries, permissionCheckV2) if err != nil { return nil, err } + if permissionCheck != nil && !permissionCheckV2 { + groupsCheckPermission(ctx, groups, permissionCheck) + } return groups, nil } @@ -135,6 +129,36 @@ func NewGroupOrganizationIdSearchQuery(id string) (SearchQuery, error) { return NewTextQuery(GroupColumnResourceOwner, id, TextEquals) } +func groupCheckPermission(ctx context.Context, resourceOwner, groupID string, permissionCheck domain.PermissionCheck) error { + return permissionCheck(ctx, domain.PermissionGroupRead, resourceOwner, groupID) +} + +func groupsCheckPermission(ctx context.Context, groups *Groups, permissionCheck domain.PermissionCheck) { + groups.Groups = slices.DeleteFunc(groups.Groups, + func(group *Group) bool { + return groupCheckPermission(ctx, group.ResourceOwner, group.ID, permissionCheck) != nil + }, + ) +} + +func (q *Queries) getGroupByID(ctx context.Context, id string, group *Group) (*Group, error) { + stmt, scan := prepareGroupQuery() + eq := sq.Eq{ + GroupColumnID.identifier(): id, + GroupColumnInstanceID.identifier(): authz.GetInstance(ctx).InstanceID(), + } + query, args, err := stmt.Where(eq).ToSql() + if err != nil { + return nil, zerrors.ThrowInternal(err, "QUERY-8bde1", "Errors.Query.SQLStatement") + } + + err = q.client.QueryRowContext(ctx, func(row *sql.Row) error { + group, err = scan(row) + return err + }, query, args...) + return group, err +} + func prepareGroupQuery() (sq.SelectBuilder, func(*sql.Row) (*Group, error)) { return sq.Select( GroupColumnID.identifier(), @@ -171,15 +195,14 @@ func prepareGroupQuery() (sq.SelectBuilder, func(*sql.Row) (*Group, error)) { } } -func (q *Queries) searchGroups(ctx context.Context, queries *GroupSearchQuery) (groups *Groups, err error) { +func (q *Queries) searchGroups(ctx context.Context, queries *GroupSearchQuery, permissionCheckV2 bool) (groups *Groups, err error) { ctx, span := tracing.NewSpan(ctx) defer func() { span.EndWithError(err) }() query, scan := prepareGroupsQuery() - eq := sq.And{ - sq.Eq{ - GroupColumnInstanceID.identifier(): authz.GetInstance(ctx).InstanceID(), - }, + query = groupPermissionCheckV2(ctx, query, queries.Queries, permissionCheckV2) + eq := sq.Eq{ + GroupColumnInstanceID.identifier(): authz.GetInstance(ctx).InstanceID(), } stmt, args, err := queries.toQuery(query).Where(eq).ToSql() if err != nil { @@ -197,6 +220,20 @@ func (q *Queries) searchGroups(ctx context.Context, queries *GroupSearchQuery) ( return groups, err } +func groupPermissionCheckV2(ctx context.Context, query sq.SelectBuilder, queries []SearchQuery, permissionCheckV2 bool) sq.SelectBuilder { + if !permissionCheckV2 { + return query + } + + join, args := PermissionClause( + ctx, + GroupColumnResourceOwner, + domain.PermissionGroupRead, + SingleOrgPermissionOption(queries), + ) + return query.JoinClause(join, args...) +} + func prepareGroupsQuery() (sq.SelectBuilder, func(*sql.Rows) (*Groups, error)) { return sq.Select( GroupColumnID.identifier(), diff --git a/internal/query/group_test.go b/internal/query/group_test.go index fa8615141..2518ce296 100644 --- a/internal/query/group_test.go +++ b/internal/query/group_test.go @@ -1,6 +1,7 @@ package query import ( + "context" "database/sql" "database/sql/driver" "errors" @@ -8,6 +9,8 @@ import ( "regexp" "testing" + "github.com/stretchr/testify/require" + "github.com/zitadel/zitadel/internal/domain" "github.com/zitadel/zitadel/internal/zerrors" ) @@ -303,3 +306,122 @@ func Test_GroupPrepares(t *testing.T) { }) } } + +func Test_GroupsCheckPermission(t *testing.T) { + tests := []struct { + name string + want []*Group + groups *Groups + permissions []string + }{ + { + name: "no permissions", + want: []*Group{}, + groups: &Groups{ + Groups: []*Group{ + {ID: "group1"}, {ID: "group2"}, {ID: "group3"}, + }, + }, + permissions: []string{}, + }, + { + name: "permissions for all groups", + want: []*Group{ + {ID: "group1"}, {ID: "group2"}, {ID: "group3"}, + }, + groups: &Groups{ + Groups: []*Group{ + {ID: "group1"}, {ID: "group2"}, {ID: "group3"}, + }, + }, + permissions: []string{"group1", "group2", "group3"}, + }, + { + name: "permissions for group1", + want: []*Group{ + {ID: "group1"}, + }, + groups: &Groups{ + Groups: []*Group{ + {ID: "group1"}, {ID: "group2"}, {ID: "group3"}, + }, + }, + permissions: []string{"group1"}, + }, + { + name: "permissions for group2", + want: []*Group{ + {ID: "group2"}, + }, + groups: &Groups{ + Groups: []*Group{ + {ID: "group1"}, {ID: "group2"}, {ID: "group3"}, + }, + }, + permissions: []string{"group2"}, + }, + { + name: "permissions for group3", + want: []*Group{ + {ID: "group3"}, + }, + groups: &Groups{ + Groups: []*Group{ + {ID: "group1"}, {ID: "group2"}, {ID: "group3"}, + }, + }, + permissions: []string{"group3"}, + }, + { + name: "permissions for group1 and group2", + want: []*Group{ + {ID: "group1"}, {ID: "group2"}, + }, + groups: &Groups{ + Groups: []*Group{ + {ID: "group1"}, {ID: "group2"}, {ID: "group3"}, + }, + }, + permissions: []string{"group1", "group2"}, + }, + { + name: "permissions for group1 and group3", + want: []*Group{ + {ID: "group1"}, {ID: "group3"}, + }, + groups: &Groups{ + Groups: []*Group{ + {ID: "group1"}, {ID: "group2"}, {ID: "group3"}, + }, + }, + permissions: []string{"group1", "group3"}, + }, + { + name: "permissions for group2 and group3", + want: []*Group{ + {ID: "group2"}, {ID: "group3"}, + }, + groups: &Groups{ + Groups: []*Group{ + {ID: "group1"}, {ID: "group2"}, {ID: "group3"}, + }, + }, + permissions: []string{"group2", "group3"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + checkPermission := func(ctx context.Context, permission, orgID, resourceID string) (err error) { + for _, perm := range tt.permissions { + if resourceID == perm { + return nil + } + } + return errors.New("failed") + } + groupsCheckPermission(context.Background(), tt.groups, checkPermission) + require.Equal(t, tt.want, tt.groups.Groups) + }) + } + +} diff --git a/proto/zitadel/group/v2/group_service.proto b/proto/zitadel/group/v2/group_service.proto index 5cf9c9108..e248a1f31 100644 --- a/proto/zitadel/group/v2/group_service.proto +++ b/proto/zitadel/group/v2/group_service.proto @@ -108,13 +108,12 @@ option (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_swagger) = { // This service provides methods to create, retrieve, update, and delete user groups in an organization. service GroupService { - // CreateGroup + // Create Group // // CreateGroup creates a new user group in an organization. // - // Required permissions: // TODO - // - "iam.member.write" for instance administrators - // - "org.member.write" for organization administrators + // Required permissions: + // - group.create rpc CreateGroup(CreateGroupRequest) returns (CreateGroupResponse) { option (zitadel.protoc_gen_zitadel.v2.options) = { auth_option: { @@ -157,12 +156,12 @@ service GroupService { }; } - // GetGroup + // Get Group // // Retrieves a group based on its ID. // // Required permission: - // - TODO + // - group.read rpc GetGroup(GetGroupRequest) returns (GetGroupResponse) { option (zitadel.protoc_gen_zitadel.v2.options) = { auth_option: { @@ -190,13 +189,12 @@ service GroupService { }; } - // ListGroups + // List Groups // // ListGroups returns all groups matching the request and necessary permissions from an organization. // - // Required permissions: // TODO - // - "iam.member.read" for instance administrators - // - "org.member.read" for organization administrators + // Required permissions: + // - group.read rpc ListGroups(ListGroupsRequest) returns (ListGroupsResponse) { option (zitadel.protoc_gen_zitadel.v2.options) = { auth_option: { @@ -225,17 +223,16 @@ service GroupService { }; } - // UpdateGroup + // Update Group // // UpdateGroup updates the user group. // - // In case the group there aren't any changes, the request will return a successful response as + // In case there aren't any changes, the request will return a successful response as // the desired state is already achieved. // You can check the change date in the response to verify if the group was updated by the request. // - // Required permissions: // TODO - // - "iam.member.write" for instance administrators - // - "org.member.write" for organization administrators + // Required permissions: + // - group.write rpc UpdateGroup(UpdateGroupRequest) returns (UpdateGroupResponse) { option (zitadel.protoc_gen_zitadel.v2.options) = { auth_option: { @@ -264,7 +261,7 @@ service GroupService { }; } - // DeleteGroup + // Delete Group // // DeleteGroup deletes the group. // @@ -272,9 +269,8 @@ service GroupService { // the desired state is already achieved. // You can check the deletion date in the response to verify if the group was deleted by the request. // - // Required permissions: // TODO - // - "iam.member.delete" for instance administrators - // - "org.member.delete" for organization administrators + // Required permissions: + // - group.delete rpc DeleteGroup(DeleteGroupRequest) returns (DeleteGroupResponse) { option (zitadel.protoc_gen_zitadel.v2.options) = { auth_option: {