[management] fix race condition in the setup flow that enables creation of multiple owner users (#5754)

management/server/instance/manager.go

@@ -64,10 +64,19 @@ type Manager interface {
 	GetVersionInfo(ctx context.Context) (*VersionInfo, error)
 }
 
+type instanceStore interface {
+	GetAccountsCounter(ctx context.Context) (int64, error)
+}
+
+type embeddedIdP interface {
+	CreateUserWithPassword(ctx context.Context, email, password, name string) (*idp.UserData, error)
+	GetAllAccounts(ctx context.Context) (map[string][]*idp.UserData, error)
+}
+
 // DefaultManager is the default implementation of Manager.
 type DefaultManager struct {
-	store              store.Store
+	store              instanceStore
-	embeddedIdpManager *idp.EmbeddedIdPManager
+	embeddedIdpManager embeddedIdP
 
 	setupRequired bool
 	setupMu       sync.RWMutex
@@ -82,18 +91,18 @@ type DefaultManager struct {
 // NewManager creates a new instance manager.
 // If idpManager is not an EmbeddedIdPManager, setup-related operations will return appropriate defaults.
 func NewManager(ctx context.Context, store store.Store, idpManager idp.Manager) (Manager, error) {
-	embeddedIdp, _ := idpManager.(*idp.EmbeddedIdPManager)
+	embeddedIdp, ok := idpManager.(*idp.EmbeddedIdPManager)
 
 	m := &DefaultManager{
 		store:         store,
-		embeddedIdpManager: embeddedIdp,
 		setupRequired: false,
 		httpClient: &http.Client{
 			Timeout: httpTimeout,
 		},
 	}
 
-	if embeddedIdp != nil {
+	if ok && embeddedIdp != nil {
+		m.embeddedIdpManager = embeddedIdp
 		err := m.loadSetupRequired(ctx)
 		if err != nil {
 			return nil, err
@@ -143,36 +152,61 @@ func (m *DefaultManager) IsSetupRequired(_ context.Context) (bool, error) {
 // CreateOwnerUser creates the initial owner user in the embedded IDP.
 func (m *DefaultManager) CreateOwnerUser(ctx context.Context, email, password, name string) (*idp.UserData, error) {
 
-	if err := m.validateSetupInfo(email, password, name); err != nil {
-		return nil, err
-	}
-
 	if m.embeddedIdpManager == nil {
 		return nil, errors.New("embedded IDP is not enabled")
 	}
 
-	m.setupMu.RLock()
+	if err := m.validateSetupInfo(email, password, name); err != nil {
-	setupRequired := m.setupRequired
+		return nil, err
-	m.setupMu.RUnlock()
+	}
 
-	if !setupRequired {
+	m.setupMu.Lock()
+	defer m.setupMu.Unlock()
+
+	if !m.setupRequired {
 		return nil, status.Errorf(status.PreconditionFailed, "setup already completed")
 	}
 
+	if err := m.checkSetupRequiredFromDB(ctx); err != nil {
+		var sErr *status.Error
+		if errors.As(err, &sErr) && sErr.Type() == status.PreconditionFailed {
+			m.setupRequired = false
+		}
+		return nil, err
+	}
+
 	userData, err := m.embeddedIdpManager.CreateUserWithPassword(ctx, email, password, name)
 	if err != nil {
 		return nil, fmt.Errorf("failed to create user in embedded IdP: %w", err)
 	}
 
-	m.setupMu.Lock()
 	m.setupRequired = false
-	m.setupMu.Unlock()
 
 	log.WithContext(ctx).Infof("created owner user %s in embedded IdP", email)
 
 	return userData, nil
 }
 
+func (m *DefaultManager) checkSetupRequiredFromDB(ctx context.Context) error {
+	numAccounts, err := m.store.GetAccountsCounter(ctx)
+	if err != nil {
+		return fmt.Errorf("failed to check accounts: %w", err)
+	}
+	if numAccounts > 0 {
+		return status.Errorf(status.PreconditionFailed, "setup already completed")
+	}
+
+	users, err := m.embeddedIdpManager.GetAllAccounts(ctx)
+	if err != nil {
+		return fmt.Errorf("failed to check IdP users: %w", err)
+	}
+	if len(users) > 0 {
+		return status.Errorf(status.PreconditionFailed, "setup already completed")
+	}
+
+	return nil
+}
+
 func (m *DefaultManager) validateSetupInfo(email, password, name string) error {
 	if email == "" {
 		return status.Errorf(status.InvalidArgument, "email is required")
@@ -189,6 +223,9 @@ func (m *DefaultManager) validateSetupInfo(email, password, name string) error {
 	if len(password) < 8 {
 		return status.Errorf(status.InvalidArgument, "password must be at least 8 characters")
 	}
+	if len(password) > 72 {
+		return status.Errorf(status.InvalidArgument, "password must be at most 72 characters")
+	}
 	return nil
 }
 

management/server/instance/manager_test.go

@@ -3,7 +3,12 @@ package instance
 import (
 	"context"
 	"errors"
+	"fmt"
+	"net/http"
+	"sync"
+	"sync/atomic"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -11,173 +16,215 @@ import (
 	"github.com/netbirdio/netbird/management/server/idp"
 )
 
-// mockStore implements a minimal store.Store for testing
+type mockIdP struct {
+	mu              sync.Mutex
+	createUserFunc  func(ctx context.Context, email, password, name string) (*idp.UserData, error)
+	users           map[string][]*idp.UserData
+	getAllAccountsErr error
+}
+
+func (m *mockIdP) CreateUserWithPassword(ctx context.Context, email, password, name string) (*idp.UserData, error) {
+	if m.createUserFunc != nil {
+		return m.createUserFunc(ctx, email, password, name)
+	}
+	return &idp.UserData{ID: "test-user-id", Email: email, Name: name}, nil
+}
+
+func (m *mockIdP) GetAllAccounts(_ context.Context) (map[string][]*idp.UserData, error) {
+	m.mu.Lock()
+	defer m.mu.Unlock()
+	if m.getAllAccountsErr != nil {
+		return nil, m.getAllAccountsErr
+	}
+	return m.users, nil
+}
+
 type mockStore struct {
 	accountsCount int64
 	err           error
 }
 
-func (m *mockStore) GetAccountsCounter(ctx context.Context) (int64, error) {
+func (m *mockStore) GetAccountsCounter(_ context.Context) (int64, error) {
 	if m.err != nil {
 		return 0, m.err
 	}
 	return m.accountsCount, nil
 }
 
-// mockEmbeddedIdPManager wraps the real EmbeddedIdPManager for testing
+func newTestManager(idpMock *mockIdP, storeMock *mockStore) *DefaultManager {
-type mockEmbeddedIdPManager struct {
+	return &DefaultManager{
-	createUserFunc func(ctx context.Context, email, password, name string) (*idp.UserData, error)
+		store:              storeMock,
-}
+		embeddedIdpManager: idpMock,
-
+		setupRequired:      true,
-func (m *mockEmbeddedIdPManager) CreateUserWithPassword(ctx context.Context, email, password, name string) (*idp.UserData, error) {
+		httpClient:         &http.Client{Timeout: httpTimeout},
-	if m.createUserFunc != nil {
-		return m.createUserFunc(ctx, email, password, name)
 	}
-	return &idp.UserData{
-		ID:    "test-user-id",
-		Email: email,
-		Name:  name,
-	}, nil
-}
-
-// testManager is a test implementation that accepts our mock types
-type testManager struct {
-	store              *mockStore
-	embeddedIdpManager *mockEmbeddedIdPManager
-}
-
-func (m *testManager) IsSetupRequired(ctx context.Context) (bool, error) {
-	if m.embeddedIdpManager == nil {
-		return false, nil
-	}
-
-	count, err := m.store.GetAccountsCounter(ctx)
-	if err != nil {
-		return false, err
-	}
-
-	return count == 0, nil
-}
-
-func (m *testManager) CreateOwnerUser(ctx context.Context, email, password, name string) (*idp.UserData, error) {
-	if m.embeddedIdpManager == nil {
-		return nil, errors.New("embedded IDP is not enabled")
-	}
-
-	return m.embeddedIdpManager.CreateUserWithPassword(ctx, email, password, name)
-}
-
-func TestIsSetupRequired_EmbeddedIdPDisabled(t *testing.T) {
-	manager := &testManager{
-		store:              &mockStore{accountsCount: 0},
-		embeddedIdpManager: nil, // No embedded IDP
-	}
-
-	required, err := manager.IsSetupRequired(context.Background())
-	require.NoError(t, err)
-	assert.False(t, required, "setup should not be required when embedded IDP is disabled")
-}
-
-func TestIsSetupRequired_NoAccounts(t *testing.T) {
-	manager := &testManager{
-		store:              &mockStore{accountsCount: 0},
-		embeddedIdpManager: &mockEmbeddedIdPManager{},
-	}
-
-	required, err := manager.IsSetupRequired(context.Background())
-	require.NoError(t, err)
-	assert.True(t, required, "setup should be required when no accounts exist")
-}
-
-func TestIsSetupRequired_AccountsExist(t *testing.T) {
-	manager := &testManager{
-		store:              &mockStore{accountsCount: 1},
-		embeddedIdpManager: &mockEmbeddedIdPManager{},
-	}
-
-	required, err := manager.IsSetupRequired(context.Background())
-	require.NoError(t, err)
-	assert.False(t, required, "setup should not be required when accounts exist")
-}
-
-func TestIsSetupRequired_MultipleAccounts(t *testing.T) {
-	manager := &testManager{
-		store:              &mockStore{accountsCount: 5},
-		embeddedIdpManager: &mockEmbeddedIdPManager{},
-	}
-
-	required, err := manager.IsSetupRequired(context.Background())
-	require.NoError(t, err)
-	assert.False(t, required, "setup should not be required when multiple accounts exist")
-}
-
-func TestIsSetupRequired_StoreError(t *testing.T) {
-	manager := &testManager{
-		store:              &mockStore{err: errors.New("database error")},
-		embeddedIdpManager: &mockEmbeddedIdPManager{},
-	}
-
-	_, err := manager.IsSetupRequired(context.Background())
-	assert.Error(t, err, "should return error when store fails")
 }
 
 func TestCreateOwnerUser_Success(t *testing.T) {
-	expectedEmail := "admin@example.com"
+	idpMock := &mockIdP{}
-	expectedName := "Admin User"
+	mgr := newTestManager(idpMock, &mockStore{})
-	expectedPassword := "securepassword123"
 
-	manager := &testManager{
+	userData, err := mgr.CreateOwnerUser(context.Background(), "admin@example.com", "password123", "Admin")
-		store: &mockStore{accountsCount: 0},
-		embeddedIdpManager: &mockEmbeddedIdPManager{
-			createUserFunc: func(ctx context.Context, email, password, name string) (*idp.UserData, error) {
-				assert.Equal(t, expectedEmail, email)
-				assert.Equal(t, expectedPassword, password)
-				assert.Equal(t, expectedName, name)
-				return &idp.UserData{
-					ID:    "created-user-id",
-					Email: email,
-					Name:  name,
-				}, nil
-			},
-		},
-	}
-
-	userData, err := manager.CreateOwnerUser(context.Background(), expectedEmail, expectedPassword, expectedName)
 	require.NoError(t, err)
-	assert.Equal(t, "created-user-id", userData.ID)
+	assert.Equal(t, "admin@example.com", userData.Email)
-	assert.Equal(t, expectedEmail, userData.Email)
+
-	assert.Equal(t, expectedName, userData.Name)
+	_, err = mgr.CreateOwnerUser(context.Background(), "admin2@example.com", "password123", "Admin2")
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "setup already completed")
+}
+
+func TestCreateOwnerUser_SetupAlreadyCompleted(t *testing.T) {
+	mgr := newTestManager(&mockIdP{}, &mockStore{})
+	mgr.setupRequired = false
+
+	_, err := mgr.CreateOwnerUser(context.Background(), "admin@example.com", "password123", "Admin")
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "setup already completed")
 }
 
 func TestCreateOwnerUser_EmbeddedIdPDisabled(t *testing.T) {
-	manager := &testManager{
+	mgr := &DefaultManager{setupRequired: true}
-		store:              &mockStore{accountsCount: 0},
-		embeddedIdpManager: nil,
-	}
 
-	_, err := manager.CreateOwnerUser(context.Background(), "admin@example.com", "password123", "Admin")
+	_, err := mgr.CreateOwnerUser(context.Background(), "admin@example.com", "password123", "Admin")
-	assert.Error(t, err, "should return error when embedded IDP is disabled")
+	require.Error(t, err)
 	assert.Contains(t, err.Error(), "embedded IDP is not enabled")
 }
 
 func TestCreateOwnerUser_IdPError(t *testing.T) {
-	manager := &testManager{
+	idpMock := &mockIdP{
-		store: &mockStore{accountsCount: 0},
+		createUserFunc: func(_ context.Context, _, _, _ string) (*idp.UserData, error) {
-		embeddedIdpManager: &mockEmbeddedIdPManager{
+			return nil, errors.New("provider error")
-			createUserFunc: func(ctx context.Context, email, password, name string) (*idp.UserData, error) {
-				return nil, errors.New("user already exists")
-			},
 		},
 	}
+	mgr := newTestManager(idpMock, &mockStore{})
 
-	_, err := manager.CreateOwnerUser(context.Background(), "admin@example.com", "password123", "Admin")
+	_, err := mgr.CreateOwnerUser(context.Background(), "admin@example.com", "password123", "Admin")
-	assert.Error(t, err, "should return error when IDP fails")
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "provider error")
+
+	required, _ := mgr.IsSetupRequired(context.Background())
+	assert.True(t, required, "setup should still be required after IdP error")
+}
+
+func TestCreateOwnerUser_TransientDBError_DoesNotBlockSetup(t *testing.T) {
+	mgr := newTestManager(&mockIdP{}, &mockStore{err: errors.New("connection refused")})
+
+	_, err := mgr.CreateOwnerUser(context.Background(), "admin@example.com", "password123", "Admin")
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "connection refused")
+
+	required, _ := mgr.IsSetupRequired(context.Background())
+	assert.True(t, required, "setup should still be required after transient DB error")
+
+	mgr.store = &mockStore{}
+	userData, err := mgr.CreateOwnerUser(context.Background(), "admin@example.com", "password123", "Admin")
+	require.NoError(t, err)
+	assert.Equal(t, "admin@example.com", userData.Email)
+}
+
+func TestCreateOwnerUser_TransientIdPError_DoesNotBlockSetup(t *testing.T) {
+	idpMock := &mockIdP{getAllAccountsErr: errors.New("connection reset")}
+	mgr := newTestManager(idpMock, &mockStore{})
+
+	_, err := mgr.CreateOwnerUser(context.Background(), "admin@example.com", "password123", "Admin")
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "connection reset")
+
+	required, _ := mgr.IsSetupRequired(context.Background())
+	assert.True(t, required, "setup should still be required after transient IdP error")
+
+	idpMock.getAllAccountsErr = nil
+	userData, err := mgr.CreateOwnerUser(context.Background(), "admin@example.com", "password123", "Admin")
+	require.NoError(t, err)
+	assert.Equal(t, "admin@example.com", userData.Email)
+}
+
+func TestCreateOwnerUser_DBCheckBlocksConcurrent(t *testing.T) {
+	idpMock := &mockIdP{
+		users: map[string][]*idp.UserData{
+			"acc1": {{ID: "existing-user"}},
+		},
+	}
+	mgr := newTestManager(idpMock, &mockStore{})
+
+	_, err := mgr.CreateOwnerUser(context.Background(), "admin@example.com", "password123", "Admin")
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "setup already completed")
+}
+
+func TestCreateOwnerUser_DBCheckBlocksWhenAccountsExist(t *testing.T) {
+	mgr := newTestManager(&mockIdP{}, &mockStore{accountsCount: 1})
+
+	_, err := mgr.CreateOwnerUser(context.Background(), "admin@example.com", "password123", "Admin")
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "setup already completed")
+}
+
+func TestCreateOwnerUser_ConcurrentRequests(t *testing.T) {
+	var idpCallCount atomic.Int32
+	var successCount atomic.Int32
+	var failCount atomic.Int32
+
+	idpMock := &mockIdP{
+		createUserFunc: func(_ context.Context, email, _, _ string) (*idp.UserData, error) {
+			idpCallCount.Add(1)
+			time.Sleep(50 * time.Millisecond)
+			return &idp.UserData{ID: "user-1", Email: email, Name: "Owner"}, nil
+		},
+	}
+	mgr := newTestManager(idpMock, &mockStore{})
+
+	var wg sync.WaitGroup
+	for i := range 10 {
+		wg.Add(1)
+		go func(idx int) {
+			defer wg.Done()
+			_, err := mgr.CreateOwnerUser(
+				context.Background(),
+				fmt.Sprintf("owner%d@example.com", idx),
+				"password1234",
+				fmt.Sprintf("Owner%d", idx),
+			)
+			if err != nil {
+				failCount.Add(1)
+			} else {
+				successCount.Add(1)
+			}
+		}(i)
+	}
+	wg.Wait()
+
+	assert.Equal(t, int32(1), successCount.Load(), "exactly one concurrent setup request should succeed")
+	assert.Equal(t, int32(9), failCount.Load(), "remaining concurrent requests should fail")
+	assert.Equal(t, int32(1), idpCallCount.Load(), "IdP CreateUser should be called exactly once")
+}
+
+func TestIsSetupRequired_EmbeddedIdPDisabled(t *testing.T) {
+	mgr := &DefaultManager{}
+
+	required, err := mgr.IsSetupRequired(context.Background())
+	require.NoError(t, err)
+	assert.False(t, required)
+}
+
+func TestIsSetupRequired_ReturnsFlag(t *testing.T) {
+	mgr := newTestManager(&mockIdP{}, &mockStore{})
+
+	required, err := mgr.IsSetupRequired(context.Background())
+	require.NoError(t, err)
+	assert.True(t, required)
+
+	mgr.setupMu.Lock()
+	mgr.setupRequired = false
+	mgr.setupMu.Unlock()
+
+	required, err = mgr.IsSetupRequired(context.Background())
+	require.NoError(t, err)
+	assert.False(t, required)
 }
 
 func TestDefaultManager_ValidateSetupRequest(t *testing.T) {
-	manager := &DefaultManager{
+	manager := &DefaultManager{setupRequired: true}
-		setupRequired: true,
-	}
 
 	tests := []struct {
 		name        string
@@ -192,7 +239,6 @@ func TestDefaultManager_ValidateSetupRequest(t *testing.T) {
 			email:    "admin@example.com",
 			password: "password123",
 			userName: "Admin User",
-			expectError: false,
 		},
 		{
 			name:        "empty email",
@@ -239,7 +285,20 @@ func TestDefaultManager_ValidateSetupRequest(t *testing.T) {
 			email:    "admin@example.com",
 			password: "12345678",
 			userName: "Admin User",
-			expectError: false,
+		},
+		{
+			name:     "password exactly 72 characters",
+			email:    "admin@example.com",
+			password: "aaaaaaaabbbbbbbbccccccccddddddddeeeeeeeeffffffffgggggggghhhhhhhhiiiiiiii",
+			userName: "Admin User",
+		},
+		{
+			name:        "password too long",
+			email:       "admin@example.com",
+			password:    "aaaaaaaabbbbbbbbccccccccddddddddeeeeeeeeffffffffgggggggghhhhhhhhiiiiiiiij",
+			userName:    "Admin User",
+			expectError: true,
+			errorMsg:    "password must be at most 72 characters",
 		},
 	}
 
@@ -255,14 +314,3 @@ func TestDefaultManager_ValidateSetupRequest(t *testing.T) {
 		})
 	}
 }
-
-func TestDefaultManager_CreateOwnerUser_SetupAlreadyCompleted(t *testing.T) {
-	manager := &DefaultManager{
-		setupRequired:      false,
-		embeddedIdpManager: &idp.EmbeddedIdPManager{},
-	}
-
-	_, err := manager.CreateOwnerUser(context.Background(), "admin@example.com", "password123", "Admin")
-	require.Error(t, err)
-	assert.Contains(t, err.Error(), "setup already completed")
-}