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
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ require (
go.opentelemetry.io/otel/metric v1.45.0 // indirect
go.opentelemetry.io/otel/trace v1.45.0 // indirect
golang.org/x/crypto v0.58.0
golang.org/x/net v0.61.0 // indirect
golang.org/x/net v0.62.0 // indirect
golang.org/x/oauth2 v0.37.0
golang.org/x/sync v0.24.0
golang.org/x/sys v0.49.0 // indirect
Expand Down
4 changes: 2 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -367,8 +367,8 @@ golang.org/x/crypto v0.58.0 h1:COkYLr4k7nDI4r6QtMML2AfzeIswWHAJthLS6K/M/54=
golang.org/x/crypto v0.58.0/go.mod h1:Xh+kl5A+M0gMiWDytH2Hhr5Z7WbNCpv/eB/EFgfSk+w=
golang.org/x/mod v0.42.0 h1:ICeL3t2L30ljSPHHxDnR4fqVcBt/zR2zJAb9juavmQg=
golang.org/x/mod v0.42.0/go.mod h1:u5fzIrRInkL8MaO1cbRkUHT64v303YIT5KEqpNr7jRs=
golang.org/x/net v0.61.0 h1:RR+6j/BTBCrggOLugxa+66R8FvoN6MHlzDlZIbN9YFw=
golang.org/x/net v0.61.0/go.mod h1:WgDCOTH8iwtB66T4/GfI94AHo/hhdb7Hah9NDseQfiI=
golang.org/x/net v0.62.0 h1:18ELalfM5+XNRwzM2nYqXsZQ9ByS1uhj2O/U7+5jqRY=
golang.org/x/net v0.62.0/go.mod h1:WgDCOTH8iwtB66T4/GfI94AHo/hhdb7Hah9NDseQfiI=
golang.org/x/oauth2 v0.37.0 h1:JUlcxA8oAtauLfiH8FX2/FkAWHAdi0QtGCGc+hofE98=
golang.org/x/oauth2 v0.37.0/go.mod h1:IxwZNxUULJmpBFf9K/9NTMSIfZZuvuTy1gGxhigP/58=
golang.org/x/sync v0.24.0 h1:iqiDrGoDgyuvAKrhLQthUEzU/R378N78gXbBjxed7oA=
Expand Down
24 changes: 22 additions & 2 deletions internal/api/handler_accounts.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,12 @@ import (
)

// CloudAccountRequest is the request body for create/update account endpoints.
// Enabled and AWSIsOrgRoot are pointers so an omitted (or null) key can be
// told apart from an explicit false: create treats absent as false, update
// keeps the stored value (issue #151, A02-013).
type CloudAccountRequest struct {
Enabled *bool `json:"enabled"`
AWSIsOrgRoot *bool `json:"aws_is_org_root"`
AWSWebIdentityTokenFile string `json:"aws_web_identity_token_file"`
AzureClientID string `json:"azure_client_id"`
Provider string `json:"provider"`
Expand All @@ -46,7 +50,6 @@ type CloudAccountRequest struct {
GCPProjectID string `json:"gcp_project_id"`
GCPClientEmail string `json:"gcp_client_email"`
GCPAuthMode string `json:"gcp_auth_mode"`
AWSIsOrgRoot bool `json:"aws_is_org_root"`
}

// CredentialsRequest is the request body for the save-credentials endpoint.
Expand Down Expand Up @@ -544,7 +547,6 @@ func cloudAccountFromRequest(req CloudAccountRequest) *config.CloudAccount {
AWSExternalID: req.AWSExternalID,
AWSBastionID: req.AWSBastionID,
AWSWebIdentityTokenFile: req.AWSWebIdentityTokenFile,
AWSIsOrgRoot: req.AWSIsOrgRoot,
AzureSubscriptionID: req.AzureSubscriptionID,
AzureTenantID: req.AzureTenantID,
AzureClientID: req.AzureClientID,
Expand All @@ -558,10 +560,27 @@ func cloudAccountFromRequest(req CloudAccountRequest) *config.CloudAccount {
if req.Enabled != nil {
a.Enabled = *req.Enabled
}
if req.AWSIsOrgRoot != nil {
a.AWSIsOrgRoot = *req.AWSIsOrgRoot
}

return a
}

// keepOmittedAccountFlags copies the stored enabled and aws_is_org_root
// values onto an update built by cloudAccountFromRequest when the request
// omitted them (or sent null), so a PUT that only renames an account or
// rotates its role does not silently disable it or clear its org-root flag.
// An explicit false in the request is still applied.
func keepOmittedAccountFlags(account, existing *config.CloudAccount, req CloudAccountRequest) {
if req.Enabled == nil {
account.Enabled = existing.Enabled
}
if req.AWSIsOrgRoot == nil {
account.AWSIsOrgRoot = existing.AWSIsOrgRoot
}
}

// getAccount handles GET /api/accounts/:id.
func (h *Handler) getAccount(ctx context.Context, req *events.LambdaFunctionURLRequest, id string) (any, error) {
if err := validateUUID(id); err != nil {
Expand Down Expand Up @@ -615,6 +634,7 @@ func (h *Handler) updateAccount(ctx context.Context, httpReq *events.LambdaFunct
}

account := cloudAccountFromRequest(req)
keepOmittedAccountFlags(account, existing, req)
account.ID = id
account.CreatedAt = existing.CreatedAt
account.CreatedBy = existing.CreatedBy
Expand Down
136 changes: 136 additions & 0 deletions internal/api/handler_accounts_update_keep_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
package api

import (
"context"
"testing"

"github.com/LeanerCloud/cloud-commitments-platform/internal/config"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

// Issue #151 (audit finding A02-013): PUT /api/accounts/{id} built the stored
// row from the request alone, so a body without an `enabled` or
// `aws_is_org_root` key silently disabled the account and cleared its
// org-root flag. An omitted (or null) key now keeps the stored value, while an
// explicit false still clears it. Create keeps its defaults (both false).

// accountWriteCapture records the account handed to the store so the tests
// can assert what would be persisted, not just what the handler returns.
type accountWriteCapture struct {
*mockConfigStoreAccounts
updated *config.CloudAccount
created *config.CloudAccount
}

func (m *accountWriteCapture) UpdateCloudAccount(_ context.Context, a *config.CloudAccount) error {
cp := *a
m.updated = &cp
return nil
}

func (m *accountWriteCapture) CreateCloudAccount(_ context.Context, a *config.CloudAccount) error {
cp := *a
m.created = &cp
return nil
}

func newAccountWriteCapture(ctx context.Context, existing *config.CloudAccount) *accountWriteCapture {
return &accountWriteCapture{
mockConfigStoreAccounts: &mockConfigStoreAccounts{
MockConfigStore: setupAdminMock(ctx),
getResult: existing,
},
}
}

// putAccountFlags sends PUT /api/accounts/{id} with the given trailing JSON
// members (empty for none) against an existing row with the given flags and
// returns the row the handler wrote to the store.
func putAccountFlags(t *testing.T, existingEnabled, existingOrgRoot bool, extra string) *config.CloudAccount {
t.Helper()
ctx := context.Background()
mockAuth := new(MockAuthService)
setupAdminAuth(ctx, mockAuth)

existing := sampleAccount()
existing.Enabled = existingEnabled
existing.AWSIsOrgRoot = existingOrgRoot
store := newAccountWriteCapture(ctx, &existing)
handler := &Handler{auth: mockAuth, config: store}

body := `{"name":"Renamed","provider":"aws","external_id":"123456789012"` + extra + `}`
result, err := handler.updateAccount(ctx, adminRequest(body), existing.ID)
require.NoError(t, err)
require.NotNil(t, store.updated, "updateAccount must write the account")

returned := result.(*config.CloudAccount)
assert.Equal(t, store.updated.Enabled, returned.Enabled, "response must match the stored enabled")
assert.Equal(t, store.updated.AWSIsOrgRoot, returned.AWSIsOrgRoot, "response must match the stored aws_is_org_root")
assert.Equal(t, "Renamed", store.updated.Name, "the rest of the body is still applied")
return store.updated
}

func TestUpdateAccount_OmittedFlagsKeepStoredValues(t *testing.T) {
cases := []struct {
name string
extra string
existingEnabled bool
existingOrgRoot bool
wantEnabled bool
wantOrgRoot bool
}{
{name: "omitted keeps true", existingEnabled: true, existingOrgRoot: true, wantEnabled: true, wantOrgRoot: true},
{name: "omitted keeps false", existingEnabled: false, existingOrgRoot: false, wantEnabled: false, wantOrgRoot: false},
{name: "null keeps true", extra: `,"enabled":null,"aws_is_org_root":null`, existingEnabled: true, existingOrgRoot: true, wantEnabled: true, wantOrgRoot: true},
{name: "explicit false clears", extra: `,"enabled":false,"aws_is_org_root":false`, existingEnabled: true, existingOrgRoot: true, wantEnabled: false, wantOrgRoot: false},
{name: "explicit true sets", extra: `,"enabled":true,"aws_is_org_root":true`, existingEnabled: false, existingOrgRoot: false, wantEnabled: true, wantOrgRoot: true},
{name: "enabled omitted, org root explicit false", extra: `,"aws_is_org_root":false`, existingEnabled: true, existingOrgRoot: true, wantEnabled: true, wantOrgRoot: false},
{name: "enabled explicit false, org root omitted", extra: `,"enabled":false`, existingEnabled: true, existingOrgRoot: true, wantEnabled: false, wantOrgRoot: true},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
stored := putAccountFlags(t, tc.existingEnabled, tc.existingOrgRoot, tc.extra)
assert.Equal(t, tc.wantEnabled, stored.Enabled, "enabled")
assert.Equal(t, tc.wantOrgRoot, stored.AWSIsOrgRoot, "aws_is_org_root")
})
}
}

// The issue's verification rule: a real false and a genuine absence go through
// the same path and must produce different outcomes.
func TestUpdateAccount_ExplicitFalseAndOmittedDiverge(t *testing.T) {
omitted := putAccountFlags(t, true, true, "")
explicit := putAccountFlags(t, true, true, `,"enabled":false,"aws_is_org_root":false`)
assert.NotEqual(t, omitted.Enabled, explicit.Enabled, "enabled: omitted and explicit false must differ")
assert.NotEqual(t, omitted.AWSIsOrgRoot, explicit.AWSIsOrgRoot, "aws_is_org_root: omitted and explicit false must differ")
}

func TestCreateAccount_FlagDefaultsUnchanged(t *testing.T) {
cases := []struct {
name string
extra string
wantEnabled bool
wantOrgRoot bool
}{
{name: "omitted defaults to false"},
{name: "null defaults to false", extra: `,"enabled":null,"aws_is_org_root":null`},
{name: "explicit true", extra: `,"enabled":true,"aws_is_org_root":true`, wantEnabled: true, wantOrgRoot: true},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
ctx := context.Background()
mockAuth := new(MockAuthService)
setupAdminAuth(ctx, mockAuth)
store := newAccountWriteCapture(ctx, nil)
handler := &Handler{auth: mockAuth, config: store}

body := `{"name":"Acme","provider":"aws","external_id":"123456789012"` + tc.extra + `}`
_, err := handler.createAccount(ctx, adminRequest(body))
require.NoError(t, err)
require.NotNil(t, store.created)
assert.Equal(t, tc.wantEnabled, store.created.Enabled, "enabled")
assert.Equal(t, tc.wantOrgRoot, store.created.AWSIsOrgRoot, "aws_is_org_root")
})
}
}
18 changes: 14 additions & 4 deletions internal/api/openapi.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -4450,9 +4450,10 @@ paths:
summary: Replace a cloud account (admin only)
description: >
Replaces the account's configuration with the request body; fields
omitted from the body are cleared, and enabled defaults to false.
The provider is validated but not changed. Stored credentials are
not affected.
omitted from the body are cleared, except enabled and
aws_is_org_root, which keep their stored values when omitted or
null (send false to clear them). The provider is validated but not
changed. Stored credentials are not affected.
parameters:
- $ref: '#/components/parameters/CSRFToken'
requestBody:
Expand Down Expand Up @@ -7731,7 +7732,11 @@ components:
format: email
enabled:
type: boolean
description: Defaults to false when omitted.
nullable: true
description: >
On create, defaults to false when omitted or null. On update,
omitted or null keeps the stored value; an explicit false
disables the account.
aws_auth_mode:
type: string
enum: [access_keys, role_arn, bastion, workload_identity_federation]
Expand All @@ -7750,6 +7755,11 @@ components:
contain "..".
aws_is_org_root:
type: boolean
nullable: true
description: >
On create, defaults to false when omitted or null. On update,
omitted or null keeps the stored value; an explicit false clears
the flag.
azure_subscription_id:
type: string
azure_tenant_id:
Expand Down
Loading