gkarr 44008 setup experience - #45202
Conversation
8b6dfd5 to
1b3b3e3
Compare
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 expands Fleet’s Apple MDM software install and setup experience flows to support Account-Driven User Enrollment (BYOD) iOS/iPadOS devices by introducing user-scoped VPP licensing (via Managed Apple IDs and VPP “client users”), improving install command behavior for user enrollments, and updating the UI to enable self-service/software installs for BYOD iOS/iPadOS hosts.
Changes:
- Persist Managed Apple ID for user-enrolled iOS/iPadOS hosts and route setup-experience enqueueing through the BYOD enrollment path.
- Add VPP “client user” persistence + Apple VPP
/users/createsupport and switch VPP asset association toclientUserIdsfor BYOD installs. - Update install handling to be more resilient (e.g., treat “already installed” as success) and enable BYOD iOS/iPadOS software installs in UI/self-service.
Reviewed changes
Copilot reviewed 32 out of 33 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/apple_mdm.go | Extends TokenUpdate/setup-experience logic for user enrollment; treats “already installed” InstallApplication errors as success; allows scheduled updates for BYOD iOS/iPadOS. |
| server/service/apple_mdm_test.go | Adds coverage for Managed Apple ID persistence, user-enrollment setup experience enqueueing, and “already installed” InstallApplication handling. |
| server/mock/datastore_mock.go | Adds datastore mock methods for Managed Apple ID and VPP client user persistence. |
| server/mdm/nanomdm/push/nanopush/provider.go | Simplifies worker count calculation using min. |
| server/mdm/apple/vpp/api.go | Adds VPP helpers: max-devices error detection, associate-assets validation + user-scoped request support, and create-users endpoint support. |
| server/mdm/apple/vpp/api_test.go | Adds tests for associate-assets validation, clientUserIds payloads, create-users, and max-devices error matching. |
| server/mdm/apple/util.go | Adds helper to detect “app already installed” InstallApplication error chains. |
| server/mdm/apple/util_test.go | Adds tests for “app already installed” error-chain detection. |
| server/mdm/apple/install_application.go | Adds IsUserEnrollment flag to omit ChangeManagementState for user-enrolled install commands. |
| server/mdm/apple/install_application_test.go | Tests that ChangeManagementState is omitted for user enrollment and plist remains valid. |
| server/fleet/vpp.go | Introduces VPPClientUser model and status enum. |
| server/fleet/hosts.go | Adds ManagedAppleID field to HostMDM struct. |
| server/fleet/datastore.go | Extends Datastore interface with Managed Apple ID + VPP client user methods. |
| server/datastore/mysql/vpp.go | Implements VPP client user CRUD; updates VPP install enqueue to pass user-enrollment flag into InstallApplication command build. |
| server/datastore/mysql/vpp_test.go | Adds integration tests for vpp_client_users CRUD/upsert/list + FK cascade behavior. |
| server/datastore/mysql/schema.sql | Adds host_mdm.managed_apple_id column and vpp_client_users table. |
| server/datastore/mysql/migrations/tables/20260512173250_AddManagedAppleIDToHostMDM.go | Migration adding managed_apple_id to host_mdm. |
| server/datastore/mysql/migrations/tables/20260512173250_AddManagedAppleIDToHostMDM_test.go | Migration test for managed_apple_id semantics (NULL default, set/update). |
| server/datastore/mysql/migrations/tables/20260512173249_CreateTableVPPClientUsers.go | Migration creating vpp_client_users table and constraints. |
| server/datastore/mysql/migrations/tables/20260512173249_CreateTableVPPClientUsers_test.go | Migration test for constraints/defaults/FK cascade. |
| server/datastore/mysql/hosts.go | Extends GetHostMDM query to read managed_apple_id; adds Get/SetHostManagedAppleID methods. |
| server/datastore/mysql/hosts_test.go | Adds tests for Managed Apple ID getters/setters and behavior before enrollment. |
| server/datastore/mysql/activities.go | Ensures in-house InstallApplication command generation omits ChangeManagementState for personal enrollments. |
| frontend/pages/hosts/details/HostDetailsPage/HostDetailsPage.tsx | Removes BYOD iOS/iPadOS “software install unsupported” gating; keeps Android gate only. |
| frontend/pages/hosts/details/cards/Software/SelfService/SelfService.tsx | Removes BYOD mobile-view self-service “unsupported” message gate. |
| frontend/pages/hosts/details/cards/Software/SelfService/SelfService.tests.tsx | Updates test to expect BYOD hosts see self-service list (not an unsupported message). |
| frontend/pages/hosts/details/cards/Software/SelfService/_styles.scss | Removes styles for the deleted BYOD unsupported self-service empty state. |
| ee/server/service/vpp_users.go | Adds ensureVPPClientUser to provision/cache VPP client users for user-scoped licensing. |
| ee/server/service/vpp_users_test.go | Adds tests for ensureVPPClientUser behavior (new, cached, retry reuse, partial failure, missing MAID). |
| ee/server/service/software_installers.go | Removes BYOD install block; routes BYOD VPP installs via client user provisioning + clientUserIds association + improved error surfacing. |
| ee/server/service/software_installers_test.go | Updates tests to assert personally-enrolled iOS/iPadOS are no longer blocked at entry points. |
| ee/server/service/install_vpp_associate_test.go | Adds focused tests ensuring associate-assets and assignments routing uses clientUserIds for BYOD and serialNumbers otherwise; tests max-devices messaging. |
| changes/31138-user-enrolled-software-install | Adds release notes describing BYOD iOS/iPadOS install + self-service + setup experience behavior changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
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 enables iOS/iPadOS installs on Account‑Driven User Enrollment hosts by persisting Managed Apple IDs, adding a vpp_client_users table and types, implementing Service.ensureVPPClientUser (CreateUsers integration, pending/registered caching), validating AssociateAssets payloads (clientUserIds vs serialNumbers), removing BYOD gating from install flows, routing installs based on enrollment type, updating MDM token handling to store managed Apple IDs and enqueue setup-experience, and adjusting InstallApplication command generation and related tests. Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ee/server/service/software_installers_test.go (1)
375-380:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSet the host fixture to personal enrollment in this test.
Line 375 builds an iOS host, but it doesn’t set
host.MDM.EnrollmentStatusto personal. This can pass without covering the BYOD regression path the test name describes.Suggested patch
host := &fleet.Host{ UUID: "personal-ios", OrbitNodeKey: ptr.String("orbit_key"), Platform: "ios", TeamID: ptr.Uint(1), + MDM: fleet.MDMHostData{ + EnrollmentStatus: ptr.String(string(fleet.MDMEnrollStatusPersonal)), + }, }🤖 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 `@ee/server/service/software_installers_test.go` around lines 375 - 380, The test builds an iOS host but never sets the host to a personal enrollment, so update the host fixture (the host variable of type fleet.Host) to set its MDM.EnrollmentStatus to the personal enrollment constant (e.g., fleet.EnrollmentStatusPersonal or the project’s equivalent) so the test exercises the BYOD/personal enrollment code path; locate the host creation block and add the EnrollmentStatus assignment on host.MDM.
🤖 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/vpp_users.go`:
- Around line 90-99: The call to svc.ds.InsertVPPClientUser currently discards
its error when recording a pending VPPClientUser after u.HasError(); update this
to check the return value, and if non-nil wrap/return that error (similar to the
pattern used around InsertVPPClientUser earlier at lines 67-76) instead of
ignoring it—locate the block that creates &fleet.VPPClientUser with VPPTokenID,
ManagedAppleID, ClientUserID and Status Pending, call
svc.ds.InsertVPPClientUser(ctx, ...), capture err, and return a wrapped ctxerr
(or the same error handling helper used elsewhere) so failures persist back to
the caller.
- Around line 67-76: The InsertVPPClientUser error is being ignored when
persisting a pending VPPClientUser after CreateUsers fails; update the error
handling in the block around svc.ds.InsertVPPClientUser so the
InsertVPPClientUser call's error is not discarded—either log it via the existing
logger or return a wrapped error that includes both the original CreateUsers
error and the InsertVPPClientUser error (reference symbols:
svc.ds.InsertVPPClientUser, fleet.VPPClientUser, VPPClientUserStatusPending,
token.ID, clientUserID); ensure the final return uses ctxerr.Wrap (or similar)
to surface both failures instead of silently discarding the insert error.
In `@server/datastore/mysql/hosts.go`:
- Around line 4909-4912: The GetHostManagedAppleID function currently queries
the write-sensitive managed_apple_id using ds.reader(ctx), which can hit a
replica and suffer lag; change the DB handle to ds.writer(ctx) so the SELECT in
GetHostManagedAppleID uses the primary. Update the sqlx.GetContext call in
GetHostManagedAppleID to use ds.writer(ctx) instead of ds.reader(ctx) so the
managed_apple_id lookup reads from the primary to avoid replica-lag misses.
- Around line 4922-4929: The SetHostManagedAppleID function currently treats
zero-row updates as success; modify it to check the Exec result's RowsAffected()
and if it returns 0, return a not-found error (e.g., ctxerr.Wrapf(ctx,
sql.ErrNoRows or a custom not-found error) indicating no host_mdm row for the
given host_id). Concretely, capture the result from ds.writer(ctx).ExecContext,
call RowsAffected(), and when it's 0 return a clear not-found error mentioning
hostID; otherwise proceed to return nil.
In `@server/datastore/mysql/vpp.go`:
- Around line 1858-1860: The not-found error returned by
notFound("VPPClientUser") currently includes the managedAppleID
(managed_apple_id) which is PII; change the WithMessage call in that return so
it does not include managedAppleID (e.g. use fmt.Sprintf("no VPP client user for
vpp_token_id=%d", tokenID)) and remove any reference to the managedAppleID
variable in this error construction (locate the return that wraps
notFound("VPPClientUser") and adjust the fmt.Sprintf/WithMessage accordingly).
In `@server/fleet/vpp.go`:
- Around line 213-226: Add an IsValid() method on the VPPClientUserStatus type
that mirrors the pattern used by HostStatus.IsValid() and others: implement func
(s VPPClientUserStatus) IsValid() bool that returns true only when s equals one
of the defined constants VPPClientUserStatusPending,
VPPClientUserStatusRegistered, or VPPClientUserStatusRetired; place the method
alongside the type/consts so callers can validate VPPClientUserStatus values
consistently.
In `@server/mdm/apple/vpp/api.go`:
- Around line 192-195: In AssociateAssets ensure you nil-check the params
pointer before calling params.Validate to avoid a panic; update AssociateAssets
to return a clear error when params is nil (e.g., "nil params" or validation
error) and only call params.Validate() if params != nil, keeping the rest of the
function logic unchanged and referencing AssociateAssets and params.Validate to
locate where to add the guard.
In `@server/service/apple_mdm.go`:
- Around line 3928-3932: Currently the code only writes a non-empty
managedAppleID for mdm.UserEnrollmentDevice, leaving old values in
host_mdm.managed_apple_id when resolution fails; change the logic in the r.Type
== mdm.UserEnrollmentDevice branch to always call
svc.ds.SetHostManagedAppleID(r.Context, info.HostID, managedAppleID) (passing
the empty string when resolution failed) so stale IDs are cleared, and
propagate/wrap any error from SetHostManagedAppleID as before.
---
Outside diff comments:
In `@ee/server/service/software_installers_test.go`:
- Around line 375-380: The test builds an iOS host but never sets the host to a
personal enrollment, so update the host fixture (the host variable of type
fleet.Host) to set its MDM.EnrollmentStatus to the personal enrollment constant
(e.g., fleet.EnrollmentStatusPersonal or the project’s equivalent) so the test
exercises the BYOD/personal enrollment code path; locate the host creation block
and add the EnrollmentStatus assignment on host.MDM.
🪄 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: 4e6693e8-e4b8-4a87-b92f-3b5be9b273ff
📒 Files selected for processing (33)
changes/31138-user-enrolled-software-installee/server/service/install_vpp_associate_test.goee/server/service/software_installers.goee/server/service/software_installers_test.goee/server/service/vpp_users.goee/server/service/vpp_users_test.gofrontend/pages/hosts/details/HostDetailsPage/HostDetailsPage.tsxfrontend/pages/hosts/details/cards/Software/SelfService/SelfService.tests.tsxfrontend/pages/hosts/details/cards/Software/SelfService/SelfService.tsxfrontend/pages/hosts/details/cards/Software/SelfService/_styles.scssserver/datastore/mysql/activities.goserver/datastore/mysql/hosts.goserver/datastore/mysql/hosts_test.goserver/datastore/mysql/migrations/tables/20260512173249_CreateTableVPPClientUsers.goserver/datastore/mysql/migrations/tables/20260512173249_CreateTableVPPClientUsers_test.goserver/datastore/mysql/migrations/tables/20260512173250_AddManagedAppleIDToHostMDM.goserver/datastore/mysql/migrations/tables/20260512173250_AddManagedAppleIDToHostMDM_test.goserver/datastore/mysql/schema.sqlserver/datastore/mysql/vpp.goserver/datastore/mysql/vpp_test.goserver/fleet/datastore.goserver/fleet/hosts.goserver/fleet/vpp.goserver/mdm/apple/install_application.goserver/mdm/apple/install_application_test.goserver/mdm/apple/util.goserver/mdm/apple/util_test.goserver/mdm/apple/vpp/api.goserver/mdm/apple/vpp/api_test.goserver/mdm/nanomdm/push/nanopush/provider.goserver/mock/datastore_mock.goserver/service/apple_mdm.goserver/service/apple_mdm_test.go
💤 Files with no reviewable changes (1)
- frontend/pages/hosts/details/cards/Software/SelfService/_styles.scss
JordanMontgomery
left a comment
There was a problem hiding this comment.
Overall this looks good. I think maybe the upgrade case should be handled if possible(backfilling old enrollments). Also would it be possible to have an end to end integration test added? We have them for several other setup experience paths. That could be a follow-on
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #45202 +/- ##
========================================
Coverage 66.75% 66.76%
========================================
Files 2740 2743 +3
Lines 219199 219459 +260
Branches 10832 10826 -6
========================================
+ Hits 146331 146516 +185
- Misses 59637 59699 +62
- Partials 13231 13244 +13
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:
|
Squashed branch commits: - Add vpp_client_users table and CRUD - Add ManagedAppleID to HostMDM - Create users on Apple VPP - Update install application to support user enrollment (new installapp package) - Self-service UI for setup experience - License management for VPP - Associate assets on install for user-enrolled hosts - Handle 'already installed' gracefully - Capture Apple ID via IDP MDM users - Lint fixes (require -> assert in handler closures, min() modernization)
- Rename `cap` to `capt` in install_vpp_associate_test.go to avoid shadowing the builtin cap() (revive: redefines-builtin-id). - Add explicit `COLLATE utf8mb4_unicode_ci` to vpp_client_users.status ENUM so TestCollation passes on MySQL 8 (which would otherwise default to utf8mb4_0900_ai_ci). - Add missing defaultEnrollmentProfileUrl mock to the "MDM not configured" SetupAssistant test so isLoading resolves and the empty state renders.
Mirror what TokenUpdate does at enrollment time so already-enrolled User Enrollment (BYOD) hosts get their Managed Apple ID populated without having to re-enroll. The source is the IDP account email linked via host_mdm_idp_accounts, filtered to is_personal_enrollment=1 to match the application path that sets the column on new enrollments.
The iphone_ipad_refetcher cron only runs every hour and skips hosts whose details_updated_at is less than 1 hour stale, so a freshly BYOD-enrolled iPhone/iPad has no app inventory in Fleet for up to an hour. That blocks software install (the host page can't show installable software until apps come back), forcing the user to click "Refetch" manually. Send InstalledApplicationList, CertificateList, and DeviceInformation from the post-manual-enrollment worker so the inventory populates as soon as enrollment finishes, and track the commands in host_mdm_commands so the cron skips them on its next tick.
Exercises the Account-Driven User Enrollment path end-to-end (SSO bearer token → AccountDriven enrollment → post-manual-enrollment worker) and verifies the four things Fleet now does for a freshly BYOD-enrolled iPhone: - persists the Managed Apple ID on host_mdm from the IdP account email, - sets refetch_requested so the UI surfaces that inventory is pending, - sends the three iOS refetch MDM commands with managedAppsOnly=true on InstalledApplicationList (Apple rejects the full-inventory variant on User Enrollments), and - records them in host_mdm_commands so the hourly iphone_ipad_refetcher cron skips them on its next tick. The setup-experience VPP install half of the BYOD flow is exercised by TestInstallVPPAppPostValidation_AssociateAssetsRouting; reproducing the full Apple /users/create + /assets/associate clientUserId dance here would need a more complete VPP mock backend.
- vpp/api.go: nil-check params in Validate() so AssociateAssets returns a clean error instead of dereferencing a nil pointer (Copilot, CodeRabbit). - fleet/hosts.go: rewrite ManagedAppleID field comment to reflect the actual source (IdP account email at TokenUpdate), not UserLongName which Apple doesn't reliably populate on User Enrollment (Copilot). - fleet/datastore.go: clarify GetHostManagedAppleID/SetHostManagedAppleID docs — NotFound when no host_mdm row, empty string when NULL (Copilot). - changelog: "Account-based" → "Account-Driven" to match Apple's term used elsewhere in code/comments (Copilot). - vpp_users.go: don't silently swallow InsertVPPClientUser errors when persisting the pending state after CreateUsers/per-user failures — log them so an insert failure doesn't get masked (CodeRabbit). - mysql/hosts.go: GetHostManagedAppleID reads from the primary (the value is set during TokenUpdate and consumed seconds later by VPP user-provisioning, so replica lag would cause false misses); SetHostManagedAppleID returns NotFound on zero RowsAffected so a missing host_mdm row isn't silently dropped (CodeRabbit). - mysql/vpp.go: drop managed_apple_id (PII) from VPPClientUser not-found error message (CodeRabbit). - apple_mdm.go: always call SetHostManagedAppleID for UserEnrollmentDevice — including with an empty value — so a stale Managed Apple ID from a prior enrollment is cleared and can't be reused for user-scoped VPP actions; log (not fail) NotFound since the lifecycle reset above should have inserted the host_mdm row already (CodeRabbit). - software_installers_test.go: set host.MDM.EnrollmentStatus to personal so TestInstallSoftwareTitleAllowsPersonallyEnrolledDevices actually exercises the BYOD code path (CodeRabbit). Tests updated: hosts_test.go asserts NotFound from SetHostManagedAppleID when host_mdm is missing; apple_mdm_test.go asserts the empty-value clear path for UserEnrollmentDevice without an IdP account.
Earlier this branch had runPostManualEnrollment send InstalledApplicationList / CertificateList / DeviceInformation directly so freshly-enrolled BYOD iPhones would have inventory immediately. That broke a number of existing integration tests (TestInHouseAppInstall, TestInstalledApplicationListCommandForBYODiDevices, TestIOSiPadOSRefetch, …) — their command-drain loops only acknowledge specific RequestTypes, so the unexpected refetch commands looped forever and the suite timed out. Move the trigger into the existing iphone_ipad_refetcher cron instead: - mdmHostEnrollFields now flags iOS/iPadOS hosts with refetch_requested=true on enrollment (osquery-capable hosts already did this; iOS has no osquery, so it needs MDM-driven refetch as its inventory source). - ListIOSAndIPadOSToRefetch's WHERE clause adds `OR h.refetch_requested = 1`, so the next 10-minute cron tick picks up a freshly-enrolled host. The flag is cleared by the DeviceInformation ack handler, so we don't re-send on every tick after the first refetch. - runPostManualEnrollment drops the synchronous refetchIOSIPadOSAfterEnrollment call (and the helper); the cron is now the single source of truth for emitting refetch commands. Tests updated to match: - testMDMAppleUpsertHostIOSIPadOS now asserts RefetchRequested=true for iOS/iPadOS. - testListIOSAndIPadOSToRefetch clears refetch_requested between setup and the staleness-path assertions so the same coverage still applies. - TestSetupExperienceBYODiOS drives IOSiPadOSRefetch explicitly (the suite doesn't tick the cron schedule) and drops the immediate-command assertions. - The worker-level "schedules refetch for iOS BYOD enrollments" test is removed since the worker no longer queues commands.
94b14e4 to
00d7634
Compare
JordanMontgomery
left a comment
There was a problem hiding this comment.
It's hard to fully review a PR this size but I think this looks good overall
| JOIN hosts h ON h.id = hm.host_id | ||
| JOIN host_mdm_idp_accounts hmia ON hmia.host_uuid = h.uuid | ||
| JOIN mdm_idp_accounts mia ON mia.uuid = hmia.account_uuid | ||
| SET hm.managed_apple_id = mia.email |
There was a problem hiding this comment.
I think this is OK just calling out this could be a long running query during migration since it does a few joins and might require some full table scans. Might be worth reviewing or trying to test with synthetic data
Summary by CodeRabbit
New Features
Bug Fixes