[management] Legacy to embedded IdP migration tool#5586
Open
shuuri-labs wants to merge 21 commits intomainfrom
Open
[management] Legacy to embedded IdP migration tool#5586shuuri-labs wants to merge 21 commits intomainfrom
shuuri-labs wants to merge 21 commits intomainfrom
Conversation
…ation tool Move ListUsers/UpdateUserID out of store.Store and activity.Store into migration-specific interfaces (MigrationStore, MigrationEventStore) so migration code can be cleanly removed later. Add tools/idp-migrate CLI that migrates user IDs and generates EmbeddedIdP management.json config
Contributor
📝 WalkthroughWalkthroughAdds a standalone IdP migration CLI plus migration interfaces, SQL/activity-store migration implementations, Dex connector mapping and defaults, embedded IdP static connector wiring, release packaging, tests, and documentation to migrate from external IdPs to the embedded Dex-based IdP. Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as "idp-migrate CLI"
participant Store as "Main SQL Store"
participant Activity as "Activity Store"
participant IdP as "External IdP/Manager"
User->>CLI: run migration (flags / management.json)
CLI->>Store: CheckSchema()
Store-->>CLI: schema valid / errors
alt schema invalid
CLI->>User: abort with schema errors
else schema valid
CLI->>IdP: Optional PopulateUserInfo / GetAllAccounts()
IdP-->>CLI: accounts (optional)
CLI->>Store: ListUsers()
Store-->>CLI: users
CLI->>CLI: resolve connector (flag > env > config)
CLI->>Store: MigrateUsersToStaticConnectors()
Store->>Store: Begin Transaction / UpdateUserID (main tables)
Store->>Activity: UpdateUserID() (update event refs)
Activity-->>Store: event refs updated
Store-->>CLI: migration complete
CLI->>CLI: generate new management.json (embed EmbeddedIdP)
CLI->>User: output result / write backup (unless dry-run)
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
…complexity, fix code smells
10 tasks
jnfrati
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
tools/idp-migrate/DEVELOPMENT.md (1)
12-31: Add language specifier to fenced code blocks for markdownlint compliance.The directory structure code blocks at lines 12 and 37 lack language specifiers, triggering MD040 warnings. Consider adding
textorplaintextas the language.📝 Suggested fix
-``` +```text tools/idp-migrate/ ├── main.go # CLI entry point, connector resolution, config generationAnd similarly for line 37:
-``` +```text netbird-idp-migrate_<version>_linux_<arch>.tar.gz🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/idp-migrate/DEVELOPMENT.md` around lines 12 - 31, The fenced directory/listing code blocks in DEVELOPMENT.md trigger markdownlint MD040 because they lack a language specifier; update the two blocks shown (the tools/idp-migrate/ tree block and the tarball name block) to use a plain text specifier (e.g., change the opening "```" to "```text") so markdownlint recognizes them as plaintext listings and the MD040 warnings are resolved.management/server/activity/store/sql_store_idp_migration_test.go (1)
17-26: Consider handling GenerateKey error in test helper.Line 19 ignores the error from
crypt.GenerateKey(). While unlikely to fail, handling it would make test failures more diagnosable.📝 Suggested fix
newStore := func(t *testing.T) *Store { t.Helper() - key, _ := crypt.GenerateKey() + key, err := crypt.GenerateKey() + if err != nil { + t.Fatal(err) + } s, err := NewSqlStore(ctx, t.TempDir(), key)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/activity/store/sql_store_idp_migration_test.go` around lines 17 - 26, The test helper newStore currently ignores the error returned by crypt.GenerateKey(); change it to capture the error (key, err := crypt.GenerateKey()) and fail the test immediately on error (e.g., t.Fatalf or t.Fatal with the error) before passing key into NewSqlStore. Update the newStore function so it checks err after GenerateKey and calls t.Fatalf("crypt.GenerateKey: %v", err) (or similar) to make failures diagnosable while keeping the existing t.Helper and t.Cleanup usage.tools/idp-migrate/MIGRATION_GUIDE.md (1)
233-242: Add language specifier to output code blocks for consistency.The code blocks showing expected output (lines 233, 266, 294, 381) lack language specifiers. Consider adding
textorconsolefor consistency with markdownlint MD040.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/idp-migrate/MIGRATION_GUIDE.md` around lines 233 - 242, Update the fenced code blocks that show expected CLI output in the migration guide so they include a language specifier (e.g., ```text or ```console) instead of bare triple backticks; specifically modify each example output block (the ones that begin with lines like "INFO resolved connector: type=oidc, id=auth0, name=auth0" and the subsequent "[DRY RUN]" summaries) to start with ```text (or ```console) and end with ``` to satisfy markdownlint MD040 and ensure consistent rendering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.goreleaser.yaml:
- Around line 173-174: The ldflags list references -X main.commit, -X main.date
and -X main.builtBy which are not declared; either remove those three -X entries
from ldflags or add matching package-level variable declarations in the
idp-migrate main package (e.g., declare commit, date, builtBy or follow the
existing pattern Version/Commit/BuildDate) so the linker substitutions have
corresponding symbols (look for the ldflags line and the tools/idp-migrate main
package entry points such as main.go to add the vars).
In `@management/server/idp/migration/migration.go`:
- Around line 164-169: The current flattening of allAccounts into idpUsers
(map[string]*idp.UserData) loses account scoping so duplicate external IDs
across accounts can overwrite each other; change idpUsers to be account-scoped
(e.g., map[string]map[string]*idp.UserData or similar keyed by accountID then
userID) and update the subsequent logic that uses idpUsers (the code around the
current lines 193-198) to look up users by accountID then userID instead of a
global lookup; also add a regression test that constructs two accounts
containing the same external ID and verifies each account's user retains its own
email/name after migration.
- Around line 214-225: The current migration logs plaintext PII (email and name)
in both dry-run and live paths; update the log calls around the dryRun branch
and after s.Store().UpdateUserInfo to avoid printing email/name and instead log
only the user identifier and which fields changed (e.g., "email" and/or "name")
and whether it was a dry run. Specifically, change the log.Infof calls that
reference user.Id, email, name to something like a dry-run message and a
live-update message that include user.Id and a concise list of changed fields
(not their values), keep updatedCount increments and the
s.Store().UpdateUserInfo call and error handling for user.Id as-is.
In `@tools/idp-migrate/main_test.go`:
- Around line 345-368: The test fixture uses a string that matches
secret-scanner patterns ("test-key-1234567890123456"); update the
DataStoreEncryptionKey value inside the configJSON fixture to a clearly
fake/non-secret placeholder (e.g. "example-encryption-key-0000") and update the
corresponding assertion that checks cfg.DataStoreEncryptionKey to expect the new
placeholder; locate the configJSON variable in the test and the assertions that
read cfg.DataStoreEncryptionKey (after loadConfig) to make these paired changes.
In `@tools/idp-migrate/main.go`:
- Around line 140-149: validateSchema currently only checks the main store
schema (migStore.CheckSchema) and misses the activity/event store; add a
readiness check for the activity store by calling the event store's schema
validator (e.g., via migStore.EventStore().CheckSchema(...) using the same
migration contract that EventStore().UpdateUserID expects) and if it returns
errors, return an error immediately (same style as the existing migStore check)
so the preflight fails early when an incompatible activity DB is present.
- Around line 335-339: Remove the debug print that can leak secrets: delete the
fmt.Println(conn, err) call after migration.SeedConnectorFromEnv() so the
decoded connector (including client secret) is not written to stdout; if you
need diagnostics, log only non-sensitive status or errors (keep the existing
error check that uses migration.ErrNoSeedInfo and the fmt.Errorf wrap in that
block).
In `@tools/idp-migrate/MIGRATION_GUIDE.md`:
- Line 22: Replace every occurrence of the literal placeholder
"<INSERT_VERSION>" in this document (notably the table entry on line 22 and the
other occurrence around line 458) with the actual release version string (e.g.,
"v0.60.0"); ensure both instances of "<INSERT_VERSION>" are updated consistently
so the table header and any other references show the real version before
publishing.
---
Nitpick comments:
In `@management/server/activity/store/sql_store_idp_migration_test.go`:
- Around line 17-26: The test helper newStore currently ignores the error
returned by crypt.GenerateKey(); change it to capture the error (key, err :=
crypt.GenerateKey()) and fail the test immediately on error (e.g., t.Fatalf or
t.Fatal with the error) before passing key into NewSqlStore. Update the newStore
function so it checks err after GenerateKey and calls
t.Fatalf("crypt.GenerateKey: %v", err) (or similar) to make failures diagnosable
while keeping the existing t.Helper and t.Cleanup usage.
In `@tools/idp-migrate/DEVELOPMENT.md`:
- Around line 12-31: The fenced directory/listing code blocks in DEVELOPMENT.md
trigger markdownlint MD040 because they lack a language specifier; update the
two blocks shown (the tools/idp-migrate/ tree block and the tarball name block)
to use a plain text specifier (e.g., change the opening "```" to "```text") so
markdownlint recognizes them as plaintext listings and the MD040 warnings are
resolved.
In `@tools/idp-migrate/MIGRATION_GUIDE.md`:
- Around line 233-242: Update the fenced code blocks that show expected CLI
output in the migration guide so they include a language specifier (e.g.,
```text or ```console) instead of bare triple backticks; specifically modify
each example output block (the ones that begin with lines like "INFO resolved
connector: type=oidc, id=auth0, name=auth0" and the subsequent "[DRY RUN]"
summaries) to start with ```text (or ```console) and end with ``` to satisfy
markdownlint MD040 and ensure consistent rendering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bf9329df-ea4c-400e-ab52-e4bba7033a25
📒 Files selected for processing (15)
.goreleaser.yamlidp/dex/config.goidp/dex/provider_test.gomanagement/internals/server/modules.gomanagement/server/activity/store/sql_store_idp_migration.gomanagement/server/activity/store/sql_store_idp_migration_test.gomanagement/server/idp/embedded.gomanagement/server/idp/migration/migration.gomanagement/server/idp/migration/migration_test.gomanagement/server/idp/migration/store.gomanagement/server/store/sql_store_idp_migration.gotools/idp-migrate/DEVELOPMENT.mdtools/idp-migrate/MIGRATION_GUIDE.mdtools/idp-migrate/main.gotools/idp-migrate/main_test.go
Comment on lines
+173
to
+174
| ldflags: | ||
| - -s -w -X github.com/netbirdio/netbird/version.version={{.Version}} -X main.commit={{.Commit}} -X main.date={{.CommitDate}} -X main.builtBy=goreleaser |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if main.go declares commit and date variables
echo "=== Checking for commit/date variable declarations in main.go ==="
rg -n "^\s*var\s+(commit|date|builtBy)" tools/idp-migrate/main.go || echo "No commit/date/builtBy variables found"
echo ""
echo "=== Checking for any package-level var blocks ==="
ast-grep --pattern $'var (
$$$
)' tools/idp-migrate/main.go || echo "No var blocks found"Repository: netbirdio/netbird
Length of output: 236
🏁 Script executed:
#!/bin/bash
# Check if commit/date/builtBy variables are actually used in the main.go file
echo "=== Searching for usage of commit, date, builtBy in tools/idp-migrate/main.go ==="
rg -n "(commit|date|builtBy)" tools/idp-migrate/main.go || echo "No usage found"
echo ""
echo "=== Checking how netbird main build target handles ldflags ==="
sed -n '100,140p' .goreleaser.yaml | grep -A 5 "ldflags:"
echo ""
echo "=== Checking how netbird-mgmt main build target handles ldflags ==="
sed -n '145,155p' .goreleaser.yaml | grep -A 5 "ldflags:"
echo ""
echo "=== Full netbird build target ldflags ==="
rg -A 2 "binary: netbird$" .goreleaser.yaml | head -20Repository: netbirdio/netbird
Length of output: 2031
🏁 Script executed:
#!/bin/bash
# Check where commit/date/builtBy variables ARE declared (if anywhere)
echo "=== Searching entire codebase for commit/date variable declarations ==="
rg -n "^\s*var\s+(commit|date|builtBy)\s+string" || echo "Not found with pattern 1"
echo ""
echo "=== Searching for all var declarations in main netbird main.go ==="
rg -n "var.*string" cmd/client/main.go | head -20
echo ""
echo "=== Check if cmd/client/main.go declares commit/date/builtBy ==="
ast-grep --pattern $'var (
$$$
)' cmd/client/main.go
echo ""
echo "=== Searching cmd/management main.go for variable declarations ==="
rg -n "var.*string" cmd/management/main.go | head -20Repository: netbirdio/netbird
Length of output: 564
🏁 Script executed:
#!/bin/bash
# Find the actual directory structure
echo "=== Finding main.go files ==="
find . -name "main.go" -type f | head -20
echo ""
echo "=== Check netbird-idp-migrate main.go full content (first 50 lines) ==="
head -50 tools/idp-migrate/main.go
echo ""
echo "=== Check if any of the main.go files use these variables ==="
rg "commit\s*=" . --type go | head -10
echo ""
echo "=== Check the actual netbird client main.go ==="
find . -path "*/cmd/*" -name "main.go" | head -5Repository: netbirdio/netbird
Length of output: 2310
🏁 Script executed:
#!/bin/bash
# Check client/main.go for variable declarations
echo "=== Check client/main.go for any var or const declarations ==="
head -100 ./client/main.go | tail -50
echo ""
echo "=== Search for global variable declarations in client/main.go ==="
rg -B 2 -A 2 "^var|^const" ./client/main.go | head -40
echo ""
echo "=== Search tools/idp-migrate/main.go for any global var/const ==="
rg -B 2 -A 2 "^var|^const" ./tools/idp-migrate/main.go
echo ""
echo "=== Check .goreleaser.yaml to see ALL build targets with ldflags ==="
rg -B 3 "ldflags:" .goreleaser.yamlRepository: netbirdio/netbird
Length of output: 1391
🏁 Script executed:
#!/bin/bash
# Search more carefully - maybe variables are initialized at build time but used elsewhere
echo "=== Search for usage of commit/date variables across entire codebase ==="
rg "commit|date|builtBy" tools/idp-migrate/main.go
echo ""
echo "=== Check if version package has anything related ==="
find . -path "*/version/*" -type f | head -10
echo ""
echo "=== Check management/main.go for variable declarations ==="
head -100 ./management/main.go | grep -A 5 "^var\|^const"
echo ""
echo "=== Check if the ldflags variables are used anywhere in management build ==="
rg "commit|date|builtBy" ./management/main.go
echo ""
echo "=== Get the exact ldflags line for netbird-idp-migrate from .goreleaser.yaml ==="
rg -B 5 -A 2 "netbird-idp-migrate" .goreleaser.yaml | grep -A 10 "ldflags"Repository: netbirdio/netbird
Length of output: 1568
🏁 Script executed:
#!/bin/bash
# Get the exact ldflags for netbird-idp-migrate more carefully
echo "=== Extract netbird-idp-migrate section ==="
sed -n '/id: netbird-idp-migrate/,/^ - id:/p' .goreleaser.yaml | head -40
echo ""
echo "=== Count how many ldflags entries exist in .goreleaser.yaml ==="
rg -c "ldflags:" .goreleaser.yaml
echo ""
echo "=== Show line numbers for all ldflags ==="
rg -n "ldflags:" .goreleaser.yamlRepository: netbirdio/netbird
Length of output: 1507
🏁 Script executed:
#!/bin/bash
# Check which build targets use main.commit and main.date
echo "=== Show all ldflags definitions ==="
rg -A 1 "ldflags:" .goreleaser.yaml
echo ""
echo "=== Check if any main.go files actually declare commit/date/builtBy variables ==="
for f in ./client/main.go ./management/main.go ./combined/main.go ./signal/main.go ./relay/main.go ./upload-server/main.go ./tools/idp-migrate/main.go; do
echo "=== $f ==="
rg "var.*commit|var.*date|var.*builtBy" "$f" || echo "No declarations"
doneRepository: netbirdio/netbird
Length of output: 2173
🏁 Script executed:
#!/bin/bash
# This is interesting - multiple build targets use the same ldflags but no variables are declared
# Let's check if maybe there's a init file or global variables declared elsewhere
echo "=== Search for var declarations at package level in all main packages ==="
rg "^var \(" . --type go -A 5 | head -50
echo ""
echo "=== Search specifically for 'commit.*string' anywhere in codebase ==="
rg "commit.*string|date.*string|builtBy.*string" . --type go
echo ""
echo "=== Check git history to see if these variables were removed ==="
git log --oneline -n 20 -- tools/idp-migrate/main.go 2>/dev/null || echo "Cannot access git history"
echo ""
echo "=== Try to understand if Go allows setting undefined variables with -X ==="
echo "Checking Go documentation understanding..."
echo "If a variable is not declared, -X should fail. Let's verify by checking recent commits."
rg "commit|date|builtBy" . --type go -B 2 -A 2 | grep -i "var\|const" | head -20Repository: netbirdio/netbird
Length of output: 50373
🏁 Script executed:
#!/bin/bash
# Final verification: Test if Go's linker actually fails on undefined -X variables
# by checking if there's a build test or CI workflow
echo "=== Check if there are build tests/CI workflows ==="
find . -name "*.yml" -o -name "*.yaml" | grep -E "(github|gitlab|ci)" | head -10
echo ""
echo "=== Check for Makefile or build scripts ==="
ls -la | grep -i make
echo ""
echo "=== Look for any comments or documentation about ldflags ==="
rg -i "ldflags|linker" . --type go -B 2 -A 2 | head -30Repository: netbirdio/netbird
Length of output: 2332
🏁 Script executed:
#!/bin/bash
# KEY FINDING: Found declarations in proxy/cmd/proxy/main.go
# Let's check if these capitalized versions exist and verify the pattern
echo "=== Check all main.go files for Version/Commit/BuildDate variable declarations ==="
for f in ./client/main.go ./management/main.go ./combined/main.go ./signal/main.go ./relay/main.go ./upload-server/main.go ./proxy/cmd/proxy/main.go ./tools/idp-migrate/main.go; do
echo "=== $f ==="
rg "var.*\(|Version|Commit|BuildDate|version|commit|date|builtBy" "$f" 2>/dev/null | head -15 || echo "Not found"
echo ""
done
echo ""
echo "=== Check which build targets reference which variable names in .goreleaser.yaml ==="
rg -B 3 "ldflags:" .goreleaser.yaml | grep -E "id:|main\."Repository: netbirdio/netbird
Length of output: 1787
🏁 Script executed:
#!/bin/bash
# KEY INSIGHT: proxy/cmd/proxy uses capitalized Version/Commit/BuildDate (declared)
# netbird-idp-migrate uses lowercase commit/date/builtBy (NOT declared)
# Let's verify this mismatch by checking ldflags for each build target
echo "=== Extract all build targets and their ldflags ==="
rg "id: (netbird|proxy)" .goreleaser.yaml -A 20 | grep -E "id:|ldflags:" -A 1
echo ""
echo "=== Verify proxy uses main.Commit (capitalized) ==="
rg "proxy" .goreleaser.yaml -B 2 -A 15 | grep -E "id: proxy|ldflags" -A 1
echo ""
echo "=== Verify netbird-idp-migrate uses main.commit (lowercase) ==="
rg "netbird-idp-migrate" .goreleaser.yaml -B 2 -A 10 | grep -E "id: netbird-idp-migrate|ldflags" -A 1Repository: netbirdio/netbird
Length of output: 2942
Remove unused ldflags or declare variables for consistency.
The ldflags reference -X main.commit, -X main.date, and -X main.builtBy, but these variables are not declared in tools/idp-migrate/main.go. While the Go linker doesn't fail on undefined -X variables, this pattern is inconsistent with netbird-proxy which properly declares Version, Commit, and BuildDate variables. Either remove these unused ldflags or add variable declarations matching the pattern used by netbird-proxy.
📝 Option A: Remove unused ldflags
ldflags:
- - -s -w -X github.com/netbirdio/netbird/version.version={{.Version}} -X main.commit={{.Commit}} -X main.date={{.CommitDate}} -X main.builtBy=goreleaser
+ - -s -w -X github.com/netbirdio/netbird/version.version={{.Version}}📝 Option B: Add variable declarations
// Add at package level in tools/idp-migrate/main.go
var (
commit string
date string
builtBy string
)📝 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.
Suggested change
| ldflags: | |
| - -s -w -X github.com/netbirdio/netbird/version.version={{.Version}} -X main.commit={{.Commit}} -X main.date={{.CommitDate}} -X main.builtBy=goreleaser | |
| ldflags: | |
| - -s -w -X github.com/netbirdio/netbird/version.version={{.Version}} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.goreleaser.yaml around lines 173 - 174, The ldflags list references -X
main.commit, -X main.date and -X main.builtBy which are not declared; either
remove those three -X entries from ldflags or add matching package-level
variable declarations in the idp-migrate main package (e.g., declare commit,
date, builtBy or follow the existing pattern Version/Commit/BuildDate) so the
linker substitutions have corresponding symbols (look for the ldflags line and
the tools/idp-migrate main package entry points such as main.go to add the
vars).
Comment on lines
+164
to
+169
| idpUsers := make(map[string]*idp.UserData) | ||
| for _, accountUsers := range allAccounts { | ||
| for _, userData := range accountUsers { | ||
| idpUsers[userData.ID] = userData | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Keep IDP lookups scoped by account.
GetAllAccounts() already gives you accountID -> users, but Lines 164-169 flatten that into a single map[userID]*idp.UserData. If the same external ID exists in two accounts, the later entry wins and Lines 193-198 can copy the wrong email/name into another account’s user. Please key this by account as well and add a regression test for duplicate IDs across accounts.
Proposed direction
- idpUsers := make(map[string]*idp.UserData)
- for _, accountUsers := range allAccounts {
- for _, userData := range accountUsers {
- idpUsers[userData.ID] = userData
- }
- }
-
- log.Infof("fetched %d users from IDP", len(idpUsers))
+ idpUsers := make(map[string]map[string]*idp.UserData, len(allAccounts))
+ idpUserCount := 0
+ for accountID, accountUsers := range allAccounts {
+ scopedUsers := make(map[string]*idp.UserData, len(accountUsers))
+ for _, userData := range accountUsers {
+ scopedUsers[userData.ID] = userData
+ idpUserCount++
+ }
+ idpUsers[accountID] = scopedUsers
+ }
+
+ log.Infof("fetched %d users from IDP", idpUserCount)
@@
- idpUser, found := idpUsers[lookupID]
+ accountUsers := idpUsers[user.AccountID]
+ idpUser, found := accountUsers[lookupID]Also applies to: 193-198
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@management/server/idp/migration/migration.go` around lines 164 - 169, The
current flattening of allAccounts into idpUsers (map[string]*idp.UserData) loses
account scoping so duplicate external IDs across accounts can overwrite each
other; change idpUsers to be account-scoped (e.g.,
map[string]map[string]*idp.UserData or similar keyed by accountID then userID)
and update the subsequent logic that uses idpUsers (the code around the current
lines 193-198) to look up users by accountID then userID instead of a global
lookup; also add a regression test that constructs two accounts containing the
same external ID and verifies each account's user retains its own email/name
after migration.
Comment on lines
+214
to
+225
| if dryRun { | ||
| log.Infof("[DRY RUN] would update user %s: email=%q, name=%q", user.Id, email, name) | ||
| updatedCount++ | ||
| continue | ||
| } | ||
|
|
||
| if err := s.Store().UpdateUserInfo(ctx, user.Id, email, name); err != nil { | ||
| return fmt.Errorf("failed to update user info for %s: %w", user.Id, err) | ||
| } | ||
|
|
||
| log.Infof("updated user %s: email=%q, name=%q", user.Id, email, name) | ||
| updatedCount++ |
Contributor
There was a problem hiding this comment.
Avoid logging raw profile data during migration.
Lines 215-225 log plaintext email and name in both dry-run and live mode. That is unnecessary PII exposure for an admin tool; log only which fields changed.
Safer logging example
+ changedEmail := email != user.Email
+ changedName := name != user.Name
if dryRun {
- log.Infof("[DRY RUN] would update user %s: email=%q, name=%q", user.Id, email, name)
+ log.Infof("[DRY RUN] would update user %s (email_changed=%t, name_changed=%t)", user.Id, changedEmail, changedName)
updatedCount++
continue
}
@@
- log.Infof("updated user %s: email=%q, name=%q", user.Id, email, name)
+ log.Infof("updated user %s (email_changed=%t, name_changed=%t)", user.Id, changedEmail, changedName)📝 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.
Suggested change
| if dryRun { | |
| log.Infof("[DRY RUN] would update user %s: email=%q, name=%q", user.Id, email, name) | |
| updatedCount++ | |
| continue | |
| } | |
| if err := s.Store().UpdateUserInfo(ctx, user.Id, email, name); err != nil { | |
| return fmt.Errorf("failed to update user info for %s: %w", user.Id, err) | |
| } | |
| log.Infof("updated user %s: email=%q, name=%q", user.Id, email, name) | |
| updatedCount++ | |
| changedEmail := email != user.Email | |
| changedName := name != user.Name | |
| if dryRun { | |
| log.Infof("[DRY RUN] would update user %s (email_changed=%t, name_changed=%t)", user.Id, changedEmail, changedName) | |
| updatedCount++ | |
| continue | |
| } | |
| if err := s.Store().UpdateUserInfo(ctx, user.Id, email, name); err != nil { | |
| return fmt.Errorf("failed to update user info for %s: %w", user.Id, err) | |
| } | |
| log.Infof("updated user %s (email_changed=%t, name_changed=%t)", user.Id, changedEmail, changedName) | |
| updatedCount++ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@management/server/idp/migration/migration.go` around lines 214 - 225, The
current migration logs plaintext PII (email and name) in both dry-run and live
paths; update the log calls around the dryRun branch and after
s.Store().UpdateUserInfo to avoid printing email/name and instead log only the
user identifier and which fields changed (e.g., "email" and/or "name") and
whether it was a dry run. Specifically, change the log.Infof calls that
reference user.Id, email, name to something like a dry-run message and a
live-update message that include user.Id and a concise list of changed fields
(not their values), keep updatedCount increments and the
s.Store().UpdateUserInfo call and error handling for user.Id as-is.
|
|
||
| | Requirement | Details | | ||
| |-------------|---------| | ||
| | NetBird version | `<INSERT_VERSION>` or later | |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace version placeholders before release.
The <INSERT_VERSION> placeholders on lines 22 and 458 should be replaced with the actual version number (e.g., v0.60.0) before this documentation is released.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/idp-migrate/MIGRATION_GUIDE.md` at line 22, Replace every occurrence of
the literal placeholder "<INSERT_VERSION>" in this document (notably the table
entry on line 22 and the other occurrence around line 458) with the actual
release version string (e.g., "v0.60.0"); ensure both instances of
"<INSERT_VERSION>" are updated consistently so the table header and any other
references show the real version before publishing.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tools/idp-migrate/main.go (1)
306-313: Consider usingfmt.Printfor the interactive prompt.
log.Infofmay include timestamps and log-level prefixes depending on the logger configuration, which can make the interactive prompt less clear. For user-facing prompts that expect input,fmt.Printprovides cleaner output.Suggested fix
func confirmPrompt(pending int) bool { - log.Infof("About to migrate %d users. This cannot be easily undone. Continue? [y/N] ", pending) + fmt.Printf("About to migrate %d users. This cannot be easily undone. Continue? [y/N] ", pending) reader := bufio.NewReader(os.Stdin) answer, _ := reader.ReadString('\n') answer = strings.TrimSpace(strings.ToLower(answer)) return answer == "y" || answer == "yes" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/idp-migrate/main.go` around lines 306 - 313, The interactive prompt in function confirmPrompt should use fmt.Print or fmt.Printf instead of log.Infof so the prompt is shown without timestamps or log prefixes; update confirmPrompt to write the prompt with fmt.Printf("About to migrate %d users. This cannot be easily undone. Continue? [y/N] ", pending), keep the same bufio.NewReader/os.Stdin read/trim logic, and add the fmt import if missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/idp-migrate/main.go`:
- Around line 626-631: The current fallback reads
cfg.IdpManagerConfig.ClientConfig.Issuer and returns its host via hostFromURL,
which is the external IdP domain not the management server domain; update the
block in main.go (the conditional referencing cfg.IdpManagerConfig,
ClientConfig.Issuer and hostFromURL) to stop using Issuer as a fallback and
instead return a clear error prompting the operator to set
HttpConfig.LetsEncryptDomain (or remove the fallback entirely). Concretely,
replace the branch that returns
hostFromURL(cfg.IdpManagerConfig.ClientConfig.Issuer) with code that returns an
error message instructing the user to explicitly provide
HttpConfig.LetsEncryptDomain for configuring EmbeddedIdP.Issuer (referencing
EmbeddedIdP.Issuer and HttpConfig.LetsEncryptDomain to locate where the value is
needed).
---
Nitpick comments:
In `@tools/idp-migrate/main.go`:
- Around line 306-313: The interactive prompt in function confirmPrompt should
use fmt.Print or fmt.Printf instead of log.Infof so the prompt is shown without
timestamps or log prefixes; update confirmPrompt to write the prompt with
fmt.Printf("About to migrate %d users. This cannot be easily undone. Continue?
[y/N] ", pending), keep the same bufio.NewReader/os.Stdin read/trim logic, and
add the fmt import if missing.
Comment on lines
+626
to
+631
| // Priority 4: parse from IdpManagerConfig.ClientConfig.Issuer | ||
| if cfg.IdpManagerConfig != nil && cfg.IdpManagerConfig.ClientConfig != nil && cfg.IdpManagerConfig.ClientConfig.Issuer != "" { | ||
| if host := hostFromURL(cfg.IdpManagerConfig.ClientConfig.Issuer); host != "" { | ||
| return host, nil | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Priority 4 extracts the external IdP domain, not the management server domain.
IdpManagerConfig.ClientConfig.Issuer contains the external IdP's issuer URL (e.g., https://auth0.com, https://login.okta.com). Using this as a fallback would configure the embedded IdP to use the wrong domain.
For example, if Issuer is https://my-tenant.auth0.com, the generated EmbeddedIdP.Issuer would become https://my-tenant.auth0.com/oauth2, which is incorrect—it should be the management server's domain.
Consider either removing this fallback or returning an error prompting the user to set HttpConfig.LetsEncryptDomain explicitly.
Suggested fix
- // Priority 4: parse from IdpManagerConfig.ClientConfig.Issuer
- if cfg.IdpManagerConfig != nil && cfg.IdpManagerConfig.ClientConfig != nil && cfg.IdpManagerConfig.ClientConfig.Issuer != "" {
- if host := hostFromURL(cfg.IdpManagerConfig.ClientConfig.Issuer); host != "" {
- return host, nil
- }
- }
-
return "", fmt.Errorf("could not determine domain: set HttpConfig.LetsEncryptDomain, HttpConfig.AuthIssuer, or HttpConfig.OIDCConfigEndpoint in management.json")📝 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.
Suggested change
| // Priority 4: parse from IdpManagerConfig.ClientConfig.Issuer | |
| if cfg.IdpManagerConfig != nil && cfg.IdpManagerConfig.ClientConfig != nil && cfg.IdpManagerConfig.ClientConfig.Issuer != "" { | |
| if host := hostFromURL(cfg.IdpManagerConfig.ClientConfig.Issuer); host != "" { | |
| return host, nil | |
| } | |
| } | |
| return "", fmt.Errorf("could not determine domain: set HttpConfig.LetsEncryptDomain, HttpConfig.AuthIssuer, or HttpConfig.OIDCConfigEndpoint in management.json") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/idp-migrate/main.go` around lines 626 - 631, The current fallback reads
cfg.IdpManagerConfig.ClientConfig.Issuer and returns its host via hostFromURL,
which is the external IdP domain not the management server domain; update the
block in main.go (the conditional referencing cfg.IdpManagerConfig,
ClientConfig.Issuer and hostFromURL) to stop using Issuer as a fallback and
instead return a clear error prompting the operator to set
HttpConfig.LetsEncryptDomain (or remove the fallback entirely). Concretely,
replace the branch that returns
hostFromURL(cfg.IdpManagerConfig.ClientConfig.Issuer) with code that returns an
error message instructing the user to explicitly provide
HttpConfig.LetsEncryptDomain for configuring EmbeddedIdP.Issuer (referencing
EmbeddedIdP.Issuer and HttpConfig.LetsEncryptDomain to locate where the value is
needed).
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (3)
tools/idp-migrate/main.go (1)
593-600:⚠️ Potential issue | 🟠 MajorDo not derive management domain from external IdP issuer.
Falling back to
IdpManagerConfig.ClientConfig.Issuercan produce an embedded issuer under the external IdP domain, which is incorrect for combined-server routing.✂️ Suggested fix
- // Priority 4: parse from IdpManagerConfig.ClientConfig.Issuer - if cfg.IdpManagerConfig != nil && cfg.IdpManagerConfig.ClientConfig != nil && cfg.IdpManagerConfig.ClientConfig.Issuer != "" { - if host := hostFromURL(cfg.IdpManagerConfig.ClientConfig.Issuer); host != "" { - return host, nil - } - } - return "", fmt.Errorf("could not determine domain: set HttpConfig.LetsEncryptDomain, HttpConfig.AuthIssuer, or HttpConfig.OIDCConfigEndpoint in management.json")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/idp-migrate/main.go` around lines 593 - 600, The code currently falls back to parsing cfg.IdpManagerConfig.ClientConfig.Issuer via hostFromURL when determining the management domain; remove this fallback so the domain is NOT derived from an external IdP issuer. Specifically, eliminate or skip the block that checks cfg.IdpManagerConfig != nil && cfg.IdpManagerConfig.ClientConfig != nil && cfg.IdpManagerConfig.ClientConfig.Issuer and any call to hostFromURL on that Issuer so the function only uses HttpConfig.LetsEncryptDomain, HttpConfig.AuthIssuer, or HttpConfig.OIDCConfigEndpoint to determine the domain and returns the existing error if none are set.management/server/idp/migration/migration.go (1)
214-225:⚠️ Potential issue | 🟠 MajorAvoid logging plaintext email/name during migration.
These logs emit profile PII in both dry-run and live paths; log changed fields only.
🔒 Safer logging pattern
+ changedEmail := email != user.Email + changedName := name != user.Name if dryRun { - log.Infof("[DRY RUN] would update user %s: email=%q, name=%q", user.Id, email, name) + log.Infof("[DRY RUN] would update user %s (email_changed=%t, name_changed=%t)", user.Id, changedEmail, changedName) updatedCount++ continue } @@ - log.Infof("updated user %s: email=%q, name=%q", user.Id, email, name) + log.Infof("updated user %s (email_changed=%t, name_changed=%t)", user.Id, changedEmail, changedName)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/idp/migration/migration.go` around lines 214 - 225, The logs in the migration loop (see variables/functions dryRun, user.Id, email, name and the log.Infof calls) currently print plaintext PII; change both the dry-run and live log statements to avoid emitting email/name values — instead log user.Id and which fields would/was changed (e.g. "email changed", "name changed") or log masked values (e.g. redacted or hashed) or simply "updated fields: [email,name]". Keep the UpdateUserInfo call and updatedCount logic as-is, only replace the log.Infof format strings to omit raw PII.tools/idp-migrate/MIGRATION_GUIDE.md (1)
22-22:⚠️ Potential issue | 🟠 MajorReplace unresolved release-version placeholders.
<INSERT_VERSION>is still present in user-facing instructions, which makes the migration prerequisites and troubleshooting guidance ambiguous at publish time.Also applies to: 458-458
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/idp-migrate/MIGRATION_GUIDE.md` at line 22, Replace the unresolved placeholder "<INSERT_VERSION>" in MIGRATION_GUIDE.md with the actual release version (or a documented template variable that the docs build will substitute) so user-facing instructions are unambiguous; search for all occurrences of "<INSERT_VERSION>" (including the table cell under "NetBird version" and the duplicate at line ~458) and update them to the concrete version string (e.g., vX.Y.Z) or a validated variable like "{{RELEASE_VERSION}}" and ensure any build/process that generates the published docs will replace that variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@management/server/activity/store/sql_store_idp_migration.go`:
- Around line 39-60: The transaction currently uses store.db.Transaction without
the provided ctx, so cancellation/deadline signals aren't propagated; change the
call to store.db.WithContext(ctx).Transaction(...) so the context is applied to
the entire transaction and all DB calls (the inner tx stays the same; e.g.
tx.Model(&activity.Event{})... and tx.Exec(...)) ensuring the transaction
respects ctx cancellation/deadlines.
In `@management/server/idp/migration/migration.go`:
- Around line 62-63: The function MigrateUsersToStaticConnectors currently
dereferences conn.ID and will panic if conn is nil; add a nil-check at the start
of MigrateUsersToStaticConnectors (and the other occurrence that encodes
connector IDs) to return a clear error when conn == nil before attempting to
access conn.ID or calling any encode/ID helper, e.g., validate the connector
argument and fail fast with an informative error rather than allowing a
nil-pointer panic.
In `@tools/idp-migrate/DEVELOPMENT.md`:
- Line 12: Update the triple-backtick code fences in DEVELOPMENT.md to include
explicit fence languages (e.g., ```text or ```bash) so the Markdown linter MD040
is satisfied; specifically edit the unnamed code blocks shown around the current
snippet (and the other block referenced at line 37) to add the appropriate
language token for each fence.
In `@tools/idp-migrate/main.go`:
- Around line 494-543: generateConfig currently only injects EmbeddedIdP and
never mutates the existing HttpConfig/PKCE settings as promised; update the
function so after unmarshalling configMap you locate or create the "HttpConfig"
object in configMap and set its management-related fields (e.g., issuer/base
URLs, callback paths, DashboardRedirectURIs) to use the derived domain, ensure
PKCE is enabled/set appropriately, and update any client settings or auth
endpoint URLs that still point to the legacy external IdP to point at the new
management server; reference the existing conn.Config→connConfig, the derived
domain variable, and the EmbeddedIdP insert so you reuse the same URL formatting
and connector redirectURI when modifying HttpConfig and client PKCE fields.
- Around line 549-552: The dry-run branch currently logs full newJSON (which
contains sensitive fields like clientSecret) causing secret leakage; update the
dry-run behavior to sanitize before printing by parsing newJSON, removing or
replacing sensitive keys (e.g., "clientSecret", "client_secret", any credential
fields) with a redaction token, then marshal and log the sanitized JSON instead
of raw newJSON; ensure this change touches the dryRun branch where newJSON is
used so only the sanitized output is printed during dry runs.
In `@tools/idp-migrate/MIGRATION_GUIDE.md`:
- Line 233: Several fenced code blocks use plain triple backticks (```) without
a language identifier, triggering markdownlint rule MD040; locate the fenced
code blocks (the ``` markers) and add appropriate language identifiers (e.g.,
```bash, ```json, ```yaml, etc.) for each block so markdownlint recognizes the
language—update every occurrence of ``` in the document that currently lacks a
language tag (including the blocks noted in the review) to include the correct
language specifier.
- Around line 385-392: The command reference table in MIGRATION_GUIDE.md omits
the --skip-populate-user-info flag; add an entry in the options list (near the
other flags shown like --skip-config and --log-level) documenting the flag name
and a short description such as "Skip populating user info from the IdP during
migration (useful for restricted/air-gapped environments)"; ensure formatting
matches the surrounding --flag string Description style and include any
default behavior or note that it is a boolean/dry-run compatible flag.
---
Duplicate comments:
In `@management/server/idp/migration/migration.go`:
- Around line 214-225: The logs in the migration loop (see variables/functions
dryRun, user.Id, email, name and the log.Infof calls) currently print plaintext
PII; change both the dry-run and live log statements to avoid emitting
email/name values — instead log user.Id and which fields would/was changed (e.g.
"email changed", "name changed") or log masked values (e.g. redacted or hashed)
or simply "updated fields: [email,name]". Keep the UpdateUserInfo call and
updatedCount logic as-is, only replace the log.Infof format strings to omit raw
PII.
In `@tools/idp-migrate/main.go`:
- Around line 593-600: The code currently falls back to parsing
cfg.IdpManagerConfig.ClientConfig.Issuer via hostFromURL when determining the
management domain; remove this fallback so the domain is NOT derived from an
external IdP issuer. Specifically, eliminate or skip the block that checks
cfg.IdpManagerConfig != nil && cfg.IdpManagerConfig.ClientConfig != nil &&
cfg.IdpManagerConfig.ClientConfig.Issuer and any call to hostFromURL on that
Issuer so the function only uses HttpConfig.LetsEncryptDomain,
HttpConfig.AuthIssuer, or HttpConfig.OIDCConfigEndpoint to determine the domain
and returns the existing error if none are set.
In `@tools/idp-migrate/MIGRATION_GUIDE.md`:
- Line 22: Replace the unresolved placeholder "<INSERT_VERSION>" in
MIGRATION_GUIDE.md with the actual release version (or a documented template
variable that the docs build will substitute) so user-facing instructions are
unambiguous; search for all occurrences of "<INSERT_VERSION>" (including the
table cell under "NetBird version" and the duplicate at line ~458) and update
them to the concrete version string (e.g., vX.Y.Z) or a validated variable like
"{{RELEASE_VERSION}}" and ensure any build/process that generates the published
docs will replace that variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 092d0151-b07c-4f17-8105-25f661dd737c
📒 Files selected for processing (7)
management/server/activity/store/sql_store_idp_migration.gomanagement/server/idp/migration/migration.gomanagement/server/idp/migration/store.gotools/idp-migrate/DEVELOPMENT.mdtools/idp-migrate/MIGRATION_GUIDE.mdtools/idp-migrate/main.gotools/idp-migrate/main_test.go
Comment on lines
+39
to
+60
| return store.db.Transaction(func(tx *gorm.DB) error { | ||
| if err := tx.Model(&activity.Event{}). | ||
| Where("initiator_id = ?", oldUserID). | ||
| Update("initiator_id", newUserID).Error; err != nil { | ||
| return fmt.Errorf("update events.initiator_id: %w", err) | ||
| } | ||
|
|
||
| if err := tx.Model(&activity.Event{}). | ||
| Where("target_id = ?", oldUserID). | ||
| Update("target_id", newUserID).Error; err != nil { | ||
| return fmt.Errorf("update events.target_id: %w", err) | ||
| } | ||
|
|
||
| // Raw exec: GORM can't update a PK via Model().Update() | ||
| if err := tx.Exec( | ||
| "UPDATE deleted_users SET id = ? WHERE id = ?", newUserID, oldUserID, | ||
| ).Error; err != nil { | ||
| return fmt.Errorf("update deleted_users.id: %w", err) | ||
| } | ||
|
|
||
| return nil | ||
| }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n management/server/activity/store/sql_store_idp_migration.go | head -70Repository: netbirdio/netbird
Length of output: 2372
🏁 Script executed:
cd management/server/activity/store && grep -n "WithContext" *.go | head -20Repository: netbirdio/netbird
Length of output: 830
🏁 Script executed:
rg "\.Transaction\(" management/server/activity/store/ -A 3 | head -50Repository: netbirdio/netbird
Length of output: 1257
🏁 Script executed:
rg "db\.WithContext" management/server/ -A 2 | head -40Repository: netbirdio/netbird
Length of output: 2613
🏁 Script executed:
rg "WithContext.*Transaction|Transaction.*WithContext" management/server/ -B 2 -A 2Repository: netbirdio/netbird
Length of output: 43
🏁 Script executed:
rg "\.WithContext\(.*\)\.Transaction" management/server/ -A 1Repository: netbirdio/netbird
Length of output: 43
🏁 Script executed:
rg "WithContext.*Begin|Begin.*WithContext" management/server/ -B 1 -A 2Repository: netbirdio/netbird
Length of output: 317
Propagate ctx into the transaction to respect cancellation and deadlines.
The ctx parameter is unused, preventing cancellation/deadline signals from reaching database operations. This is inconsistent with the codebase pattern and can cause operations to continue even when the context is cancelled.
🛠️ Suggested fix
func (store *Store) UpdateUserID(ctx context.Context, oldUserID, newUserID string) error {
- return store.db.Transaction(func(tx *gorm.DB) error {
+ return store.db.WithContext(ctx).Transaction(func(tx *gorm.DB) error {
if err := tx.Model(&activity.Event{}).
Where("initiator_id = ?", oldUserID).
Update("initiator_id", newUserID).Error; err != nil {
return fmt.Errorf("update events.initiator_id: %w", 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.
Suggested change
| return store.db.Transaction(func(tx *gorm.DB) error { | |
| if err := tx.Model(&activity.Event{}). | |
| Where("initiator_id = ?", oldUserID). | |
| Update("initiator_id", newUserID).Error; err != nil { | |
| return fmt.Errorf("update events.initiator_id: %w", err) | |
| } | |
| if err := tx.Model(&activity.Event{}). | |
| Where("target_id = ?", oldUserID). | |
| Update("target_id", newUserID).Error; err != nil { | |
| return fmt.Errorf("update events.target_id: %w", err) | |
| } | |
| // Raw exec: GORM can't update a PK via Model().Update() | |
| if err := tx.Exec( | |
| "UPDATE deleted_users SET id = ? WHERE id = ?", newUserID, oldUserID, | |
| ).Error; err != nil { | |
| return fmt.Errorf("update deleted_users.id: %w", err) | |
| } | |
| return nil | |
| }) | |
| return store.db.WithContext(ctx).Transaction(func(tx *gorm.DB) error { | |
| if err := tx.Model(&activity.Event{}). | |
| Where("initiator_id = ?", oldUserID). | |
| Update("initiator_id", newUserID).Error; err != nil { | |
| return fmt.Errorf("update events.initiator_id: %w", err) | |
| } | |
| if err := tx.Model(&activity.Event{}). | |
| Where("target_id = ?", oldUserID). | |
| Update("target_id", newUserID).Error; err != nil { | |
| return fmt.Errorf("update events.target_id: %w", err) | |
| } | |
| // Raw exec: GORM can't update a PK via Model().Update() | |
| if err := tx.Exec( | |
| "UPDATE deleted_users SET id = ? WHERE id = ?", newUserID, oldUserID, | |
| ).Error; err != nil { | |
| return fmt.Errorf("update deleted_users.id: %w", err) | |
| } | |
| return nil | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@management/server/activity/store/sql_store_idp_migration.go` around lines 39
- 60, The transaction currently uses store.db.Transaction without the provided
ctx, so cancellation/deadline signals aren't propagated; change the call to
store.db.WithContext(ctx).Transaction(...) so the context is applied to the
entire transaction and all DB calls (the inner tx stays the same; e.g.
tx.Model(&activity.Event{})... and tx.Exec(...)) ensuring the transaction
respects ctx cancellation/deadlines.
Comment on lines
+62
to
+63
| func MigrateUsersToStaticConnectors(s Server, conn *dex.Connector) error { | ||
| ctx := context.Background() |
Contributor
There was a problem hiding this comment.
Guard against nil connector input before encoding IDs.
This exported function will panic on conn.ID if called with a nil connector.
🧩 Suggested guard
func MigrateUsersToStaticConnectors(s Server, conn *dex.Connector) error {
+ if conn == nil || conn.ID == "" {
+ return fmt.Errorf("connector is nil or has empty ID")
+ }
ctx := context.Background()Also applies to: 91-91
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@management/server/idp/migration/migration.go` around lines 62 - 63, The
function MigrateUsersToStaticConnectors currently dereferences conn.ID and will
panic if conn is nil; add a nil-check at the start of
MigrateUsersToStaticConnectors (and the other occurrence that encodes connector
IDs) to return a clear error when conn == nil before attempting to access
conn.ID or calling any encode/ID helper, e.g., validate the connector argument
and fail fast with an informative error rather than allowing a nil-pointer
panic.
|
|
||
| ## Code Layout | ||
|
|
||
| ``` |
Contributor
There was a problem hiding this comment.
Specify fence languages for the code blocks.
Both fences trigger MD040; adding text/bash will clear lint and improve rendering.
Also applies to: 37-37
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 12-12: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/idp-migrate/DEVELOPMENT.md` at line 12, Update the triple-backtick code
fences in DEVELOPMENT.md to include explicit fence languages (e.g., ```text or
```bash) so the Markdown linter MD040 is satisfied; specifically edit the
unnamed code blocks shown around the current snippet (and the other block
referenced at line 37) to add the appropriate language token for each fence.
Comment on lines
+494
to
+543
| // generateConfig reads the existing management.json as raw JSON, removes | ||
| // IdpManagerConfig, adds EmbeddedIdP, updates HttpConfig fields, and writes | ||
| // the result. In dry-run mode, it prints the new config to stdout instead. | ||
| func generateConfig(configPath string, conn *dex.Connector, cfg *nbconfig.Config, dryRun bool) error { | ||
| domain, err := deriveDomain(cfg) | ||
| if err != nil { | ||
| return fmt.Errorf("derive domain: %w", err) | ||
| } | ||
| log.Infof("derived domain for embedded IdP: %s", domain) | ||
|
|
||
| // Read existing config as raw JSON to preserve all fields | ||
| raw, err := os.ReadFile(configPath) | ||
| if err != nil { | ||
| return fmt.Errorf("read config file: %w", err) | ||
| } | ||
|
|
||
| var configMap map[string]interface{} | ||
| if err := json.Unmarshal(raw, &configMap); err != nil { | ||
| return fmt.Errorf("parse config JSON: %w", err) | ||
| } | ||
|
|
||
| // Remove old IdP config | ||
| delete(configMap, "IdpManagerConfig") | ||
|
|
||
| // Ensure the connector's redirectURI points to the management server (Dex callback), | ||
| // not the external IdP. The auto-detection may have used the IdP issuer URL. | ||
| connConfig := make(map[string]interface{}, len(conn.Config)) | ||
| for k, v := range conn.Config { | ||
| connConfig[k] = v | ||
| } | ||
| connConfig["redirectURI"] = fmt.Sprintf("https://%s/oauth2/callback", domain) | ||
|
|
||
| // Add minimal EmbeddedIdP section | ||
| configMap["EmbeddedIdP"] = map[string]interface{}{ | ||
| "Enabled": true, | ||
| "Issuer": fmt.Sprintf("https://%s/oauth2", domain), | ||
| "DashboardRedirectURIs": []string{ | ||
| fmt.Sprintf("https://%s/nb-auth", domain), | ||
| fmt.Sprintf("https://%s/nb-silent-auth", domain), | ||
| }, | ||
| "StaticConnectors": []interface{}{ | ||
| map[string]interface{}{ | ||
| "type": conn.Type, | ||
| "name": conn.Name, | ||
| "id": conn.ID, | ||
| "config": connConfig, | ||
| }, | ||
| }, | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
generateConfig does not actually update HttpConfig/PKCE despite the contract.
The function comment states it updates HttpConfig fields, but this implementation only injects EmbeddedIdP. That can leave auth endpoints/client settings on the legacy external IdP after migration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/idp-migrate/main.go` around lines 494 - 543, generateConfig currently
only injects EmbeddedIdP and never mutates the existing HttpConfig/PKCE settings
as promised; update the function so after unmarshalling configMap you locate or
create the "HttpConfig" object in configMap and set its management-related
fields (e.g., issuer/base URLs, callback paths, DashboardRedirectURIs) to use
the derived domain, ensure PKCE is enabled/set appropriately, and update any
client settings or auth endpoint URLs that still point to the legacy external
IdP to point at the new management server; reference the existing
conn.Config→connConfig, the derived domain variable, and the EmbeddedIdP insert
so you reuse the same URL formatting and connector redirectURI when modifying
HttpConfig and client PKCE fields.
Comment on lines
+549
to
+552
| if dryRun { | ||
| log.Info("[DRY RUN] new management.json would be:") | ||
| log.Infoln(string(newJSON)) | ||
| return nil |
Contributor
There was a problem hiding this comment.
Dry-run currently prints secrets to logs/stdout.
newJSON includes static connector credentials (clientSecret), so dry-run leaks sensitive material into log collectors and shell history pipelines.
🔐 Suggested safer dry-run output
if dryRun {
- log.Info("[DRY RUN] new management.json would be:")
- log.Infoln(string(newJSON))
+ log.Infof("[DRY RUN] generated config preview: embedded issuer=%s, connector_id=%s, connector_type=%s",
+ fmt.Sprintf("https://%s/oauth2", domain), conn.ID, conn.Type)
return nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/idp-migrate/main.go` around lines 549 - 552, The dry-run branch
currently logs full newJSON (which contains sensitive fields like clientSecret)
causing secret leakage; update the dry-run behavior to sanitize before printing
by parsing newJSON, removing or replacing sensitive keys (e.g., "clientSecret",
"client_secret", any credential fields) with a redaction token, then marshal and
log the sanitized JSON instead of raw newJSON; ensure this change touches the
dryRun branch where newJSON is used so only the sanitized output is printed
during dry runs.
|
|
||
| You should see output like: | ||
|
|
||
| ``` |
Contributor
There was a problem hiding this comment.
Add language identifiers to fenced code blocks.
These blocks currently violate MD040 (fenced-code-language) and will keep markdownlint noisy.
Also applies to: 266-266, 294-294, 381-381
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 233-233: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/idp-migrate/MIGRATION_GUIDE.md` at line 233, Several fenced code blocks
use plain triple backticks (```) without a language identifier, triggering
markdownlint rule MD040; locate the fenced code blocks (the ``` markers) and add
appropriate language identifiers (e.g., ```bash, ```json, ```yaml, etc.) for
each block so markdownlint recognizes the language—update every occurrence of
``` in the document that currently lacks a language tag (including the blocks
noted in the review) to include the correct language specifier.
Comment on lines
+385
to
+392
| --config string Path to management.json (required) | ||
| --datadir string Override data directory from config | ||
| --idp-seed-info string Base64-encoded connector JSON (overrides auto-detection) | ||
| --dry-run Preview changes without writing | ||
| --force Skip confirmation prompt | ||
| --skip-config Skip config generation (DB migration only) | ||
| --log-level string Log level: debug, info, warn, error (default "info") | ||
| ``` |
Contributor
There was a problem hiding this comment.
Document the --skip-populate-user-info flag in the command reference.
The CLI exposes this flag, but the reference table omits it, which can mislead operators in restricted/air-gapped migrations.
📌 Suggested doc patch
Flags:
--config string Path to management.json (required)
--datadir string Override data directory from config
--idp-seed-info string Base64-encoded connector JSON (overrides auto-detection)
--dry-run Preview changes without writing
--force Skip confirmation prompt
--skip-config Skip config generation (DB migration only)
+ --skip-populate-user-info
+ Skip populating user info (user ID migration only)
--log-level string Log level: debug, info, warn, error (default "info")📝 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.
Suggested change
| --config string Path to management.json (required) | |
| --datadir string Override data directory from config | |
| --idp-seed-info string Base64-encoded connector JSON (overrides auto-detection) | |
| --dry-run Preview changes without writing | |
| --force Skip confirmation prompt | |
| --skip-config Skip config generation (DB migration only) | |
| --log-level string Log level: debug, info, warn, error (default "info") | |
| ``` | |
| --config string Path to management.json (required) | |
| --datadir string Override data directory from config | |
| --idp-seed-info string Base64-encoded connector JSON (overrides auto-detection) | |
| --dry-run Preview changes without writing | |
| --force Skip confirmation prompt | |
| --skip-config Skip config generation (DB migration only) | |
| --skip-populate-user-info | |
| Skip populating user info (user ID migration only) | |
| --log-level string Log level: debug, info, warn, error (default "info") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/idp-migrate/MIGRATION_GUIDE.md` around lines 385 - 392, The command
reference table in MIGRATION_GUIDE.md omits the --skip-populate-user-info flag;
add an entry in the options list (near the other flags shown like --skip-config
and --log-level) documenting the flag name and a short description such as "Skip
populating user info from the IdP during migration (useful for
restricted/air-gapped environments)"; ensure formatting matches the surrounding
--flag string Description style and include any default behavior or note
that it is a boolean/dry-run compatible flag.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes
This PR introduces a standalone
netbird-idp-migrateCLI tool to help operators migrate from an external IdP to the embedded Dex-based IdP introduced in Netbird some versions ago.What it does:
Key components added:
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#658
Summary by CodeRabbit
New Features
Documentation
Tests