Fix to populate host_emails during Linux/Windows SSO enrollment - #45951
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.
There was a problem hiding this comment.
Pull request overview
This PR fixes missing device_mapping (specifically mdm_idp_accounts/user email) for newly enrolled Linux/Windows hosts that go through End User Authentication (EUA) during Orbit enrollment, aligning List hosts?device_mapping=true behavior with Get host for the same IdP-linked hosts (issue #45066).
Changes:
- Defers Windows MSI EUA token processing to return the IdP account UUID, and performs IdP association + host email reconciliation after the
hostsrow exists. - Adds a post-enrollment reconciliation step in
EnrollOrbitto populatehost_emailsfromhost_mdm_idp_accountsfor Linux/Windows EUA enrollments. - Adds enterprise integration tests to cover both the browser-based Orbit Setup Experience SSO callback flow and the Windows MSI EUA-token flow.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/orbit.go | Defers Windows token association and adds post-enrollment reconciliation to ensure host_emails is populated for device mapping. |
| server/service/orbit_eua_test.go | Updates unit tests to match the new processWindowsEUAToken contract (returns IdP UUID; no association side-effect). |
| server/service/integration_enterprise_test.go | Adds end-to-end tests proving device_mapping is populated for Linux/Windows EUA enrollment flows, including Windows EUA token. |
| server/datastore/mysql/mdm_idp_accounts_test.go | Extends reconciliation coverage to include Windows/Linux platforms. |
| changes/45066-windows-idp-device-mapping | Adds user-visible changelog entry for the fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR ensures device_mapping includes IdP email for Windows and Linux EUA enrollments. processWindowsEUAToken now returns an IdP account UUID (UPN, device ID, idpAcctUUID) but defers associating it to a host. EnrollOrbit captures that UUID, persists the host (reads back hosts.uuid), and runs a post-enrollment reconciliation on Linux/Windows to associate the host with mdm_idp_accounts. Integration tests exercise both browser EUA and EUA-token flows; unit tests expand reconciliation coverage and a changelog entry was added. 🚥 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
server/service/integration_enterprise_test.go (1)
30761-30766: ⚡ Quick winAssert the hosts list API response here too.
The regression is about the hosts list endpoint returning
device_mapping=null, but these checks only validates.ds.ListHostDeviceMapping. An API-layer regression in handler wiring or response serialization would still pass. Please add an assertion against the hosts list endpoint withdevice_mapping=truein both flows.Also applies to: 30896-30900
🤖 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/integration_enterprise_test.go` around lines 30761 - 30766, The test currently only checks the datastore via s.ds.ListHostDeviceMapping but must also assert the hosts list API surface returns device_mapping=true; after the existing datastore assertions (the s.ds.ListHostDeviceMapping check that confirms dms[0].Email == idpEmail and dms[0].Source == fleet.DeviceMappingMDMIdpAccounts), add an assertion that calls the hosts list handler/API used elsewhere in tests (the same endpoint exercised in the integration test) and verify the returned host object(s) include device_mapping: true (do this in both places referenced by the reviewer, i.e., the block around the s.ds.ListHostDeviceMapping assertion and the other similar block at the later location).
🤖 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 `@changes/45066-windows-idp-device-mapping`:
- Line 1: Update the changelog entry to explicitly mention that the fix
populates the device_mapping (specifically the mdm_idp_accounts field) for
Windows and Linux hosts enrolled with end-user authentication so that the User
email is correctly returned by GET /api/v1/fleet/hosts; reference
"device_mapping" and "mdm_idp_accounts" in the description to improve clarity
and align with the linked issue.
In `@server/service/orbit.go`:
- Around line 296-304: The lookup for the host's IdP account
(svc.ds.GetMDMIdPAccountByHostUUID) must be forced to read from the primary DB
when idpAcctUUID == "" to avoid replica lag causing missing rows; change the
call to use a context that enforces primary/writer reads (e.g., create a
primaryCtx and pass that into svc.ds.GetMDMIdPAccountByHostUUID) so that the
recently-written SSO callback row is visible and AssociateHostMDMIdPAccount is
not skipped.
---
Nitpick comments:
In `@server/service/integration_enterprise_test.go`:
- Around line 30761-30766: The test currently only checks the datastore via
s.ds.ListHostDeviceMapping but must also assert the hosts list API surface
returns device_mapping=true; after the existing datastore assertions (the
s.ds.ListHostDeviceMapping check that confirms dms[0].Email == idpEmail and
dms[0].Source == fleet.DeviceMappingMDMIdpAccounts), add an assertion that calls
the hosts list handler/API used elsewhere in tests (the same endpoint exercised
in the integration test) and verify the returned host object(s) include
device_mapping: true (do this in both places referenced by the reviewer, i.e.,
the block around the s.ds.ListHostDeviceMapping assertion and the other similar
block at the later location).
🪄 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: c34f1181-dc7f-4d3c-93a7-c623c49cb7ea
📒 Files selected for processing (5)
changes/45066-windows-idp-device-mappingserver/datastore/mysql/mdm_idp_accounts_test.goserver/service/integration_enterprise_test.goserver/service/orbit.goserver/service/orbit_eua_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #45951 +/- ##
==========================================
+ Coverage 66.78% 66.80% +0.02%
==========================================
Files 2751 2754 +3
Lines 219892 220115 +223
Branches 11008 11008
==========================================
+ Hits 146848 147050 +202
- Misses 59771 59772 +1
- Partials 13273 13293 +20
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:
|
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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 `@server/datastore/mysql/hosts.go`:
- Around line 2339-2349: The returned host struct is being forced to
hostInfo.HardwareUUID (host.UUID = hostInfo.HardwareUUID) even when the UPDATE
path preserves a different non-empty database UUID via COALESCE(NULLIF(uuid,
''), ?), leading to a divergence when matching was done by osquery_host_id;
change the logic in the enrollment/update flow (around the
matchHostDuringEnrollment handling and the code that sets host.UUID) to read and
return the effective UUID that was actually persisted: after performing the
INSERT/UPDATE, query or capture the resulting hosts.uuid value (or use the DB
RETURNING/SELECT of COALESCE(NULLIF(uuid, ''), ?)) and set host.UUID to that
persisted value instead of unconditionally using hostInfo.HardwareUUID so the
returned host.UUID matches the database.
🪄 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: e7f12af1-9384-4a25-bac7-e8ac780e88b2
📒 Files selected for processing (3)
changes/45066-windows-idp-device-mappingserver/datastore/mysql/hosts.goserver/service/orbit.go
✅ Files skipped from review due to trivial changes (1)
- changes/45066-windows-idp-device-mapping
nulmete
left a comment
nulmete
left a comment
There was a problem hiding this comment.
LGTM, holding off approval until coderabbit's comment is resolved.
Resolves #45066.
changes/,orbit/changes/oree/fleetd-chrome/changes.Testing
Summary by CodeRabbit
Bug Fixes
Tests