Adding create vpp user recovery if the user is missing from db - #46345
Adding create vpp user recovery if the user is missing from db#46345georgekarrv wants to merge 1 commit into
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
Adds a recovery path for Apple VPP /users/create failures with error 9635 ("Apple Account can't be associated with registered user"). When the local vpp_client_users mapping is missing but Apple still has the Managed Apple ID registered under a different clientUserId, Fleet now adopts Apple's existing identifier via a new /users/get call instead of failing the install. This recovery is applied for both top-level and per-user variants of the 9635 error.
Changes:
- Add
ErrorNumberUserAlreadyRegisteredconstant,IsUserAlreadyRegisteredErrorhelper,GetUserAPI wrapper, andItsIdHashhelper in the VPP API package. - Add
recoverExistingVPPClientUserflow inensureVPPClientUserto look up the existing user viaitsIdHashand re-seatvpp_client_userswith Apple'sclientUserId. - Add unit tests for
GetUser,ItsIdHash,IsUserAlreadyRegisteredError, and a service-level recovery test.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| server/mdm/apple/vpp/api.go | Adds 9635 error helpers, /users/get wrapper, and ItsIdHash. |
| server/mdm/apple/vpp/api_test.go | Tests for GetUser, ItsIdHash, IsUserAlreadyRegisteredError. |
| ee/server/service/vpp_users.go | Recovers from 9635 by adopting Apple's existing clientUserId. |
| ee/server/service/vpp_users_test.go | Test that 9635 → /users/get recovery re-seats the cache. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Caution Review failedFailed to post review comments WalkthroughThe PR extends the VPP user provisioning flow to query Apple for existing users when the local cache lacks a registered row. Instead of immediately registering new users, 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 |
![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/mdm/apple/vpp/api_test.go (1)
745-751: ⚡ Quick winAdd coverage for the “both fields set” invalid request case.
TestGetUsershould assert rejection when bothClientUserIdandItsIdHashare provided, to lock the exactly-one contract and prevent regressions.Suggested test addition
func TestGetUser(t *testing.T) { t.Run("rejects empty request", func(t *testing.T) { _, err := GetUser("token", nil) require.Error(t, err) _, err = GetUser("token", &GetUserRequest{}) require.Error(t, err) }) + + t.Run("rejects request with both clientUserId and itsIdHash", func(t *testing.T) { + _, err := GetUser("token", &GetUserRequest{ + ClientUserId: "uuid-1", + ItsIdHash: ItsIdHash("user@example.com"), + }) + require.Error(t, err) + require.Contains(t, err.Error(), "mutually exclusive") + })🤖 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_test.go` around lines 745 - 751, Add a test asserting that GetUser rejects requests where both identifying fields are provided: call GetUser("token", &GetUserRequest{ClientUserId: "...", ItsIdHash: "..."}) and require an error; place this in the same TestGetUser t.Run block (or a new t.Run "rejects both fields set") alongside the existing empty-request checks so the exactly-one-of ClientUserId/ItsIdHash invariant is covered. Ensure you reference GetUser and GetUserRequest in the assertion so the test fails if both fields are accepted.
🤖 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 175-176: The log in svc.logger.InfoContext that currently emits
the raw managedAppleID should instead log a non-PII surrogate (e.g., a SHA-256
hex or other irreversible hash) to avoid exposing an email/identifier; compute
the hash of managedAppleID just before the log call and replace the
"managed_apple_id" value with the hashed string (or call a shared helper like
hashPII if available) while keeping "host_id" and "vpp_token_id" unchanged so
the log still correlates records without storing raw PII.
In `@server/mdm/apple/vpp/api.go`:
- Around line 345-348: Update the GetUser input validation in GetUser(token
string, params *GetUserRequest) to enforce "exactly one" of ClientUserId or
ItsIdHash: instead of allowing both to be set, return an error when both
ClientUserId and ItsIdHash are non-empty, and keep the existing error for both
empty; adjust the conditional that currently checks params == nil ||
(params.ClientUserId == "" && params.ItsIdHash == "") to also reject the case
where both fields are provided so the function only proceeds when exactly one
identifier is present.
---
Nitpick comments:
In `@server/mdm/apple/vpp/api_test.go`:
- Around line 745-751: Add a test asserting that GetUser rejects requests where
both identifying fields are provided: call GetUser("token",
&GetUserRequest{ClientUserId: "...", ItsIdHash: "..."}) and require an error;
place this in the same TestGetUser t.Run block (or a new t.Run "rejects both
fields set") alongside the existing empty-request checks so the exactly-one-of
ClientUserId/ItsIdHash invariant is covered. Ensure you reference GetUser and
GetUserRequest in the assertion so the test fails if both fields are accepted.
🪄 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: 17beeb19-f9b1-4d84-8afb-1d42fac5e5df
📥 Commits
Reviewing files that changed from the base of the PR and between d313300 and 5ed862ad8a5dcef6ffd9f2ecfffe0b4d7950db6c.
📒 Files selected for processing (4)
ee/server/service/vpp_users.goee/server/service/vpp_users_test.goserver/mdm/apple/vpp/api.goserver/mdm/apple/vpp/api_test.go
| func GetUser(token string, params *GetUserRequest) (*CreateUsersResult, error) { | ||
| if params == nil || (params.ClientUserId == "" && params.ItsIdHash == "") { | ||
| return nil, errors.New("GetUserRequest: one of ClientUserId or ItsIdHash is required") | ||
| } |
There was a problem hiding this comment.
Enforce mutual exclusivity in GetUser input validation.
Line 346 currently permits both ClientUserId and ItsIdHash, but this endpoint is documented here as “exactly one.” Allowing both can produce ambiguous requests and brittle behavior.
Suggested fix
func GetUser(token string, params *GetUserRequest) (*CreateUsersResult, error) {
- if params == nil || (params.ClientUserId == "" && params.ItsIdHash == "") {
- return nil, errors.New("GetUserRequest: one of ClientUserId or ItsIdHash is required")
- }
+ if params == nil {
+ return nil, errors.New("GetUserRequest: params cannot be nil")
+ }
+ hasClientUserID := params.ClientUserId != ""
+ hasItsIDHash := params.ItsIdHash != ""
+ switch {
+ case hasClientUserID && hasItsIDHash:
+ return nil, errors.New("GetUserRequest: ClientUserId and ItsIdHash are mutually exclusive")
+ case !hasClientUserID && !hasItsIDHash:
+ return nil, errors.New("GetUserRequest: one of ClientUserId or ItsIdHash is required")
+ }🤖 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 345 - 348, Update the GetUser input
validation in GetUser(token string, params *GetUserRequest) to enforce "exactly
one" of ClientUserId or ItsIdHash: instead of allowing both to be set, return an
error when both ClientUserId and ItsIdHash are non-empty, and keep the existing
error for both empty; adjust the conditional that currently checks params == nil
|| (params.ClientUserId == "" && params.ItsIdHash == "") to also reject the case
where both fields are provided so the function only proceeds when exactly one
identifier is present.
5ed862a to
daf1fb8
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #46345 +/- ##
=======================================
Coverage 66.89% 66.90%
=======================================
Files 2783 2783
Lines 221736 221735 -1
Branches 11221 11221
=======================================
+ Hits 148335 148342 +7
+ Misses 60001 59994 -7
+ Partials 13400 13399 -1
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:
|
|
We went a different way with this. CLosing this one |
Fixing unreleased behavior where if a user was missing from the vpp users db that it would retrieve the current vpp user from apple to continue installing.
Summary by CodeRabbit
Bug Fixes
Tests