Repository navigation
Conversation
Add ManagedConfig type to the Go SDK that allows native Android and iOS apps to pass MDM-provided configuration values (management URL, setup key, admin URL, pre-shared key, rosenpass settings, auto-connect) into the NetBird client configuration. - Add ManagedConfig struct with gomobile-compatible setter API - Add Apply() to write MDM values to client config via profilemanager - Add LoginWithSetupKeySync() for silent setup key registration - Add key constant getters for native code to reference MDM keys - Android variant uses UpdateOrCreateConfig() - iOS variant uses DirectUpdateOrCreateConfig() for tvOS compatibility - Include unit tests for all ManagedConfig functionality Fixes netbirdio#1918
📝 WalkthroughWalkthroughAdds synchronous setup-key login methods for Android and iOS and introduces ManagedConfig for both platforms to accept MDM/EMM-provided values and persist persistent fields via the profile manager (setup key treated as non-persistent). ChangesAndroid Login
Android Managed Config
iOS Login
iOS Managed Config
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as ManagedConfig
participant Logger as Logger
participant PM as profilemanager
participant FS as FileSystem
Note over App,Logger: Apply(configPath)
App->>Logger: log "Applying managed config"
App->>PM: build ConfigInput with only set persistent fields
PM->>FS: read existing config (if present)
PM->>FS: write/update config file
FS-->>PM: ack / error
PM-->>App: return success / error
App->>Logger: log success or wrap error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
client/ios/NetBirdSDK/managed_config.go (1)
134-168: Minor:log.Infofwithout format verbs.Several of these calls (
log.Infof("MDM: setting management URL"),log.Infof("MDM: setting admin URL"),log.Infof("MDM: setting pre-shared key")) have no format arguments and should just belog.Info(...). Go vet /govet.printfwill not catch this, but some linters (e.g.,staticcheckSA1006 variants) will. Trivial.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/ios/NetBirdSDK/managed_config.go` around lines 134 - 168, In the managed_config.go code that builds profilemanager.ConfigInput (the block referencing variable m and the ConfigInput struct), replace the log.Infof calls that have no format verbs—specifically log.Infof("MDM: setting management URL"), log.Infof("MDM: setting admin URL"), and log.Infof("MDM: setting pre-shared key")—with log.Info(...) calls; keep the other log.Infof usages that include formatting (e.g., Rosenpass and disableAutoConn) unchanged so formatting verbs match their arguments.client/android/managed_config.go (1)
1-174: Significant duplication between Android and iOSmanaged_config.go.The two files are ~95% identical — same struct, setters,
HasConfig, key constants, andApplybody. The only real difference isUpdateOrCreateConfigvsDirectUpdateOrCreateConfig. Consider extracting the shared logic into an internal unexported package (e.g.client/internal/managedconfig) or a shared file with build tags, with the platform files only providing the persistence callback. This will prevent the two implementations from drifting over time (e.g. when a new MDM field is added and one side is missed).Not a blocker — fine to defer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/android/managed_config.go` around lines 1 - 174, The Android managed_config.go is nearly identical to the iOS version causing duplication; refactor by extracting shared types and logic (ManagedConfig struct, setters, HasConfig, Get* key getters, Apply logic minus persistence call) into an internal package or shared file (e.g., client/internal/managedconfig) and keep platform files minimal: each platform file should only call into the shared package and provide its specific persistence function (Android uses profilemanager.UpdateOrCreateConfig, iOS uses DirectUpdateOrCreateConfig) — move constants and methods like NewManagedConfig, SetManagementURL, SetSetupKey, SetAdminURL, SetPreSharedKey, SetRosenpassEnabled, SetRosenpassPermissive, SetDisableAutoConnect, HasSetupKey, GetSetupKey, HasConfig, and the Apply orchestration logic into the shared package, and replace the Apply persistence call in android.ManagedConfig.Apply to invoke the platform-specific persistence callback supplied by the shared package.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/android/managed_config_test.go`:
- Around line 137-157: The test TestManagedConfig_SetupKeyNotWrittenToConfig
currently reads back with profilemanager.ReadConfig which strips unknown JSON
fields, so change the assertion to read the raw file bytes (use os.ReadFile)
after calling m.Apply(cfgFile) and assert the literal setup key string
("secret-setup-key") is not present (use bytes.Contains and fail the test if it
is); keep the existing ManagementURL check via profilemanager.ReadConfig but add
the raw-file assertion to ensure Apply() doesn't write the SetupKey.
In `@client/android/managed_config.go`:
- Around line 82-85: The SetPreSharedKey method on ManagedConfig currently
stores a pointer even for an empty string which can overwrite an existing PSK in
Apply() via input.PreSharedKey; change SetPreSharedKey to treat empty-string as
"absent" by not setting m.preSharedKey (leave nil) when key == "" and only
assign a pointer when key is non-empty, and apply the same guard in the iOS
managed_config.go counterpart so Apply() only sees a non-nil PreSharedKey when a
real PSK was provided.
- Around line 113-132: HasConfig() currently returns true when only setupKey is
set, which causes ManagedConfig.Apply to call
profilemanager.UpdateOrCreateConfig and create a default config; change the
logic so Apply only proceeds when there is at least one field that will actually
be persisted. Concretely, update ManagedConfig.HasConfig or add a short guard in
ManagedConfig.Apply to ignore setupKey (i.e., treat setupKey as non-persistent)
so Apply returns early when the only set field is setupKey; ensure this same
change is applied to the iOS counterpart (client/ios/.../managed_config.go) so
profilemanager.UpdateOrCreateConfig / createNewConfig / config.apply are not
invoked for setup-key-only MDM payloads.
---
Nitpick comments:
In `@client/android/managed_config.go`:
- Around line 1-174: The Android managed_config.go is nearly identical to the
iOS version causing duplication; refactor by extracting shared types and logic
(ManagedConfig struct, setters, HasConfig, Get* key getters, Apply logic minus
persistence call) into an internal package or shared file (e.g.,
client/internal/managedconfig) and keep platform files minimal: each platform
file should only call into the shared package and provide its specific
persistence function (Android uses profilemanager.UpdateOrCreateConfig, iOS uses
DirectUpdateOrCreateConfig) — move constants and methods like NewManagedConfig,
SetManagementURL, SetSetupKey, SetAdminURL, SetPreSharedKey,
SetRosenpassEnabled, SetRosenpassPermissive, SetDisableAutoConnect, HasSetupKey,
GetSetupKey, HasConfig, and the Apply orchestration logic into the shared
package, and replace the Apply persistence call in android.ManagedConfig.Apply
to invoke the platform-specific persistence callback supplied by the shared
package.
In `@client/ios/NetBirdSDK/managed_config.go`:
- Around line 134-168: In the managed_config.go code that builds
profilemanager.ConfigInput (the block referencing variable m and the ConfigInput
struct), replace the log.Infof calls that have no format verbs—specifically
log.Infof("MDM: setting management URL"), log.Infof("MDM: setting admin URL"),
and log.Infof("MDM: setting pre-shared key")—with log.Info(...) calls; keep the
other log.Infof usages that include formatting (e.g., Rosenpass and
disableAutoConn) unchanged so formatting verbs match their arguments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d43f7d00-3514-459e-9b92-b78f93081c77
📒 Files selected for processing (5)
client/android/login.goclient/android/managed_config.goclient/android/managed_config_test.goclient/ios/NetBirdSDK/login.goclient/ios/NetBirdSDK/managed_config.go
|
Thank you, I'll review these automated code review comments. |
- SetPreSharedKey: treat empty string as absent (no override) - Apply: skip config file write when only setupKey is set (not persisted) - Add hasPersistentConfig() to separate setup-key-only from persistent fields - Fix log.Infof → log.Info for calls without format verbs - Test: verify setup key absence via raw file bytes (os.ReadFile) - Add tests for empty PSK and setup-key-only Apply scenarios
|
David Brieck seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
client/android/managed_config.go (1)
116-137: Optional: deriveHasConfigfromhasPersistentConfigto avoid duplicated field lists.The two predicates enumerate the same six persistent fields. If a new persistent field is ever added (e.g., another
*booloverride), both lists must be kept in sync orApply()and externalHasConfig()callers will diverge silently.♻️ Proposed dedup
// HasConfig returns true if any configuration value was set by MDM func (m *ManagedConfig) HasConfig() bool { - return m.managementURL != "" || - m.setupKey != "" || - m.adminURL != "" || - m.preSharedKey != nil || - m.rosenpassEnabled != nil || - m.rosenpassPerm != nil || - m.disableAutoConn != nil + return m.setupKey != "" || m.hasPersistentConfig() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/android/managed_config.go` around lines 116 - 137, HasConfig currently duplicates the persistent-field checks present in hasPersistentConfig; change ManagedConfig.HasConfig to simply return m.hasPersistentConfig() || m.setupKey != "" so the persistent-field list is maintained in one place (keep function names ManagedConfig.HasConfig and ManagedConfig.hasPersistentConfig and the setupKey field check to preserve the original behavior).client/android/managed_config_test.go (1)
25-48: Optional: extendApplycoverage to the remaining persistent fields.The table-driven test confirms setters flip
HasConfig(), but theApply(...) → ReadConfig(...)round-trip is only exercised forManagementURL,PreSharedKey, andRosenpassEnabled(withinTestManagedConfig_ApplyOverridesExisting). A regression that fails to wire, e.g.,input.AdminURL,input.RosenpassPermissive, orinput.DisableAutoConnectfromManagedConfigintoConfigInputinApply()would not be caught today.🧪 Sketch of a table-driven persistence test
func TestManagedConfig_ApplyPersistsAllFields(t *testing.T) { cfgFile := filepath.Join(t.TempDir(), "netbird.json") m := NewManagedConfig() m.SetManagementURL("https://mgmt.example.com:443") m.SetAdminURL("https://admin.example.com:443") m.SetPreSharedKey("psk") m.SetRosenpassEnabled(true) m.SetRosenpassPermissive(true) m.SetDisableAutoConnect(true) if err := m.Apply(cfgFile); err != nil { t.Fatalf("Apply failed: %v", err) } cfg, err := profilemanager.ReadConfig(cfgFile) if err != nil { t.Fatalf("ReadConfig failed: %v", err) } if cfg.AdminURL.String() != "https://admin.example.com:443" { t.Errorf("AdminURL = %q", cfg.AdminURL.String()) } if !cfg.RosenpassPermissive { t.Error("RosenpassPermissive not persisted") } if !cfg.DisableAutoConnect { t.Error("DisableAutoConnect not persisted") } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/android/managed_config_test.go` around lines 25 - 48, The Apply→ReadConfig round-trip test coverage is missing for AdminURL, RosenpassPermissive, and DisableAutoConnect, so regressions in ManagedConfig.Apply not copying those fields to ConfigInput can go unnoticed; add/extend a table-driven test (e.g., TestManagedConfig_ApplyPersistsAllFields) that sets m.SetAdminURL(...), m.SetRosenpassPermissive(true), and m.SetDisableAutoConnect(true) on a NewManagedConfig(), calls m.Apply(cfgFile), then uses profilemanager.ReadConfig(cfgFile) and asserts cfg.AdminURL, cfg.RosenpassPermissive, and cfg.DisableAutoConnect match the values set to ensure Apply() correctly persists these fields from ManagedConfig into ConfigInput.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@client/android/managed_config_test.go`:
- Around line 25-48: The Apply→ReadConfig round-trip test coverage is missing
for AdminURL, RosenpassPermissive, and DisableAutoConnect, so regressions in
ManagedConfig.Apply not copying those fields to ConfigInput can go unnoticed;
add/extend a table-driven test (e.g., TestManagedConfig_ApplyPersistsAllFields)
that sets m.SetAdminURL(...), m.SetRosenpassPermissive(true), and
m.SetDisableAutoConnect(true) on a NewManagedConfig(), calls m.Apply(cfgFile),
then uses profilemanager.ReadConfig(cfgFile) and asserts cfg.AdminURL,
cfg.RosenpassPermissive, and cfg.DisableAutoConnect match the values set to
ensure Apply() correctly persists these fields from ManagedConfig into
ConfigInput.
In `@client/android/managed_config.go`:
- Around line 116-137: HasConfig currently duplicates the persistent-field
checks present in hasPersistentConfig; change ManagedConfig.HasConfig to simply
return m.hasPersistentConfig() || m.setupKey != "" so the persistent-field list
is maintained in one place (keep function names ManagedConfig.HasConfig and
ManagedConfig.hasPersistentConfig and the setupKey field check to preserve the
original behavior).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: de4eee7c-4107-48ec-a253-15300f53a199
📒 Files selected for processing (3)
client/android/managed_config.goclient/android/managed_config_test.goclient/ios/NetBirdSDK/managed_config.go
✅ Files skipped from review due to trivial changes (1)
- client/ios/NetBirdSDK/managed_config.go
Add GetManagementURL() to ManagedConfig so native apps can pass the MDM-provided management URL to NewAuth for setup key registration, instead of passing an empty string which defaults to api.netbird.io.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
client/ios/NetBirdSDK/managed_config.go (1)
54-199: Optional: consider deduplicating the iOS/AndroidManagedConfigimplementations.This file and
client/android/managed_config.goare structurally identical — same key constants, same struct, same setters/getters, sameHasConfig/hasPersistentConfig/Applyscaffolding. The only substantive difference isDirectUpdateOrCreateConfigvsUpdateOrCreateConfigat Line 192. Any future change to the managed-config schema (e.g., adding a new MDM key) has to be made in lockstep across both files, and the test file currently only covers the Android variant.One approach: move the shared logic into a small internal package (e.g.,
client/internal/managedconfig) that takes the persistence function as a dependency, and keep the two platform files as thin gomobile-facing wrappers. Non-blocking; happy to leave as-is if you prefer to keep the platform SDK surfaces fully self-contained.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/ios/NetBirdSDK/managed_config.go` around lines 54 - 199, The Android/iOS ManagedConfig implementations are duplicated; refactor by extracting the shared ManagedConfig type and its logic (fields, NewManagedConfig, Set*/Get*/HasConfig/hasPersistentConfig, Apply scaffolding) into a new internal package (e.g., client/internal/managedconfig) that accepts the platform-specific persistence function as a dependency; keep the iOS file's wrapper to construct the shared ManagedConfig and call it with profilemanager.DirectUpdateOrCreateConfig (and Android will call UpdateOrCreateConfig), so only the persistence call differs between platform files.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@client/ios/NetBirdSDK/managed_config.go`:
- Around line 54-199: The Android/iOS ManagedConfig implementations are
duplicated; refactor by extracting the shared ManagedConfig type and its logic
(fields, NewManagedConfig, Set*/Get*/HasConfig/hasPersistentConfig, Apply
scaffolding) into a new internal package (e.g., client/internal/managedconfig)
that accepts the platform-specific persistence function as a dependency; keep
the iOS file's wrapper to construct the shared ManagedConfig and call it with
profilemanager.DirectUpdateOrCreateConfig (and Android will call
UpdateOrCreateConfig), so only the persistence call differs between platform
files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0cd11c02-20d3-492a-aa31-20129273c2e8
📒 Files selected for processing (2)
client/android/managed_config.goclient/ios/NetBirdSDK/managed_config.go
Mirrors the coverage in client/android/managed_config_test.go: - HasConfig / HasSetupKey / GetSetupKey / GetManagementURL behaviour - Each setter marks HasConfig as true - Apply writes management URL, PSK, Rosenpass flags to disk - Apply overrides an existing user-set value - Setup key is never written to the config file (verified via raw bytes) - Setup-key-only config does not create a config file - Empty PSK is ignored - All key constant getters return expected strings Uses //go:build ios to match the package build constraint, consistent with the existing preferences_test.go in the same package. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Update pushed in dbrieck:feat/mdm-managed-config at
Validation note:
|
|
|
Small validation clarification: I have not been able to test the Android managed-config path locally on a device. The end-to-end device validation was done on iOS through Intune/AppConfig with the companion ios-client PR. The Android-related SDK behavior remains covered by the existing managed-config implementation/tests in this PR branch, but reviewers should treat Android device validation as still pending. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
client/ios/NetBirdSDK/managed_config_test.go (1)
101-149: ⚡ Quick winExpand
Applypersistence coverage for the remaining managed keys.The
Applytests validate persistence forManagementURL,RosenpassEnabled, andPreSharedKey, but they don’t assert persisted behavior forAdminURL,RosenpassPermissive, andDisableAutoConnect. Please add one table-drivenApplytest that writes each key and verifies it round-trips fromnetbird.json(including override semantics where relevant).Also applies to: 205-217
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/ios/NetBirdSDK/managed_config_test.go` around lines 101 - 149, Add a table-driven test that uses NewManagedConfig, calls SetAdminURL, SetRosenpassPermissive, and SetDisableAutoConnect (and reuses SetManagementURL/SetRosenpassEnabled/SetPreSharedKey where helpful), calls m.Apply(cfgFile), then reads back the file with profilemanager.ReadConfig and asserts cfg.AdminURL, cfg.RosenpassPermissive, and cfg.DisableAutoConnect round-trip correctly (and include a case for override semantics e.g., ManagementURL present in Preferences then overridden by Apply). Structure as a single TestManagedConfig_Apply_Table or extend existing tests to loop over cases (name, setter func, expected field accessor) to cover AdminURL, RosenpassPermissive, DisableAutoConnect and ensure each case writes, applies, reads and asserts equality.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@client/ios/NetBirdSDK/managed_config_test.go`:
- Around line 101-149: Add a table-driven test that uses NewManagedConfig, calls
SetAdminURL, SetRosenpassPermissive, and SetDisableAutoConnect (and reuses
SetManagementURL/SetRosenpassEnabled/SetPreSharedKey where helpful), calls
m.Apply(cfgFile), then reads back the file with profilemanager.ReadConfig and
asserts cfg.AdminURL, cfg.RosenpassPermissive, and cfg.DisableAutoConnect
round-trip correctly (and include a case for override semantics e.g.,
ManagementURL present in Preferences then overridden by Apply). Structure as a
single TestManagedConfig_Apply_Table or extend existing tests to loop over cases
(name, setter func, expected field accessor) to cover AdminURL,
RosenpassPermissive, DisableAutoConnect and ensure each case writes, applies,
reads and asserts equality.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: db2b5b9b-21b5-43fa-abc0-708233e80044
📒 Files selected for processing (1)
client/ios/NetBirdSDK/managed_config_test.go
|
Hey, is here an update on this? What is left to get this ingegrated? |
I don't have my Mac with me today, but I'll sync and push to the PR after re-implementing it based on what the maintainers have done recently with the overlay. I'll have to test it tomorrow, but it'll at least get this close to actually being accepted. |
|
Closing this because the core SDK portion has been superseded by #6435, which is now merged into Development of the native integrations continues in:
Thank you to everyone who reviewed and helped shape this work. |
4 participants
Summary
Add MDM (Mobile Device Management) managed app configuration support to the Go SDK, enabling Android and iOS native apps to receive and apply configuration from enterprise MDM solutions (Intune, Google Workspace, Jamf, etc.).
Fixes #1918
Changes
Go SDK - Android (
client/android/)managed_config.go— NewManagedConfigstruct with gomobile-compatible setter API for 7 MDM keys:managementUrl,setupKey,adminUrl,preSharedKey,rosenpassEnabled,rosenpassPermissive,disableAutoConnectmanaged_config_test.go— Unit tests covering setters, Apply(), config override, key constantslogin.go— AddedLoginWithSetupKeySync()for silent setup key registration from MDMGo SDK - iOS (
client/ios/NetBirdSDK/)managed_config.go— iOS variant usingDirectUpdateOrCreateConfig()for tvOS sandbox compatibilitylogin.go— AddedLoginWithSetupKeySync()matching Android APIHow It Works
RestrictionsManager, iOS:UserDefaultsmanaged domain)ManagedConfigvia setter methods (gomobile-compatible)Apply(configPath)to write values to NetBird client configLoginWithSetupKeySync()for zero-touch enrollmentMDM values are authoritative — they override any existing user configuration on every app launch.
Companion PRs
Summary by CodeRabbit
New Features
Tests