diff --git a/management/server/http/handlers/instance/instance_handler_test.go b/management/server/http/handlers/instance/instance_handler_test.go index 191fe28e6..711e01964 100644 --- a/management/server/http/handlers/instance/instance_handler_test.go +++ b/management/server/http/handlers/instance/instance_handler_test.go @@ -10,6 +10,7 @@ import ( "net/mail" "testing" + "github.com/golang/mock/gomock" "github.com/gorilla/mux" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -18,6 +19,7 @@ import ( "github.com/netbirdio/netbird/management/server/idp" nbinstance "github.com/netbirdio/netbird/management/server/instance" "github.com/netbirdio/netbird/management/server/mock_server" + nbstore "github.com/netbirdio/netbird/management/server/store" "github.com/netbirdio/netbird/management/server/types" "github.com/netbirdio/netbird/shared/auth" "github.com/netbirdio/netbird/shared/management/http/api" @@ -384,6 +386,7 @@ func TestSetup_PAT_MissingExpireIn_DefaultsToOneDay(t *testing.T) { router.ServeHTTP(rec, req) assert.Equal(t, http.StatusOK, rec.Code) + assert.Equal(t, "no-store", rec.Header().Get("Cache-Control")) assert.True(t, createCalled) var response api.SetupResponse require.NoError(t, json.NewDecoder(rec.Body).Decode(&response)) @@ -454,11 +457,16 @@ func TestSetup_PAT_Success(t *testing.T) { require.NotNil(t, response.PersonalAccessToken) assert.Equal(t, "nbp_plain", *response.PersonalAccessToken) assert.Equal(t, "owner-id", gotAccountArgs.userID) + assert.Equal(t, "admin@example.com", gotAccountArgs.email) } func TestSetup_PAT_AccountCreationFails_Rollback(t *testing.T) { t.Setenv(nbinstance.SetupPATEnabledEnvKey, "true") + ctrl := gomock.NewController(t) + accountStore := nbstore.NewMockStore(ctrl) + accountStore.EXPECT().GetAccountIDByUserID(gomock.Any(), nbstore.LockingStrengthNone, "owner-id").Return("", status.NewAccountNotFoundError("owner-id")) + rolledBackFor := "" manager := &mockInstanceManager{ isSetupRequired: true, @@ -474,6 +482,9 @@ func TestSetup_PAT_AccountCreationFails_Rollback(t *testing.T) { GetAccountIDByUserIdFunc: func(_ context.Context, _ auth.UserAuth) (string, error) { return "", errors.New("db down") }, + GetStoreFunc: func() nbstore.Store { + return accountStore + }, } router := setupTestRouterWithPAT(manager, accountMgr) @@ -492,6 +503,12 @@ func TestSetup_PAT_AccountCreationFails_Rollback(t *testing.T) { func TestSetup_PAT_CreatePATFails_Rollback(t *testing.T) { t.Setenv(nbinstance.SetupPATEnabledEnvKey, "true") + ctrl := gomock.NewController(t) + accountStore := nbstore.NewMockStore(ctrl) + account := &types.Account{Id: "acc-1"} + accountStore.EXPECT().GetAccount(gomock.Any(), "acc-1").Return(account, nil) + accountStore.EXPECT().DeleteAccount(gomock.Any(), account).Return(nil) + rolledBackFor := "" manager := &mockInstanceManager{ isSetupRequired: true, @@ -510,6 +527,9 @@ func TestSetup_PAT_CreatePATFails_Rollback(t *testing.T) { CreatePATFunc: func(_ context.Context, _, _, _, _ string, _ int) (*types.PersonalAccessTokenGenerated, error) { return nil, status.Errorf(status.Internal, "token store unavailable") }, + GetStoreFunc: func() nbstore.Store { + return accountStore + }, } router := setupTestRouterWithPAT(manager, accountMgr) diff --git a/management/server/instance/setup_service.go b/management/server/instance/setup_service.go index d9ece1ffc..92a4923be 100644 --- a/management/server/instance/setup_service.go +++ b/management/server/instance/setup_service.go @@ -83,7 +83,8 @@ func normalizeSetupOptions(opts SetupOptions, setupPATEnabled bool) (SetupOption // SetupOwner creates the initial owner user and, when requested and enabled by // SetupPATEnabledEnvKey, provisions the account and a setup Personal Access // Token. If account or PAT provisioning fails, created resources are rolled -// back so setup can be retried. +// back so setup can be retried. If account rollback fails, user rollback is +// skipped to avoid leaving an account without its owner user. func (m *SetupService) SetupOwner(ctx context.Context, email, password, name string, opts SetupOptions) (*SetupResult, error) { opts, err := normalizeSetupOptions(opts, m.setupPATEnabled) if err != nil { @@ -113,14 +114,18 @@ func (m *SetupService) SetupOwner(ctx context.Context, email, password, name str accountID, err := m.accountManager.GetAccountIDByUserID(ctx, userAuth) if err != nil { err = fmt.Errorf("create account for setup user: %w", err) - m.rollbackSetup(ctx, userData.ID, "account provisioning failed", err, "") + if rollbackErr := m.rollbackSetup(ctx, userData.ID, "account provisioning failed", err, ""); rollbackErr != nil { + return nil, fmt.Errorf("%w; failed to roll back setup resources: %v", err, rollbackErr) + } return nil, err } pat, err := m.accountManager.CreatePAT(ctx, accountID, userData.ID, userData.ID, setupPATTokenName, *opts.PATExpireInDays) if err != nil { err = fmt.Errorf("create setup PAT: %w", err) - m.rollbackSetup(ctx, userData.ID, "setup PAT provisioning failed", err, accountID) + if rollbackErr := m.rollbackSetup(ctx, userData.ID, "setup PAT provisioning failed", err, accountID); rollbackErr != nil { + return nil, fmt.Errorf("%w; failed to roll back setup resources: %v", err, rollbackErr) + } return nil, err } @@ -128,28 +133,33 @@ func (m *SetupService) SetupOwner(ctx context.Context, email, password, name str return result, nil } -func (m *SetupService) rollbackSetup(ctx context.Context, userID, reason string, origErr error, accountID string) { +func (m *SetupService) rollbackSetup(ctx context.Context, userID, reason string, origErr error, accountID string) error { if accountID == "" { resolvedAccountID, err := m.lookupSetupAccountIDForRollback(ctx, userID) if err != nil { - log.WithContext(ctx).Errorf("failed to resolve setup account for user %s after %s: original error: %v, rollback error: %v", userID, reason, origErr, err) + rollbackErr := fmt.Errorf("resolve setup account for rollback: %w", err) + log.WithContext(ctx).Errorf("failed to resolve setup account for user %s after %s: original error: %v, rollback error: %v", userID, reason, origErr, rollbackErr) + return rollbackErr } accountID = resolvedAccountID } if accountID != "" { if err := m.rollbackSetupAccount(ctx, accountID); err != nil { - log.WithContext(ctx).Errorf("failed to roll back setup account %s for user %s after %s: original error: %v, rollback error: %v", accountID, userID, reason, origErr, err) - } else { - log.WithContext(ctx).Warnf("rolled back setup account %s for user %s after %s: %v", accountID, userID, reason, origErr) + rollbackErr := fmt.Errorf("roll back setup account %s: %w", accountID, err) + log.WithContext(ctx).Errorf("failed to roll back setup account %s for user %s after %s: original error: %v, rollback error: %v", accountID, userID, reason, origErr, rollbackErr) + return rollbackErr } + log.WithContext(ctx).Warnf("rolled back setup account %s for user %s after %s: %v", accountID, userID, reason, origErr) } if err := m.instanceManager.RollbackSetup(ctx, userID); err != nil { - log.WithContext(ctx).Errorf("failed to roll back setup user %s after %s: original error: %v, rollback error: %v", userID, reason, origErr, err) - return + rollbackErr := fmt.Errorf("roll back setup user %s: %w", userID, err) + log.WithContext(ctx).Errorf("failed to roll back setup user %s after %s: original error: %v, rollback error: %v", userID, reason, origErr, rollbackErr) + return rollbackErr } log.WithContext(ctx).Warnf("rolled back setup user %s after %s: %v", userID, reason, origErr) + return nil } func (m *SetupService) lookupSetupAccountIDForRollback(ctx context.Context, userID string) (string, error) { diff --git a/management/server/instance/setup_service_test.go b/management/server/instance/setup_service_test.go index d6e5894b8..12ec7d0fa 100644 --- a/management/server/instance/setup_service_test.go +++ b/management/server/instance/setup_service_test.go @@ -153,9 +153,11 @@ func TestSetupOwner_AccountProvisioningFails_RollsBackSideEffectAccountAndUser(t accountStore.EXPECT().DeleteAccount(gomock.Any(), account).Return(nil) rolledBackFor := "" + rollbackCalls := 0 setupManager := NewSetupService( &setupInstanceManagerMock{ rollbackSetupFn: func(_ context.Context, userID string) error { + rollbackCalls++ rolledBackFor = userID return nil }, @@ -180,6 +182,7 @@ func TestSetupOwner_AccountProvisioningFails_RollsBackSideEffectAccountAndUser(t assert.Nil(t, result) assert.Contains(t, err.Error(), "create account for setup user") assert.Equal(t, "owner-id", rolledBackFor) + assert.Equal(t, 1, rollbackCalls) } func TestSetupOwner_CreatePATFails_RollsBackSetupAccountAndUser(t *testing.T) { @@ -238,9 +241,11 @@ func TestSetupOwner_CreatePATFails_AccountAlreadyGoneStillRollsBackUser(t *testi accountStore.EXPECT().GetAccount(gomock.Any(), "acc-1").Return(nil, status.NewAccountNotFoundError("acc-1")) rolledBackFor := "" + rollbackCalls := 0 setupManager := NewSetupService( &setupInstanceManagerMock{ rollbackSetupFn: func(_ context.Context, userID string) error { + rollbackCalls++ rolledBackFor = userID return nil }, @@ -267,9 +272,10 @@ func TestSetupOwner_CreatePATFails_AccountAlreadyGoneStillRollsBackUser(t *testi assert.Nil(t, result) assert.Contains(t, err.Error(), "create setup PAT") assert.Equal(t, "owner-id", rolledBackFor) + assert.Equal(t, 1, rollbackCalls) } -func TestSetupOwner_CreatePATFails_AccountRollbackFailureStillRollsBackUser(t *testing.T) { +func TestSetupOwner_CreatePATFails_AccountRollbackFailureStopsBeforeUserRollback(t *testing.T) { t.Setenv(SetupPATEnabledEnvKey, "true") ctrl := gomock.NewController(t) @@ -278,11 +284,11 @@ func TestSetupOwner_CreatePATFails_AccountRollbackFailureStillRollsBackUser(t *t accountStore.EXPECT().GetAccount(gomock.Any(), "acc-1").Return(account, nil) accountStore.EXPECT().DeleteAccount(gomock.Any(), account).Return(errors.New("delete failed")) - rolledBackFor := "" + rollbackCalls := 0 setupManager := NewSetupService( &setupInstanceManagerMock{ rollbackSetupFn: func(_ context.Context, userID string) error { - rolledBackFor = userID + rollbackCalls++ return nil }, }, @@ -307,5 +313,6 @@ func TestSetupOwner_CreatePATFails_AccountRollbackFailureStillRollsBackUser(t *t require.Error(t, err) assert.Nil(t, result) assert.Contains(t, err.Error(), "create setup PAT") - assert.Equal(t, "owner-id", rolledBackFor) + assert.Contains(t, err.Error(), "failed to roll back setup resources") + assert.Equal(t, 0, rollbackCalls) } diff --git a/shared/management/http/api/openapi.yml b/shared/management/http/api/openapi.yml index 79da0f507..f810e879e 100644 --- a/shared/management/http/api/openapi.yml +++ b/shared/management/http/api/openapi.yml @@ -3455,6 +3455,8 @@ components: personal_access_token: description: Plain text Personal Access Token created during setup. Present only when create_pat was requested and the NB_SETUP_PAT_ENABLED feature was enabled on the server. type: string + format: password + readOnly: true example: nbp_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx required: - user_id @@ -4997,7 +4999,7 @@ paths: description: | Creates the initial admin user for the instance. This endpoint does not require authentication but only works when setup is required (no accounts exist and embedded IDP is enabled). - When the management server is started with `NB_SETUP_PAT_ENABLED=true` and the request includes `create_pat: true`, the endpoint also provisions the NetBird account for the new owner user and returns the plain text Personal Access Token in `personal_access_token`. The optional `pat_expire_in` value applies only when `create_pat` is true and defaults to 1 day when omitted. If any post-user step fails, created setup resources are rolled back and setup remains retryable. + When the management server is started with `NB_SETUP_PAT_ENABLED=true` and the request includes `create_pat: true`, the endpoint also provisions the NetBird account for the new owner user and returns the plain text Personal Access Token in `personal_access_token`. The optional `pat_expire_in` value applies only when `create_pat` is true and defaults to 1 day when omitted. If a post-user step fails, setup-created resources are rolled back when safe; if account cleanup fails, the owner user is left in place to avoid leaving an account without its admin user. tags: [ Instance ] security: [ ] requestBody: