diff --git a/apps/console/src/pages/iam/organizations/people/PersonPage.tsx b/apps/console/src/pages/iam/organizations/people/PersonPage.tsx index 6dbfbc357..fb87d5ef1 100644 --- a/apps/console/src/pages/iam/organizations/people/PersonPage.tsx +++ b/apps/console/src/pages/iam/organizations/people/PersonPage.tsx @@ -36,6 +36,7 @@ export const personPageQuery = graphql` source state canDelete: permission(action: "iam:membership-profile:delete") + canRemoveMember: permission(action: "iam:membership:delete") ...PersonFormFragment } } @@ -140,7 +141,7 @@ export function PersonPage(props: { queryRef: PreloadedQuery }) }; const canArchive = person.canDelete && person.source !== "SCIM" && person.state !== "INACTIVE"; - const canRemove = person.canDelete && person.source !== "SCIM"; + const canRemove = person.canRemoveMember && person.source !== "SCIM"; return (
diff --git a/apps/console/src/pages/iam/organizations/people/_components/PeopleListItem.tsx b/apps/console/src/pages/iam/organizations/people/_components/PeopleListItem.tsx index 93d4075e1..87f51ebb2 100644 --- a/apps/console/src/pages/iam/organizations/people/_components/PeopleListItem.tsx +++ b/apps/console/src/pages/iam/organizations/people/_components/PeopleListItem.tsx @@ -65,6 +65,7 @@ const fragment = graphql` canUpdate: permission(action: "iam:membership-profile:update") canInvite: permission(action: "iam:invitation:create") canDelete: permission(action: "iam:membership-profile:delete") + canRemoveMember: permission(action: "iam:membership:delete") } `; @@ -137,7 +138,7 @@ export function PeopleListItem(props: { const canSendActivationMail = isInactive && profile.source !== "SCIM" && profile.canInvite; const canArchive = profile.canDelete && profile.source !== "SCIM" && profile.state !== "INACTIVE"; - const canRemove = profile.canDelete && profile.source !== "SCIM"; + const canRemove = profile.canRemoveMember && profile.source !== "SCIM"; const [inviteUser] = useMutationWithToasts(inviteUserMutation, { diff --git a/cmd/probod/CHANGELOG.md b/cmd/probod/CHANGELOG.md index 65fee1cd1..d5b4799b5 100644 --- a/cmd/probod/CHANGELOG.md +++ b/cmd/probod/CHANGELOG.md @@ -4,6 +4,14 @@ All notable changes to `probod` (the server, including the bundled `@probo/conso ## Unreleased +### Fixed + +- Enforced owner-only member removal: `removeUser` (API resolver and MCP `RemoveUserTool`) now requires the owner-only `iam:membership:delete` gate instead of the weaker `iam:membership-profile:delete`, so an organization ADMIN can no longer remove members (including OWNERs) + +### Changed + +- Consolidated ownership-grant authorization into policy: granting OWNER (via `createUser` or `updateMembership`) is now restricted to organization owners through role-scoped allow policies conditioned on the assigned role, replacing the per-resolver custom checks and the now-removed `iam:membership-role:set-owner` action. The `permission` field gained an optional typed `attributes` argument so the console can refine dry-run checks (e.g. by target role) without loosening the base grants + ## [0.223.3] - 2026-07-06 ### Fixed diff --git a/e2e/console/user_test.go b/e2e/console/user_test.go index b5da34a22..79fc2670b 100644 --- a/e2e/console/user_test.go +++ b/e2e/console/user_test.go @@ -26,8 +26,8 @@ import ( // TestUser_AdminCannotCreateOwner is a non-regression test for the vertical // privilege escalation where an ADMIN could mint an OWNER membership via -// createUser, bypassing the owner-only set-owner authorization that -// updateMembership enforces. +// createUser. Granting ownership (whether by creating an OWNER member or +// promoting one) is owner-only, enforced by policy on the target role. func TestUser_AdminCannotCreateOwner(t *testing.T) { t.Parallel() owner := testutil.NewClient(t, testutil.RoleOwner) @@ -97,6 +97,60 @@ func TestUser_AdminCannotCreateOwner(t *testing.T) { assert.Equal(t, "OWNER", ownerCreate.CreateUser.ProfileEdge.Node.Membership.Role) } +// TestUser_AdminCannotRemoveMembers is a non-regression test for the broken +// access control where an ADMIN could hard-remove members (including OWNERs) +// via removeUser because the mutation only checked the weaker +// iam:membership-profile:delete gate instead of the owner-only +// iam:membership:delete gate. Removing members is owner-only. +func TestUser_AdminCannotRemoveMembers(t *testing.T) { + t.Parallel() + owner := testutil.NewClient(t, testutil.RoleOwner) + admin := testutil.NewClientInOrg(t, testutil.RoleAdmin, owner) + viewer := testutil.NewClientInOrg(t, testutil.RoleViewer, owner) + secondOwner := testutil.NewClientInOrg(t, testutil.RoleOwner, owner) + + const removeUserMutation = ` + mutation($input: RemoveUserInput!) { + removeUser(input: $input) { + deletedProfileId + } + } + ` + + removeInput := func(profileID string) map[string]any { + return map[string]any{ + "input": map[string]any{ + "organizationId": owner.GetOrganizationID().String(), + "profileId": profileID, + }, + } + } + + // An ADMIN must not be able to remove a lower-privileged member. + _, err := admin.DoConnect(removeUserMutation, removeInput(viewer.GetProfileID().String())) + testutil.RequireForbiddenError(t, err) + + // An ADMIN must not be able to remove another ADMIN (itself). + _, err = admin.DoConnect(removeUserMutation, removeInput(admin.GetProfileID().String())) + testutil.RequireForbiddenError(t, err) + + // An ADMIN must not be able to remove an OWNER, even when more than one + // active owner remains. + _, err = admin.DoConnect(removeUserMutation, removeInput(secondOwner.GetProfileID().String())) + testutil.RequireForbiddenError(t, err) + + // An OWNER remains able to remove members. + var removeResult struct { + RemoveUser struct { + DeletedProfileID string `json:"deletedProfileId"` + } `json:"removeUser"` + } + + err = owner.ExecuteConnect(removeUserMutation, removeInput(viewer.GetProfileID().String()), &removeResult) + require.NoError(t, err) + assert.Equal(t, viewer.GetProfileID().String(), removeResult.RemoveUser.DeletedProfileID) +} + func TestUser_UpdateMembership(t *testing.T) { t.Parallel() owner := testutil.NewClient(t, testutil.RoleOwner) diff --git a/pkg/coredata/membership_profile.go b/pkg/coredata/membership_profile.go index a713d89e8..e6b62a550 100644 --- a/pkg/coredata/membership_profile.go +++ b/pkg/coredata/membership_profile.go @@ -99,7 +99,8 @@ func (p *MembershipProfile) AuthorizationAttributes( SELECT id, organization_id, - identity_id + identity_id, + source FROM iam_membership_profiles WHERE @@ -123,9 +124,10 @@ WHERE id gid.GID organizationID gid.GID identityID gid.GID + source ProfileSource ) - err = rows.Scan(&id, &organizationID, &identityID) + err = rows.Scan(&id, &organizationID, &identityID, &source) if err != nil { return nil, fmt.Errorf("cannot scan profile authorization attributes: %w", err) } @@ -133,6 +135,7 @@ WHERE attrsByID[id] = policy.Attributes{ "organization_id": organizationID.String(), "identity_id": identityID.String(), + "source": source.String(), } } diff --git a/pkg/iam/iam_actions.go b/pkg/iam/iam_actions.go index e177e389e..9bf00839e 100644 --- a/pkg/iam/iam_actions.go +++ b/pkg/iam/iam_actions.go @@ -48,9 +48,6 @@ const ( ActionMembershipUpdate = "iam:membership:update" ActionMembershipDelete = "iam:membership:delete" - // Membership role actions - ActionMembershipRoleSetOwner = "iam:membership-role:set-owner" - // Membership Profile actions ActionMembershipProfileGet = "iam:membership-profile:get" ActionMembershipProfileList = "iam:membership-profile:list" diff --git a/pkg/iam/iam_policies.go b/pkg/iam/iam_policies.go index 385ef0d18..65ff85ff0 100644 --- a/pkg/iam/iam_policies.go +++ b/pkg/iam/iam_policies.go @@ -183,11 +183,6 @@ var IAMOwnerPolicy = policy.NewPolicy( policy.NotEquals("resource.source", "SCIM"), ), - // Can set other members OWNER - policy.Allow(ActionMembershipRoleSetOwner). - WithSID("membership-role-owner-access"). - When(policy.Equals("principal.organization_id", "resource.organization_id")), - // Full access to membership profiles (scoped to own organization) policy.Allow( ActionMembershipProfileGet, @@ -318,6 +313,15 @@ var IAMAdminPolicy = policy.NewPolicy( policy.Deny(ActionMembershipDelete). WithSID("deny-remove-member"), + // Cannot grant ownership, whether by creating an OWNER member or promoting an + // existing member to OWNER (only owner can grant ownership) + policy.Deny(ActionMembershipProfileCreate). + WithSID("deny-create-owner"). + When(policy.Equals("resource.target_role", "OWNER")), + policy.Deny(ActionMembershipUpdate). + WithSID("deny-promote-owner"). + When(policy.Equals("resource.target_role", "OWNER")), + // Cannot manage SAML configurations (only owner can) policy.Deny( ActionSAMLConfigurationCreate, diff --git a/pkg/iam/oauth2_scopes.go b/pkg/iam/oauth2_scopes.go index 7ab59e82c..89e4abf7d 100644 --- a/pkg/iam/oauth2_scopes.go +++ b/pkg/iam/oauth2_scopes.go @@ -85,7 +85,6 @@ var IAMOAuth2ScopeMappings = map[coredata.OAuth2Scope][]string{ ActionInvitationDelete, ActionMembershipUpdate, ActionMembershipDelete, - ActionMembershipRoleSetOwner, ActionMembershipProfileCreate, ActionMembershipProfileUpdate, ActionMembershipProfileDelete, diff --git a/pkg/iam/organization_service.go b/pkg/iam/organization_service.go index 4dd01a17c..3e68cb08c 100644 --- a/pkg/iam/organization_service.go +++ b/pkg/iam/organization_service.go @@ -309,11 +309,10 @@ func (s *OrganizationService) UpdateMembership( func (s *OrganizationService) RemoveUser( ctx context.Context, + scope coredata.Scoper, organizationID gid.GID, profileID gid.GID, ) error { - scope := coredata.NewScopeFromObjectID(organizationID) - return s.pg.WithTx( ctx, func(ctx context.Context, tx pg.Tx) error { @@ -987,13 +986,12 @@ func (s *OrganizationService) DeleteOrganization(ctx context.Context, organizati ) } -func (s *OrganizationService) CreateUser(ctx context.Context, req *CreateUserRequest) (*coredata.MembershipProfile, error) { +func (s *OrganizationService) CreateUser(ctx context.Context, scope coredata.Scoper, req *CreateUserRequest) (*coredata.MembershipProfile, error) { if err := req.Validate(); err != nil { return nil, fmt.Errorf("invalid request: %w", err) } var ( - scope = coredata.NewScopeFromObjectID(req.OrganizationID) profile *coredata.MembershipProfile now = time.Now() ) diff --git a/pkg/server/api/connect/v1/membership_resolvers.go b/pkg/server/api/connect/v1/membership_resolvers.go index c3453ea68..8b06b45fb 100644 --- a/pkg/server/api/connect/v1/membership_resolvers.go +++ b/pkg/server/api/connect/v1/membership_resolvers.go @@ -10,7 +10,6 @@ import ( "errors" "go.gearno.de/kit/log" - "go.probo.inc/probo/pkg/coredata" "go.probo.inc/probo/pkg/iam" "go.probo.inc/probo/pkg/server/api/authn" "go.probo.inc/probo/pkg/server/api/authz" @@ -51,16 +50,15 @@ func (r *membershipResolver) Permission(ctx context.Context, obj *types.Membersh // UpdateMembership is the resolver for the updateMembership field. func (r *mutationResolver) UpdateMembership(ctx context.Context, input types.UpdateMembershipInput) (*types.UpdateMembershipPayload, error) { - if _, err := r.authorize(ctx, input.MembershipID, iam.ActionMembershipUpdate); err != nil { + if _, err := r.authorize( + ctx, + input.MembershipID, + iam.ActionMembershipUpdate, + authz.WithAttr("target_role", input.Role.String()), + ); err != nil { return nil, err } - if input.Role == coredata.MembershipRoleOwner { - if _, err := r.authorize(ctx, input.MembershipID, iam.ActionMembershipRoleSetOwner); err != nil { - return nil, err - } - } - membership, err := r.iam.OrganizationService.UpdateMembership(ctx, input.OrganizationID, input.MembershipID, input.Role) if err != nil { if _, ok := errors.AsType[*iam.ErrLastActiveOwner](err); ok { diff --git a/pkg/server/api/connect/v1/profile_resolvers.go b/pkg/server/api/connect/v1/profile_resolvers.go index 7ce17015d..239c848a1 100644 --- a/pkg/server/api/connect/v1/profile_resolvers.go +++ b/pkg/server/api/connect/v1/profile_resolvers.go @@ -22,18 +22,19 @@ import ( // CreateUser is the resolver for the createUser field. func (r *mutationResolver) CreateUser(ctx context.Context, input types.CreateUserInput) (*types.CreateUserPayload, error) { - if _, err := r.authorize(ctx, input.OrganizationID, iam.ActionMembershipProfileCreate); err != nil { + scope, err := r.authorize( + ctx, + input.OrganizationID, + iam.ActionMembershipProfileCreate, + authz.WithAttr("target_role", input.Role.String()), + ) + if err != nil { return nil, err } - if input.Role == coredata.MembershipRoleOwner { - if _, err := r.authorize(ctx, input.OrganizationID, iam.ActionMembershipRoleSetOwner); err != nil { - return nil, err - } - } - profile, err := r.iam.OrganizationService.CreateUser( ctx, + scope, &iam.CreateUserRequest{ OrganizationID: input.OrganizationID, EmailAddress: input.EmailAddress, @@ -137,11 +138,12 @@ func (r *mutationResolver) ArchiveUser(ctx context.Context, input types.ArchiveU // RemoveUser is the resolver for the removeUser field. func (r *mutationResolver) RemoveUser(ctx context.Context, input types.RemoveUserInput) (*types.RemoveUserPayload, error) { - if _, err := r.authorize(ctx, input.ProfileID, iam.ActionMembershipProfileDelete); err != nil { + scope, err := r.authorize(ctx, input.ProfileID, iam.ActionMembershipDelete) + if err != nil { return nil, err } - err := r.iam.OrganizationService.RemoveUser(ctx, input.OrganizationID, input.ProfileID) + err = r.iam.OrganizationService.RemoveUser(ctx, scope, input.OrganizationID, input.ProfileID) if err != nil { if _, ok := errors.AsType[*iam.ErrUserManagedBySCIM](err); ok { return nil, gqlutils.Conflictf(ctx, "user is managed by SCIM and cannot be removed") diff --git a/pkg/server/api/mcp/v1/resolver.go b/pkg/server/api/mcp/v1/resolver.go index 60c28ed8a..c7af653fa 100644 --- a/pkg/server/api/mcp/v1/resolver.go +++ b/pkg/server/api/mcp/v1/resolver.go @@ -35,6 +35,7 @@ import ( "go.probo.inc/probo/pkg/resourcealias" "go.probo.inc/probo/pkg/riskmanagement" "go.probo.inc/probo/pkg/server/api/authn" + "go.probo.inc/probo/pkg/server/api/authz" "go.probo.inc/probo/pkg/thirdparty" ) @@ -65,17 +66,21 @@ func markdownToProseMirrorJSON(markdown string) (string, error) { return string(out), nil } -func (r *Resolver) Authorize(ctx context.Context, entityID gid.GID, action iam.Action) (*coredata.Scope, error) { +func (r *Resolver) Authorize(ctx context.Context, entityID gid.GID, action iam.Action, opts ...authz.AuthorizeFuncOption) (*coredata.Scope, error) { identity := authn.IdentityFromContext(ctx) - scope, err := r.iamSvc.Authorizer.Authorize( - ctx, - iam.AuthorizeParams{ - Principal: identity.ID, - Resource: entityID, - Action: action, - }, - ) + params := iam.AuthorizeParams{ + Principal: identity.ID, + Resource: entityID, + Action: action, + ResourceAttributes: make(map[string]string), + } + + for _, opt := range opts { + opt(¶ms) + } + + scope, err := r.iamSvc.Authorizer.Authorize(ctx, params) if err == nil { return scope, nil } diff --git a/pkg/server/api/mcp/v1/schema.resolvers.go b/pkg/server/api/mcp/v1/schema.resolvers.go index 8377aa095..13bda79c6 100644 --- a/pkg/server/api/mcp/v1/schema.resolvers.go +++ b/pkg/server/api/mcp/v1/schema.resolvers.go @@ -24,6 +24,7 @@ import ( "go.probo.inc/probo/pkg/resourcealias" "go.probo.inc/probo/pkg/riskmanagement" "go.probo.inc/probo/pkg/server/api/authn" + "go.probo.inc/probo/pkg/server/api/authz" "go.probo.inc/probo/pkg/server/api/mcp/v1/types" "go.probo.inc/probo/pkg/thirdparty" "go.probo.inc/probo/pkg/validator" @@ -2815,16 +2816,16 @@ func (r *Resolver) GetUserTool(ctx context.Context, req *mcp.CallToolRequest, in } func (r *Resolver) CreateUserTool(ctx context.Context, req *mcp.CallToolRequest, input *types.CreateUserInput) (*mcp.CallToolResult, types.CreateUserOutput, error) { - if _, err := r.Authorize(ctx, input.OrganizationID, iam.ActionMembershipProfileCreate); err != nil { + scope, err := r.Authorize( + ctx, + input.OrganizationID, + iam.ActionMembershipProfileCreate, + authz.WithAttr("target_role", input.Role.String()), + ) + if err != nil { return nil, types.CreateUserOutput{}, err } - if input.Role == coredata.MembershipRoleOwner { - if _, err := r.Authorize(ctx, input.OrganizationID, iam.ActionMembershipRoleSetOwner); err != nil { - return nil, types.CreateUserOutput{}, err - } - } - var contractStart, contractEnd **time.Time if input.ContractStartDate != nil { contractStart = &input.ContractStartDate @@ -2834,7 +2835,7 @@ func (r *Resolver) CreateUserTool(ctx context.Context, req *mcp.CallToolRequest, contractEnd = &input.ContractEndDate } - profile, err := r.iamSvc.OrganizationService.CreateUser(ctx, &iam.CreateUserRequest{ + profile, err := r.iamSvc.OrganizationService.CreateUser(ctx, scope, &iam.CreateUserRequest{ OrganizationID: input.OrganizationID, EmailAddress: input.EmailAddress, Role: input.Role, @@ -2921,16 +2922,15 @@ func (r *Resolver) UpdateUserTool(ctx context.Context, req *mcp.CallToolRequest, } func (r *Resolver) UpdateMembershipTool(ctx context.Context, req *mcp.CallToolRequest, input *types.UpdateMembershipInput) (*mcp.CallToolResult, types.UpdateMembershipOutput, error) { - if _, err := r.Authorize(ctx, input.MembershipID, iam.ActionMembershipUpdate); err != nil { + if _, err := r.Authorize( + ctx, + input.MembershipID, + iam.ActionMembershipUpdate, + authz.WithAttr("target_role", input.Role.String()), + ); err != nil { return nil, types.UpdateMembershipOutput{}, err } - if input.Role == coredata.MembershipRoleOwner { - if _, err := r.Authorize(ctx, input.MembershipID, iam.ActionMembershipRoleSetOwner); err != nil { - return nil, types.UpdateMembershipOutput{}, err - } - } - membership, err := r.iamSvc.OrganizationService.UpdateMembership(ctx, input.OrganizationID, input.MembershipID, input.Role) if err != nil { return nil, types.UpdateMembershipOutput{}, fmt.Errorf("update membership: %w", err) @@ -2946,11 +2946,12 @@ func (r *Resolver) UpdateMembershipTool(ctx context.Context, req *mcp.CallToolRe } func (r *Resolver) RemoveUserTool(ctx context.Context, req *mcp.CallToolRequest, input *types.RemoveUserInput) (*mcp.CallToolResult, types.RemoveUserOutput, error) { - if _, err := r.Authorize(ctx, input.ProfileID, iam.ActionMembershipProfileDelete); err != nil { + scope, err := r.Authorize(ctx, input.ProfileID, iam.ActionMembershipDelete) + if err != nil { return nil, types.RemoveUserOutput{}, err } - err := r.iamSvc.OrganizationService.RemoveUser(ctx, input.OrganizationID, input.ProfileID) + err = r.iamSvc.OrganizationService.RemoveUser(ctx, scope, input.OrganizationID, input.ProfileID) if err != nil { if _, ok := errors.AsType[*iam.ErrUserManagedBySCIM](err); ok { return nil, types.RemoveUserOutput{}, fmt.Errorf("user is managed by SCIM and cannot be removed: %w", err)