Fixing unreleased issue with vpp installs on byod - #46108
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #46108 +/- ##
==========================================
+ Coverage 66.82% 66.84% +0.01%
==========================================
Files 2754 2761 +7
Lines 220138 220945 +807
Branches 10905 10919 +14
==========================================
+ Hits 147117 147699 +582
- Misses 59730 59870 +140
- Partials 13291 13376 +85
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:
|
![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.
|
@claude review once |
|
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 migrates Fleet's VPP client-user registration from asynchronous v2 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 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: 1
🧹 Nitpick comments (2)
server/mdm/apple/vpp/api.go (1)
107-109: 💤 Low valueRedundant substring check.
The third clause
strings.Contains(msg, "user")makes the first two clauses ("registered user"and"client user") redundant, since both contain"user". If this broad matching is intentional, the code can be simplified:- if strings.Contains(msg, "unable to find") && - (strings.Contains(msg, "registered user") || strings.Contains(msg, "client user") || strings.Contains(msg, "user")) { + if strings.Contains(msg, "unable to find") && strings.Contains(msg, "user") {If you intended narrower matching (only "registered user" or "client user"), remove the final
|| strings.Contains(msg, "user").🤖 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/mdm/apple/vpp/api.go` around lines 107 - 109, The conditional in api.go that checks error messages uses strings.Contains(msg, "registered user") || strings.Contains(msg, "client user") || strings.Contains(msg, "user"), making the first two checks redundant; update the condition in the function containing this check to either (A) narrow matching by removing the final strings.Contains(msg, "user") so it only matches "registered user" or "client user", or (B) simplify by keeping only strings.Contains(msg, "user") if the broad match was intended — adjust the boolean expression accordingly where this conditional appears.ee/server/service/install_vpp_associate_test.go (1)
378-379: 💤 Low valueMinor typo in comment: "non-9612" should be "non-9609".
The self-heal logic triggers on error 9609 ("Unable to find the registered user"), not 9612. The test correctly uses 9610 to verify self-heal doesn't run for unrelated errors.
- t.Run("personal enrollment does not self-heal on unrelated associate error", func(t *testing.T) { - // Make sure a non-9612 associate error still bubbles up — we don't + t.Run("personal enrollment does not self-heal on unrelated associate error", func(t *testing.T) { + // Make sure a non-9609 associate error still bubbles up — we don't🤖 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/install_vpp_associate_test.go` around lines 378 - 379, Update the inline comment in the test t.Run("personal enrollment does not self-heal on unrelated associate error", ...) to correct the typo: change "non-9612" to "non-9609" so it correctly references the self-heal trigger error code used by the self-heal logic (and leave the rest of the comment and the test using 9610 as-is).
🤖 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/mdm/apple/util.go`:
- Around line 117-123: Update the user-facing constant
AppAlreadyInstalledBYODUserMessage to correct grammar and clarity: change its
string from "Failed. This app is already installed. Please delete app first, an
install via self-service." to the proposed wording "Failed. This app is already
installed. Please delete the app first, then reinstall via Self Service." so it
includes the missing article and a complete, actionable instruction; make the
edit where AppAlreadyInstalledBYODUserMessage is defined.
---
Nitpick comments:
In `@ee/server/service/install_vpp_associate_test.go`:
- Around line 378-379: Update the inline comment in the test t.Run("personal
enrollment does not self-heal on unrelated associate error", ...) to correct the
typo: change "non-9612" to "non-9609" so it correctly references the self-heal
trigger error code used by the self-heal logic (and leave the rest of the
comment and the test using 9610 as-is).
In `@server/mdm/apple/vpp/api.go`:
- Around line 107-109: The conditional in api.go that checks error messages uses
strings.Contains(msg, "registered user") || strings.Contains(msg, "client user")
|| strings.Contains(msg, "user"), making the first two checks redundant; update
the condition in the function containing this check to either (A) narrow
matching by removing the final strings.Contains(msg, "user") so it only matches
"registered user" or "client user", or (B) simplify by keeping only
strings.Contains(msg, "user") if the broad match was intended — adjust the
boolean expression accordingly where this conditional appears.
🪄 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: afd630d8-0ad6-4d13-af86-3524cda273fb
📒 Files selected for processing (10)
ee/server/service/install_vpp_associate_test.goee/server/service/software_installers.goee/server/service/vpp_users.goee/server/service/vpp_users_test.goserver/mdm/apple/util.goserver/mdm/apple/vpp/api.goserver/mdm/apple/vpp/api_test.goserver/service/apple_mdm.goserver/service/apple_mdm_test.goserver/service/integration_mdm_setup_experience_test.go
There was a problem hiding this comment.
Pull request overview
This PR fixes several BYOD (Account-Driven User Enrollment) VPP install flow issues introduced under #31138 by (1) switching VPP user registration to Apple’s synchronous v1 endpoint, (2) correctly treating “already installed” as a terminal failure on BYOD to avoid infinite verification loops, and (3) adding a self-heal path when Fleet’s cached clientUserId drifts from Apple’s record.
Changes:
- Replace async v2 VPP user creation with synchronous v1
registerVPPUserSrv, and remove v2 create-user types/tests. - Enrollment-aware handling of InstallApplication “already installed” errors (fail on BYOD, keep “success” behavior on fully-managed).
- Add BYOD-only recovery flow for Apple error 9609 by looking up user via v2
/usersand resyncing (or re-registering) before retrying associate once.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/integration_mdm_setup_experience_test.go | Updates test commentary to reflect the v1 registration endpoint. |
| server/service/apple_mdm.go | Adds enrollment-type lookup and routes “already installed” handling + retry behavior accordingly. |
| server/service/apple_mdm_test.go | Extends test coverage for managed vs BYOD “already installed” behavior. |
| server/mdm/apple/vpp/api.go | Adds v1 RegisterUser + v2 lookup-by-managedAppleID + unknown-client-user detection; updates auth header handling for v1. |
| server/mdm/apple/vpp/api_test.go | Replaces CreateUsers tests with RegisterUser/GetUserByManagedAppleID/IsUnknownClientUserError tests. |
| server/mdm/apple/util.go | Adds BYOD-specific user-facing error message constant. |
| ee/server/service/vpp_users.go | Switches ensureVPPClientUser to v1 registration and adds registerVPPClientUser helper. |
| ee/server/service/vpp_users_test.go | Updates tests for v1 registration behavior and error handling. |
| ee/server/service/software_installers.go | Adds BYOD “unknown clientUserId” self-heal and single-retry associate logic. |
| ee/server/service/install_vpp_associate_test.go | Updates mock VPP server routing for v1 register + adds self-heal test cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/service/apple_mdm.go (1)
4033-4045:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't coerce lookup misses into the managed-host path.
When either lookup returns
NotFound, this helper reportsfalse, nil, so the caller can't distinguish "managed" from "unknown". In theInstallApplication12042 path, that can incorrectly promote a BYOD result toAcknowledged, which re-enters verification/retry instead of preserving the terminal BYOD failure behavior.Suggested direction
-func (svc *MDMAppleCheckinAndCommandService) hostIsPersonalEnrollment(ctx context.Context, enrollmentIdentifier string) (bool, error) { +func (svc *MDMAppleCheckinAndCommandService) hostIsPersonalEnrollment(ctx context.Context, enrollmentIdentifier string) (isPersonal bool, known bool, err error) { host, err := svc.ds.HostLiteByIdentifier(ctx, enrollmentIdentifier) if err != nil { if fleet.IsNotFound(err) { - return false, nil + return false, false, nil } - return false, ctxerr.Wrap(ctx, err, "host lite by identifier") + return false, false, ctxerr.Wrap(ctx, err, "host lite by identifier") } hostMDM, err := svc.ds.GetHostMDM(ctx, host.ID) if err != nil { if fleet.IsNotFound(err) { - return false, nil + return false, false, nil } - return false, ctxerr.Wrap(ctx, err, "get host mdm") + return false, false, ctxerr.Wrap(ctx, err, "get host mdm") } - return hostMDM.IsPersonalEnrollment, nil + return hostMDM.IsPersonalEnrollment, true, nil }Then keep the original error/no-retry behavior when
known == false, instead of treating it as fully managed.🤖 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/apple_mdm.go` around lines 4033 - 4045, The helper is swallowing NotFound from svc.ds.HostLiteByIdentifier and svc.ds.GetHostMDM by returning (false, nil), which hides lookup misses from callers (e.g., InstallApplication) and can promote BYOD to managed; change those branches so NotFound is propagated (return the original NotFound error or a wrapped ctxerr — e.g., return false, err or return false, ctxerr.Wrap(ctx, err, "host lite by identifier"/"get host mdm")) instead of returning false,nil so callers can distinguish unknown hosts from managed ones.
🤖 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 57-80: The current resync logic runs only when a stale local row
(existing != nil) is present; add the same Apple lookup/resync on the cache-miss
path or on duplicate-register errors so a transient DB write failure doesn't
become permanent. Concretely: before calling
registerVPPUserSrv/registerVPPClientUser when existing == nil, call
vpp.GetUserByManagedAppleID(ctx, token.Token, managedAppleID) and if it returns
an appleUser, create a fleet.VPPClientUser and persist it via
svc.ds.InsertVPPClientUser and return appleUser.ClientUserID; additionally, if
registerVPPUserSrv/registerVPPClientUser fails with a duplicate/409-style error,
catch that error, call GetUserByManagedAppleID, resync via
svc.ds.InsertVPPClientUser, and return the found ClientUserID. Ensure you use
the same ctx, token.ID and managedAppleID when constructing the VPPClientUser.
---
Outside diff comments:
In `@server/service/apple_mdm.go`:
- Around line 4033-4045: The helper is swallowing NotFound from
svc.ds.HostLiteByIdentifier and svc.ds.GetHostMDM by returning (false, nil),
which hides lookup misses from callers (e.g., InstallApplication) and can
promote BYOD to managed; change those branches so NotFound is propagated (return
the original NotFound error or a wrapped ctxerr — e.g., return false, err or
return false, ctxerr.Wrap(ctx, err, "host lite by identifier"/"get host mdm"))
instead of returning false,nil so callers can distinguish unknown hosts from
managed ones.
🪄 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: 88a26eca-94b7-4e5a-b2ba-239d93eb32d1
📒 Files selected for processing (5)
ee/server/service/software_installers.goee/server/service/vpp_users.goee/server/service/vpp_users_test.goserver/mdm/apple/util.goserver/service/apple_mdm.go
💤 Files with no reviewable changes (1)
- ee/server/service/software_installers.go
| svc.logger.ErrorContext(ctx, "persisting pending vpp client user after CreateUsers failure", | ||
| "host_id", host.ID, "vpp_token_id", token.ID, "err", insertErr) | ||
| // Non-registered row (typically a legacy 'pending' entry from the prior | ||
| // v2 async flow, or a row left over from a failed registration). Apple |
There was a problem hiding this comment.
This comment is a little misleading since we never technically shipped the v2 flow. Not a huge deal
Co-authored-by: Jordan Montgomery <elijah.jordan.montgomery@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/service/apple_mdm.go (1)
4263-4266: ⚡ Quick winDrop the stale
errbranch after switching tor.Type.
isPersonalis computed locally now, so Lines 4264-4266 only read the outererr, which is already nil on this path. Keeping the old wrap here is dead code and leaves a misleading failure message.♻️ Proposed cleanup
- isPersonal := r.Type == mdm.UserEnrollmentDevice - if err != nil { - return nil, ctxerr.Wrap(r.Context, err, "looking up enrollment type for InstallApplication already-installed result") - } + isPersonal := r.Type == mdm.UserEnrollmentDevice🤖 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/apple_mdm.go` around lines 4263 - 4266, The if err { ctxerr.Wrap(...) } block after computing isPersonal := r.Type == mdm.UserEnrollmentDevice is dead code and should be removed: since r.Type is computed locally here and the outer err is nil on this path, delete the error-checking branch (the lines that reference err and call ctxerr.Wrap for "looking up enrollment type for InstallApplication already-installed result") so the function no longer reads a stale outer err; leave the isPersonal assignment and continue the InstallApplication result handling unchanged.
🤖 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.
Nitpick comments:
In `@server/service/apple_mdm.go`:
- Around line 4263-4266: The if err { ctxerr.Wrap(...) } block after computing
isPersonal := r.Type == mdm.UserEnrollmentDevice is dead code and should be
removed: since r.Type is computed locally here and the outer err is nil on this
path, delete the error-checking branch (the lines that reference err and call
ctxerr.Wrap for "looking up enrollment type for InstallApplication
already-installed result") so the function no longer reads a stale outer err;
leave the isPersonal assignment and continue the InstallApplication result
handling unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 378d32a9-bce2-4f33-ae7e-674145839065
📒 Files selected for processing (1)
server/service/apple_mdm.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/pages/hosts/details/cards/HostSoftwareLibrary/helpers.tsx (1)
52-54: ⚡ Quick winAdd coverage for the new “Software title is not available for install” passthrough.
This new user-visible branch should get a dedicated
getInstallErrorMessagetest to prevent regressions in toast text behavior.Suggested test addition
diff --git a/frontend/pages/hosts/details/cards/HostSoftwareLibrary/helpers.tests.tsx b/frontend/pages/hosts/details/cards/HostSoftwareLibrary/helpers.tests.tsx @@ it("returns no available licenses message as-is", () => { const result = getInstallErrorMessage(makeErr("No available licenses.")); expect(result).toBe("No available licenses"); }); + it("returns software title unavailable message as-is", () => { + const result = getInstallErrorMessage( + makeErr( + "Couldn't install software. Software title is not available for install. Please add software package or App Store app to install." + ) + ); + expect(result).toBe( + "Couldn't install software. Software title is not available for install. Please add software package or App Store app to install" + ); + }); + it("surfaces the missing Managed Apple ID message as-is", () => {🤖 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 `@frontend/pages/hosts/details/cards/HostSoftwareLibrary/helpers.tsx` around lines 52 - 54, The new passthrough branch that checks for "Software title is not available for install" in getInstallErrorMessage needs a dedicated unit test to prevent regressions: add a test for the getInstallErrorMessage function that passes an error message containing "Software title is not available for install" and asserts the returned toast text matches the expected user-facing string; mirror the existing test style used for the "hasn't received a Managed Apple ID" case (same file helper reference: getInstallErrorMessage) so both branches are covered.
🤖 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.
Nitpick comments:
In `@frontend/pages/hosts/details/cards/HostSoftwareLibrary/helpers.tsx`:
- Around line 52-54: The new passthrough branch that checks for "Software title
is not available for install" in getInstallErrorMessage needs a dedicated unit
test to prevent regressions: add a test for the getInstallErrorMessage function
that passes an error message containing "Software title is not available for
install" and asserts the returned toast text matches the expected user-facing
string; mirror the existing test style used for the "hasn't received a Managed
Apple ID" case (same file helper reference: getInstallErrorMessage) so both
branches are covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d0ae9f51-941c-419e-82a7-c2fd4d691e8e
📒 Files selected for processing (3)
frontend/pages/hosts/details/cards/HostSoftwareLibrary/helpers.tests.tsxfrontend/pages/hosts/details/cards/HostSoftwareLibrary/helpers.tsxfrontend/pages/hosts/details/cards/Software/SelfService/SelfService.tsx
Summary
Three fixes for the BYOD/User-Enrollment VPP install flow shipped under #31138, all caught during QA on
gkarr-fix-byod-vpp:1. Switch VPP user registration from v2 (async) to v1 (sync)
The original implementation called Apple's
/mdm/v2/users/create, which is asynchronous: a 200 response only confirms the registration was queued, with nouserIdreturned. Fleet was storing the row asregisteredbased on theeventIdalone, which masked failures and leftapple_user_idpermanentlyNULL. Switched to Apple's legacy v1registerVPPUserSrvendpoint, which returns the full user record (including Apple'suserId) in the same response, so we know definitively whether registration succeeded.vpp.RegisterUser(token, clientUserID, managedAppleID)posts tohttps://vpp.itunes.apple.com/mdm/registerVPPUserSrvwith the token in the request body (v1 doesn't use theAuthorizationheader).ensureVPPClientUserno longer writes "pending" rows on failure — v1's sync contract means we either succeed or surface the error.CreateUserstypes and tests.2. Show a clear error when an end user already has the app installed personally
On BYOD/User-Enrolled hosts, when an end user has already installed an app from the App Store outside of Fleet, Apple's
InstallApplicationcommand fails with"The app with iTunes Store ID <id> is already installed."(code 12042). Previously Fleet treated this as success and entered theInstalledApplicationListverification loop — which never resolves on User Enrollment, since Apple'smanagedAppsOnly: truefilter doesn't return personally-installed apps. The install appeared stuck."Failed. This app is already installed. Please delete app first, an install via self-service."3. Self-heal a stale
vpp_client_userscache against AppleIf Fleet's cached
client_user_idever drifts from Apple's record (DB restore from a stale backup, manual tampering, future bug), Apple rejectsAssociateAssetswith code 9609"Unable to find the registered user."and the install fails terminally. Apple enforces one VPP user per(location, managed_apple_id), so blindly re-registering hits a different error (9635). The recovery is two-step:GET /mdm/v2/users?managedAppleId=…. If Apple has an active record, upsert thatclientUserIdback intovpp_client_usersand retry the associate.Each branch logs a distinct
WARNso the self-heal is visible in operator dashboards. Only one retry — if the follow-up associate fails, the second error is surfaced.Test plan
make lint-go-incremental— cleango test ./server/mdm/apple/vpp/... ./ee/server/service/... ./server/service/ -run '<targeted>'— all greenapple_user_idNULL.vpp_client_users.client_user_idand observing the WARN line + restored UUID.Summary by CodeRabbit
Bug Fixes
New Features
Tests