Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion app/controlplane/internal/service/organization.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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,
Expand Down
72 changes: 72 additions & 0 deletions app/controlplane/internal/service/organization_test.go
Original file line number Diff line number Diff line change
@@ -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
}
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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{
Expand Down
8 changes: 8 additions & 0 deletions app/controlplane/pkg/biz/organization.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
90 changes: 89 additions & 1 deletion app/controlplane/pkg/biz/organization_integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package biz_test

import (
"context"
"fmt"
"testing"

v1 "github.com/chainloop-dev/chainloop/app/controlplane/api/controlplane/v1"
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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
Expand Down
Loading