diff --git a/go.mod b/go.mod index 5fb3a2f6..abcf4fb6 100644 --- a/go.mod +++ b/go.mod @@ -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 diff --git a/go.sum b/go.sum index 06deac0b..83df52e6 100644 --- a/go.sum +++ b/go.sum @@ -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= diff --git a/internal/api/handler_accounts.go b/internal/api/handler_accounts.go index bec9a02c..8686bcf1 100644 --- a/internal/api/handler_accounts.go +++ b/internal/api/handler_accounts.go @@ -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"` @@ -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. @@ -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, @@ -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 { @@ -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 diff --git a/internal/api/handler_accounts_update_keep_test.go b/internal/api/handler_accounts_update_keep_test.go new file mode 100644 index 00000000..65c230e3 --- /dev/null +++ b/internal/api/handler_accounts_update_keep_test.go @@ -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") + }) + } +} diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index a7782101..e3850221 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -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: @@ -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] @@ -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: