[client] MDM Android mobile wiring#6435
Open
riccardomanfrin wants to merge 5 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughRefactors MDM policy loading system across desktop, server, and Android from package-level global functions and test hooks to dependency-injected ChangesMDM Loader Dependency Injection Refactor
Sequence Diagram(s)sequenceDiagram
participant Kotlin as Kotlin/Native
participant AndroidClient as Android Client
participant MDMLoader as MDM Loader
participant PlatformImpl as Platform Impl
participant Config as Config
Kotlin->>AndroidClient: SetMDMPolicyFetcher(fetcher)
AndroidClient->>MDMLoader: NewLoader(jsonFetcherAdapter)
Note over AndroidClient: Stores in mdmLoader field
rect rgba(100, 150, 200, 0.5)
Note over AndroidClient,Config: Config Load Path
AndroidClient->>Config: UpdateOrCreateConfig()
Config->>Config: apply() with NewPolicy(nil)
AndroidClient->>MDMLoader: applyMDMOverlay(cfg)
MDMLoader->>MDMLoader: Load()
MDMLoader->>PlatformImpl: loadPlatform()
PlatformImpl->>AndroidClient: fetcher.Fetch()
AndroidClient->>Kotlin: FetchJSON()
Kotlin-->>AndroidClient: JSON string
AndroidClient-->>PlatformImpl: map[string]any
PlatformImpl-->>MDMLoader: policy map
MDMLoader->>Config: ApplyMDMPolicy(policy)
Config-->>AndroidClient: config with overlay
end
sequenceDiagram
participant Server as Server.Start()
participant MDMLoader as MDM Loader
participant Ticker as MDM Ticker
participant Fetcher as Platform Fetcher
Server->>MDMLoader: NewLoader(nil)
Note over Server: Stores in mdmLoader field
Server->>Ticker: NewTicker(interval, mdmLoader)
Ticker->>MDMLoader: Load() init prev
MDMLoader->>Fetcher: loadPlatform()
Fetcher-->>MDMLoader: policy map (or nil)
MDMLoader-->>Ticker: Policy
rect rgba(150, 200, 100, 0.5)
Note over Ticker,Fetcher: Ticker Run Loop (periodic)
loop Each Tick
Ticker->>MDMLoader: Load() current
MDMLoader->>Fetcher: loadPlatform()
Fetcher-->>MDMLoader: updated policy
MDMLoader-->>Ticker: Policy
Ticker->>Ticker: Compare prev vs curr
alt Policy Changed
Ticker->>Ticker: Emit diffs, invoke onChange
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@client/mdm/policy_mobile.go`:
- Around line 20-41: The global fetcher variable accessed by
SetMobilePolicyFetcher and loadPlatformPolicy lacks synchronization, creating a
potential data race if SetMobilePolicyFetcher is called concurrently or after
initialization. Add a sync.Mutex to protect access to the fetcher variable. In
SetMobilePolicyFetcher, acquire the lock before writing to fetcher, and in
loadPlatformPolicy, acquire the lock before reading from fetcher. This ensures
atomic registration and read operations regardless of when
SetMobilePolicyFetcher is invoked.
🪄 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: 21ce5b76-f5ba-4cd4-b900-185874005e16
📒 Files selected for processing (2)
client/android/mdm.goclient/mdm/policy_mobile.go
Release artifactsBuilt for PR head
GHCR images (amd64)
This comment is updated by the Release workflow. Artifact links expire according to the workflow retention policy. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
client/mdm/policy.go (1)
108-111: ⚡ Quick winAlign
NewLoaderdocumentation with actual platform behavior.Line 108–111 says the fetcher is consulted only on mobile, but desktop loaders also short-circuit to
l.fetcher.Fetch()when a fetcher is injected. Please update this comment to match the real cross-platform contract and avoid misleading call sites.🤖 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/mdm/policy.go` around lines 108 - 111, The comment for NewLoader function (lines 108-111) incorrectly states that the fetcher is consulted only on mobile builds and is unused on desktop. Update this documentation comment to accurately reflect that the fetcher is consulted on both mobile and desktop platforms when a fetcher is injected, not just on mobile. The comment should clarify the actual cross-platform contract to avoid misleading callers.client/internal/profilemanager/config.go (1)
187-193: ⚡ Quick winClarify
ApplyMDMPolicyreset semantics in the doc comment.The current wording implies that an empty policy clears prior overlay effects on the same
Configinstance, butapplyMDMPolicyis overlay-only and does not restore previously overridden fields. Please tighten the comment to reflect “apply on freshly resolved config” semantics (or implement explicit revert behavior).🤖 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/internal/profilemanager/config.go` around lines 187 - 193, The doc comment for the ApplyMDMPolicy function currently states that passing an empty Policy will clear any prior overlay, but this is misleading since the function only applies overlays and does not restore previously overridden fields. Update the doc comment to clarify the actual behavior: that ApplyMDMPolicy applies MDM policy overlays on top of the currently resolved config values and should be called on freshly resolved config (as the lifecycle owner does), rather than implying it can be used to revert or clear prior overlays by passing an empty policy. Remove or revise the wording about clearing prior overlays to accurately reflect overlay-only semantics.
🤖 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.
Inline comments:
In `@client/mdm/policy_test.go`:
- Around line 153-161: The test TestLoader_NilFetcherReturnsEmpty is
non-deterministic because calling NewLoader(nil).Load() may read real MDM state
on Windows/macOS, causing the emptiness assertions to fail on managed hosts.
Either add build tags (such as //go:build linux) to gate this test to platforms
where nil fetcher produces a deterministic empty result, or refactor the test to
use a mock fetcher that ensures deterministic behavior regardless of actual MDM
state on the host. This ensures the assertions in the test remain valid and
consistent across all platforms.
---
Nitpick comments:
In `@client/internal/profilemanager/config.go`:
- Around line 187-193: The doc comment for the ApplyMDMPolicy function currently
states that passing an empty Policy will clear any prior overlay, but this is
misleading since the function only applies overlays and does not restore
previously overridden fields. Update the doc comment to clarify the actual
behavior: that ApplyMDMPolicy applies MDM policy overlays on top of the
currently resolved config values and should be called on freshly resolved config
(as the lifecycle owner does), rather than implying it can be used to revert or
clear prior overlays by passing an empty policy. Remove or revise the wording
about clearing prior overlays to accurately reflect overlay-only semantics.
In `@client/mdm/policy.go`:
- Around line 108-111: The comment for NewLoader function (lines 108-111)
incorrectly states that the fetcher is consulted only on mobile builds and is
unused on desktop. Update this documentation comment to accurately reflect that
the fetcher is consulted on both mobile and desktop platforms when a fetcher is
injected, not just on mobile. The comment should clarify the actual
cross-platform contract to avoid misleading callers.
🪄 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: 239e01a4-4e58-4765-ad8d-d153e6b35e88
📒 Files selected for processing (15)
client/android/client.goclient/android/mdm.goclient/internal/profilemanager/config.goclient/internal/profilemanager/config_mdm_test.goclient/mdm/policy.goclient/mdm/policy_darwin.goclient/mdm/policy_mobile.goclient/mdm/policy_other.goclient/mdm/policy_test.goclient/mdm/policy_windows.goclient/mdm/ticker.goclient/mdm/ticker_test.goclient/server/mdm.goclient/server/server.goclient/server/setconfig_mdm_test.go
💤 Files with no reviewable changes (1)
- client/server/mdm.go
Comment on lines
+153
to
161
| func TestLoader_NilFetcherReturnsEmpty(t *testing.T) { | ||
| // Loader.Load with no fetcher (desktop construction) must degrade | ||
| // gracefully and never return nil; on linux loadPlatform is a stub | ||
| // returning (nil, nil), and Load is expected to translate that | ||
| // into a non-nil empty Policy. | ||
| p := NewLoader(nil).Load() | ||
| require.NotNil(t, p) | ||
| assert.True(t, p.IsEmpty()) | ||
| assert.Empty(t, p.ManagedKeys()) |
Contributor
There was a problem hiding this comment.
Make this test deterministic across platforms.
On Line 158, NewLoader(nil).Load() can read real MDM state on Windows/macOS, so the emptiness assertion on Line 160 may fail on managed hosts. Please gate this test to platforms where nil fetcher means “no source,” or switch the test to a deterministic seam.
One stabilization option
+import "runtime"
+
func TestLoader_NilFetcherReturnsEmpty(t *testing.T) {
+ if runtime.GOOS == "windows" || runtime.GOOS == "darwin" {
+ t.Skip("nil fetcher may read real OS-managed policy on this platform")
+ }
p := NewLoader(nil).Load()
require.NotNil(t, p)
assert.True(t, p.IsEmpty())🤖 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/mdm/policy_test.go` around lines 153 - 161, The test
TestLoader_NilFetcherReturnsEmpty is non-deterministic because calling
NewLoader(nil).Load() may read real MDM state on Windows/macOS, causing the
emptiness assertions to fail on managed hosts. Either add build tags (such as
//go:build linux) to gate this test to platforms where nil fetcher produces a
deterministic empty result, or refactor the test to use a mock fetcher that
ensures deterministic behavior regardless of actual MDM state on the host. This
ensures the assertions in the test remain valid and consistent across all
platforms.
|
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
1 participant
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
Adds the Android-side of the MDM bridge that lets the mobile gomobile
binding accept a managed-configuration snapshot from the native layer
(Kotlin) and surface it through the same
Config.apply → applyMDMPolicy → loadMDMPolicy → loadPlatformPolicychain the desktoploaders already use.
Two files:
client/mdm/policy_mobile.go(replaces the previous(nil, nil)stub). Defines an internal
PolicyFetcherinterface and aSetMobilePolicyFetchersetter; the registered fetcher is whatloadPlatformPolicycalls on every read. Set-once at gomobileinit, never mutated at runtime → no synchronisation required for
the read path.
client/android/mdm.go(new). The gomobile-facing bridge.Exposes a Java/Kotlin-friendly
PolicyFetcherinterface with asingle
FetchJSON() stringmethod —map[string]anycannotcross the gomobile JNI boundary, so the native layer returns a
JSON-encoded snapshot and a Go-side
jsonFetcherAdapterparses itback into the internal
mdm.PolicyFetcher. AddsClient.OnMDMPolicyChanged()as the entry point the nativebroadcast receiver invokes when the OS reports a managed-config
change; the method cancels the current engine context so the
outer native loop relaunches
Run()and the new run picks upthe fresh policy.
Runtime flow on a managed-config change (mobile):
ACTION_APPLICATION_RESTRICTIONS_CHANGED(Android)/
UserDefaults.didChangeNotification(iOS).Client.OnMDMPolicyChanged()→Stop()→engine teardown.
Run().UpdateOrCreateConfig→Config.apply→applyMDMPolicy(loadMDMPolicy())→LoadPolicy → loadPlatformPolicy→fetcher.Fetch()→ adapterFetchJSON()callback into Kotlin→
RestrictionsManager.getApplicationRestrictions()parsed andreturned to Go.
No ticker on mobile (the desktop ticker stays unchanged in
client/server, build-tagged!ios && !androidat the call sitevia
service_controller.go). The OS notification is authoritative;no Go-side diff loop, no cached policy snapshot in either Go or
Kotlin —
RestrictionsManageris the single source of truth.The Kotlin / Java side of the wiring (the BroadcastReceiver, the
fetcher implementation, the
app_restrictions.xmlschema, theSettings UI lock-on-managed-keys, the EngineRestarter wiring) lives
in the
netbirdio/android-clientrepo and is shipped in a separatePR there.
e2e testing done on Android.
Issue ticket number and link
No public issue. Internal task to land the Android half of the MDM
managed-configuration feature originally shipped for desktop in PR
#6374. The mobile half was
called out in that PR's description as a follow-up. This PR is the
Go-side of that follow-up; the Kotlin side is a companion PR against
netbirdio/android-client.Stack
Checklist
Documentation
Select exactly one:
The desktop MDM documentation in
netbirdio/docsalready describesthe managed-key set and the per-platform mappings (Windows registry,
macOS plist). The mobile path consumes the exact same key set, so no
new key documentation is needed. The Kotlin / Java integration of
the bridge —
MDMPolicyFetcher, the BroadcastReceiver, theapp_restrictions.xmlmanifest entry — is shipped and documented inthe companion
netbirdio/android-clientPR; the Go-side publicsurface added here (
Client.OnMDMPolicyChanged,SetMobilePolicyFetcher,PolicyFetcher) is implementation gluethat only the in-repo native bridge calls, not an API end users
configure directly.
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
N/A
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests