Android commands backend - #46031
Merged
Merged
Conversation
Tracks AMAPI commands issued by Fleet via EnterprisesDevicesService.IssueCommand
(Lock, Wipe, Clear passcode for Android hosts). host_mdm_actions.{lock_ref, wipe_ref}
points into this table via command_uuid for Android hosts, mirroring how those
columns point into nano_commands for Apple and mdm_windows_commands for Windows.
MDMAndroidCommand maps to the mdm_android_commands table. Status and type values mirror the strings AMAPI uses on the wire so the same enum doubles as the value we send in IssueCommand and the value we read back from Pub/Sub COMMAND notifications.
Adds the four CRUD methods Fleet needs at AMAPI command-issue and
Pub/Sub COMMAND-notification time:
- NewMDMAndroidCommand: insert at issue time (auto-generates command_uuid
if caller leaves it empty)
- GetMDMAndroidCommandByUUID: lookup by Fleet command_uuid (the value
host_mdm_actions.{lock_ref, wipe_ref} points to)
- GetMDMAndroidCommandByOperationName: lookup by AMAPI operation name,
used by the Pub/Sub handler to correlate a notification back to the
originating Fleet command
- UpdateMDMAndroidCommandStatus: transition status (and optional error
code/message) when the device acks or AMAPI rejects
Interface declared on fleet.Datastore (mirrors NewAndroidPolicyRequest /
GetAndroidPolicyRequestByUUID), with matching mock entries and an
integration test exercising all four methods plus NotFound paths.
Teaches HostLockWipeStatus to interpret AMAPI lock/wipe state stored in mdm_android_commands, so the existing PendingAction / DeviceStatus / IsLocked / IsWiped helpers work for android hosts the same way they do for darwin, ios, ipados, windows, and linux. - GetHostLockWipeStatus: new android case that maps lock_ref/wipe_ref through mdm_android_commands and populates Lock/WipeMDMCommand + Lock/WipeMDMCommandResult. Pending status leaves the result nil so IsPendingLock/IsPendingWipe report "pending". Orphan refs log and return zero-state (mirrors apple/windows). - GetHostsLockWipeStatusBatch: matching android collection + batch query against mdm_android_commands. - HostLockWipeStatus methods (server/fleet/scripts.go): android arms added to IsPendingLock, IsPendingWipe (covered by the existing fall through), IsLocked, and IsWiped. They compare against the literal "acknowledged" string (the value of android.MDMAndroidCommandStatusAcknowledged) to avoid pulling the android package into server/fleet.
Wires the Android Management API IssueCommand endpoint through Fleet's three Client implementations so the service layer can dispatch LOCK, RESET_PASSWORD, and WIPE commands to enrolled Android devices: - Client interface (androidmgmt/client.go): add the method declaration with a docstring pointing at the AMAPI reference. - GoogleClient: direct AMAPI passthrough with the standard error wrapping pattern used by neighbouring methods. - ProxyClient: same passthrough, plus the Authorization: Bearer header the proxy needs for every call. - mock/Client: hand-maintained mock additions (Func type, struct field + Invoked flag, method wrapper) so service-layer tests can swap in fakes without contacting real AMAPI. Validated by porting the spike code that exercised this against real BYO and COBO devices for all three command types.
Replaces the spike's log-only implementations with the real
issue-and-persist flow:
1. Service layer (server/mdm/android/service.go + service/service.go):
- LockAndroidHost: AMAPI LOCK -> NewMDMAndroidCommand +
host_mdm_actions.lock_ref (via LockHostViaAndroidMDM).
- ClearAndroidPasscode: AMAPI RESET_PASSWORD with newPassword=""
(clears, does not regenerate, per product); persists the row but
does NOT touch host_mdm_actions (one-shot, no UI state).
- WipeAndroidHost: AMAPI WIPE -> NewMDMAndroidCommand +
host_mdm_actions.wipe_ref (via WipeHostViaAndroidMDM). WipeParams
intentionally set to an empty struct -- the spike confirmed AMAPI
rejects WIPE without it, despite the SDK marking it Optional.
- resolveAndroidCommandTarget helper centralises the host/enterprise
lookup + secret refresh shared by all three methods.
- longCommandDuration = "315360000s" (10 years) on every command, so
undelivered commands stay queued forever -- matches Apple/Windows
MDM semantics. AMAPI default of 600s would silently expire.
2. Datastore (server/fleet/datastore.go + datastore/mysql/android.go):
- LockHostViaAndroidMDM / WipeHostViaAndroidMDM: transactional
two-write helpers that insert the mdm_android_commands row and
upsert host_mdm_actions in a single retried tx. Mirrors
WipeHostViaWindowsMDM. Generated mocks updated.
- Integration tests cover the lock-pending path and the wipe-requeue
path (later wipe_ref overwrites earlier, both command rows preserved
for audit).
Pub/Sub COMMAND ack handling (Phase 9) and EE dispatch (Phase 10) are
follow-ups; this commit gets the command-issue side end-to-end.
Per product (2026-05-20): unenrolling a BYO Android host should run an AMAPI WIPE command, which on a personal device only wipes the work profile and leaves the personal side intact. The mdm_unenrolled activity is still emitted on the subsequent Pub/Sub COMMAND path (unchanged from the previous behavior). COBO unenroll keeps the existing EnterprisesDevicesDelete call -- it terminates management without factory-resetting the device. UnenrollAndroidHost branches on host_mdm.is_personal_enrollment. The BYO branch persists an mdm_android_commands row (audit trail) but does NOT write host_mdm_actions.wipe_ref, because BYO unenroll surfaces in the UI as "unenroll", not as a wipe.
AMAPI delivers a COMMAND notification when a device acks (or rejects) an issued command. The notification is an Operation envelope whose Name matches the operation_name we recorded at IssueCommand time. This commit turns the spike's log-only handler into a real state machine: - Decodes the payload as androidmanagement.Operation. - Looks up the Fleet row by operation_name. NotFound (e.g. a command issued from a previous deployment) is acked without retry to keep Pub/Sub from looping. - Empty op.Name is logged and acked (malformed/foreign payload). - If op.Error is set, transitions the row to "error" with error_code (the int google.rpc.Code rendered as string) and error_message. - Otherwise transitions to "acknowledged". - Already-terminal rows are ignored -- AMAPI delivers at-least-once, so we must be idempotent. host_mdm_actions does not need a separate update because the android branch of GetHostLockWipeStatus reads the row status string directly, and HostLockWipeStatus.IsLocked/IsWiped compare against "acknowledged". Unit tests cover all five paths (ack, error, idempotent re-delivery, unknown op, empty op.Name).
Threads the android.Service Lock/Wipe/ClearPasscode helpers (added in the previous commit) into the EE Service layer so they're reachable from the existing /hosts/:id/lock, /hosts/:id/wipe, /hosts/:id/mdm/passcode API endpoints. - LockHost case "android": validates AndroidEnabledAndConfigured + IsHostConnectedToFleetMDM. Lock is supported on both BYO and COBO. - enqueueLockHostRequest case "android": dispatches to LockAndroidHost and explicitly resets activity.ViewPIN -- Android has no unlock PIN. - WipeHost case "android": rejects BYO with a descriptive error (BYO unenroll already runs an AMAPI WIPE under the hood) and validates AndroidEnabledAndConfigured. requireMDM is set so the existing MDM-connection precondition runs. - enqueueWipeHostRequest case "android": dispatches to WipeAndroidHost. - ClearPasscode: new clearPasscodeAndroid branch mirroring clearPasscodeApple's shape. Validates AndroidEnabledAndConfigured, dispatches to ClearAndroidPasscode, emits the existing cleared_passcode activity (shared with Apple), returns a CommandEnqueueResult shaped for API compatibility. Error message updated to mention Android.
Mirrors the existing lock/unlock/wipe subcommands and exposes clear- passcode at the CLI for iOS/iPadOS (existing) and Android (new in this PR). The server endpoint at /hosts/:id/clear_passcode already dispatches by platform, so the CLI doesn't need android-specific branching. - Promotes the previously-private clearPasscodeResponse to fleet.ClearPasscodeResponse, matching LockHostResponse / WipeHostResponse and letting client + server share the type. - Adds Client.MDMClearPasscodeHost in server/service/client_mdm.go. - Adds mdmClearPasscodeCommand in cmd/fleetctl/fleetctl/mdm.go and registers it under `fleetctl mdm clear-passcode` (alias clear_passcode). - Unit test TestMDMClearPasscodeCommand exercises the failure paths (missing flag, unknown host, MDM-off). Happy paths against real iOS/Android hosts are covered by integration tests.
End-to-end coverage against the real Fleet HTTP handler stack with the
mock AMAPI client. Verifies that for each command the API endpoint
issues the right AMAPI call, persists the mdm_android_commands row,
updates host_mdm_actions where applicable, and surfaces the right
device/pending status on the host detail endpoint.
- TestAndroidHostUnenrollMDM: updated to match the new BYO behavior
-- BYO unenroll now issues an AMAPI WIPE (work-profile only), not
EnterprisesDevicesDelete. Added a COBO sub-case to assert the
delete path is still used for company-owned hosts.
- TestAndroidLockWipeClearPasscode (new):
- Lock COBO: AMAPI LOCK + Duration=315360000s; GET host returns
PendingAction=lock + DeviceStatus=unlocked; Pub/Sub COMMAND ack
transitions the row to acknowledged and the host page to locked.
- Wipe BYO rejected with the "personally-owned" error message.
- Wipe COBO: AMAPI WIPE with non-nil empty WipeParams + long
duration; PendingAction=wipe surfaces.
- Clear passcode: AMAPI RESET_PASSWORD with newPassword=""; no
host_mdm_actions write.
- Pub/Sub COMMAND with op.Error transitions the row to "error" and
populates error_code (the int rpc code as string) and
error_message.
To support both BYO and COBO test fixtures, factored
createAndroidHostWithStorage into createAndroidHostForTest taking an
explicit companyOwned bool; the existing helper now just wraps it for
BYO callers.
Also replaced the private clearPasscodeResponse reference in the
existing iOS clear-passcode integration test with the public
fleet.ClearPasscodeResponse promoted earlier in this branch.
Member
Author
|
@coderabbitai full review |
Member
Author
|
/agentic_review |
Contributor
✅ Actions performedFull review triggered. |
Code Review by Qodo
1.
|
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:
WalkthroughThis PR implements Android MDM command support for Lock, Wipe, and Clear Passcode via the Android Management API. It adds a mdm_android_commands table and migration, datastore CRUD and transaction helpers to persist commands and set host lock/wipe refs, AMAPI client IssueCommand wiring, service methods to issue LOCK/RESET_PASSWORD/WIPE (using a 10-year duration) and persist pending rows, Pub/Sub COMMAND handling to transition command status (acknowledged/error) and store error details, host lock/wipe status resolution for Android in batch and single-host flows, HTTP/API/CLI surfaces (clear-passcode endpoint and fleetctl mdm clear-passcode), mock/test support, and unit/integration tests covering end-to-end behavior and edge cases. Possibly related issues
🚥 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: 2
🤖 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 `@ee/server/service/mdm.go`:
- Around line 1847-1851: The CommandEnqueueResult currently returns a newly
generated CommandUUID (uuid.NewString()) that does not match the persisted
command UUID created inside ClearAndroidPasscode, so callers cannot look up the
real command via GetMDMAndroidCommandByUUID; update ClearAndroidPasscode to
return the created command UUID (or add an out param) and propagate that UUID
into CommandEnqueueResult.CommandUUID here (replacing uuid.NewString()), or
alternatively change the comment and API contract to explicitly document that
CommandUUID is decorative and not usable for lookups; reference
ClearAndroidPasscode, CommandEnqueueResult.CommandUUID and
GetMDMAndroidCommandByUUID when making the change.
In
`@server/datastore/mysql/migrations/tables/20260521205417_AddMDMAndroidCommands.go`:
- Around line 37-40: Change the non-unique index on operation_name to a unique
index to enforce deterministic correlation: replace "INDEX
idx_mdm_android_commands_operation_name (operation_name)" with "UNIQUE INDEX
idx_mdm_android_commands_operation_name (operation_name)" in the
AddMDMAndroidCommands migration; before applying the change ensure the migration
handles existing duplicate operation_name values (either deduplicate/merge them
or fail with a clear pre-check) and update the down/rollback path to drop the
UNIQUE index accordingly.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f224c67f-72cb-4b43-9c23-660a45ef391f
📒 Files selected for processing (30)
changes/41683-android-lock-wipe-clear-passcodecmd/fleetctl/fleetctl/mdm.gocmd/fleetctl/fleetctl/mdm_test.goee/server/service/hosts.goee/server/service/mdm.goserver/datastore/mysql/android.goserver/datastore/mysql/android_test.goserver/datastore/mysql/hosts_test.goserver/datastore/mysql/migrations/tables/20260521205417_AddMDMAndroidCommands.goserver/datastore/mysql/migrations/tables/20260521205417_AddMDMAndroidCommands_test.goserver/datastore/mysql/schema.sqlserver/datastore/mysql/scripts.goserver/fleet/api_scripts.goserver/fleet/datastore.goserver/fleet/scripts.goserver/mdm/android/android.goserver/mdm/android/mock/client.goserver/mdm/android/service.goserver/mdm/android/service/androidmgmt/client.goserver/mdm/android/service/androidmgmt/google_client.goserver/mdm/android/service/androidmgmt/proxy_client.goserver/mdm/android/service/pubsub.goserver/mdm/android/service/pubsub_test.goserver/mdm/android/service/service.goserver/mock/datastore_mock.goserver/service/client_mdm.goserver/service/integration_core_test.goserver/service/integration_mdm_commands_test.goserver/service/integration_mdm_test.goserver/service/mdm.go
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds full backend support for issuing Android MDM “device commands” (Lock, Wipe, Clear passcode) via Google’s Android Management API (AMAPI), including persistence of issued commands and asynchronous state transitions via Pub/Sub COMMAND notifications so host lock/wipe status can be surfaced consistently alongside Apple/Windows.
Changes:
- Add a new
mdm_android_commandstable + datastore APIs to persist Android commands (pending → acknowledged/error) and to link lock/wipe tohost_mdm_actions. - Implement AMAPI
IssueCommandsupport (client + service) for Android Lock/Wipe/ResetPassword, plus Pub/Sub COMMAND handling to update command status. - Extend EE service endpoints and fleetctl to support Android clear-passcode, and adjust Android unenroll behavior (BYO uses AMAPI WIPE).
Reviewed changes
Copilot reviewed 29 out of 30 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/mdm.go | Switch clear-passcode endpoint response to shared fleet.ClearPasscodeResponse. |
| server/service/integration_mdm_test.go | Expand Android unenroll test and add end-to-end Android lock/wipe/clear-passcode + Pub/Sub ack integration coverage. |
| server/service/integration_mdm_commands_test.go | Update clear-passcode integration test to use shared response type. |
| server/service/integration_core_test.go | Add helper to create Android BYO vs COBO hosts for tests. |
| server/service/client_mdm.go | Add client method for clear-passcode endpoint. |
| server/mock/datastore_mock.go | Extend mock datastore with Android command CRUD + lock/wipe helpers. |
| server/mdm/android/service/service.go | Implement Android Lock/Wipe/ClearPasscode issuing + BYO unenroll WIPE behavior and shared target resolution. |
| server/mdm/android/service/pubsub.go | Add Pub/Sub COMMAND handler to transition Android command status. |
| server/mdm/android/service/pubsub_test.go | Add unit tests for Pub/Sub COMMAND handling. |
| server/mdm/android/service/androidmgmt/{client,google_client,proxy_client}.go | Add AMAPI EnterprisesDevicesIssueCommand support for both direct and proxy clients. |
| server/mdm/android/service.go | Extend Android service interface with lock/wipe/clear-passcode methods. |
| server/mdm/android/mock/client.go | Extend Android AMAPI mock client with IssueCommand support. |
| server/mdm/android/android.go | Add MDMAndroidCommand model + command/status enums. |
| server/fleet/scripts.go | Extend lock/wipe status helpers to support Android acknowledged semantics. |
| server/fleet/datastore.go | Add datastore interface methods for Android command persistence and lock/wipe transactional updates. |
| server/fleet/api_scripts.go | Add shared ClearPasscodeResponse type in fleet API shapes. |
| server/datastore/mysql/{android.go,scripts.go} | Implement Android command CRUD, transactional lock/wipe ref writes, and host lock/wipe status hydration for Android. |
| server/datastore/mysql/schema.sql | Add mdm_android_commands table to schema snapshot. |
| server/datastore/mysql/migrations/tables/20260521205417_* | Add migration + test for mdm_android_commands. |
| server/datastore/mysql/{hosts_test.go,android_test.go} | Add test coverage for Android lock/wipe status and command CRUD/helpers. |
| ee/server/service/{hosts.go,mdm.go} | Wire Android lock/wipe/clear-passcode at EE service layer + BYO wipe rejection behavior. |
| cmd/fleetctl/fleetctl/{mdm.go,mdm_test.go} | Add fleetctl mdm clear-passcode subcommand and CLI validation tests. |
| changes/41683-android-lock-wipe-clear-passcode | Add changelog entry describing Android command support and long-duration semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #46031 +/- ##
==========================================
+ Coverage 66.82% 66.84% +0.01%
==========================================
Files 2754 2755 +1
Lines 220158 220694 +536
Branches 10996 10996
==========================================
+ Hits 147131 147514 +383
- Misses 59733 59839 +106
- Partials 13294 13341 +47
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Addresses the two failing CI checks on PR #46031: test-mock-changes: server/mock/datastore_mock.go had manual edits from the earlier commits adding android Lock/Wipe/Clear-passcode datastore methods. CI runs `make mock` and fails on any diff. Regenerated via `make mock`; the only change is field/type ordering. lint-incremental: - modernize: `ptr.Bool(true)` -> `new(true)` in the new clear-passcode CLI test fixture. - modernize: `map[string]interface{}` -> `map[string]any` in the BYO- unenroll and Wipe payload-marshal helpers (4 sites). - testifylint: `require.Equal(t, "", ...)` -> `require.Empty(t, ...)` in three places and `require.Equal(t, n, len(s))` -> `require.Len(t, s, n)` in one place within TestAndroidLockWipeClearPasscode.
getvictor
changed the title
After merging main (#46079 renumbered 11 migrations to 20260522195224-..235), our 20260521205417_AddMDMAndroidCommands ended up before them in the migration timeline. Bump our timestamp to 20260522195236 so the migration runs last, and regenerate schema.sql.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
Member
Author
|
@claude review once |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/mdm/android/service/service.go (2)
1050-1060: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winEnforce the COBO-only wipe invariant in the service too.
WipeAndroidHostcurrently issuesWIPEfor any Android host that resolves. The HTTP layer may reject BYO today, but this exported service method is the last safe place to keep the contract from drifting; otherwise a future caller can turn “wipe device” into the BYO work-profile unenroll path while Fleet records it as a wipe.♻️ Possible guard
func (svc *Service) WipeAndroidHost(ctx context.Context, hostID uint) error { host, deviceName, err := svc.resolveAndroidCommandTarget(ctx, hostID, "wipe") if err != nil { return err } + hostMDM, err := svc.fleetDS.GetHostMDM(ctx, host.ID) + switch { + case fleet.IsNotFound(err): + return &fleet.BadRequestError{Message: "host is not MDM-enrolled"} + case err != nil: + return ctxerr.Wrap(ctx, err, "getting host_mdm for android wipe") + case hostMDM.IsPersonalEnrollment: + return &fleet.BadRequestError{Message: "Android wipe is only supported for company-owned devices"} + } op, err := svc.androidAPIClient.EnterprisesDevicesIssueCommand(ctx, deviceName, &androidmanagement.Command{ Type: string(android.MDMAndroidCommandTypeWipe),🤖 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 `@server/mdm/android/service/service.go` around lines 1050 - 1060, WipeAndroidHost currently issues a full WIPE for any resolved Android host; add a guard in the Service.WipeAndroidHost method that enforces the COBO-only invariant by inspecting the resolved host (the host variable returned by resolveAndroidCommandTarget) and returning an error if the host is not corporate-owned (e.g., host.IsCOBO or host.EnrollmentProfile != "COBO" depending on your Host model), before calling androidAPIClient.EnterprisesDevicesIssueCommand; return a clear, typed error like "wipe only allowed for COBO devices" so callers cannot bypass the HTTP-layer check.
893-914:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAvoid returning retriable errors after AMAPI has already accepted the command.
Each of these paths sends the AMAPI command first and only then writes Fleet’s correlation state. If that local write fails, the API returns an error even though the device already accepted the command, so a client/user retry can enqueue a second lock/wipe/reset while the first Pub/Sub result has no row to attach to.
Also applies to: 986-1007, 1022-1042, 1056-1076
🤖 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 `@server/mdm/android/service/service.go` around lines 893 - 914, The AMAPI command is already accepted before we attempt to persist Fleet’s correlation row (svc.androidAPIClient.EnterprisesDevicesIssueCommand -> cmd persisted by svc.fleetDS.NewMDMAndroidCommand), so if NewMDMAndroidCommand fails we must not return a retriable error; instead log the persistence failure (svc.logger.ErrorContext) with details and return success to the caller so clients won't re-issue the same device command. Update the paths around EnterprisessDevicesIssueCommand and svc.fleetDS.NewMDMAndroidCommand (including the other affected blocks) to swallow persistence errors: keep the call to NewMDMAndroidCommand, log the failure and any identifying info (host.ID, op.Name, CommandUUID), optionally emit a metric, but return nil (or a non-retriable acknowledgement) rather than propagating the error.
🤖 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.
Outside diff comments:
In `@server/mdm/android/service/service.go`:
- Around line 1050-1060: WipeAndroidHost currently issues a full WIPE for any
resolved Android host; add a guard in the Service.WipeAndroidHost method that
enforces the COBO-only invariant by inspecting the resolved host (the host
variable returned by resolveAndroidCommandTarget) and returning an error if the
host is not corporate-owned (e.g., host.IsCOBO or host.EnrollmentProfile !=
"COBO" depending on your Host model), before calling
androidAPIClient.EnterprisesDevicesIssueCommand; return a clear, typed error
like "wipe only allowed for COBO devices" so callers cannot bypass the
HTTP-layer check.
- Around line 893-914: The AMAPI command is already accepted before we attempt
to persist Fleet’s correlation row
(svc.androidAPIClient.EnterprisesDevicesIssueCommand -> cmd persisted by
svc.fleetDS.NewMDMAndroidCommand), so if NewMDMAndroidCommand fails we must not
return a retriable error; instead log the persistence failure
(svc.logger.ErrorContext) with details and return success to the caller so
clients won't re-issue the same device command. Update the paths around
EnterprisessDevicesIssueCommand and svc.fleetDS.NewMDMAndroidCommand (including
the other affected blocks) to swallow persistence errors: keep the call to
NewMDMAndroidCommand, log the failure and any identifying info (host.ID,
op.Name, CommandUUID), optionally emit a metric, but return nil (or a
non-retriable acknowledgement) rather than propagating the error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 23c555e9-ee39-44ff-9359-210f0007e2c7
📒 Files selected for processing (20)
changes/41683-android-lock-wipe-clear-passcodecmd/fleetctl/fleetctl/mdm.gocmd/fleetctl/fleetctl/mdm_test.goee/server/service/hosts.goee/server/service/mdm.goserver/datastore/mysql/android.goserver/datastore/mysql/android_test.goserver/datastore/mysql/hosts_test.goserver/datastore/mysql/migrations/tables/20260522195236_AddMDMAndroidCommands.goserver/datastore/mysql/schema.sqlserver/datastore/mysql/scripts.goserver/fleet/datastore.goserver/fleet/scripts.goserver/mdm/android/android.goserver/mdm/android/service.goserver/mdm/android/service/pubsub.goserver/mdm/android/service/pubsub_test.goserver/mdm/android/service/service.goserver/mock/datastore_mock.goserver/service/integration_mdm_test.go
✅ Files skipped from review due to trivial changes (1)
- server/mock/datastore_mock.go
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/mdm/android/service/service.go (1)
896-917:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftPersist command intent before calling AMAPI.
These paths issue the remote command first and only then write Fleet state. If the DB write fails, the method returns an error after the device has already accepted a wipe/lock/reset-password operation. A retry can enqueue the same destructive command again, and Fleet still has no durable record for the first operation. Please invert this flow so the pending command intent is stored before
IssueCommand(or use an outbox/update-after-ack pattern).Also applies to: 989-1010, 1025-1045, 1059-1079
🤖 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 `@server/mdm/android/service/service.go` around lines 896 - 917, The code issues the remote AMAPI command via svc.androidAPIClient.EnterprisesDevicesIssueCommand before persisting the local intent with svc.fleetDS.NewMDMAndroidCommand, which risks lost durable state if the DB write fails; change the flow to create and persist an android.MDMAndroidCommand record (with CommandUUID, HostUUID, CommandType, Status pending) via NewMDMAndroidCommand prior to calling EnterprisesDevicesIssueCommand, then call the API, capture op.Name, and update the persisted row (e.g., OperationName field) with the returned operation name; apply the same change to the other similar blocks referenced (lines ~989-1010, 1025-1045, 1059-1079) so the DB always contains the pending intent before issuing destructive commands.
🤖 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.
Outside diff comments:
In `@server/mdm/android/service/service.go`:
- Around line 896-917: The code issues the remote AMAPI command via
svc.androidAPIClient.EnterprisesDevicesIssueCommand before persisting the local
intent with svc.fleetDS.NewMDMAndroidCommand, which risks lost durable state if
the DB write fails; change the flow to create and persist an
android.MDMAndroidCommand record (with CommandUUID, HostUUID, CommandType,
Status pending) via NewMDMAndroidCommand prior to calling
EnterprisesDevicesIssueCommand, then call the API, capture op.Name, and update
the persisted row (e.g., OperationName field) with the returned operation name;
apply the same change to the other similar blocks referenced (lines ~989-1010,
1025-1045, 1059-1079) so the DB always contains the pending intent before
issuing destructive commands.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 25f93d64-c447-445a-b8d6-c6699cc65395
📒 Files selected for processing (3)
cmd/fleetctl/fleetctl/mdm.goee/server/service/mdm.goserver/mdm/android/service/service.go
ksykulev
reviewed
May 23, 2026
ksykulev
left a comment
ksykulev
left a comment
Contributor
There was a problem hiding this comment.
If a command notification is dropped, it looks like the command stays pending forever. Apple uses CheckIn polling and Windows uses a sync session. The Android path has no equivalent. A stuck pending row means IsPendingLock/IsPendingWipe returns true indefinitely and the user can't re-issue the command. are there plans to handle this somehow? clear them out, or some sort of timeout?
| // which is the key used to correlate Pub/Sub COMMAND notifications back to the originating Fleet command. | ||
| func Up_20260522195236(tx *sql.Tx) error { | ||
| _, err := tx.Exec(` | ||
| CREATE TABLE mdm_android_commands ( |
Contributor
There was a problem hiding this comment.
i see that apple has nano_commands and nano_command_results. windows has windows_mdm_commands and windows_mdm_command_results. What was the reason behind merging it into one table for android? I assume it has to do with the lack of a response payload from the AMAPI?
Member
Author
There was a problem hiding this comment.
Yes, that's the reason. AMAPI's IssueCommand is per-device by API shape (enterprises/X/devices/Y/issueCommand), so each command is 1:1 with a device. There's no fan-out like Apple/Windows where one command row can have many result rows.
Member
Author
@ksykulev Apple and Windows have a similar "pending forever" failure mode if they (or Fleet) drops a response, so this is not a new failure mode for Fleet. That said, I filed a reliability issue since the Android fix is somewhat straightforward: #46145 |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/fleetctl/fleetctl/mdm_test.go (1)
1344-1389: ⚡ Quick winAdd an Android clear-passcode case to protect the new CLI path.
This matrix validates clear-passcode failures, but the enrollment/MDM-off assertion currently only covers a darwin host. Adding one Android host case here would better guard the new Android command flow from regressions.
Suggested test expansion
func TestMDMClearPasscodeCommand(t *testing.T) { @@ macNotEnrolled := testhost{ host: &fleet.Host{ID: 2, UUID: "mac-not-enrolled-cp", Platform: "darwin"}, } + androidNotEnrolled := testhost{ + host: &fleet.Host{ + ID: 3, UUID: "android-not-enrolled-cp", Platform: "android", + MDM: fleet.MDMHostData{Name: fleet.WellKnownMDMFleet, EnrollmentStatus: ptr.String("Off"), ConnectedToFleet: ptr.Bool(false)}, + }, + } @@ hostByUUID := map[string]testhost{ - macEnrolled.host.UUID: macEnrolled, - macNotEnrolled.host.UUID: macNotEnrolled, + macEnrolled.host.UUID: macEnrolled, + macNotEnrolled.host.UUID: macNotEnrolled, + androidNotEnrolled.host.UUID: androidNotEnrolled, } hostsByID := map[uint]testhost{ - macEnrolled.host.ID: macEnrolled, - macNotEnrolled.host.ID: macNotEnrolled, + macEnrolled.host.ID: macEnrolled, + macNotEnrolled.host.ID: macNotEnrolled, + androidNotEnrolled.host.ID: androidNotEnrolled, } @@ {appCfgAllMDM, "darwin not enrolled", []string{"--host", macNotEnrolled.host.UUID}, "Can't clear passcode for the host because it doesn't have MDM turned on."}, + {appCfgAllMDM, "android not enrolled", []string{"--host", androidNotEnrolled.host.UUID}, "Can't clear passcode for the host because it doesn't have MDM turned on."}, }🤖 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 `@cmd/fleetctl/fleetctl/mdm_test.go` around lines 1344 - 1389, Add a non‑enrolled Android testhost and a corresponding case in the cases slice: define androidNotEnrolled as a testhost with Platform "android" (no mdmInfo/enrollment), add it to hostByUUID and hostsByID maps so existing DS mocks (IsHostConnectedToFleetMDMFunc, GetHostLockWipeStatusFunc, GetHostDEPAssignmentFunc) will cover it, and append a test case using appCfgAllMDM with flags []{"--host", androidNotEnrolled.host.UUID} expecting the same error "Can't clear passcode for the host because it doesn't have MDM turned on." so the Android CLI path is exercised.
🤖 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 `@cmd/fleetctl/fleetctl/mdm_test.go`:
- Around line 1344-1389: Add a non‑enrolled Android testhost and a corresponding
case in the cases slice: define androidNotEnrolled as a testhost with Platform
"android" (no mdmInfo/enrollment), add it to hostByUUID and hostsByID maps so
existing DS mocks (IsHostConnectedToFleetMDMFunc, GetHostLockWipeStatusFunc,
GetHostDEPAssignmentFunc) will cover it, and append a test case using
appCfgAllMDM with flags []{"--host", androidNotEnrolled.host.UUID} expecting the
same error "Can't clear passcode for the host because it doesn't have MDM turned
on." so the Android CLI path is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6a89e727-a194-487f-950e-190a566efd69
📒 Files selected for processing (1)
cmd/fleetctl/fleetctl/mdm_test.go
Member
Author
|
@ksykulev ready to re-review |
ksykulev
approved these changes
May 26, 2026
This was referenced May 26, 2026
7 tasks
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
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.
Related issue: Resolves #41683
Support for Android lock, wipe, and clear passcode commands. Behavior is slightly different between BYOD and CODO. The fleetdm.com proxy isn't wired up, so they only work with direct Google connection.
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.Testing
Added/updated automated tests
Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)
QA'd all new/changed functionality manually
Database migrations
COLLATE utf8mb4_unicode_ci).Summary by CodeRabbit
New Features
Improvements
Documentation
Tests