Preserve android device team assignment - #46868
Conversation
![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.
ksykulev
changed the title
ccb6f9c to
9c816d3
Compare
|
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 adds persistent Possibly related PRs
🚥 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 |
There was a problem hiding this comment.
Pull request overview
This PR fixes a Fleet Android MDM re-enrollment regression by persisting an Android device’s last-known team assignment (and using it during subsequent enrollments) so that team-scoped certificate template delivery can resume automatically after host record churn.
Changes:
- Persist
team_idonandroid_devices(migration + schema snapshot) and sync it from the host record. - Restore the last-known Android team during enrollment/re-enrollment (prefer persisted team over enroll secret default).
- Preserve
android_devicesrows across host deletion and extend mocks to support the new datastore method.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| server/mock/datastore_mock.go | Adds a mock hook for GetAndroidDeviceLastTeamID. |
| server/mdm/android/service/pubsub.go | Uses last-known Android team during enrollment and adjusts cert-template team selection. |
| server/mdm/android/service/enterprises_test.go | Initializes the new mock method in common Android service mocks. |
| server/fleet/datastore.go | Extends AndroidDatastore interface with GetAndroidDeviceLastTeamID. |
| server/datastore/mysql/schema.sql | Adds android_devices.team_id + FK to teams and updates schema snapshot metadata. |
| server/datastore/mysql/migrations/tables/20260604221206_AddTeamIDToAndroidDevices.go | Migration adding/backfilling android_devices.team_id. |
| server/datastore/mysql/migrations/tables/20260604221206_AddTeamIDToAndroidDevices_test.go | Migration test verifying backfill + ON DELETE SET NULL. |
| server/datastore/mysql/hosts.go | Stops deleting android_devices rows when hosts are deleted. |
| server/datastore/mysql/android.go | Syncs android_devices.team_id and implements GetAndroidDeviceLastTeamID. |
| changes/45263-android-transfer-bug | User-visible changelog entry for the fix. |
💡 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 #46868 +/- ##
==========================================
+ Coverage 67.01% 67.02% +0.01%
==========================================
Files 2858 2855 -3
Lines 224622 224767 +145
Branches 11595 11581 -14
==========================================
+ Hits 150524 150653 +129
- Misses 60449 60451 +2
- Partials 13649 13663 +14
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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
server/datastore/mysql/android_test.go (1)
3217-3225: ⚡ Quick winAssert host2 remains assigned after clearing host1.
After clearing team for
h1, add an assertion thath2is still onteam.ID. This test currently misses regressions where UPDATE scope is broader than intended.🧪 Suggested assertion add
ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error { return sqlx.GetContext(ctx, q, &teamID1, `SELECT team_id FROM android_devices WHERE host_id = ?`, h1.Host.ID) }) require.Nil(t, teamID1) + + ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error { + return sqlx.GetContext(ctx, q, &teamID2, `SELECT team_id FROM android_devices WHERE host_id = ?`, h2.Host.ID) + }) + require.NotNil(t, teamID2) + require.Equal(t, team.ID, *teamID2)🤖 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/datastore/mysql/android_test.go` around lines 3217 - 3225, Add an assertion that h2 remains assigned to team after clearing h1: after the UpdateAndroidDeviceTeamID call and the existing check that h1's team_id is nil, query the android_devices row for h2 (use ExecAdhocSQL/ sqlx.GetContext like the existing query and read into a new variable e.g. teamID2) and require that teamID2 equals team.ID (use require.Equal or require.NotNil+require.Equal) so the test ensures UpdateAndroidDeviceTeamID(h1) didn't affect h2.server/service/hosts_test.go (1)
1973-1975: ⚡ Quick winAssert
UpdateAndroidDeviceTeamIDinvocation and arguments in these transfer tests.These new stubs only suppress unexpected-call failures; they don’t validate that Android team sync actually happens with the expected UUIDs and destination team. Please add assertions (invoked + expected
hostUUIDs/teamID) so regressions in transfer-side team persistence are caught.Suggested test hardening
ds.UpdateAndroidDeviceTeamIDFunc = func(ctx context.Context, hostUUIDs []string, teamID *uint) error { + require.Equal(t, []string{"android-uuid-1"}, hostUUIDs) + require.NotNil(t, teamID) + require.Equal(t, uint(5), *teamID) return nil }Based on learnings from the PR objectives/review context, this path is expected to preserve Android team assignment during transfer and should be explicitly verified in tests.
Also applies to: 2051-2053
🤖 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/service/hosts_test.go` around lines 1973 - 1975, The test currently stubs ds.UpdateAndroidDeviceTeamIDFunc to always return nil which hides whether Android device team syncing ran; change the stub (ds.UpdateAndroidDeviceTeamIDFunc) to record that it was invoked and capture its hostUUIDs and teamID parameters (e.g., store into local variables and a called bool), then after the transfer action assert the function was called and that the captured hostUUIDs match the expected UUID list and that the captured teamID pointer equals the expected destination team ID (including nil vs non-nil handling); apply the same pattern to the other stubs referenced around lines 2051-2053 so all transfer tests explicitly verify UpdateAndroidDeviceTeamID was invoked with correct args.
🤖 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 339-346: In UpdateAndroidDeviceTeamID, avoid passing empty UUIDs
into the sqlx.In IN clause by filtering hostUUIDs before building the query:
remove any empty or all-whitespace strings (e.g., trim and drop ""), then if the
filtered slice is empty return nil; otherwise use the filtered slice in the
sqlx.In call so the WHERE h.uuid IN (?) is precisely scoped to intended hosts
and cannot match hosts with an empty uuid.
---
Nitpick comments:
In `@server/datastore/mysql/android_test.go`:
- Around line 3217-3225: Add an assertion that h2 remains assigned to team after
clearing h1: after the UpdateAndroidDeviceTeamID call and the existing check
that h1's team_id is nil, query the android_devices row for h2 (use
ExecAdhocSQL/ sqlx.GetContext like the existing query and read into a new
variable e.g. teamID2) and require that teamID2 equals team.ID (use
require.Equal or require.NotNil+require.Equal) so the test ensures
UpdateAndroidDeviceTeamID(h1) didn't affect h2.
In `@server/service/hosts_test.go`:
- Around line 1973-1975: The test currently stubs
ds.UpdateAndroidDeviceTeamIDFunc to always return nil which hides whether
Android device team syncing ran; change the stub
(ds.UpdateAndroidDeviceTeamIDFunc) to record that it was invoked and capture its
hostUUIDs and teamID parameters (e.g., store into local variables and a called
bool), then after the transfer action assert the function was called and that
the captured hostUUIDs match the expected UUID list and that the captured teamID
pointer equals the expected destination team ID (including nil vs non-nil
handling); apply the same pattern to the other stubs referenced around lines
2051-2053 so all transfer tests explicitly verify UpdateAndroidDeviceTeamID was
invoked with correct args.
🪄 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: 8c458e29-0870-49f6-bc1e-529f943a91e7
📒 Files selected for processing (10)
server/datastore/mysql/android.goserver/datastore/mysql/android_test.goserver/datastore/mysql/migrations/tables/20260604221206_AddTeamIDToAndroidDevices_test.goserver/fleet/datastore.goserver/mdm/android/service/pubsub.goserver/mdm/android/service/pubsub_test.goserver/mdm/android/tests/testing_utils.goserver/mock/datastore_mock.goserver/service/hosts.goserver/service/hosts_test.go
✅ Files skipped from review due to trivial changes (1)
- server/mock/datastore_mock.go
🚧 Files skipped from review as they are similar to previous changes (2)
- server/fleet/datastore.go
- server/mdm/android/service/pubsub.go
getvictor
left a comment
getvictor
left a comment
There was a problem hiding this comment.
Made a few comments.
Note: I don't think other platforms re-enroll into the same team as before. But it makes sense for Android so we don't churn the agent state.
| // - android_devices: preserved so the device's last-known team_id survives host | ||
| // deletion and is available during re-enrollment. CreateDeviceTx finds the | ||
| // existing row by enterprise_specific_id and reuses it. |
There was a problem hiding this comment.
If the Android device is disconnected from MDM and then deleted, we should delete this row then, right?
There was a problem hiding this comment.
We don't delete the row on unenrollment. The device may re-enroll, and the whole point of this fix is preserving state across that cycle. As far as I can tell, Apple does exactly the same thing, their nano_devices, nano_enrollments, and host_dep_assignments rows all persist across host deletion. The host_dep_assignments table has a deleted_at soft-delete column, when a device is removed from ABM, the row gets a timestamp but isn't actually deleted.
There was a problem hiding this comment.
I meant when device fist unenrolls, and then we also delete it. I guess if the device re-enrolls with the same enterprise, it will still have the same id.
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/datastore/mysql/android.go (1)
358-363:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the primary for this last-team lookup.
This helper feeds the re-enrollment restore path in
server/mdm/android/service/pubsub.go:625-643, but it reads fromds.reader(ctx)while the corresponding team syncs write throughds.writer(ctx). Under replica lag, a device transferred or updated just before re-enrollment can restore a stale or nilteam_idand come back on the wrong team, which breaks the persistence guarantee this PR is adding.Suggested fix
func (ds *Datastore) GetAndroidDeviceLastTeamID(ctx context.Context, enterpriseSpecificID string) (*uint, bool, error) { var teamID *uint - err := sqlx.GetContext(ctx, ds.reader(ctx), &teamID, + err := sqlx.GetContext(ctx, ds.writer(ctx), &teamID, `SELECT team_id FROM android_devices WHERE enterprise_specific_id = ?`, enterpriseSpecificID, )🤖 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/datastore/mysql/android.go` around lines 358 - 363, GetAndroidDeviceLastTeamID currently reads from the read-replica via ds.reader(ctx) which can return stale team_id; change it to use the primary writer (ds.writer(ctx)) so the SELECT reads from the authoritative DB (adjust the sqlx.GetContext call to use ds.writer(ctx) in the GetAndroidDeviceLastTeamID function) to prevent replica-lag stale results when re-enrolling devices.
🤖 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/datastore/mysql/android.go`:
- Around line 358-363: GetAndroidDeviceLastTeamID currently reads from the
read-replica via ds.reader(ctx) which can return stale team_id; change it to use
the primary writer (ds.writer(ctx)) so the SELECT reads from the authoritative
DB (adjust the sqlx.GetContext call to use ds.writer(ctx) in the
GetAndroidDeviceLastTeamID function) to prevent replica-lag stale results when
re-enrolling devices.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 70c99736-c357-4755-9d4e-a82d6cd24ca7
📒 Files selected for processing (9)
server/datastore/mysql/android.goserver/datastore/mysql/android_test.goserver/datastore/mysql/hosts.goserver/fleet/datastore.goserver/mdm/android/service/pubsub.goserver/mdm/android/tests/testing_utils.goserver/mock/datastore_mock.goserver/service/hosts.goserver/service/hosts_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- server/mdm/android/tests/testing_utils.go
- server/fleet/datastore.go
- server/datastore/mysql/android_test.go
- server/mock/datastore_mock.go
- server/service/hosts.go
- server/mdm/android/service/pubsub.go
Related issue: Resolves #45263
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
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
Summary by CodeRabbit