DeviceLocation command handling and DB support #37668

jahzielv · 2025-12-24T02:32:22Z

Related issue: Resolves #37265

Checklist for submitter

If some of the following don't apply, delete the relevant line.

Input data is properly validated, SELECT * is avoided, SQL injection is prevented (using placeholders for values in statements)

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

jahzielv · 2025-12-24T02:35:10Z

Note: integration testing will come after implementing the next backend subtask for this story.

jahzielv · 2025-12-24T02:36:27Z

server/service/apple_mdm.go

+	if ok {
+		for _, f := range handlers {
+			if err := f(ctx, result); err != nil {
+				// TODO: should we run as many as we can? if so we have to collect into a multierror


I can address this in a subsequent PR, since this is the existing behavior

Guessing we'll need this for automatic VPP updates anyway? /cc @lucasmrod

Gonna merge on top of this and @lucasmrod can stack on top.

Guessing we'll need this for automatic VPP updates anyway?

Do we? Will there be any commands that will be processed by more than one handler?

Depends on where you're hooking in on MDM-driven software inventory, I suppose.

Guessing you already built that out though so maybe we're good to go?

Do we? Will there be any commands that will be processed by more than one handler?

I think yes, eventually. The goal with the handlers was to make a pattern for handling MDM command results for independent operations. That way we don't end up with 1 mega-function per MDM command result type that tries to contain all possible Fleet logic for that type.

server/fleet/apple_mdm.go

server/datastore/mysql/migrations/tables/20251222174712_AddHostLocationSupport.go

server/datastore/mysql/migrations/tables/20251222174712_AddHostLocationSupport_test.go

server/datastore/mysql/apple_mdm.go

iansltx · 2025-12-24T02:46:49Z

server/datastore/mysql/apple_mdm_test.go

+	ctx := context.Background()
+	iOSHost := newTestHostWithPlatform(t, ds, "iphone_"+t.Name(), string(fleet.IOSPlatform), nil)
+
+	err := ds.InsertHostLocationData(ctx, iOSHost.ID, 42.42, -42.42)


Thoughts on rolling this up into the HostLocation struct, including adding host ID in? At which point you can just check equality on the round trip (which for this low of precision should work despite dealing with a float to decimal conversion).

Also, since we aren't doing math, maybe specifying the struct fields for lat/long as strings is a better idea here, so we don't lose anything going from string-formatted in the plist to floats and then back to fixed-point in the database.

Thoughts on rolling this up into the HostLocation struct, including adding host ID in? At which point you can just check equality on the round trip (which for this low of precision should work despite dealing with a float to decimal conversion).

sgtm 👍

Also, since we aren't doing math, maybe specifying the struct fields for lat/long as strings is a better idea here, so we don't lose anything going from string-formatted in the plist to floats and then back to fixed-point in the database.

I have a slight preference for leaving it as floats, but not huge. Seems like floats might be something we need in future.

Gonna continue pushing on the floats thing because we can maintain precision from the Apple result of we don't do that.

If we need to do math on these later, it'll probably be in the database anyway, which is (correctly) defined as fixed-point.

Hrm, seems like I can't unmarshal the plist fields into a string.

I get

err="build device location command result: device location command result: xml unmarshal: plist: cannot unmarshal $VERY_LONG_DECIMAL_HERE into Go value of type string"

Does it make sense to unmarshal into float64, then turn those into strings?

Nope, doesn't make sense after we hit floating point. Can leave this as is then.

iansltx · 2025-12-24T02:50:48Z

server/service/apple_mdm.go

+	if ok {
+		for _, f := range handlers {
+			if err := f(ctx, result); err != nil {
+				// TODO: should we run as many as we can? if so we have to collect into a multierror


Guessing we'll need this for automatic VPP updates anyway? /cc @lucasmrod

server/service/apple_mdm_cmd_results.go

codecov · 2025-12-24T03:00:21Z

Codecov Report

❌ Patch coverage is 67.46032% with 41 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.91%. Comparing base (9adfe73) to head (7dcb38d).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
server/service/apple_mdm.go	51.11%	15 Missing and 7 partials ⚠️
server/service/apple_mdm_cmd_results.go	40.90%	12 Missing and 1 partial ⚠️
...ns/tables/20251222174712_AddHostLocationSupport.go	76.47%	3 Missing and 1 partial ⚠️
cmd/fleet/serve.go	0.00%	1 Missing ⚠️
server/datastore/mysql/apple_mdm.go	96.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #37668      +/-   ##
==========================================
+ Coverage   65.88%   65.91%   +0.02%     
==========================================
  Files        2360     2362       +2     
  Lines      187285   187295      +10     
  Branches     8006     7989      -17     
==========================================
+ Hits       123402   123456      +54     
+ Misses      52605    52550      -55     
- Partials    11278    11289      +11

Flag	Coverage Δ
backend	`67.73% <67.46%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

jahzielv · 2025-12-24T05:10:48Z

Looks like the failing test is unrelated FYI

iansltx

Sorry, found a few more things. Setting this to draft in the mean time.

iansltx · 2025-12-24T12:28:07Z

server/service/apple_mdm.go

+		host, err := svc.ds.HostByIdentifier(r.Context, cmdResult.Identifier())
+		if err != nil {
+			return nil, ctxerr.Wrap(r.Context, err, "DisableLostMode: get host by identifier")
+		}
+
+		if err := svc.ds.DeleteHostLocationData(r.Context, host.ID); err != nil {
+			return nil, ctxerr.Wrap(r.Context, err, "DisableLostMode: delete host location data")
+		}


Thinking we want to put this, plus the device location command on lost mode entry, inside the status gate rather than before it? Otherwise we could be clearing location on a pending lost mode request right?

iansltx · 2025-12-24T12:28:39Z

server/service/apple_mdm.go

 		}
+
+	case fleet.DeviceLocationCmdName:
+


Do we need to check for command status here?

@iansltx I added a check for command success status (Acknowledged). Any error handling is a product decision (e.g. should we retry?) and can be added in followup PRs relatively easily.

@jahzielv In case of an error, we have this state in the Location modal:

We will soon surface MDM commands in the activity, allowing users to track if a location command has failed.

iansltx · 2025-12-24T12:30:17Z

server/service/apple_mdm_cmd_results.go

+		if err != nil {
+			return ctxerr.Wrap(ctx, err, "device location command result: insert host location data")
+		}
+
+		return nil


We can just return the wrap here, right?

jahzielv added 3 commits December 23, 2025 13:01

wip

6c09d24

add handlers for devicelocation, test

e886d56

merge main

fb613dd

jahzielv temporarily deployed to Docker Hub December 24, 2025 02:33 — with GitHub Actions Inactive

jahzielv assigned iansltx Dec 24, 2025

jahzielv commented Dec 24, 2025

View reviewed changes

server/fleet/apple_mdm.go Show resolved Hide resolved

generate mocks

39b883e

jahzielv temporarily deployed to Docker Hub December 24, 2025 02:42 — with GitHub Actions Inactive

iansltx reviewed Dec 24, 2025

View reviewed changes

address feedback

7e91c32

jahzielv had a problem deploying to Docker Hub December 24, 2025 04:10 — with GitHub Actions Error

address feedback

e916a58

jahzielv temporarily deployed to Docker Hub December 24, 2025 04:15 — with GitHub Actions Inactive

fix test

1570610

jahzielv temporarily deployed to Docker Hub December 24, 2025 04:26 — with GitHub Actions Inactive

fix tests

db09e96

jahzielv had a problem deploying to Docker Hub December 24, 2025 04:50 — with GitHub Actions Error

revert string change

8f078be

jahzielv temporarily deployed to Docker Hub December 24, 2025 04:55 — with GitHub Actions Inactive

jahzielv changed the title ~~37265 be database and devicelocation mdm command~~ DeviceLocation command handling and DB support Dec 24, 2025

jahzielv marked this pull request as ready for review December 24, 2025 05:10

jahzielv requested a review from a team as a code owner December 24, 2025 05:10

iansltx reviewed Dec 24, 2025

View reviewed changes

iansltx marked this pull request as draft December 24, 2025 12:31

address feedback

7dcb38d

jahzielv temporarily deployed to Docker Hub December 24, 2025 17:36 — with GitHub Actions Inactive

DeviceLocation command handling and DB support #37668

Are you sure you want to change the base?

DeviceLocation command handling and DB support #37668

Conversation

jahzielv commented Dec 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist for submitter

Testing

Uh oh!

jahzielv commented Dec 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jahzielv Dec 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Dec 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jahzielv commented Dec 24, 2025

Uh oh!

iansltx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jahzielv commented Dec 24, 2025 •

edited

Loading

jahzielv Dec 24, 2025 •

edited

Loading

codecov bot commented Dec 24, 2025 •

edited

Loading