Custom android mdm commands - #50728
Conversation
There was a problem hiding this comment.
Pull request overview
Adds first-pass support for issuing custom Android (AMAPI) device commands through Fleet’s existing “run MDM command” pathway, including persistence of the original Android JSON payload and the eventual AMAPI Operation result for retrieval.
Changes:
- Extend
RunMDMCommandto accept Android targets and enqueue AMAPI “custom commands” (currently limited to single-host targeting). - Persist Android command payload (
raw_command) on issue and store AMAPI Operation JSON (raw_result) on Pub/Sub ack/error. - Add datastore interfaces + migration for new
mdm_android_commandscolumns (raw_command,raw_result).
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/mdm.go | Adds Android as a supported platform for RunMDMCommand and enqueues AMAPI custom commands. |
| server/service/client_mdm.go | Allows fleetctl/client to submit Android JSON payloads without XML preparation. |
| server/mock/datastore_mock.go | Updates Android datastore mocks for new methods/signatures. |
| server/mdm/android/service/service.go | Implements IssueCustomCommand to issue AMAPI commands and persist a command row (with raw payload). |
| server/mdm/android/service/pubsub.go | Stores raw AMAPI Operation JSON into raw_result when updating command status from Pub/Sub. |
| server/mdm/android/service.go | Adds IssueCustomCommand to the Android MDM service interface. |
| server/mdm/android/android.go | Extends Android command model with raw_command and raw_result. |
| server/fleet/datastore.go | Extends AndroidDatastore interface to support inserting custom commands and storing raw results. |
| server/datastore/mysql/migrations/tables/20260805155427_AddRawCommandAndResultToAndroidCommands.go | Migration adding raw_command and raw_result columns to mdm_android_commands. |
| server/datastore/mysql/migrations/tables/20260805155427_AddRawCommandAndResultToAndroidCommands_test.go | Migration test verifying new columns default NULL for existing rows and are insertable. |
| server/datastore/mysql/mdm.go | Treats Android hosts as “connected to Fleet MDM” based on host_mdm.enrolled. |
| server/datastore/mysql/android.go | Adds insert helper for custom commands and stores raw_result when updating status. |
| server/datastore/mysql/schema.sql.backup | Adds a full schema dump/backup file (appears accidental). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #50728 +/- ##
========================================
Coverage 68.77% 68.77%
========================================
Files 4001 4002 +1
Lines 258512 258630 +118
Branches 13860 13860
========================================
+ Hits 177792 177874 +82
- Misses 64922 64955 +33
- Partials 15798 15801 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
server/mdm/android/service/reconcile_commands.go:161
- The reconciler path transitions commands to a terminal state without persisting
raw_resulteven though it has the fullOperationobject available. If Pub/Sub is missed and the cron reconciler resolves the command,raw_resultwill stay NULL, which breaks the goal of retrieving raw custom command results via the API. Consider marshallingopto JSON (same as the Pub/Sub handler) and passing it asrawResulttosetAndroidCommandTerminalState.
default:
status, errCode, errMsg := androidOperationTerminalState(op)
if err := setAndroidCommandTerminalState(ctx, ds, newActivityFn, cmd, status, errCode, errMsg, nil, nil); err != nil {
logger.ErrorContext(ctx, "failed to apply reconciled android command status",
e35a22a to
8e91689
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.
Suppressed comments (3)
server/service/mdm.go:666
- Premium-gated Android commands currently return fleet.ErrMissingLicense without a cause, which surfaces the generic message "Requires Fleet Premium license". The linked issue explicitly calls for an error message that identifies the gated command/feature (e.g. LOCK / RESET_PASSWORD) so users understand what is premium-only.
if androidMDMPremiumCommands[cmdType] {
lic, err := svc.License(ctx)
if err != nil {
return nil, ctxerr.Wrap(ctx, err, "get license")
}
if !lic.IsPremium() {
return nil, fleet.ErrMissingLicense
}
server/service/mdm.go:640
- Android custom commands reject requests targeting more than one host (len(hosts) != 1). This conflicts with the intended API/CLI behavior described in the linked issue (examples show multiple hosts and engineering notes suggest serial per-host issuance). If multi-host is intentionally unsupported for now, the public API/docs/CLI should be updated to reflect that constraint; otherwise, this needs a batching strategy (serial loop + aggregated response).
// enqueueAndroidMDMCommand issues an AMAPI custom command for each targeted Android host.
// rawJSON is the base64-decoded JSON bytes of the AMAPI Command object.
// For now, only single-host targeting is supported.
func (svc *Service) enqueueAndroidMDMCommand(ctx context.Context, rawJSON []byte, hosts []*fleet.Host) (*fleet.CommandEnqueueResult, error) {
if len(hosts) != 1 {
return nil, fleet.NewInvalidArgumentError("host_uuids",
"Android custom commands can only target a single host at a time.").WithStatus(http.StatusBadRequest)
}
server/mdm/android/service/service.go:1137
- When the AMAPI command type is omitted (relying on parameter inference like clearAppsDataParams → CLEAR_APP_DATA), IssueCustomCommand persists and returns RequestType as "CUSTOM". This prevents clients from seeing the real inferred type and doesn’t meet the linked issue’s acceptance criteria (expects inferred type in results/response). Consider extracting the inferred type from Operation metadata or implementing a deterministic inference from the request payload so the stored/returned command_type matches what was actually executed.
// Determine the command type from the AMAPI response metadata or the request.
cmdType := amapiCmd.Type
if cmdType == "" {
// AMAPI infers the type from params fields (e.g. clearAppsDataParams → CLEAR_APP_DATA).
// The type is reflected back in the Operation metadata but not trivially accessible here,
// so fall back to "CUSTOM" for now.
cmdType = "CUSTOM"
}
|
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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe change adds Android command execution to Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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}
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 `@server/datastore/mysql/android.go`:
- Around line 1234-1246: Update the SELECT list used by getMDMAndroidCommand to
include both raw_command and raw_result, ensuring the returned command preserves
payloads written by InsertMDMAndroidCommand and UpdateMDMAndroidCommandStatus.
Add a round-trip test covering both fields after insertion and status update.
In `@server/mdm/android/service/reconcile_commands.go`:
- Line 160: In the completed-operation branch of the reconciliation flow,
marshal the AMAPI operation value op and pass the resulting raw payload instead
of nil to setAndroidCommandTerminalState. Update TestReconcileAndroidCommands to
assert that the completed-operation callback receives this raw result while
preserving the existing terminal status handling.
🪄 Autofix
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 Plus
Run ID: 87ef3f27-31d8-418f-b23f-505e581c7023
⛔ Files ignored due to path filters (1)
changes/android-custom-commands.mdis excluded by!**/*.md
📒 Files selected for processing (21)
server/datastore/mysql/android.goserver/datastore/mysql/android_test.goserver/datastore/mysql/hosts_test.goserver/datastore/mysql/mdm.goserver/datastore/mysql/mdm_test.goserver/datastore/mysql/migrations/tables/20260812165318_AddRawCommandAndResultToAndroidCommands.goserver/datastore/mysql/migrations/tables/20260812165318_AddRawCommandAndResultToAndroidCommands_test.goserver/datastore/mysql/schema.sqlserver/fleet/datastore.goserver/mdm/android/android.goserver/mdm/android/service.goserver/mdm/android/service/pubsub.goserver/mdm/android/service/pubsub_dedup_test.goserver/mdm/android/service/pubsub_test.goserver/mdm/android/service/reconcile_commands.goserver/mdm/android/service/reconcile_commands_test.goserver/mdm/android/service/service.goserver/mock/datastore_mock.goserver/service/client_mdm.goserver/service/mdm.goserver/service/mdm_test.go
| // enqueueAndroidMDMCommand issues an AMAPI custom command for each targeted Android host. | ||
| // rawJSON is the base64-decoded JSON bytes of the AMAPI Command object. | ||
| // For now, only single-host targeting is supported. | ||
| func (svc *Service) enqueueAndroidMDMCommand(ctx context.Context, rawJSON []byte, hosts []*fleet.Host) (*fleet.CommandEnqueueResult, error) { |
There was a problem hiding this comment.
/commands/results 404s for Android custom commands — GetMDMCommandPlatform (datastore/mysql/mdm.go:37) never checks mdm_android_commands, and the case "android" branch in getMDMCommandResults (mdm.go:992) returns empty even once it does, so the new raw_command/raw_result columns are write-only.
There was a problem hiding this comment.
GetMDMCommandResults for Android is part of #50733. The columns are write-only for now until that ticket wires up the read path.
dantecatalfamo
left a comment
dantecatalfamo
left a comment
There was a problem hiding this comment.
🥬 (L)ettuce
🧄 (G)arlic
🍅 (T)omato
🥩 (M)eat
Related issue: Resolves #50447, #23232
Checklist for submitter
If some of the following don't apply, delete the relevant line.
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Input data is properly validated,
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.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