From 097acb01dae0a273a016ffe67aab9b50dff2c66c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 06:54:12 +0100 Subject: [PATCH] fix(registrations): clear the stored credential once a registration is decided A registration's encrypted credential stayed in account_registrations.reg_credential_payload after approval had copied it to the credential store, and after a rejection that never used it. - Approval clears the payload, but only after SaveCredential succeeds. When the save fails or there is nothing to save, the payload is kept, because it is the only copy. - Rejection sets the payload to NULL in the same status UPDATE. Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_registrations.go | 8 ++- .../api/handler_registrations_credentials.go | 10 ++-- .../handler_registrations_credentials_test.go | 40 ++++++++++++++ internal/config/interfaces.go | 3 ++ .../config/store_postgres_registrations.go | 13 +++++ ...postgres_registrations_integration_test.go | 53 +++++++++++++++++++ internal/mocks/stores.go | 6 +++ 7 files changed, 128 insertions(+), 5 deletions(-) diff --git a/internal/api/handler_registrations.go b/internal/api/handler_registrations.go index 1ab2b700d..c92b80deb 100644 --- a/internal/api/handler_registrations.go +++ b/internal/api/handler_registrations.go @@ -307,7 +307,13 @@ func (h *Handler) approveRegistration(ctx context.Context, httpReq *events.Lambd } // Auto-store credentials if the registration included them. - h.storeRegistrationCredentials(ctx, reg, account.ID, regCred) + // The registration's copy is dropped only after the save succeeds; until + // then it is the only copy. The account exists, so a failed clear is logged. + if h.storeRegistrationCredentials(ctx, reg, account.ID, regCred) { + if err := h.config.ClearRegistrationCredentialPayload(ctx, reg.ID); err != nil { + logging.Errorf("registration %s: failed to clear stored credential payload: %v", reg.ID, err) + } + } h.notifyRegistrant(reg, email.RegistrationDecisionData{ AccountName: reg.AccountName, Provider: reg.Provider, diff --git a/internal/api/handler_registrations_credentials.go b/internal/api/handler_registrations_credentials.go index a9bb24ebc..630ab081a 100644 --- a/internal/api/handler_registrations_credentials.go +++ b/internal/api/handler_registrations_credentials.go @@ -91,12 +91,14 @@ func (h *Handler) registrationCredential(reg *config.AccountRegistration, provid // storeRegistrationCredentials saves a registration credential that // registrationCredential has checked into the account_credentials table. // The account already exists at this point, so a store error is logged, not -// returned. -func (h *Handler) storeRegistrationCredentials(ctx context.Context, reg *config.AccountRegistration, accountID string, payload []byte) { +// returned. It reports whether the credential was saved. +func (h *Handler) storeRegistrationCredentials(ctx context.Context, reg *config.AccountRegistration, accountID string, payload []byte) bool { if payload == nil || h.credStore == nil { - return + return false } if err := h.credStore.SaveCredential(ctx, accountID, reg.RegCredentialType, payload); err != nil { - logging.Warnf("registration %s: failed to store credential: %v", reg.ID, err) + logging.Errorf("registration %s: failed to store credential: %v", reg.ID, err) + return false } + return true } diff --git a/internal/api/handler_registrations_credentials_test.go b/internal/api/handler_registrations_credentials_test.go index c810153a4..52ce40a29 100644 --- a/internal/api/handler_registrations_credentials_test.go +++ b/internal/api/handler_registrations_credentials_test.go @@ -15,6 +15,7 @@ import ( "github.com/stretchr/testify/require" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/credentials" ) const regCredRegID = "6f1c2a8e-3b4d-4e5f-8a9b-0c1d2e3f4a5b" @@ -166,6 +167,7 @@ func approveWithCredential(t *testing.T, credType, payload, approvalBody string) store.On("ApproveAccountRegistration", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() store.On("TransitionRegistrationStatus", mock.Anything, mock.Anything, "pending", mock.Anything).Return(nil).Maybe() store.On("UpdateAccountRegistration", mock.Anything, mock.Anything).Return(nil).Maybe() + store.On("ClearRegistrationCredentialPayload", mock.Anything, mock.Anything).Return(nil).Maybe() // The router enforces the approve permission; called directly with no // bearer token, approveRegistration resolves no session, so the auth mock // carries no expectations (an unexpected auth call would panic). @@ -227,3 +229,41 @@ func TestApproveRegistration_RefusesInvalidStoredCredential(t *testing.T) { }) } } + +// failingCredStore fails every SaveCredential call. +type failingCredStore struct{ MockCredentialStore } + +func (failingCredStore) SaveCredential(context.Context, string, string, []byte) error { + return assert.AnError +} + +func approveWithCredStore(t *testing.T, creds credentials.CredentialStore, payload string) *MockConfigStore { + t.Helper() + store := new(MockConfigStore) + store.On("GetAccountRegistration", mock.Anything, regCredRegID).Return(&config.AccountRegistration{ + ID: regCredRegID, Status: "pending", Provider: "gcp", ExternalID: "test-project", + RegCredentialType: "gcp_workload_identity_config", RegCredentialPayload: payload, + }, nil) + store.On("ApproveAccountRegistration", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) + store.On("ClearRegistrationCredentialPayload", mock.Anything, regCredRegID).Return(nil).Maybe() + h := &Handler{auth: new(MockAuthService), config: store, credStore: creds} + _, err := h.approveRegistration(context.Background(), &events.LambdaFunctionURLRequest{Body: gcpApprovalBody}, regCredRegID) + require.NoError(t, err) + return store +} + +func TestApproveRegistration_ClearsPayloadAfterSave(t *testing.T) { + store := approveWithCredStore(t, &recordingCredStore{}, mustJSON(t, gcloudWIFConfig())) + store.AssertNumberOfCalls(t, "ClearRegistrationCredentialPayload", 1) + store.AssertCalled(t, "ClearRegistrationCredentialPayload", mock.Anything, regCredRegID) +} + +func TestApproveRegistration_KeepsPayloadWhenSaveFails(t *testing.T) { + store := approveWithCredStore(t, &failingCredStore{}, mustJSON(t, gcloudWIFConfig())) + store.AssertNotCalled(t, "ClearRegistrationCredentialPayload", mock.Anything, mock.Anything) +} + +func TestApproveRegistration_KeepsPayloadWhenNothingSaved(t *testing.T) { + store := approveWithCredStore(t, &recordingCredStore{}, "") + store.AssertNotCalled(t, "ClearRegistrationCredentialPayload", mock.Anything, mock.Anything) +} diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index 14ca8b13c..75363819f 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -780,6 +780,9 @@ type StoreInterface interface { // TransitionRegistrationStatus atomically updates a registration's workflow fields. // actor is the UUID of the reviewer (nil for system-initiated transitions). TransitionRegistrationStatus(ctx context.Context, reg *AccountRegistration, fromStatus string, actor *string) error + // ClearRegistrationCredentialPayload removes the encrypted credential kept + // with a registration. A missing id is not an error. + ClearRegistrationCredentialPayload(ctx context.Context, registrationID string) error // ApproveAccountRegistration atomically approves a pending registration, // creates its cloud account and links them. Returns ErrRegistrationConflict // when the registration is no longer pending; on any error nothing is saved. diff --git a/internal/config/store_postgres_registrations.go b/internal/config/store_postgres_registrations.go index 2bbbc01da..77027c608 100644 --- a/internal/config/store_postgres_registrations.go +++ b/internal/config/store_postgres_registrations.go @@ -219,6 +219,7 @@ func (s *PostgresStore) TransitionRegistrationStatus(ctx context.Context, reg *A reviewed_by = $5, reviewed_at = $6, updated_at = $7, + reg_credential_payload = CASE WHEN $10 THEN NULL ELSE reg_credential_payload END, transitioned_by = $9, transitioned_at = NOW() WHERE id = $1 AND status = $8 @@ -239,6 +240,7 @@ func (s *PostgresStore) TransitionRegistrationStatus(ctx context.Context, reg *A reg.UpdatedAt, fromStatus, actor, + reg.Status == "rejected", ) if err != nil { return fmt.Errorf("failed to transition account registration status: %w", err) @@ -249,6 +251,17 @@ func (s *PostgresStore) TransitionRegistrationStatus(ctx context.Context, reg *A return nil } +// ClearRegistrationCredentialPayload removes the encrypted credential kept with +// a registration once it has been saved elsewhere. A missing id affects no rows +// and is not an error. +func (s *PostgresStore) ClearRegistrationCredentialPayload(ctx context.Context, registrationID string) error { + _, err := s.db.Exec(ctx, `UPDATE account_registrations SET reg_credential_payload = NULL WHERE id = $1`, registrationID) + if err != nil { + return fmt.Errorf("failed to clear registration credential payload: %w", err) + } + return nil +} + // ApproveAccountRegistration moves a pending registration to approved, creates // its cloud account and links the two, all in one transaction. If any step // fails nothing is written, so a registration is never left approved with no diff --git a/internal/config/store_postgres_registrations_integration_test.go b/internal/config/store_postgres_registrations_integration_test.go index ad45f929f..0ddec7b56 100644 --- a/internal/config/store_postgres_registrations_integration_test.go +++ b/internal/config/store_postgres_registrations_integration_test.go @@ -65,3 +65,56 @@ func TestCreateAccountRegistration_UniquenessPostgres(t *testing.T) { require.NoError(t, err) require.NoError(t, store.CreateAccountRegistration(ctx, newPendingRegistration("tok-137-d", "111111111111"))) } + +func registrationPayload(t *testing.T, store *PostgresStore, id string) *string { + t.Helper() + var payload *string + require.NoError(t, store.db.QueryRow(context.Background(), + `SELECT reg_credential_payload FROM account_registrations WHERE id = $1`, id).Scan(&payload)) + return payload +} + +func newRegistrationWithPayload(token, externalID string) *AccountRegistration { + reg := newPendingRegistration(token, externalID) + reg.RegCredentialType = "gcp_workload_identity_config" + reg.RegCredentialPayload = "encrypted-blob" + return reg +} + +func TestRegistrationCredentialPayload_ClearedAfterApprovalPostgres(t *testing.T) { + ctx := context.Background() + container := testhelpers.RequireMigratedDatabase(ctx, t, getTestMigrationsPath()) + store := NewPostgresStore(container.DB) + + reg := newRegistrationWithPayload("tok-clear-a", "111111111111") + require.NoError(t, store.CreateAccountRegistration(ctx, reg)) + require.NotNil(t, registrationPayload(t, store, reg.ID)) + + account := &CloudAccount{Name: "Prod", Provider: "aws", ExternalID: "111111111111", Enabled: true} + require.NoError(t, store.ApproveAccountRegistration(ctx, reg, account, nil)) + assert.NotNil(t, registrationPayload(t, store, reg.ID), "approval alone keeps the payload") + + require.NoError(t, store.ClearRegistrationCredentialPayload(ctx, reg.ID)) + assert.Nil(t, registrationPayload(t, store, reg.ID)) +} + +func TestRegistrationCredentialPayload_ClearedOnRejectionPostgres(t *testing.T) { + ctx := context.Background() + container := testhelpers.RequireMigratedDatabase(ctx, t, getTestMigrationsPath()) + store := NewPostgresStore(container.DB) + + reg := newRegistrationWithPayload("tok-clear-b", "222222222222") + require.NoError(t, store.CreateAccountRegistration(ctx, reg)) + + reg.Status = "rejected" + require.NoError(t, store.TransitionRegistrationStatus(ctx, reg, "pending", nil)) + assert.Nil(t, registrationPayload(t, store, reg.ID)) +} + +func TestClearRegistrationCredentialPayload_MissingIDIsNoOpPostgres(t *testing.T) { + ctx := context.Background() + container := testhelpers.RequireMigratedDatabase(ctx, t, getTestMigrationsPath()) + store := NewPostgresStore(container.DB) + + require.NoError(t, store.ClearRegistrationCredentialPayload(ctx, "00000000-0000-0000-0000-000000000000")) +} diff --git a/internal/mocks/stores.go b/internal/mocks/stores.go index 16cd49f26..3664f30f6 100644 --- a/internal/mocks/stores.go +++ b/internal/mocks/stores.go @@ -2144,6 +2144,12 @@ func (m *MockConfigStore) TransitionRegistrationStatus(ctx context.Context, reg return args.Error(0) } +func (m *MockConfigStore) ClearRegistrationCredentialPayload(ctx context.Context, registrationID string) error { + m.record("ClearRegistrationCredentialPayload", ctx, registrationID) + args := m.Called(ctx, registrationID) + return args.Error(0) +} + func (m *MockConfigStore) ApproveAccountRegistration(ctx context.Context, reg *config.AccountRegistration, account *config.CloudAccount, actor *string) error { m.record("ApproveAccountRegistration", ctx, reg, account, actor) args := m.Called(ctx, reg, account, actor)