From c2b71b8cb60441ae1722488b14b020889d68db88 Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Mon, 3 Aug 2026 00:03:34 +0200 Subject: [PATCH] fix(controlplane): restrict organization updates to admins of the target org (CP-N1) OrganizationService.Update took the organization to modify from the request body while the authorization middleware evaluated the caller's role against the organization selected in the request headers. A user who was an admin of one organization could therefore change the security settings of any other organization they held a membership in, including disabling policy violation blocking, pointing the policy engine at arbitrary hostnames and re-enabling runner environment variable capture. The update is now pinned to the currently selected organization at the service layer, and the biz layer authorizes against the membership held in the organization being updated, requiring an admin or owner role. Assisted-by: Claude Code Signed-off-by: Miguel Martinez Trivino Chainloop-Trace-Sessions: 61088ea5-dc57-47f9-9284-1a7c3527eb94 Signed-off-by: Miguel Martinez Trivino --- .../internal/service/organization.go | 15 +++- .../internal/service/organization_test.go | 72 +++++++++++++++ ...apitoken_stale_revoker_integration_test.go | 5 +- app/controlplane/pkg/biz/organization.go | 8 ++ .../pkg/biz/organization_integration_test.go | 90 ++++++++++++++++++- 5 files changed, 186 insertions(+), 4 deletions(-) create mode 100644 app/controlplane/internal/service/organization_test.go diff --git a/app/controlplane/internal/service/organization.go b/app/controlplane/internal/service/organization.go index f97abcb24..7bd462118 100644 --- a/app/controlplane/internal/service/organization.go +++ b/app/controlplane/internal/service/organization.go @@ -82,6 +82,19 @@ func (s *OrganizationService) Update(ctx context.Context, req *pb.OrganizationSe return nil, err } + currentOrg, err := requireCurrentOrg(ctx) + if err != nil { + return nil, err + } + + // The authorization middleware evaluates the caller's role against the organization + // selected in the request headers, so the update has to target that same organization. + // Honoring an arbitrary name here would let an admin of one organization change the + // settings of another one they merely belong to. + if req.Name != currentOrg.Name { + return nil, errors.Forbidden("forbidden", "the organization to update must be the currently selected one") + } + // we want to differentiate between setting the value to empty or not setting it at all // to do that we will use a nil slice to represent not setting it at all var policiesAllowedHostnames []string @@ -102,7 +115,7 @@ func (s *OrganizationService) Update(ctx context.Context, req *pb.OrganizationSe apiTokenMaxDaysInactive = &days } - org, err := s.orgUC.Update(ctx, currentUser.ID, req.Name, &biz.OrganizationUpdateOpts{ + org, err := s.orgUC.Update(ctx, currentUser.ID, currentOrg.Name, &biz.OrganizationUpdateOpts{ BlockOnPolicyViolation: req.BlockOnPolicyViolation, PoliciesAllowedHostnames: policiesAllowedHostnames, PreventImplicitWorkflowCreation: req.PreventImplicitWorkflowCreation, diff --git a/app/controlplane/internal/service/organization_test.go b/app/controlplane/internal/service/organization_test.go new file mode 100644 index 000000000..901d76a8a --- /dev/null +++ b/app/controlplane/internal/service/organization_test.go @@ -0,0 +1,72 @@ +// +// Copyright 2026 The Chainloop Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package service + +import ( + "context" + "testing" + + pb "github.com/chainloop-dev/chainloop/app/controlplane/api/controlplane/v1" + "github.com/chainloop-dev/chainloop/app/controlplane/internal/usercontext/entities" + "github.com/go-kratos/kratos/v2/errors" + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestUpdateIsPinnedToCurrentOrg is a regression test for CP-N1. The authz +// middleware evaluates the caller's role against the organization selected in +// the request headers, so Update must refuse to operate on any other +// organization. Otherwise an admin of org A could target org B by naming it in +// the request body. +func TestUpdateIsPinnedToCurrentOrg(t *testing.T) { + // A nil use case is deliberate: a request that reaches the biz layer means + // the guard did not run, and the test fails loudly instead of silently + // passing. + svc := NewOrganizationService(nil, nil) + + ctxWithOrg := func(orgName string) context.Context { + ctx := entities.WithCurrentUser(context.Background(), &entities.User{ID: uuid.NewString(), Email: "user@test.com"}) + return entities.WithCurrentOrg(ctx, &entities.Org{ID: uuid.NewString(), Name: orgName}) + } + + testCases := []struct { + name string + currentOrg string + reqName string + }{ + {name: "different organization", currentOrg: "my-org", reqName: "victim-org"}, + {name: "empty name", currentOrg: "my-org", reqName: ""}, + {name: "case variation", currentOrg: "my-org", reqName: "My-Org"}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + got, err := svc.Update(ctxWithOrg(tc.currentOrg), &pb.OrganizationServiceUpdateRequest{ + Name: tc.reqName, + BlockOnPolicyViolation: toPtrBool(false), + }) + + require.Error(t, err) + assert.Nil(t, got) + assert.True(t, errors.IsForbidden(err), "want forbidden, got %v", err) + }) + } +} + +func toPtrBool(b bool) *bool { + return &b +} diff --git a/app/controlplane/pkg/biz/apitoken_stale_revoker_integration_test.go b/app/controlplane/pkg/biz/apitoken_stale_revoker_integration_test.go index fef122a94..12478d05a 100644 --- a/app/controlplane/pkg/biz/apitoken_stale_revoker_integration_test.go +++ b/app/controlplane/pkg/biz/apitoken_stale_revoker_integration_test.go @@ -22,6 +22,7 @@ import ( "testing" "time" + "github.com/chainloop-dev/chainloop/app/controlplane/pkg/authz" "github.com/chainloop-dev/chainloop/app/controlplane/pkg/biz" "github.com/chainloop-dev/chainloop/app/controlplane/pkg/biz/testhelpers" "github.com/google/uuid" @@ -195,8 +196,8 @@ func (s *staleRevokerTestSuite) createOrgWithThreshold(ctx context.Context, days org, err := s.Organization.CreateWithRandomName(ctx) require.NoError(s.T(), err) - // Need a membership so Update works - _, err = s.Membership.Create(ctx, org.ID, s.user.ID, biz.WithCurrentMembership()) + // Need an admin membership so Update works + _, err = s.Membership.Create(ctx, org.ID, s.user.ID, biz.WithMembershipRole(authz.RoleOwner), biz.WithCurrentMembership()) require.NoError(s.T(), err) org, err = s.Organization.Update(ctx, s.user.ID, org.Name, &biz.OrganizationUpdateOpts{ diff --git a/app/controlplane/pkg/biz/organization.go b/app/controlplane/pkg/biz/organization.go index 83911e403..bde0e071b 100644 --- a/app/controlplane/pkg/biz/organization.go +++ b/app/controlplane/pkg/biz/organization.go @@ -245,6 +245,14 @@ func (uc *OrganizationUseCase) Update(ctx context.Context, userID, orgName strin return nil, NewErrNotFound("membership") } + // These settings are organization-wide security controls, so they require an + // admin membership in the organization being updated. Authorizing against this + // membership, and not against the caller's current role, is what keeps a user + // from tampering with another organization they happen to belong to. + if !membership.Role.IsAdmin() { + return nil, NewErrUnauthorizedStr("only organization admins can update the organization settings") + } + orgUUID, err := uuid.Parse(membership.Org.ID) if err != nil { return nil, NewErrInvalidUUID(err) diff --git a/app/controlplane/pkg/biz/organization_integration_test.go b/app/controlplane/pkg/biz/organization_integration_test.go index 50041a20d..40c9d2191 100644 --- a/app/controlplane/pkg/biz/organization_integration_test.go +++ b/app/controlplane/pkg/biz/organization_integration_test.go @@ -17,6 +17,7 @@ package biz_test import ( "context" + "fmt" "testing" v1 "github.com/chainloop-dev/chainloop/app/controlplane/api/controlplane/v1" @@ -196,6 +197,93 @@ func (s *OrgIntegrationTestSuite) TestUpdate() { }) } +// TestUpdateRequiresAdminMembership verifies that changing organization-wide +// security settings requires an admin/owner membership in the organization +// being updated. Holding any membership is not enough: the settings gated here +// (policy enforcement, allowed policy hostnames, runner env-var capture) are +// security controls for the whole org. +func (s *OrgIntegrationTestSuite) TestUpdateRequiresAdminMembership() { + ctx := context.Background() + + testCases := []struct { + name string + role authz.Role + allowed bool + }{ + {name: "owner can update", role: authz.RoleOwner, allowed: true}, + {name: "admin can update", role: authz.RoleAdmin, allowed: true}, + {name: "viewer cannot update", role: authz.RoleViewer}, + {name: "member cannot update", role: authz.RoleOrgMember}, + {name: "contributor cannot update", role: authz.RoleOrgContributor}, + } + + for _, tc := range testCases { + s.Run(tc.name, func() { + org, err := s.Organization.CreateWithRandomName(ctx) + require.NoError(s.T(), err) + + user, err := s.User.UpsertByEmail(ctx, fmt.Sprintf("%s@test.com", uuid.NewString()), nil) + require.NoError(s.T(), err) + + _, err = s.Membership.Create(ctx, org.ID, user.ID, biz.WithMembershipRole(tc.role)) + require.NoError(s.T(), err) + + got, err := s.Organization.Update(ctx, user.ID, org.Name, &biz.OrganizationUpdateOpts{ + BlockOnPolicyViolation: toPtrBool(false), + }) + + if tc.allowed { + s.NoError(err) + s.False(got.BlockOnPolicyViolation) + return + } + + s.Error(err) + s.True(biz.IsErrUnauthorized(err), "want unauthorized, got %v", err) + s.Nil(got) + }) + } +} + +// TestUpdateCrossOrgTampering is a regression test for CP-N1: a user who is an +// owner of their own organization but only a viewer of a victim organization +// must not be able to change the victim's security settings. The authz +// middleware evaluates the caller's role against the organization in the +// request header, so the biz layer has to authorize against the organization +// actually being updated. +func (s *OrgIntegrationTestSuite) TestUpdateCrossOrgTampering() { + ctx := context.Background() + + victimOrg, err := s.Organization.CreateWithRandomName(ctx) + require.NoError(s.T(), err) + attackerOrg, err := s.Organization.CreateWithRandomName(ctx) + require.NoError(s.T(), err) + + attacker, err := s.User.UpsertByEmail(ctx, "attacker@test.com", nil) + require.NoError(s.T(), err) + + // Owner of their own org, which is what gets them past the authz middleware + _, err = s.Membership.Create(ctx, attackerOrg.ID, attacker.ID, biz.WithMembershipRole(authz.RoleOwner), biz.WithCurrentMembership()) + require.NoError(s.T(), err) + // ...but only a viewer of the victim org + _, err = s.Membership.Create(ctx, victimOrg.ID, attacker.ID, biz.WithMembershipRole(authz.RoleViewer)) + require.NoError(s.T(), err) + + got, err := s.Organization.Update(ctx, attacker.ID, victimOrg.Name, &biz.OrganizationUpdateOpts{ + BlockOnPolicyViolation: toPtrBool(false), + PoliciesAllowedHostnames: []string{"evil.example.com"}, + SkipRunnerEnvVars: toPtrBool(false), + }) + s.Error(err) + s.True(biz.IsErrUnauthorized(err), "want unauthorized, got %v", err) + s.Nil(got) + + // The victim org keeps its settings + victim, err := s.Organization.FindByName(ctx, victimOrg.Name) + s.NoError(err) + s.Empty(victim.PoliciesAllowedHostnames) +} + // We are doing an integration test here because there are some database constraints // and delete cascades that we want to validate that they work too func (s *OrgIntegrationTestSuite) TestDeleteOrg() { @@ -288,7 +376,7 @@ func (s *OrgIntegrationTestSuite) SetupTest() { s.user, err = s.User.UpsertByEmail(ctx, "foo@test.com", nil) assert.NoError(err) - _, err = s.Membership.Create(ctx, s.org.ID, s.user.ID, biz.WithCurrentMembership()) + _, err = s.Membership.Create(ctx, s.org.ID, s.user.ID, biz.WithMembershipRole(authz.RoleOwner), biz.WithCurrentMembership()) assert.NoError(err) // Integration