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