[management, infrastructure, idp] Simplified IdP Management - Embedded IdP#5008
[management, infrastructure, idp] Simplified IdP Management - Embedded IdP#5008
Conversation
- Add explicit return 0 statements at end of functions - Use [[ ]] instead of [ ] for conditional tests - Rename camelCase functions to snake_case 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
# Conflicts: # go.mod # go.sum
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @management/server/user_test.go:
- Around line 1885-2029: The test TestUser_Operations_WithEmbeddedIDP currently
logs the created user's password via t.Logf (see the log line referencing
userInfo.Password); remove or redact the password from the log to avoid leaking
sensitive data—either remove the t.Logf call entirely or change it to log only
non-sensitive fields (e.g., ID and Email) or a masked password placeholder when
referencing userInfo.Password.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
management/server/user.gomanagement/server/user_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
management/server/user_test.go (3)
shared/auth/user.go (1)
UserAuth(7-37)idp/dex/config.go (1)
Storage(111-114)idp/dex/provider.go (2)
Config(30-38)DecodeDexUserID(515-562)
management/server/user.go (3)
management/server/types/user.go (2)
NewUser(228-241)UserInfo(56-74)management/server/account.go (1)
IsEmbeddedIdp(772-778)idp/dex/provider.go (1)
DecodeDexUserID(515-562)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: Android / Build
- GitHub Check: iOS / Build
- GitHub Check: release
- GitHub Check: release_ui
- GitHub Check: Linux
- GitHub Check: release_ui_darwin
- GitHub Check: Darwin
- GitHub Check: Client / Unit
- GitHub Check: Windows
- GitHub Check: JS / Lint
- GitHub Check: test-docker-compose (mysql)
- GitHub Check: Build Cache
- GitHub Check: Client / Unit
- GitHub Check: test-getting-started-script
- GitHub Check: test-docker-compose (postgres)
- GitHub Check: Client / Unit
- GitHub Check: test-docker-compose (sqlite)
- GitHub Check: Check External GPL/AGPL Licenses
🔇 Additional comments (9)
management/server/user_test.go (3)
63-63: LGTM: Signature updates applied consistently.All callsites for
newAccountWithIdhave been updated to pass the additional parameters (domain, email, name, boolean). The empty strings andfalsevalues are appropriate for tests that don't exercise embedded IdP functionality.Also applies to: 110-110, 138-138, 170-170, 195-195, 220-220, 263-263, 303-303, 415-415, 462-462, 510-510, 541-541, 648-648, 687-687, 714-714, 808-808, 976-976, 1012-1012, 1054-1054, 1122-1122, 1156-1156, 1523-1523, 1532-1532, 1559-1559, 1581-1581, 1778-1778, 1837-1837
1013-1014: LGTM: NewRegularUser signature updated consistently.The
types.NewRegularUsersignature now accepts email and name parameters. All callsites correctly pass empty strings for these fields, which is appropriate for tests not focused on embedded IdP flows.Also applies to: 1111-1111, 1327-1327, 1333-1333, 1789-1789, 1814-1814, 1819-1819, 1848-1848, 1864-1864, 1874-1874
367-368: LGTM: User struct fields validated in copy test.The Email and Name fields are now properly included in the
TestUser_Copytest, ensuring theCopy()method handles these new fields correctly.management/server/user.go (6)
44-44: LGTM: Service user creation updated correctly.The
types.NewUsercall now includes email and name parameters. Passing empty strings for these fields is correct for service users, which don't have associated email addresses or human names.
108-146: Well-structured embedded IdP integration.The embedded IdP support is cleanly integrated:
- Branching uses the
IsEmbeddedIdphelper to route to the appropriate user creation path- Email and Name fields are now properly set on the new user (addresses past review feedback)
- Cache refresh is appropriately skipped for embedded IdP since data is stored locally
- Event types distinguish between user invitation (external IdP) and direct user creation (embedded IdP)
189-215: Robust duplicate email detection for embedded IdP.The
createEmbeddedIdpUserfunction properly handles user creation for the embedded IdP:
- Fetches inviter data directly from the database (appropriate for local storage)
- Uses case-insensitive email comparison (
strings.EqualFold) to prevent duplicate accounts with different casing- Returns clear error messages for validation failures
The approach of iterating through account users is reasonable for embedded IdP scenarios where data is stored locally.
804-811: LGTM: Skip external lookups for embedded IdP.The condition correctly skips external IdP cache lookups when using the embedded IdP, since user data is stored locally in the database.
855-869: Improved API with structured auth parameter.The signature change from separate parameters to a
auth.UserAuthobject is a good improvement:
- Provides a more structured and extensible API
- Enables passing email and name for account/user creation
- Aligns with authentication flow patterns elsewhere in the codebase
934-961: Embedded IdP support with Dex ID decoding.The changes properly integrate embedded IdP:
- Skips external IdP lookups when embedded IdP is enabled (data is local)
- Decodes Dex-encoded user IDs to extract the connector ID (
IdPID), which identifies the identity provider source- Uses locally stored email from the database for embedded IdP users
- Silent error handling for
DecodeDexUserIDis intentional and appropriate—decode failures are expected for non-Dex user IDsAlso applies to: 972-975, 997-997, 1006-1009
| func TestUser_Operations_WithEmbeddedIDP(t *testing.T) { | ||
| ctx := context.Background() | ||
|
|
||
| // Create temporary directory for Dex | ||
| tmpDir := t.TempDir() | ||
| dexDataDir := tmpDir + "/dex" | ||
| require.NoError(t, os.MkdirAll(dexDataDir, 0700)) | ||
|
|
||
| // Create embedded IDP config | ||
| embeddedIdPConfig := &idp.EmbeddedIdPConfig{ | ||
| Enabled: true, | ||
| Issuer: "http://localhost:5556/dex", | ||
| Storage: idp.EmbeddedStorageConfig{ | ||
| Type: "sqlite3", | ||
| Config: idp.EmbeddedStorageTypeConfig{ | ||
| File: dexDataDir + "/dex.db", | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| // Create embedded IDP manager | ||
| embeddedIdp, err := idp.NewEmbeddedIdPManager(ctx, embeddedIdPConfig, nil) | ||
| require.NoError(t, err) | ||
| defer func() { _ = embeddedIdp.Stop(ctx) }() | ||
|
|
||
| // Create test store | ||
| testStore, cleanup, err := store.NewTestStoreFromSQL(ctx, "", tmpDir) | ||
| require.NoError(t, err) | ||
| defer cleanup() | ||
|
|
||
| // Create account with owner user | ||
| account := newAccountWithId(ctx, mockAccountID, mockUserID, "", "owner@test.com", "Owner User", false) | ||
| require.NoError(t, testStore.SaveAccount(ctx, account)) | ||
|
|
||
| // Create mock network map controller | ||
| ctrl := gomock.NewController(t) | ||
| networkMapControllerMock := network_map.NewMockController(ctrl) | ||
| networkMapControllerMock.EXPECT(). | ||
| OnPeersDeleted(gomock.Any(), gomock.Any(), gomock.Any()). | ||
| Return(nil). | ||
| AnyTimes() | ||
|
|
||
| // Create account manager with embedded IDP | ||
| permissionsManager := permissions.NewManager(testStore) | ||
| am := DefaultAccountManager{ | ||
| Store: testStore, | ||
| eventStore: &activity.InMemoryEventStore{}, | ||
| permissionsManager: permissionsManager, | ||
| idpManager: embeddedIdp, | ||
| cacheLoading: map[string]chan struct{}{}, | ||
| networkMapController: networkMapControllerMock, | ||
| } | ||
|
|
||
| // Initialize cache manager | ||
| cacheStore, err := nbcache.NewStore(ctx, nbcache.DefaultIDPCacheExpirationMax, nbcache.DefaultIDPCacheCleanupInterval, nbcache.DefaultIDPCacheOpenConn) | ||
| require.NoError(t, err) | ||
| am.cacheManager = nbcache.NewAccountUserDataCache(am.loadAccount, cacheStore) | ||
| am.externalCacheManager = nbcache.NewUserDataCache(cacheStore) | ||
|
|
||
| t.Run("create regular user returns password", func(t *testing.T) { | ||
| userInfo, err := am.CreateUser(ctx, mockAccountID, mockUserID, &types.UserInfo{ | ||
| Email: "newuser@test.com", | ||
| Name: "New User", | ||
| Role: "user", | ||
| AutoGroups: []string{}, | ||
| IsServiceUser: false, | ||
| }) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, userInfo) | ||
|
|
||
| // Verify user data | ||
| assert.Equal(t, "newuser@test.com", userInfo.Email) | ||
| assert.Equal(t, "New User", userInfo.Name) | ||
| assert.Equal(t, "user", userInfo.Role) | ||
| assert.NotEmpty(t, userInfo.ID) | ||
|
|
||
| // IMPORTANT: Password should be returned for embedded IDP | ||
| assert.NotEmpty(t, userInfo.Password, "Password should be returned for embedded IDP user") | ||
| t.Logf("Created user: ID=%s, Email=%s, Password=%s", userInfo.ID, userInfo.Email, userInfo.Password) | ||
|
|
||
| // Verify user ID is in Dex encoded format | ||
| rawUserID, connectorID, err := dex.DecodeDexUserID(userInfo.ID) | ||
| require.NoError(t, err) | ||
| assert.NotEmpty(t, rawUserID) | ||
| assert.Equal(t, "local", connectorID) | ||
| t.Logf("Decoded user ID: rawUserID=%s, connectorID=%s", rawUserID, connectorID) | ||
|
|
||
| // Verify user exists in database with correct data | ||
| dbUser, err := testStore.GetUserByUserID(ctx, store.LockingStrengthNone, userInfo.ID) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, "newuser@test.com", dbUser.Email) | ||
| assert.Equal(t, "New User", dbUser.Name) | ||
|
|
||
| // Store user ID for delete test | ||
| createdUserID := userInfo.ID | ||
|
|
||
| t.Run("delete user works", func(t *testing.T) { | ||
| err := am.DeleteUser(ctx, mockAccountID, mockUserID, createdUserID) | ||
| require.NoError(t, err) | ||
|
|
||
| // Verify user is deleted from database | ||
| _, err = testStore.GetUserByUserID(ctx, store.LockingStrengthNone, createdUserID) | ||
| assert.Error(t, err, "User should be deleted from database") | ||
| }) | ||
| }) | ||
|
|
||
| t.Run("create service user does not return password", func(t *testing.T) { | ||
| userInfo, err := am.CreateUser(ctx, mockAccountID, mockUserID, &types.UserInfo{ | ||
| Name: "Service User", | ||
| Role: "user", | ||
| AutoGroups: []string{}, | ||
| IsServiceUser: true, | ||
| }) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, userInfo) | ||
|
|
||
| assert.True(t, userInfo.IsServiceUser) | ||
| assert.Equal(t, "Service User", userInfo.Name) | ||
| // Service users don't have passwords | ||
| assert.Empty(t, userInfo.Password, "Service users should not have passwords") | ||
| }) | ||
|
|
||
| t.Run("duplicate email fails", func(t *testing.T) { | ||
| // Create first user | ||
| _, err := am.CreateUser(ctx, mockAccountID, mockUserID, &types.UserInfo{ | ||
| Email: "duplicate@test.com", | ||
| Name: "First User", | ||
| Role: "user", | ||
| AutoGroups: []string{}, | ||
| IsServiceUser: false, | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| // Try to create second user with same email | ||
| _, err = am.CreateUser(ctx, mockAccountID, mockUserID, &types.UserInfo{ | ||
| Email: "duplicate@test.com", | ||
| Name: "Second User", | ||
| Role: "user", | ||
| AutoGroups: []string{}, | ||
| IsServiceUser: false, | ||
| }) | ||
| assert.Error(t, err, "Creating user with duplicate email should fail") | ||
| t.Logf("Duplicate email error: %v", err) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Comprehensive embedded IdP test with minor security concern.
The new test TestUser_Operations_WithEmbeddedIDP provides excellent coverage of embedded IdP functionality including user creation, Dex ID encoding/decoding, DB persistence, deletion, service users, and duplicate email handling.
Minor concern: Line 1963 logs the generated password via t.Logf. While this is test code, consider removing or redacting sensitive data from test logs to avoid accidental exposure in CI/CD logs.
🔎 Suggested change to avoid logging sensitive data
- t.Logf("Created user: ID=%s, Email=%s, Password=%s", userInfo.ID, userInfo.Email, userInfo.Password)
+ t.Logf("Created user: ID=%s, Email=%s, Password=[REDACTED]", userInfo.ID, userInfo.Email)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestUser_Operations_WithEmbeddedIDP(t *testing.T) { | |
| ctx := context.Background() | |
| // Create temporary directory for Dex | |
| tmpDir := t.TempDir() | |
| dexDataDir := tmpDir + "/dex" | |
| require.NoError(t, os.MkdirAll(dexDataDir, 0700)) | |
| // Create embedded IDP config | |
| embeddedIdPConfig := &idp.EmbeddedIdPConfig{ | |
| Enabled: true, | |
| Issuer: "http://localhost:5556/dex", | |
| Storage: idp.EmbeddedStorageConfig{ | |
| Type: "sqlite3", | |
| Config: idp.EmbeddedStorageTypeConfig{ | |
| File: dexDataDir + "/dex.db", | |
| }, | |
| }, | |
| } | |
| // Create embedded IDP manager | |
| embeddedIdp, err := idp.NewEmbeddedIdPManager(ctx, embeddedIdPConfig, nil) | |
| require.NoError(t, err) | |
| defer func() { _ = embeddedIdp.Stop(ctx) }() | |
| // Create test store | |
| testStore, cleanup, err := store.NewTestStoreFromSQL(ctx, "", tmpDir) | |
| require.NoError(t, err) | |
| defer cleanup() | |
| // Create account with owner user | |
| account := newAccountWithId(ctx, mockAccountID, mockUserID, "", "owner@test.com", "Owner User", false) | |
| require.NoError(t, testStore.SaveAccount(ctx, account)) | |
| // Create mock network map controller | |
| ctrl := gomock.NewController(t) | |
| networkMapControllerMock := network_map.NewMockController(ctrl) | |
| networkMapControllerMock.EXPECT(). | |
| OnPeersDeleted(gomock.Any(), gomock.Any(), gomock.Any()). | |
| Return(nil). | |
| AnyTimes() | |
| // Create account manager with embedded IDP | |
| permissionsManager := permissions.NewManager(testStore) | |
| am := DefaultAccountManager{ | |
| Store: testStore, | |
| eventStore: &activity.InMemoryEventStore{}, | |
| permissionsManager: permissionsManager, | |
| idpManager: embeddedIdp, | |
| cacheLoading: map[string]chan struct{}{}, | |
| networkMapController: networkMapControllerMock, | |
| } | |
| // Initialize cache manager | |
| cacheStore, err := nbcache.NewStore(ctx, nbcache.DefaultIDPCacheExpirationMax, nbcache.DefaultIDPCacheCleanupInterval, nbcache.DefaultIDPCacheOpenConn) | |
| require.NoError(t, err) | |
| am.cacheManager = nbcache.NewAccountUserDataCache(am.loadAccount, cacheStore) | |
| am.externalCacheManager = nbcache.NewUserDataCache(cacheStore) | |
| t.Run("create regular user returns password", func(t *testing.T) { | |
| userInfo, err := am.CreateUser(ctx, mockAccountID, mockUserID, &types.UserInfo{ | |
| Email: "newuser@test.com", | |
| Name: "New User", | |
| Role: "user", | |
| AutoGroups: []string{}, | |
| IsServiceUser: false, | |
| }) | |
| require.NoError(t, err) | |
| require.NotNil(t, userInfo) | |
| // Verify user data | |
| assert.Equal(t, "newuser@test.com", userInfo.Email) | |
| assert.Equal(t, "New User", userInfo.Name) | |
| assert.Equal(t, "user", userInfo.Role) | |
| assert.NotEmpty(t, userInfo.ID) | |
| // IMPORTANT: Password should be returned for embedded IDP | |
| assert.NotEmpty(t, userInfo.Password, "Password should be returned for embedded IDP user") | |
| t.Logf("Created user: ID=%s, Email=%s, Password=%s", userInfo.ID, userInfo.Email, userInfo.Password) | |
| // Verify user ID is in Dex encoded format | |
| rawUserID, connectorID, err := dex.DecodeDexUserID(userInfo.ID) | |
| require.NoError(t, err) | |
| assert.NotEmpty(t, rawUserID) | |
| assert.Equal(t, "local", connectorID) | |
| t.Logf("Decoded user ID: rawUserID=%s, connectorID=%s", rawUserID, connectorID) | |
| // Verify user exists in database with correct data | |
| dbUser, err := testStore.GetUserByUserID(ctx, store.LockingStrengthNone, userInfo.ID) | |
| require.NoError(t, err) | |
| assert.Equal(t, "newuser@test.com", dbUser.Email) | |
| assert.Equal(t, "New User", dbUser.Name) | |
| // Store user ID for delete test | |
| createdUserID := userInfo.ID | |
| t.Run("delete user works", func(t *testing.T) { | |
| err := am.DeleteUser(ctx, mockAccountID, mockUserID, createdUserID) | |
| require.NoError(t, err) | |
| // Verify user is deleted from database | |
| _, err = testStore.GetUserByUserID(ctx, store.LockingStrengthNone, createdUserID) | |
| assert.Error(t, err, "User should be deleted from database") | |
| }) | |
| }) | |
| t.Run("create service user does not return password", func(t *testing.T) { | |
| userInfo, err := am.CreateUser(ctx, mockAccountID, mockUserID, &types.UserInfo{ | |
| Name: "Service User", | |
| Role: "user", | |
| AutoGroups: []string{}, | |
| IsServiceUser: true, | |
| }) | |
| require.NoError(t, err) | |
| require.NotNil(t, userInfo) | |
| assert.True(t, userInfo.IsServiceUser) | |
| assert.Equal(t, "Service User", userInfo.Name) | |
| // Service users don't have passwords | |
| assert.Empty(t, userInfo.Password, "Service users should not have passwords") | |
| }) | |
| t.Run("duplicate email fails", func(t *testing.T) { | |
| // Create first user | |
| _, err := am.CreateUser(ctx, mockAccountID, mockUserID, &types.UserInfo{ | |
| Email: "duplicate@test.com", | |
| Name: "First User", | |
| Role: "user", | |
| AutoGroups: []string{}, | |
| IsServiceUser: false, | |
| }) | |
| require.NoError(t, err) | |
| // Try to create second user with same email | |
| _, err = am.CreateUser(ctx, mockAccountID, mockUserID, &types.UserInfo{ | |
| Email: "duplicate@test.com", | |
| Name: "Second User", | |
| Role: "user", | |
| AutoGroups: []string{}, | |
| IsServiceUser: false, | |
| }) | |
| assert.Error(t, err, "Creating user with duplicate email should fail") | |
| t.Logf("Duplicate email error: %v", err) | |
| }) | |
| } | |
| func TestUser_Operations_WithEmbeddedIDP(t *testing.T) { | |
| ctx := context.Background() | |
| // Create temporary directory for Dex | |
| tmpDir := t.TempDir() | |
| dexDataDir := tmpDir + "/dex" | |
| require.NoError(t, os.MkdirAll(dexDataDir, 0700)) | |
| // Create embedded IDP config | |
| embeddedIdPConfig := &idp.EmbeddedIdPConfig{ | |
| Enabled: true, | |
| Issuer: "http://localhost:5556/dex", | |
| Storage: idp.EmbeddedStorageConfig{ | |
| Type: "sqlite3", | |
| Config: idp.EmbeddedStorageTypeConfig{ | |
| File: dexDataDir + "/dex.db", | |
| }, | |
| }, | |
| } | |
| // Create embedded IDP manager | |
| embeddedIdp, err := idp.NewEmbeddedIdPManager(ctx, embeddedIdPConfig, nil) | |
| require.NoError(t, err) | |
| defer func() { _ = embeddedIdp.Stop(ctx) }() | |
| // Create test store | |
| testStore, cleanup, err := store.NewTestStoreFromSQL(ctx, "", tmpDir) | |
| require.NoError(t, err) | |
| defer cleanup() | |
| // Create account with owner user | |
| account := newAccountWithId(ctx, mockAccountID, mockUserID, "", "owner@test.com", "Owner User", false) | |
| require.NoError(t, testStore.SaveAccount(ctx, account)) | |
| // Create mock network map controller | |
| ctrl := gomock.NewController(t) | |
| networkMapControllerMock := network_map.NewMockController(ctrl) | |
| networkMapControllerMock.EXPECT(). | |
| OnPeersDeleted(gomock.Any(), gomock.Any(), gomock.Any()). | |
| Return(nil). | |
| AnyTimes() | |
| // Create account manager with embedded IDP | |
| permissionsManager := permissions.NewManager(testStore) | |
| am := DefaultAccountManager{ | |
| Store: testStore, | |
| eventStore: &activity.InMemoryEventStore{}, | |
| permissionsManager: permissionsManager, | |
| idpManager: embeddedIdp, | |
| cacheLoading: map[string]chan struct{}{}, | |
| networkMapController: networkMapControllerMock, | |
| } | |
| // Initialize cache manager | |
| cacheStore, err := nbcache.NewStore(ctx, nbcache.DefaultIDPCacheExpirationMax, nbcache.DefaultIDPCacheCleanupInterval, nbcache.DefaultIDPCacheOpenConn) | |
| require.NoError(t, err) | |
| am.cacheManager = nbcache.NewAccountUserDataCache(am.loadAccount, cacheStore) | |
| am.externalCacheManager = nbcache.NewUserDataCache(cacheStore) | |
| t.Run("create regular user returns password", func(t *testing.T) { | |
| userInfo, err := am.CreateUser(ctx, mockAccountID, mockUserID, &types.UserInfo{ | |
| Email: "newuser@test.com", | |
| Name: "New User", | |
| Role: "user", | |
| AutoGroups: []string{}, | |
| IsServiceUser: false, | |
| }) | |
| require.NoError(t, err) | |
| require.NotNil(t, userInfo) | |
| // Verify user data | |
| assert.Equal(t, "newuser@test.com", userInfo.Email) | |
| assert.Equal(t, "New User", userInfo.Name) | |
| assert.Equal(t, "user", userInfo.Role) | |
| assert.NotEmpty(t, userInfo.ID) | |
| // IMPORTANT: Password should be returned for embedded IDP | |
| assert.NotEmpty(t, userInfo.Password, "Password should be returned for embedded IDP user") | |
| t.Logf("Created user: ID=%s, Email=%s, Password=[REDACTED]", userInfo.ID, userInfo.Email) | |
| // Verify user ID is in Dex encoded format | |
| rawUserID, connectorID, err := dex.DecodeDexUserID(userInfo.ID) | |
| require.NoError(t, err) | |
| assert.NotEmpty(t, rawUserID) | |
| assert.Equal(t, "local", connectorID) | |
| t.Logf("Decoded user ID: rawUserID=%s, connectorID=%s", rawUserID, connectorID) | |
| // Verify user exists in database with correct data | |
| dbUser, err := testStore.GetUserByUserID(ctx, store.LockingStrengthNone, userInfo.ID) | |
| require.NoError(t, err) | |
| assert.Equal(t, "newuser@test.com", dbUser.Email) | |
| assert.Equal(t, "New User", dbUser.Name) | |
| // Store user ID for delete test | |
| createdUserID := userInfo.ID | |
| t.Run("delete user works", func(t *testing.T) { | |
| err := am.DeleteUser(ctx, mockAccountID, mockUserID, createdUserID) | |
| require.NoError(t, err) | |
| // Verify user is deleted from database | |
| _, err = testStore.GetUserByUserID(ctx, store.LockingStrengthNone, createdUserID) | |
| assert.Error(t, err, "User should be deleted from database") | |
| }) | |
| }) | |
| t.Run("create service user does not return password", func(t *testing.T) { | |
| userInfo, err := am.CreateUser(ctx, mockAccountID, mockUserID, &types.UserInfo{ | |
| Name: "Service User", | |
| Role: "user", | |
| AutoGroups: []string{}, | |
| IsServiceUser: true, | |
| }) | |
| require.NoError(t, err) | |
| require.NotNil(t, userInfo) | |
| assert.True(t, userInfo.IsServiceUser) | |
| assert.Equal(t, "Service User", userInfo.Name) | |
| // Service users don't have passwords | |
| assert.Empty(t, userInfo.Password, "Service users should not have passwords") | |
| }) | |
| t.Run("duplicate email fails", func(t *testing.T) { | |
| // Create first user | |
| _, err := am.CreateUser(ctx, mockAccountID, mockUserID, &types.UserInfo{ | |
| Email: "duplicate@test.com", | |
| Name: "First User", | |
| Role: "user", | |
| AutoGroups: []string{}, | |
| IsServiceUser: false, | |
| }) | |
| require.NoError(t, err) | |
| // Try to create second user with same email | |
| _, err = am.CreateUser(ctx, mockAccountID, mockUserID, &types.UserInfo{ | |
| Email: "duplicate@test.com", | |
| Name: "Second User", | |
| Role: "user", | |
| AutoGroups: []string{}, | |
| IsServiceUser: false, | |
| }) | |
| assert.Error(t, err, "Creating user with duplicate email should fail") | |
| t.Logf("Duplicate email error: %v", err) | |
| }) | |
| } |
🤖 Prompt for AI Agents
In @management/server/user_test.go around lines 1885 - 2029, The test
TestUser_Operations_WithEmbeddedIDP currently logs the created user's password
via t.Logf (see the log line referencing userInfo.Password); remove or redact
the password from the log to avoid leaking sensitive data—either remove the
t.Logf call entirely or change it to log only non-sensitive fields (e.g., ID and
Email) or a masked password placeholder when referencing userInfo.Password.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @management/server/instance/manager_test.go:
- Around line 1-268: The tests currently exercise only testManager and
validation logic; add integration tests that instantiate the real DefaultManager
via NewManager and exercise loadSetupRequired, DefaultManager.IsSetupRequired
and DefaultManager.CreateOwnerUser flows. Specifically, add tests:
TestNewManager_WithEmbeddedIdP to call NewManager with an idp.EmbeddedIdPManager
mock/store that lets loadSetupRequired query GetAllAccounts and assert
setupRequired is set; TestNewManager_WithoutEmbeddedIdP to ensure NewManager
leaves setupRequired false when idp manager is not EmbeddedIdPManager;
TestDefaultManager_IsSetupRequired_FullFlow to call
DefaultManager.IsSetupRequired on a DefaultManager created with a mock store
returning 0 and >0 accounts and assert true/false; and
TestDefaultManager_CreateOwnerUser_FullFlow to call
DefaultManager.CreateOwnerUser (validating via validateSetupInfo) with
embeddedIdpManager mocked to return user or error and assert that on success
setupRequired flips to false and on failure appropriate errors are returned.
Ensure mocks implement calls used by NewManager and loadSetupRequired
(GetAllAccounts/GetAccountsCounter) and reference DefaultManager, NewManager,
loadSetupRequired, DefaultManager.IsSetupRequired, and
DefaultManager.CreateOwnerUser in the tests.
- Around line 43-68: testManager diverges from DefaultManager (notably in
CreateOwnerUser missing validateSetupInfo and setupRequired checks), so update
tests to exercise the real logic: replace testManager with an instance of
DefaultManager (constructed with the same mockStore and mockEmbeddedIdPManager)
or, if you must keep testManager, implement the same behavior as
DefaultManager.CreateOwnerUser by invoking validateSetupInfo and checking
IsSetupRequired before calling embeddedIdpManager.CreateUserWithPassword; ensure
tests use the DefaultManager constructor or call validateSetupInfo and
IsSetupRequired so they catch regressions in DefaultManager.CreateOwnerUser.
🧹 Nitpick comments (3)
management/server/http/handlers/instance/instance_handler.go (1)
61-61: Avoid logging PII in production logs.Logging the user email is considered PII and should be avoided in production logs. Consider logging only the user ID or a generic success message instead.
🔎 Suggested change
- log.WithContext(r.Context()).Infof("instance setup completed: created user %s", req.Email) + log.WithContext(r.Context()).Infof("instance setup completed: created user ID %s", userData.ID)management/server/http/handlers/instance/instance_handler_test.go (1)
198-200: Clarify or correct the comment about status codes.The comment states invalid email format "returns 400 Bad Request" but the assertion on line 200 expects HTTP 422 (Unprocessable Entity). The assertion is correct based on the error mapping in util.WriteError (InvalidArgument → 422), so the comment should be updated.
🔎 Suggested change
- // Note: Invalid email format uses mail.ParseAddress which is treated differently - // and returns 400 Bad Request instead of 422 Unprocessable Entity + // Note: Invalid email format returns InvalidArgument which maps to 422 Unprocessable Entity assert.Equal(t, http.StatusUnprocessableEntity, rec.Code)management/server/instance/manager.go (1)
114-114: Avoid logging PII in production logs.The email address is personally identifiable information (PII) and should not be logged in production. This was flagged in a previous review and marked as addressed in commit 9466744, but the PII logging is still present.
🔎 Suggested change
- log.WithContext(ctx).Infof("created owner user %s in embedded IdP", email) + log.WithContext(ctx).Infof("created owner user %s in embedded IdP", userData.ID)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
management/server/http/handlers/instance/instance_handler.gomanagement/server/http/handlers/instance/instance_handler_test.gomanagement/server/instance/manager.gomanagement/server/instance/manager_test.go
🧰 Additional context used
🧬 Code graph analysis (3)
management/server/http/handlers/instance/instance_handler.go (3)
management/server/instance/manager.go (1)
Manager(18-26)shared/management/http/util/util.go (3)
WriteErrorResponse(70-80)WriteJSONObject(27-35)WriteError(84-120)shared/management/http/api/types.gen.go (3)
InstanceStatus(713-716)SetupRequest(1899-1908)SetupResponse(1911-1917)
management/server/instance/manager_test.go (3)
management/server/idp/idp.go (1)
UserData(70-76)management/server/instance/manager.go (1)
DefaultManager(29-35)management/server/idp/embedded.go (1)
EmbeddedIdPManager(161-165)
management/server/instance/manager.go (3)
management/server/idp/idp.go (1)
UserData(70-76)management/server/idp/embedded.go (1)
EmbeddedIdPManager(161-165)shared/management/status/error.go (3)
Errorf(70-75)PreconditionFailed(15-15)InvalidArgument(27-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
- GitHub Check: Management / Benchmark (API) (amd64, postgres)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Benchmark (API) (amd64, sqlite)
- GitHub Check: Management / Benchmark (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Management / Integration (amd64, postgres)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Client / Unit (386)
- GitHub Check: Management / Benchmark (amd64, postgres)
- GitHub Check: Darwin
- GitHub Check: Windows
- GitHub Check: Linux
- GitHub Check: test-docker-compose (mysql)
- GitHub Check: test-docker-compose (postgres)
- GitHub Check: test-docker-compose (sqlite)
- GitHub Check: test-getting-started-script
- GitHub Check: Client / Unit
- GitHub Check: iOS / Build
- GitHub Check: Android / Build
- GitHub Check: release_ui_darwin
- GitHub Check: release
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
🔇 Additional comments (8)
management/server/instance/manager_test.go (1)
177-257: Good validation test coverage.The validation tests comprehensively cover the
validateSetupInfologic including edge cases (empty fields, invalid email format, password length boundaries). The tests properly verify error messages for each validation failure.management/server/http/handlers/instance/instance_handler.go (2)
15-29: Clean handler structure and endpoint registration.The handler properly encapsulates the instance manager and registers unauthenticated endpoints for bootstrap setup. OPTIONS support for CORS is appropriate.
31-44: LGTM: Status endpoint is straightforward and correct.management/server/http/handlers/instance/instance_handler_test.go (1)
77-281: Excellent test coverage for the HTTP handler.The tests comprehensively cover happy paths, validation errors, precondition failures, and error propagation. The mock appropriately isolates the HTTP layer from the manager implementation.
management/server/instance/manager.go (4)
17-35: Well-designed interface and implementation structure.The Manager interface provides a clean abstraction for instance setup operations. The DefaultManager appropriately caches the setup state in memory with proper synchronization, addressing concerns about expensive DB calls on unauthenticated endpoints.
37-69: Setup state is efficiently cached at startup.The
loadSetupRequiredmethod is called once duringNewManagerinitialization, caching the setup state in memory. This design properly addresses the concern about avoiding database calls on every request to unauthenticated endpoints. Subsequent calls toIsSetupRequiredsimply read the cached flag with appropriate locking.
71-84: LGTM: Correct thread-safe implementation.The method properly uses RLock for reading the cached setup state and handles the case when embedded IdP is not enabled.
119-136: Comprehensive input validation with proper error handling.The validation logic covers all required fields with appropriate checks:
- Email presence and format validation using
mail.ParseAddress- Name and password presence checks
- Password minimum length enforcement
Error messages are clear and use appropriate status codes (InvalidArgument).
| package instance | ||
|
|
||
| import ( | ||
| "context" | ||
| "errors" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
|
|
||
| "github.com/netbirdio/netbird/management/server/idp" | ||
| ) | ||
|
|
||
| // mockStore implements a minimal store.Store for testing | ||
| type mockStore struct { | ||
| accountsCount int64 | ||
| err error | ||
| } | ||
|
|
||
| func (m *mockStore) GetAccountsCounter(ctx context.Context) (int64, error) { | ||
| if m.err != nil { | ||
| return 0, m.err | ||
| } | ||
| return m.accountsCount, nil | ||
| } | ||
|
|
||
| // mockEmbeddedIdPManager wraps the real EmbeddedIdPManager for testing | ||
| type mockEmbeddedIdPManager struct { | ||
| createUserFunc func(ctx context.Context, email, password, name string) (*idp.UserData, error) | ||
| } | ||
|
|
||
| func (m *mockEmbeddedIdPManager) 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 | ||
| } | ||
|
|
||
| // 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" | ||
| expectedName := "Admin User" | ||
| expectedPassword := "securepassword123" | ||
|
|
||
| manager := &testManager{ | ||
| 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, expectedEmail, userData.Email) | ||
| assert.Equal(t, expectedName, userData.Name) | ||
| } | ||
|
|
||
| func TestCreateOwnerUser_EmbeddedIdPDisabled(t *testing.T) { | ||
| manager := &testManager{ | ||
| store: &mockStore{accountsCount: 0}, | ||
| embeddedIdpManager: nil, | ||
| } | ||
|
|
||
| _, err := manager.CreateOwnerUser(context.Background(), "admin@example.com", "password123", "Admin") | ||
| assert.Error(t, err, "should return error when embedded IDP is disabled") | ||
| assert.Contains(t, err.Error(), "embedded IDP is not enabled") | ||
| } | ||
|
|
||
| func TestCreateOwnerUser_IdPError(t *testing.T) { | ||
| manager := &testManager{ | ||
| store: &mockStore{accountsCount: 0}, | ||
| embeddedIdpManager: &mockEmbeddedIdPManager{ | ||
| createUserFunc: func(ctx context.Context, email, password, name string) (*idp.UserData, error) { | ||
| return nil, errors.New("user already exists") | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| _, err := manager.CreateOwnerUser(context.Background(), "admin@example.com", "password123", "Admin") | ||
| assert.Error(t, err, "should return error when IDP fails") | ||
| } | ||
|
|
||
| func TestDefaultManager_ValidateSetupRequest(t *testing.T) { | ||
| manager := &DefaultManager{ | ||
| setupRequired: true, | ||
| } | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| email string | ||
| password string | ||
| userName string | ||
| expectError bool | ||
| errorMsg string | ||
| }{ | ||
| { | ||
| name: "valid request", | ||
| email: "admin@example.com", | ||
| password: "password123", | ||
| userName: "Admin User", | ||
| expectError: false, | ||
| }, | ||
| { | ||
| name: "empty email", | ||
| email: "", | ||
| password: "password123", | ||
| userName: "Admin User", | ||
| expectError: true, | ||
| errorMsg: "email is required", | ||
| }, | ||
| { | ||
| name: "invalid email format", | ||
| email: "not-an-email", | ||
| password: "password123", | ||
| userName: "Admin User", | ||
| expectError: true, | ||
| errorMsg: "invalid email format", | ||
| }, | ||
| { | ||
| name: "empty name", | ||
| email: "admin@example.com", | ||
| password: "password123", | ||
| userName: "", | ||
| expectError: true, | ||
| errorMsg: "name is required", | ||
| }, | ||
| { | ||
| name: "empty password", | ||
| email: "admin@example.com", | ||
| password: "", | ||
| userName: "Admin User", | ||
| expectError: true, | ||
| errorMsg: "password is required", | ||
| }, | ||
| { | ||
| name: "password too short", | ||
| email: "admin@example.com", | ||
| password: "short", | ||
| userName: "Admin User", | ||
| expectError: true, | ||
| errorMsg: "password must be at least 8 characters", | ||
| }, | ||
| { | ||
| name: "password exactly 8 characters", | ||
| email: "admin@example.com", | ||
| password: "12345678", | ||
| userName: "Admin User", | ||
| expectError: false, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| err := manager.validateSetupInfo(tt.email, tt.password, tt.userName) | ||
| if tt.expectError { | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), tt.errorMsg) | ||
| } else { | ||
| require.NoError(t, err) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| 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") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add integration tests for DefaultManager and NewManager.
The test suite is missing coverage for key DefaultManager methods:
NewManager: Type assertion logic andloadSetupRequiredinitialization are untested.loadSetupRequired: GetAllAccounts call and setupRequired flag setting are untested.DefaultManager.IsSetupRequiredandDefaultManager.CreateOwnerUser: Full flows with the actual implementation (including validation, setup flag checks, and state updates) are not tested.
Current tests at lines 70-175 use testManager which has different behavior, and tests at lines 177-268 only cover input validation and a single error case.
💡 Consider adding these test cases
func TestNewManager_WithEmbeddedIdP(t *testing.T) {
// Test NewManager when idpManager is EmbeddedIdPManager
// Verify loadSetupRequired is called and setupRequired is set correctly
}
func TestNewManager_WithoutEmbeddedIdP(t *testing.T) {
// Test NewManager when idpManager is not EmbeddedIdPManager
// Verify setupRequired defaults to false
}
func TestDefaultManager_IsSetupRequired_FullFlow(t *testing.T) {
// Test IsSetupRequired with real DefaultManager instance
// covering both true and false scenarios
}
func TestDefaultManager_CreateOwnerUser_FullFlow(t *testing.T) {
// Test CreateOwnerUser with real DefaultManager
// including validation, setup flag check, user creation, and flag update
}🤖 Prompt for AI Agents
In @management/server/instance/manager_test.go around lines 1 - 268, The tests
currently exercise only testManager and validation logic; add integration tests
that instantiate the real DefaultManager via NewManager and exercise
loadSetupRequired, DefaultManager.IsSetupRequired and
DefaultManager.CreateOwnerUser flows. Specifically, add tests:
TestNewManager_WithEmbeddedIdP to call NewManager with an idp.EmbeddedIdPManager
mock/store that lets loadSetupRequired query GetAllAccounts and assert
setupRequired is set; TestNewManager_WithoutEmbeddedIdP to ensure NewManager
leaves setupRequired false when idp manager is not EmbeddedIdPManager;
TestDefaultManager_IsSetupRequired_FullFlow to call
DefaultManager.IsSetupRequired on a DefaultManager created with a mock store
returning 0 and >0 accounts and assert true/false; and
TestDefaultManager_CreateOwnerUser_FullFlow to call
DefaultManager.CreateOwnerUser (validating via validateSetupInfo) with
embeddedIdpManager mocked to return user or error and assert that on success
setupRequired flips to false and on failure appropriate errors are returned.
Ensure mocks implement calls used by NewManager and loadSetupRequired
(GetAllAccounts/GetAccountsCounter) and reference DefaultManager, NewManager,
loadSetupRequired, DefaultManager.IsSetupRequired, and
DefaultManager.CreateOwnerUser in the tests.
| // 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) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consider using DefaultManager directly in tests or document the purpose of testManager.
The testManager implementation has different behavior from DefaultManager:
testManager.CreateOwnerUserlacks input validation thatDefaultManager.CreateOwnerUserperforms viavalidateSetupInfo.testManager.CreateOwnerUserdoesn't check thesetupRequiredflag before creating a user, whileDefaultManager.CreateOwnerUserdoes.
Tests using testManager (lines 70-175) won't catch bugs in the actual DefaultManager implementation. Consider refactoring tests to use DefaultManager with appropriate mocks for the store and embedded IdP manager, or clearly document why a separate test implementation is necessary.
🤖 Prompt for AI Agents
In @management/server/instance/manager_test.go around lines 43 - 68, testManager
diverges from DefaultManager (notably in CreateOwnerUser missing
validateSetupInfo and setupRequired checks), so update tests to exercise the
real logic: replace testManager with an instance of DefaultManager (constructed
with the same mockStore and mockEmbeddedIdPManager) or, if you must keep
testManager, implement the same behavior as DefaultManager.CreateOwnerUser by
invoking validateSetupInfo and checking IsSetupRequired before calling
embeddedIdpManager.CreateUserWithPassword; ensure tests use the DefaultManager
constructor or call validateSetupInfo and IsSetupRequired so they catch
regressions in DefaultManager.CreateOwnerUser.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shared/management/http/api/openapi.yml (1)
208-225: Clarify and harden semantics aroundUser.passwordexposureThe
Userschema now includes apasswordproperty, and the Users POST/GET endpoints reuse this model. Even though the description says it’s “only present when user is created…”, there’s nothing in the schema enforcingwriteOnlybehavior or preventing it from being returned on normal GETs.To reduce risk of accidental password leakage, consider:
- Marking
passwordaswriteOnly: trueand returning any one‑time secrets via a dedicated response type (similar to how PATs/SetupKeys are handled), or- Ensuring the server never populates
passwordon read paths and tightening the docs to make this explicit.This is not a functional bug, but tightening the contract here would improve security posture.
Also applies to: 218-221, 268-271
🤖 Fix all issues with AI Agents
In @management/cmd/management.go:
- Around line 238-243: The skip condition in applyOIDCConfig currently returns
early when cfg.EmbeddedIdP != nil, which erroneously blocks external OIDC
discovery even if the embedded IdP is present but disabled; update the condition
to only skip when an embedded IdP exists and is enabled (i.e., check
cfg.EmbeddedIdP != nil && cfg.EmbeddedIdP.Enabled) so that external discovery
proceeds when Enabled is false, keeping the rest of logic around oidcEndpoint
(cfg.HttpConfig.OIDCConfigEndpoint) intact.
- Around line 183-188: loadMgmtConfig must not mutate the global
userDeleteFromIDPEnabled; instead return the computed value (or include it in a
config struct) from loadMgmtConfig and propagate that value to where the server
is constructed (newServer) so global state is not changed during config loading;
remove the assignment to the global inside loadMgmtConfig, update callers (e.g.,
PreRunE / RunE) to receive the returned flag/config and set
userDeleteFromIDPEnabled explicitly or pass the flag into newServer when
creating the server.
In @management/server/http/handlers/idp/idp_handler.go:
- Around line 115-147: The updateIdentityProvider handler currently uses
fromAPIRequest which unconditionally copies IdentityProviderRequest.ClientSecret
and thus overwrites an existing credential when the update request omits
client_secret; fix by loading the existing provider before calling
UpdateIdentityProvider (e.g. call h.accountManager.GetIdentityProvider or
similar to fetch current provider using accountID and idpID) and, after building
idp from fromAPIRequest(&req), if req.ClientSecret is the empty string then set
idp.ClientSecret = existing.ClientSecret so the secret is preserved when not
provided in the request; then call h.accountManager.UpdateIdentityProvider as
before.
In @management/server/identity_provider.go:
- Around line 133-135: UpdateIdentityProvider currently returns Internal for any
error from embeddedManager.UpdateConnector; special-case storage.ErrNotFound and
return a NotFound status instead to match DeleteIdentityProvider. Modify the
error handling in UpdateIdentityProvider (where UpdateConnector is called) to
check errors.Is(err, storage.ErrNotFound) (or compare to storage.ErrNotFound)
and return status.Errorf(status.NotFound, "identity provider not found: %v",
err) otherwise keep the existing Internal error path; apply the same fix to the
similar block referenced around lines 155-160 that also calls UpdateConnector.
In @shared/management/http/api/openapi.yml:
- Around line 2346-2366: The OpenAPI schema for SetupRequest has a mismatch: the
property SetupRequest.name is marked required but its description still says
"defaults to email if not provided"; update the schema so the description
matches the required behavior by removing the "defaults to email if not
provided" phrase (or alternatively, if you prefer defaulting, make
SetupRequest.name not required); ensure the change is applied to the name
property description in shared/management/http/api/openapi.yml so the
description and required list are consistent.
🧹 Nitpick comments (4)
management/cmd/management.go (2)
321-338: Consider the implications of writing config during load.This function generates and persists an encryption key by writing to the config file during the load phase. While intentional, this introduces side effects:
- Config loading is no longer read-only, which can surprise callers
- If the write fails (lines 333-335), the in-memory config has the key but the file doesn't, causing a mismatch on next startup
- No indication whether
util.DirectWriteJsonis atomic or creates a backupConsider:
- Documenting this side effect in the function's godoc
- Verifying that writes are atomic or implement a backup strategy
- Logging a warning before writing to help operators understand the file modification
Example: Add documentation and pre-write warning
-// ensureEncryptionKey generates and saves a DataStoreEncryptionKey if not set +// ensureEncryptionKey generates and saves a DataStoreEncryptionKey if not set. +// Note: This function modifies the config file on disk to persist the generated key. func ensureEncryptionKey(ctx context.Context, configPath string, cfg *nbconfig.Config) error { if cfg.DataStoreEncryptionKey != "" { return nil } - log.WithContext(ctx).Infof("DataStoreEncryptionKey is not set, generating a new key") + log.WithContext(ctx).Warnf("DataStoreEncryptionKey is not set, generating a new key and writing to config file: %s", configPath) key, err := crypt.GenerateKey()
350-354: Add timeout to HTTP client for OIDC config fetch.Line 351 uses
http.Getwhich relies on the default client with no timeout. If the OIDC endpoint is slow or unresponsive, this will hang indefinitely and block server startup.🔎 Recommended fix
func fetchOIDCConfig(ctx context.Context, oidcEndpoint string) (OIDCConfigResponse, error) { - res, err := http.Get(oidcEndpoint) + client := &http.Client{Timeout: 10 * time.Second} + req, err := http.NewRequestWithContext(ctx, http.MethodGet, oidcEndpoint, nil) + if err != nil { + return OIDCConfigResponse{}, fmt.Errorf("failed creating OIDC config request: %v", err) + } + res, err := client.Do(req) if err != nil { return OIDCConfigResponse{}, fmt.Errorf("failed fetching OIDC configuration from endpoint %s %v", oidcEndpoint, err) }This adds a 10-second timeout and respects context cancellation.
management/server/http/handlers/idp/idp_handler.go (1)
38-172: Consider extracting repeated patterns into helper functions.The handlers contain several repeated patterns:
- UserAuth extraction (lines 40-46, 64-70, 90-96, 117-123, 151-157): Identical code in 5 handlers
- idpID validation (lines 72-77, 125-130, 159-164): Similar code in 3 handlers
- JSON decode error handling (lines 99-102, 133-136): Repeated pattern in 2 handlers
Consider extracting helper functions to reduce duplication and improve maintainability:
func extractUserAuth(r *http.Request) (accountID, userID string, err error) { userAuth, err := nbcontext.GetUserAuthFromContext(r.Context()) if err != nil { return "", "", err } return userAuth.AccountId, userAuth.UserId, nil } func extractIDPID(r *http.Request) (string, error) { idpID := mux.Vars(r)["idpId"] if idpID == "" { return "", status.Errorf(status.InvalidArgument, "identity provider ID is required") } return idpID, nil }management/server/identity_provider.go (1)
190-216: KeepgenerateIdentityProviderIDcomment in sync with supported typesThe function now prefixes IDs for additional types (
authentik,keycloak) beyond those listed in the comment. The implementation is fine, but the doc comment is stale and can confuse future readers.Update the comment to mention all prefixed types or to describe the behavior generically (“well‑known provider types get
<type>-<xid>; generic OIDC gets bare<xid>”).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
management/cmd/management.gomanagement/server/http/handlers/idp/idp_handler.gomanagement/server/identity_provider.gomanagement/server/types/identity_provider.goshared/management/http/api/openapi.ymlshared/management/http/api/types.gen.go
🧰 Additional context used
🧬 Code graph analysis (4)
management/server/http/handlers/idp/idp_handler.go (5)
management/server/context/auth.go (1)
GetUserAuthFromContext(25-30)shared/management/http/util/util.go (2)
WriteError(84-120)WriteJSONObject(27-35)shared/management/http/api/types.gen.go (2)
IdentityProvider(538-553)IdentityProviderType(574-574)management/server/types/identity_provider.go (2)
IdentityProvider(43-58)IdentityProviderType(19-19)route/route.go (1)
ID(48-48)
management/server/identity_provider.go (4)
management/server/types/identity_provider.go (10)
IdentityProvider(43-58)IdentityProviderType(19-19)IdentityProviderTypeOkta(31-31)IdentityProviderTypeZitadel(25-25)IdentityProviderTypeEntra(27-27)IdentityProviderTypeGoogle(29-29)IdentityProviderTypePocketID(33-33)IdentityProviderTypeMicrosoft(35-35)IdentityProviderTypeAuthentik(37-37)IdentityProviderTypeKeycloak(39-39)shared/management/status/error.go (4)
NewPermissionValidationError(213-215)NewPermissionDeniedError(209-211)Errorf(70-75)Error(54-57)management/server/idp/embedded.go (1)
EmbeddedIdPManager(161-165)idp/dex/provider.go (1)
ConnectorConfig(629-644)
shared/management/http/api/types.gen.go (2)
management/server/types/identity_provider.go (1)
IdentityProviderType(19-19)idp/dex/config.go (1)
Password(117-117)
management/server/types/identity_provider.go (1)
shared/management/http/api/types.gen.go (7)
IdentityProviderType(574-574)IdentityProvider(538-553)IdentityProviderTypeZitadel(94-94)IdentityProviderTypeEntra(88-88)IdentityProviderTypeGoogle(89-89)IdentityProviderTypeOkta(92-92)IdentityProviderTypeMicrosoft(90-90)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (32)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Benchmark (API) (amd64, postgres)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Benchmark (API) (amd64, sqlite)
- GitHub Check: Management / Benchmark (amd64, postgres)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Benchmark (amd64, sqlite)
- GitHub Check: Client / Unit (386)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Management / Integration (amd64, sqlite)
- GitHub Check: Management / Integration (amd64, postgres)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Client / Unit
- GitHub Check: Darwin
- GitHub Check: Linux
- GitHub Check: Windows
- GitHub Check: Client / Unit
- GitHub Check: iOS / Build
- GitHub Check: test-docker-compose (sqlite)
- GitHub Check: JS / Lint
- GitHub Check: Android / Build
- GitHub Check: release_ui_darwin
- GitHub Check: test-docker-compose (postgres)
- GitHub Check: test-docker-compose (mysql)
- GitHub Check: test-getting-started-script
- GitHub Check: release
- GitHub Check: release_ui
- GitHub Check: FreeBSD Port / Build & Test
- GitHub Check: Client / Unit
- GitHub Check: Check External GPL/AGPL Licenses
🔇 Additional comments (17)
management/cmd/management.go (6)
137-159: LGTM! Clean modularization of config loading.The refactored orchestration flow improves readability and separation of concerns. The error handling at each step is appropriate.
161-173: LGTM!The command-line override logic is clear and correct.
268-294: LGTM!The device auth flow configuration logic is clear, with appropriate error handling for URL parsing and sensible defaults for scope.
296-308: LGTM!The PKCE flow configuration is straightforward and follows the same pattern as the device auth flow.
310-318: LGTM!The logging provides useful runtime visibility into the embedded IdP and Relay configuration.
220-223: Note: Monitor for startup ordering issues with JWT key validation.Based on PR feedback, there's a reported issue where the JWT validator attempts to fetch keys from this endpoint before the embedded IdP is fully initialized, resulting in EOF errors.
While this config-loading code correctly sets the keys location, the actual initialization ordering would need to be addressed in the server startup sequence (likely in
server.NewServeror related code). Consider verifying that the embedded IdP's keys endpoint is available before the JWT validator is initialized.As per PR comments by mlsmaycon.
management/server/http/handlers/idp/idp_handler.go (8)
1-15: LGTM: Clean imports and package structure.The imports are well-organized and appropriate for the handler's functionality.
22-30: LGTM: RESTful endpoint registration.The route registration follows REST conventions and properly separates concerns. The OPTIONS method is included for CORS support, which should be handled by middleware.
38-60: LGTM: Clean list implementation.The handler properly authenticates, retrieves providers, and converts them to the API format. Good use of preallocated slice capacity.
62-86: LGTM: Proper parameter validation.The handler correctly validates the required idpID parameter and returns appropriate errors.
149-172: LGTM: Clean delete implementation.The handler properly validates the idpID and returns an appropriate empty response on successful deletion.
174-186: LGTM: Excellent security practice for secret handling.The converter correctly excludes
ClientSecretfrom API responses, preventing credential leakage. The explicit comment on line 184 documents this important security decision.
188-196: Converter correctly maps request fields.The function properly translates API request fields to internal types. Note that ClientSecret handling concerns are addressed in the create/update handler comments above.
88-113: Input validation for identity provider creation is already performed at the business logic layer.The
accountManager.CreateIdentityProvider()method callsidpConfig.Validate(), which validates:
- Required fields: Name, Type, ClientID, and Issuer (when not using built-in issuer)
- Type is a valid enum value
- Issuer is a properly formatted URL
This validation occurs in the business logic layer rather than the HTTP handler, which is the appropriate architectural pattern. Handler-level validation is not required here.
Note: The
ClientSecretfield is not validated for minimum length or format, which could be a separate enhancement to consider.Likely an incorrect or invalid review comment.
management/server/types/identity_provider.go (1)
18-40: Identity provider type/validation model looks consistent and self‑containedEnum coverage,
Validate, andHasBuiltInIssuerare coherent and match the intended provider set; no issues from this file alone. Downstream OpenAPI enum alignment is handled in the YAML file comments.Also applies to: 82-121
shared/management/http/api/types.gen.go (1)
86-95: Generated Identity Provider & setup types match the OpenAPI additionsThe new
IdentityProviderTypeconstants,IdentityProvider/IdentityProviderRequest,InstanceStatus,SetupRequest/SetupResponse,Userextensions (IdpId,Password), and the associated request‑body aliases are consistent with the OpenAPI schemas and with the server’s domain types. No issues on the generated client surface itself; remaining alignment tweaks are confined to the YAML.Also applies to: 537-575, 709-714, 1895-1914, 1917-1960, 2092-2097, 2152-2154
shared/management/http/api/openapi.yml (1)
2270-2281: ExtendIdentityProviderTypeenum to include all backend‑supported typesThe OpenAPI
IdentityProviderTypeenum only listsoidc,zitadel,entra,okta,pocketid,microsoft, but the server type (management/server/types/identity_provider.go) also definesauthentikandkeycloak. As‑is, clients cannot legally send those values according to the spec even though the backend supports them.Recommend adding
authentikandkeycloakto this enum so the API surface matches server capabilities.Likely an incorrect or invalid review comment.
| if disableSingleAccMode { | ||
| // Embedded IdP requires single account mode - multiple account mode is not supported | ||
| return fmt.Errorf("embedded IdP requires single account mode; multiple account mode is not supported with embedded IdP. Please remove --disable-single-account-mode flag") | ||
| } | ||
| // Enable user deletion from IDP by default if EmbeddedIdP is enabled | ||
| userDeleteFromIDPEnabled = true |
There was a problem hiding this comment.
Avoid modifying global variable during config loading.
Line 188 sets the global userDeleteFromIDPEnabled flag as a side effect of loading embedded IdP configuration. This tight coupling can cause issues:
- Test isolation problems if config is loaded multiple times
- Hidden state mutation makes the function harder to reason about
- The global is later passed to
newServer()at line 112, but its value depends on whether this function ran
Consider returning the userDeleteFromIDPEnabled value from loadMgmtConfig or passing it through a config struct, then setting it explicitly in PreRunE or RunE.
🔎 Suggested refactoring to eliminate global mutation
Option 1: Return the flag from loadMgmtConfig:
-func loadMgmtConfig(ctx context.Context, mgmtConfigPath string) (*nbconfig.Config, error) {
+func loadMgmtConfig(ctx context.Context, mgmtConfigPath string) (*nbconfig.Config, bool, error) {
loadedConfig := &nbconfig.Config{}
if _, err := util.ReadJsonWithEnvSub(mgmtConfigPath, loadedConfig); err != nil {
- return nil, err
+ return nil, false, err
}
- applyCommandLineOverrides(loadedConfig)
+ userDeleteEnabled := false
+ applyCommandLineOverrides(loadedConfig)
// Apply EmbeddedIdP config to HttpConfig if embedded IdP is enabled
- applyEmbeddedIdPConfig(loadedConfig)
+ if err := applyEmbeddedIdPConfig(loadedConfig, &userDeleteEnabled); err != nil {
+ return nil, false, err
+ }
if err := applyOIDCConfig(ctx, loadedConfig); err != nil {
- return nil, err
+ return nil, false, err
}
logConfigInfo(loadedConfig)
if err := ensureEncryptionKey(ctx, mgmtConfigPath, loadedConfig); err != nil {
- return nil, err
+ return nil, false, err
}
- return loadedConfig, nil
+ return loadedConfig, userDeleteEnabled, nil
}
-func applyEmbeddedIdPConfig(cfg *nbconfig.Config) error {
+func applyEmbeddedIdPConfig(cfg *nbconfig.Config, userDeleteEnabled *bool) error {
if cfg.EmbeddedIdP == nil || !cfg.EmbeddedIdP.Enabled {
return nil
}
// apply some defaults based on the EmbeddedIdP config
if disableSingleAccMode {
// Embedded IdP requires single account mode - multiple account mode is not supported
return fmt.Errorf("embedded IdP requires single account mode; multiple account mode is not supported with embedded IdP. Please remove --disable-single-account-mode flag")
}
// Enable user deletion from IDP by default if EmbeddedIdP is enabled
- userDeleteFromIDPEnabled = true
+ *userDeleteEnabled = trueThen in PreRunE at line 58:
- config, err = loadMgmtConfig(ctx, nbconfig.MgmtConfigPath)
+ var userDeleteEnabled bool
+ config, userDeleteEnabled, err = loadMgmtConfig(ctx, nbconfig.MgmtConfigPath)
if err != nil {
return fmt.Errorf("failed reading provided config file: %s: %v", nbconfig.MgmtConfigPath, err)
}
+ if userDeleteEnabled {
+ userDeleteFromIDPEnabled = true
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @management/cmd/management.go around lines 183 - 188, loadMgmtConfig must not
mutate the global userDeleteFromIDPEnabled; instead return the computed value
(or include it in a config struct) from loadMgmtConfig and propagate that value
to where the server is constructed (newServer) so global state is not changed
during config loading; remove the assignment to the global inside
loadMgmtConfig, update callers (e.g., PreRunE / RunE) to receive the returned
flag/config and set userDeleteFromIDPEnabled explicitly or pass the flag into
newServer when creating the server.
| // updateIdentityProvider updates an existing identity provider | ||
| func (h *handler) updateIdentityProvider(w http.ResponseWriter, r *http.Request) { | ||
| userAuth, err := nbcontext.GetUserAuthFromContext(r.Context()) | ||
| if err != nil { | ||
| util.WriteError(r.Context(), err, w) | ||
| return | ||
| } | ||
|
|
||
| accountID, userID := userAuth.AccountId, userAuth.UserId | ||
|
|
||
| vars := mux.Vars(r) | ||
| idpID := vars["idpId"] | ||
| if idpID == "" { | ||
| util.WriteError(r.Context(), status.Errorf(status.InvalidArgument, "identity provider ID is required"), w) | ||
| return | ||
| } | ||
|
|
||
| var req api.IdentityProviderRequest | ||
| if err := json.NewDecoder(r.Body).Decode(&req); err != nil { | ||
| util.WriteErrorResponse("couldn't parse JSON request", http.StatusBadRequest, w) | ||
| return | ||
| } | ||
|
|
||
| idp := fromAPIRequest(&req) | ||
|
|
||
| updated, err := h.accountManager.UpdateIdentityProvider(r.Context(), accountID, idpID, userID, idp) | ||
| if err != nil { | ||
| util.WriteError(r.Context(), err, w) | ||
| return | ||
| } | ||
|
|
||
| util.WriteJSONObject(r.Context(), w, toAPIResponse(updated)) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the API request structure definition
rg -A 20 "type IdentityProviderRequest" --type=goRepository: netbirdio/netbird
Length of output: 1439
🏁 Script executed:
# Find the fromAPIRequest function to see how it handles ClientSecret
rg -B 5 -A 20 "func fromAPIRequest" --type=goRepository: netbirdio/netbird
Length of output: 1150
🏁 Script executed:
# Find the UpdateIdentityProvider implementation in accountManager
rg -B 5 -A 30 "func.*UpdateIdentityProvider" --type=goRepository: netbirdio/netbird
Length of output: 9152
🏁 Script executed:
# Look for ClientSecret field in the API request and understand its properties
rg -B 2 -A 2 "ClientSecret" management/server/http/handlers/idp/ --type=goRepository: netbirdio/netbird
Length of output: 2842
Fix ClientSecret handling in update operations—credentials will be overwritten with empty values.
The update handler uses fromAPIRequest() which unconditionally copies the ClientSecret from the request body. Since the IdentityProviderRequest schema defines ClientSecret as a non-nullable string field (not omitempty), omitting it from update requests results in an empty string that overwrites the existing credential.
The test case confirms this: the update request deliberately omits client_secret, which will set the internal ClientSecret to an empty string and break the identity provider configuration.
Standard patterns for credential updates require either:
- Preserving the existing secret when not provided in the request
- Using a pointer or optional wrapper type to distinguish "not provided" from "clear secret"
- Implementing a separate endpoint for credential updates
🤖 Prompt for AI Agents
In @management/server/http/handlers/idp/idp_handler.go around lines 115 - 147,
The updateIdentityProvider handler currently uses fromAPIRequest which
unconditionally copies IdentityProviderRequest.ClientSecret and thus overwrites
an existing credential when the update request omits client_secret; fix by
loading the existing provider before calling UpdateIdentityProvider (e.g. call
h.accountManager.GetIdentityProvider or similar to fetch current provider using
accountID and idpID) and, after building idp from fromAPIRequest(&req), if
req.ClientSecret is the empty string then set idp.ClientSecret =
existing.ClientSecret so the secret is preserved when not provided in the
request; then call h.accountManager.UpdateIdentityProvider as before.
| type: object | ||
| description: Request to set up the initial admin user | ||
| properties: | ||
| email: | ||
| description: Email address for the admin user | ||
| type: string | ||
| example: admin@example.com | ||
| password: | ||
| description: Password for the admin user (minimum 8 characters) | ||
| type: string | ||
| format: password | ||
| minLength: 8 | ||
| example: securepassword123 | ||
| name: | ||
| description: Display name for the admin user (defaults to email if not provided) | ||
| type: string | ||
| example: Admin User | ||
| required: | ||
| - password | ||
| - name |
There was a problem hiding this comment.
Fix SetupRequest.name description to match that it is required
SetupRequest marks name as required, but the description still says “defaults to email if not provided”. With name now required, the “defaults” behavior no longer applies.
Either:
- Remove the “defaults to email if not provided” text and keep
namerequired, or - Make
nameoptional again if you still want the defaulting behavior.
Right now the description and schema disagree.
🤖 Prompt for AI Agents
In @shared/management/http/api/openapi.yml around lines 2346 - 2366, The OpenAPI
schema for SetupRequest has a mismatch: the property SetupRequest.name is marked
required but its description still says "defaults to email if not provided";
update the schema so the description matches the required behavior by removing
the "defaults to email if not provided" phrase (or alternatively, if you prefer
defaulting, make SetupRequest.name not required); ensure the change is applied
to the name property description in shared/management/http/api/openapi.yml so
the description and required list are consistent.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
management/server/user.go (1)
189-215: Consider optimizing duplicate email check for large accounts.The duplicate email check (lines 203-212) loads all account users via
GetAccountUsersand iterates through them. For accounts with many users, this could be inefficient.Suggestion: Add a Store method to check email existence directly (e.g.,
Store.GetUserByEmail) to avoid loading all users into memory.Note: The embedded IdP's
CreateUsershould also enforce email uniqueness, providing an additional safety layer.💡 Potential optimization
Add a Store method for direct email lookup:
// In store interface: GetUserByEmail(ctx context.Context, accountID, email string) (*types.User, error)Then update the duplicate check:
- // check if the user is already registered with this email => reject - existingUsers, err := am.Store.GetAccountUsers(ctx, store.LockingStrengthNone, accountID) - if err != nil { - return nil, err - } - - for _, user := range existingUsers { - if strings.EqualFold(user.Email, invite.Email) { - return nil, status.Errorf(status.UserAlreadyExists, "can't invite a user with an existing NetBird account") - } - } + // check if the user is already registered with this email => reject + existingUser, err := am.Store.GetUserByEmail(ctx, accountID, invite.Email) + if err != nil && !errors.Is(err, status.ErrNotFound) { + return nil, err + } + if existingUser != nil { + return nil, status.Errorf(status.UserAlreadyExists, "can't invite a user with an existing NetBird account") + }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
management/server/user.go
🧰 Additional context used
🧬 Code graph analysis (1)
management/server/user.go (3)
management/server/types/user.go (2)
NewUser(228-241)UserInfo(56-74)management/server/account.go (1)
IsEmbeddedIdp(772-778)idp/dex/provider.go (1)
DecodeDexUserID(515-562)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Client / Unit (386)
- GitHub Check: Management / Benchmark (amd64, postgres)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Benchmark (API) (amd64, postgres)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Management / Benchmark (amd64, sqlite)
- GitHub Check: Management / Integration (amd64, postgres)
- GitHub Check: Management / Benchmark (API) (amd64, sqlite)
- GitHub Check: Signal / Unit (amd64)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Management / Integration (amd64, sqlite)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Client / Unit
- GitHub Check: release
- GitHub Check: test-docker-compose (sqlite)
- GitHub Check: release_ui_darwin
- GitHub Check: test-docker-compose (postgres)
- GitHub Check: test-docker-compose (mysql)
- GitHub Check: test-getting-started-script
- GitHub Check: Linux
- GitHub Check: Android / Build
- GitHub Check: Windows
- GitHub Check: JS / Lint
- GitHub Check: Client / Unit
- GitHub Check: Darwin
- GitHub Check: Client / Unit
🔇 Additional comments (8)
management/server/user.go (8)
44-44: LGTM: Service user initialization with empty email/name is appropriate.Service users are system accounts and don't require email or name fields, so passing empty strings is correct.
108-113: Routing logic is appropriate given different validation requirements.While a past review comment suggested consolidation, the two paths (
createEmbeddedIdpUservs.createNewIdpUser) have fundamentally different implementations:
- Embedded IdP validates against local DB users (lines 203-212)
- External IdP validates against IdP cache and external system (lines 168-184)
The conditional branching keeps each path's logic clear and separated.
134-145: LGTM: Cache refresh and event type correctly differentiate embedded vs. external IdP flows.The conditional logic appropriately handles the two scenarios:
- Lines 134-139: Cache refresh is skipped for embedded IdP (no external cache exists) and performed for external IdP to sync account data.
- Lines 141-145: Event type is
UserCreatedfor embedded IdP (user created locally) andUserInvitedfor external IdP (user invited but not yet activated).This addresses the past review comments about handling create vs. invite flows with distinct event types.
804-804: LGTM: Correctly skips IdP cache lookup for embedded IdP.The guard condition ensures that cache lookups are only performed for external IdP users, which is correct since embedded IdP stores user data locally in the database.
934-975: LGTM: Embedded IdP support correctly integrated with Dex ID decoding.The changes appropriately handle both embedded and external IdP scenarios:
- Line 935: Skips external cache lookups when using embedded IdP (user data is stored locally).
- Lines 972-975: Extracts the IdP connector ID from Dex-encoded user IDs to populate
IdPID. Decode errors are safely ignored since not all user IDs are Dex-encoded.
997-997: LGTM: Email field and Dex ID decoding correctly handled.
- Line 997: Using
localUser.Emailfor users not found inqueriedUsers(e.g., service users) is correct for embedded IdP and acceptable for external IdP (where service users may have empty emails).- Lines 1006-1009: Dex ID decoding ensures the
IdPIDfield is populated for all users with Dex-encoded IDs, matching the logic at lines 972-975.Also applies to: 1006-1009
855-869: All callers ofGetOrCreateAccountByUserhave been updated to the new signature. No issues found—the method signature change from(ctx, userID, domain string)to(ctx, userAuth auth.UserAuth)is consistently reflected across all 22 call sites in production code (account.go:751), test files, mock implementations, and the interface definition.
126-127: Email and Name fields are correctly set during user invitation and appropriately handled by architecture.The concerns raised are:
"User email and name are empty here" — This is incorrect. Lines 126-127 populate Email and Name from the invite object before saving to the database.
"Risk of constant DB writes with inconsistent data" — This is not a valid concern. Each user invitation populates both fields consistently. There's no scenario where one request saves with values and another without.
Data staleness for external IdP — This is addressed by design:
BuildUserInfosForAccount(lines 930-950) retrieves user data from the external cache manager for non-embedded IdP users, not from the stored DB fields. The Email/Name stored in the database are not actively used for external IdP users—the cache is authoritative.Email/Name cannot be updated after creation — The
SaveOrAddUsercomment confirms onlyAutoGroups,Role, andBlockedfields are updatable. Email/Name are intentionally immutable after user creation, which is consistent with the system's user identity model.No action required—the current implementation is correct.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @management/server/identity_provider.go:
- Around line 30-34: GetIdentityProviders currently returns an empty slice and a
warning when am.idpManager isn't an *idp.EmbeddedIdPManager, while
GetIdentityProvider/CreateIdentityProvider/UpdateIdentityProvider/DeleteIdentityProvider
return Internal errors—make behavior consistent by returning the same Internal
gRPC error in GetIdentityProviders as the others; detect the failed type
assertion on embeddedManager (am.idpManager.(*idp.EmbeddedIdPManager)) and
return status.Errorf(codes.Internal, "identity provider management requires
embedded IdP") (matching the other methods) instead of returning an empty slice
and nil error.
- Around line 136-138: Update UpdateIdentityProvider to special-case
storage.ErrNotFound from embeddedManager.UpdateConnector: after calling
embeddedManager.UpdateConnector(ctx, connCfg) check if errors.Is(err,
storage.ErrNotFound) and return a status.NotFound error (with a clear message
referencing the provider ID), otherwise preserve the existing status.Internal
behavior for other errors; reference UpdateIdentityProvider,
embeddedManager.UpdateConnector, storage.ErrNotFound and status.NotFound when
making the change.
- Around line 85-87: The Validate() method on the IdP config currently checks
Name, Type, Issuer, and ClientID but omits ClientSecret; update the Validate()
implementation to require a non-empty ClientSecret (e.g., trim spaces and ensure
len>0) and return status.Errorf(status.InvalidArgument, "%s", "<meaningful
error>") (or propagate the same error pattern used for other fields) when
ClientSecret is missing so callers like the idpConfig.Validate() call will
reject configs without a client secret.
🧹 Nitpick comments (2)
management/server/identity_provider.go (2)
102-105: Consider handling duplicate ID conflicts explicitly.
CreateConnectormay fail if an identity provider with the same ID already exists, but the current error handling treats all errors asInternal. While the generated IDs are highly unlikely to collide, explicitly checking for duplicate/conflict errors and returningAlreadyExistsstatus would provide clearer feedback to API consumers.🔎 Example adjustment
_, err = embeddedManager.CreateConnector(ctx, connCfg) if err != nil { + if errors.Is(err, storage.ErrAlreadyExists) { + return nil, status.Errorf(status.AlreadyExists, "identity provider with ID %s already exists", idpConfig.ID) + } return nil, status.Errorf(status.Internal, "failed to create identity provider: %v", err) }
161-175: Potential TOCTOU race between get and delete operations.The function fetches the connector (line 161) and then deletes it (line 170) in two separate calls. In a concurrent environment, another request could delete the same connector between these calls, causing the delete operation to fail with
NotFoundeven though the get succeeded.While the error handling on line 171-173 correctly maps this to
NotFound, consider whether this two-step approach could cause confusion or if a single atomic delete-and-return operation would be preferable (if supported by the embedded IdP manager).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
management/server/identity_provider.go
🧰 Additional context used
🧬 Code graph analysis (1)
management/server/identity_provider.go (5)
management/server/types/identity_provider.go (2)
IdentityProvider(43-58)IdentityProviderType(19-19)shared/management/status/error.go (3)
NewPermissionValidationError(213-215)NewPermissionDeniedError(209-211)Error(54-57)management/server/idp/embedded.go (1)
EmbeddedIdPManager(161-165)management/server/activity/codes.go (3)
IdentityProviderCreated(186-186)IdentityProviderUpdated(187-187)IdentityProviderDeleted(188-188)idp/dex/provider.go (1)
ConnectorConfig(629-644)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (31)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Benchmark (API) (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Client / Unit (386)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Management / Benchmark (API) (amd64, postgres)
- GitHub Check: Management / Benchmark (amd64, postgres)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Management / Benchmark (amd64, sqlite)
- GitHub Check: Management / Integration (amd64, postgres)
- GitHub Check: Management / Integration (amd64, sqlite)
- GitHub Check: Relay / Unit (386)
- GitHub Check: release_ui_darwin
- GitHub Check: release_ui
- GitHub Check: release
- GitHub Check: Linux
- GitHub Check: iOS / Build
- GitHub Check: test-docker-compose (postgres)
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
- GitHub Check: Darwin
- GitHub Check: Windows
- GitHub Check: test-docker-compose (sqlite)
- GitHub Check: test-getting-started-script
- GitHub Check: test-docker-compose (mysql)
- GitHub Check: Check External GPL/AGPL Licenses
🔇 Additional comments (3)
management/server/identity_provider.go (3)
50-73: LGTM!The function correctly handles permission checks, type assertions, and error cases including
storage.ErrNotFound. The logic is sound.
196-205: LGTM!The conversion correctly preserves all fields including
ClientSecret, which is appropriate for create/update operations where the secret must be passed to the embedded IdP.
210-234: LGTM!The ID generation logic is sound:
- Uses
xidfor uniqueness guarantees- Prefixes known provider types for clarity
- Falls back to unprefixed IDs for generic OIDC
The switch statement covers all provider types defined in the codebase.
| embeddedManager, ok := am.idpManager.(*idp.EmbeddedIdPManager) | ||
| if !ok { | ||
| log.Warn("identity provider management requires embedded IdP") | ||
| return []*types.IdentityProvider{}, nil | ||
| } |
There was a problem hiding this comment.
Inconsistent behavior when embedded IdP is unavailable.
GetIdentityProviders returns an empty slice with a warning when the embedded IdP is unavailable, but GetIdentityProvider, CreateIdentityProvider, UpdateIdentityProvider, and DeleteIdentityProvider all return Internal errors in the same situation. This inconsistency may confuse API consumers.
Consider returning a consistent error (e.g., status.Errorf(status.Internal, "identity provider management requires embedded IdP")) across all methods.
🤖 Prompt for AI Agents
In @management/server/identity_provider.go around lines 30 - 34,
GetIdentityProviders currently returns an empty slice and a warning when
am.idpManager isn't an *idp.EmbeddedIdPManager, while
GetIdentityProvider/CreateIdentityProvider/UpdateIdentityProvider/DeleteIdentityProvider
return Internal errors—make behavior consistent by returning the same Internal
gRPC error in GetIdentityProviders as the others; detect the failed type
assertion on embeddedManager (am.idpManager.(*idp.EmbeddedIdPManager)) and
return status.Errorf(codes.Internal, "identity provider management requires
embedded IdP") (matching the other methods) instead of returning an empty slice
and nil error.
| if err := idpConfig.Validate(); err != nil { | ||
| return nil, status.Errorf(status.InvalidArgument, "%s", err.Error()) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the IdentityProvider type definition and Validate method
rg -A 30 "func.*Validate\(\).*error" management/server/identity_provider.goRepository: netbirdio/netbird
Length of output: 43
🏁 Script executed:
# Also search for the IdentityProvider struct definition to understand its fields
rg -B 5 -A 20 "type IdentityProvider struct" management/server/Repository: netbirdio/netbird
Length of output: 2124
🏁 Script executed:
# Check if there are any proto definitions for IdentityProvider
fd -e proto | xargs rg "IdentityProvider" 2>/dev/null | head -20Repository: netbirdio/netbird
Length of output: 43
🏁 Script executed:
# Search in the types directory for Validate method
rg -B 2 -A 25 "func.*\*IdentityProvider.*Validate" management/server/types/Repository: netbirdio/netbird
Length of output: 2076
🏁 Script executed:
# Also check the actual file being reviewed for Validate implementation
cat management/server/identity_provider.go | head -100Repository: netbirdio/netbird
Length of output: 3644
🏁 Script executed:
# Search for any Validate method related to IdentityProvider
rg "Validate\(\)" management/server/types/identity_provider.go -B 3 -A 30Repository: netbirdio/netbird
Length of output: 1145
🏁 Script executed:
# Check if ClientSecret is used in the code and if there are any hints about whether it should be required
rg "ClientSecret" management/server/types/identity_provider.go -B 2 -A 2Repository: netbirdio/netbird
Length of output: 283
🏁 Script executed:
# Check how ClientSecret is used when creating connectors
rg "ClientSecret" management/server/identity_provider.go -B 3 -A 3Repository: netbirdio/netbird
Length of output: 347
🏁 Script executed:
# Check the HasBuiltInIssuer method to understand which types have special handling
rg -A 15 "func.*HasBuiltInIssuer" management/server/types/Repository: netbirdio/netbird
Length of output: 331
Add validation for ClientSecret field.
The Validate() method validates Name, Type, Issuer (conditionally), and ClientID, but does not validate ClientSecret. Since ClientSecret is essential for OAuth2/OIDC authentication flows and is passed to all connector configurations, it should be validated as a required field.
🤖 Prompt for AI Agents
In @management/server/identity_provider.go around lines 85 - 87, The Validate()
method on the IdP config currently checks Name, Type, Issuer, and ClientID but
omits ClientSecret; update the Validate() implementation to require a non-empty
ClientSecret (e.g., trim spaces and ensure len>0) and return
status.Errorf(status.InvalidArgument, "%s", "<meaningful error>") (or propagate
the same error pattern used for other fields) when ClientSecret is missing so
callers like the idpConfig.Validate() call will reject configs without a client
secret.
| if err := embeddedManager.UpdateConnector(ctx, connCfg); err != nil { | ||
| return nil, status.Errorf(status.Internal, "failed to update identity provider: %v", err) | ||
| } |
There was a problem hiding this comment.
Missing NotFound error handling for non-existent identity providers.
UpdateIdentityProvider returns Internal status for any error from UpdateConnector, including when the identity provider doesn't exist. In contrast, DeleteIdentityProvider (lines 171-173) correctly maps storage.ErrNotFound to a NotFound status. This inconsistency means clients receive a 500 error on update but a 404 on delete for the same missing resource.
Special-case storage.ErrNotFound to return status.NotFound and align with the delete behavior.
🔎 Proposed fix
if err := embeddedManager.UpdateConnector(ctx, connCfg); err != nil {
+ if errors.Is(err, storage.ErrNotFound) {
+ return nil, status.Errorf(status.NotFound, "identity provider not found")
+ }
return nil, status.Errorf(status.Internal, "failed to update identity provider: %v", err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err := embeddedManager.UpdateConnector(ctx, connCfg); err != nil { | |
| return nil, status.Errorf(status.Internal, "failed to update identity provider: %v", err) | |
| } | |
| if err := embeddedManager.UpdateConnector(ctx, connCfg); err != nil { | |
| if errors.Is(err, storage.ErrNotFound) { | |
| return nil, status.Errorf(status.NotFound, "identity provider not found") | |
| } | |
| return nil, status.Errorf(status.Internal, "failed to update identity provider: %v", err) | |
| } |
🤖 Prompt for AI Agents
In @management/server/identity_provider.go around lines 136 - 138, Update
UpdateIdentityProvider to special-case storage.ErrNotFound from
embeddedManager.UpdateConnector: after calling
embeddedManager.UpdateConnector(ctx, connCfg) check if errors.Is(err,
storage.ErrNotFound) and return a status.NotFound error (with a clear message
referencing the provider ID), otherwise preserve the existing status.Internal
behavior for other errors; reference UpdateIdentityProvider,
embeddedManager.UpdateConnector, storage.ErrNotFound and status.NotFound when
making the change.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @management/cmd/management.go:
- Around line 241-246: The skip condition in applyOIDCConfig incorrectly skips
external OIDC discovery whenever cfg.EmbeddedIdP is present, even if it's
disabled; update the condition to only skip when an embedded IdP is present AND
enabled by changing the guard to check (cfg.EmbeddedIdP != nil &&
cfg.EmbeddedIdP.Enabled) so external discovery runs when EmbeddedIdP exists but
Enabled is false.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
management/cmd/management.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: Build Cache
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
- GitHub Check: FreeBSD Port / Build & Test
- GitHub Check: Darwin
- GitHub Check: release
- GitHub Check: JS / Lint
- GitHub Check: release_ui
- GitHub Check: Client / Unit
- GitHub Check: release_ui_darwin
- GitHub Check: Windows
- GitHub Check: Linux
- GitHub Check: test-docker-compose (postgres)
- GitHub Check: test-getting-started-script
- GitHub Check: test-docker-compose (sqlite)
- GitHub Check: test-docker-compose (mysql)
- GitHub Check: Android / Build
- GitHub Check: iOS / Build
- GitHub Check: Check External GPL/AGPL Licenses
🔇 Additional comments (7)
management/cmd/management.go (7)
137-162: Well-structured config loading flow.The refactored
loadMgmtConfigfunction now follows a clear staged initialization pattern with modular helpers. This improves maintainability and testability.
165-176: LGTM!Command-line override logic is clear and straightforward.
272-297: LGTM!Device authorization flow configuration logic is sound. The URL parsing to extract the domain is appropriate, and the default scope handling is reasonable.
300-311: LGTM!PKCE flow configuration is straightforward and correct.
314-321: LGTM!Informational logging is helpful for debugging and operational visibility.
186-191: Avoid modifying global variable during config loading.Line 191 sets the global
userDeleteFromIDPEnabledflag as a side effect of loading embedded IdP configuration. This tight coupling can cause issues with test isolation and makes the function harder to reason about.Consider returning the
userDeleteFromIDPEnabledvalue fromloadMgmtConfigor passing it through a config struct, then setting it explicitly inPreRunEorRunE.Likely an incorrect or invalid review comment.
324-341: No changes needed. The implementation ofcrypt.GenerateKey()is cryptographically sound:
- Generates a 32-byte key suitable for AES-256
- Uses
crypto/rand.Reader(a cryptographically secure RNG)- Employs
io.ReadFull()to ensure full entropy is read- Returns a base64-encoded string compatible with
FieldEncrypt
| // applyOIDCConfig fetches and applies OIDC configuration if endpoint is specified | ||
| func applyOIDCConfig(ctx context.Context, cfg *nbconfig.Config) error { | ||
| oidcEndpoint := cfg.HttpConfig.OIDCConfigEndpoint | ||
| if oidcEndpoint == "" || cfg.EmbeddedIdP != nil { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Verify the skip condition for OIDC config fetching.
Line 244 skips OIDC configuration if cfg.EmbeddedIdP != nil, but it should probably check cfg.EmbeddedIdP != nil && cfg.EmbeddedIdP.Enabled.
As written, if the EmbeddedIdP struct exists in config but Enabled is false, external OIDC discovery will be skipped even though the embedded IdP is not active. This prevents users from disabling the embedded IdP and falling back to an external IdP without removing the EmbeddedIdP config section.
🔎 Proposed fix
func applyOIDCConfig(ctx context.Context, cfg *nbconfig.Config) error {
oidcEndpoint := cfg.HttpConfig.OIDCConfigEndpoint
- if oidcEndpoint == "" || cfg.EmbeddedIdP != nil {
+ if oidcEndpoint == "" || (cfg.EmbeddedIdP != nil && cfg.EmbeddedIdP.Enabled) {
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // applyOIDCConfig fetches and applies OIDC configuration if endpoint is specified | |
| func applyOIDCConfig(ctx context.Context, cfg *nbconfig.Config) error { | |
| oidcEndpoint := cfg.HttpConfig.OIDCConfigEndpoint | |
| if oidcEndpoint == "" || cfg.EmbeddedIdP != nil { | |
| return nil | |
| } | |
| // applyOIDCConfig fetches and applies OIDC configuration if endpoint is specified | |
| func applyOIDCConfig(ctx context.Context, cfg *nbconfig.Config) error { | |
| oidcEndpoint := cfg.HttpConfig.OIDCConfigEndpoint | |
| if oidcEndpoint == "" || (cfg.EmbeddedIdP != nil && cfg.EmbeddedIdP.Enabled) { | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In @management/cmd/management.go around lines 241 - 246, The skip condition in
applyOIDCConfig incorrectly skips external OIDC discovery whenever
cfg.EmbeddedIdP is present, even if it's disabled; update the condition to only
skip when an embedded IdP is present AND enabled by changing the guard to check
(cfg.EmbeddedIdP != nil && cfg.EmbeddedIdP.Enabled) so external discovery runs
when EmbeddedIdP exists but Enabled is false.
mlsmaycon
changed the title
|
@CodeRabbit, please update the summary |
|
@mlsmaycon, I'll regenerate the summary for this PR. I'll incorporate the feedback from the comment chain, including the dashboard UI visibility issue raised by markcst and the runtime initialization error you encountered with the JWT validator. Context for summary update:
✅ Actions performedSummary regeneration triggered. |
@CodeRabbit, better to summarize the changes of this PR and update the initial summary you provided in the PR description section |
|
@mlsmaycon, here's a comprehensive summary of the changes in this PR that you can use to update the PR description: SummaryThis PR introduces an embedded Dex OIDC identity provider to NetBird, eliminating the need for external IdP setup during self-hosting. Users can now manage local NetBird users directly and optionally configure multiple external identity providers through the Dashboard UI. Core Components1. Embedded Dex IdP (
|
5 participants
Describe your changes
This branch adds embedded Dex OIDC identity provider support to NetBird, eliminating the need for external IdP setup during self-hosting. The users can now add local NetBird users without a need for an external IdP. They also can add multiple external providers if needed right from the Dahsboard UI (optional).
Key Features
- Built-in OIDC identity provider using Dex
- User management (create, delete, list)
- External IdP connector support (Google, GitHub, OIDC, SAML)
- Device auth flow for CLI authentication
- New onboarding endpoints for initial setup
- Instance manager to check if setup is required
- Create owner user flow when no accounts exist
- Field-level encryption for sensitive data (email, name)
- AES-256-GCM encryption with base64 encoding
- Encryption key auto-generation in config
- Simplified setup without external IdP container
- No reverse proxy complexity - IdP built into management
- Auto-generates encryption keys and configs
- IdP connector CRUD endpoints
- User management with embedded IdP
- OAuth config provider interface
Issue ticket number and link
#3630
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
netbirdio/docs#533
Summary by CodeRabbit
New Features
Security
API
Chores
Tests / Infra
✏️ Tip: You can customize this high-level summary in your review settings.