Support VPP apps from non-US App Store regions - #44368
Merged
Merged
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.
marko-lisica
had a problem deploying
to
Docker Hub
April 29, 2026 06:34 — with 
GitHub Actions
Failure
GitHub Actions
Failure
Contributor
|
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:
WalkthroughAdds storefront country support for VPP by introducing a nullable 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)
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #44368 +/- ##
========================================
Coverage 66.68% 66.69%
========================================
Files 2665 2667 +2
Lines 214783 215275 +492
Branches 9759 9759
========================================
+ Hits 143227 143574 +347
- Misses 58521 58626 +105
- Partials 13035 13075 +40
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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
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/vpp.go (1)
1432-1461:⚠️ Potential issue | 🟠 MajorPreserve existing token country when update payload omits it
On Line 1433,
country_code = ?combined with nilcountryCode(Line 1448-1451) will overwrite an existing token country toNULL. That can silently break country matching/anchoring flows after a token update.Proposed fix
stmt := ` UPDATE vpp_tokens SET organization_name = ?, location = ?, renew_at = ?, token = ?, - country_code = ? + country_code = COALESCE(?, country_code) WHERE id = ? `🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/datastore/mysql/vpp.go` around lines 1432 - 1461, The update currently binds a nil countryCode which will set country_code to NULL when the payload omits it; instead preserve the existing value by either changing the UPDATE to use COALESCE(?, country_code) for the country_code assignment or by fetching the current token row and passing its country when tok.CountryCode is empty; update the code around vppTokenDataToVppTokenDB, the countryCode variable, and the ExecContext call (stmt / ds.writer(ctx).ExecContext) so an omitted tok.CountryCode does not overwrite the stored country.
🧹 Nitpick comments (3)
cmd/fleet/serve.go (1)
1392-1395: Avoid a startup race with the VPP refresh cron.Because this backfill is fire-and-forget, the first refresh can still observe partially populated country codes if the scheduler fires immediately after startup. Consider waiting for the one-shot pass to complete before registering the VPP refresh schedule.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/fleet/serve.go` around lines 1392 - 1395, The one-shot backfill (vpp.BackfillLegacyCountries(ctx, ds, logger)) should not be fire-and-forget; call it synchronously and wait for it to complete (and handle/propagate any error) before registering or starting the VPP refresh cron/scheduler so the first scheduled refresh never sees partially populated country_code values; replace the goroutine call (go vpp.BackfillLegacyCountries(...)) with a blocking call that returns/handles errors, then only after it finishes invoke the code that registers/starts the VPP refresh schedule.frontend/pages/SoftwarePage/components/forms/SoftwareVppForm/_styles.scss (1)
70-76: Consider a viewport-relative cap here.A fixed
718pxmax-heightcan still push the list past the fold on smaller dialogs;min(718px, 80vh)would keep the longer list without forcing a second scroll region.♻️ Possible tweak
- max-height: 718px; + max-height: min(718px, 80vh);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/pages/SoftwarePage/components/forms/SoftwareVppForm/_styles.scss` around lines 70 - 76, Replace the fixed pixel cap on the list's height to use a viewport-relative limit so the list won't overflow smaller dialogs: update the &__list rule (the selector with max-height: 718px) to use a min() between 718px and a vh value (for example min(718px, 80vh)) so the list respects both the design max and the viewport height.server/datastore/mysql/vpp.go (1)
1473-1506: Validate and normalize ISO country code format at datastore boundaryLine 1474 and Line 1493 only reject empty strings. These methods should also normalize casing and enforce ISO-2 shape to protect data integrity (
[a-z]{2}), especially since matching logic is exact.Proposed hardening
func (ds *Datastore) UpdateVPPTokenCountryCode(ctx context.Context, tokenID uint, countryCode string) error { - if countryCode == "" { + countryCode = strings.ToLower(strings.TrimSpace(countryCode)) + if len(countryCode) != 2 { return ctxerr.New(ctx, "country code cannot be empty") } @@ func (ds *Datastore) UpdateVPPAppCountryCode(ctx context.Context, adamID string, platform fleet.InstallableDevicePlatform, countryCode string) error { - if countryCode == "" { + countryCode = strings.ToLower(strings.TrimSpace(countryCode)) + if len(countryCode) != 2 { return ctxerr.New(ctx, "country code cannot be empty") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/datastore/mysql/vpp.go` around lines 1473 - 1506, Both UpdateVPPTokenCountryCode and UpdateVPPAppCountryCode currently only reject empty strings; add validation at the start of each function to normalize the incoming countryCode to lowercase and enforce an ISO-2 shape (two ASCII letters a–z). If normalization yields a string not matching exactly two letters, return a context-wrapped validation error (similar to ctxerr.New) before executing the SQL. Implement the check using a simple length and alphabetic test or a small regexp and apply the normalized value in the ExecContext call so stored values are always lowercase two-letter codes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ee/server/service/vpp.go`:
- Around line 93-114: The code uses the token returned by
GetVPPTokenOwningAppInCountry directly as fetchSecret which may be expired;
update the logic in the owning-token branch (the tok variable returned by
GetVPPTokenOwningAppInCountry) to verify token validity (e.g., check
tok.RenewDate or equivalent expiry field) and if the token is expired treat it
as “no eligible token” so you return a vppAddAnchor with reAnchor: true (or
otherwise fall back to the addingTeam secret), otherwise populate fetchSecret
with tok.Token and reAnchor: false; alternatively ensure
GetVPPTokenOwningAppInCountry only returns active tokens (renew_at > NOW()) so
tok is guaranteed valid before being used as fetchSecret.
- Around line 1350-1354: When constructing fleet.VPPTokenData (the data variable
with Token, Location, CountryCode) for token upload/update, add a guard that
rejects the request if clientCfg.CountryCode is empty: check
clientCfg.CountryCode before creating the fleet.VPPTokenData and return an
appropriate error (e.g., validation/400 response or wrapped error) instead of
persisting; apply the same check in both places where fleet.VPPTokenData is
built for create and update so tokens with empty CountryCode are not stored.
- Around line 1013-1023: The code currently indexes anchors by
byAdamID[id.AdamID] which resolves a single anchor for an Adam ID but anchoring
is per (adam_id, platform); change the lookup to resolve and cache anchors per
(adamID, platform) instead (e.g., use a composite key like fmt.Sprintf("%d:%s")
or a nested map keyed by AdamID then Platform) so that svc.resolveAddAnchor(ctx,
id.AdamID, id.Platform, teamTokenInfo) is called and stored per unique
(adamID,Platform) and subsequent logic that uses entry.anchor.reAnchor and
anchor.anchorCountry reads the correct platform-specific anchor; update uses of
byAdamID, construction of perApp entries, and reAnchors append to use the
composite-keyed cache.
In `@server/datastore/mysql/vpp.go`:
- Around line 1515-1524: The UPDATE in const stmt joins vpp_apps (va) ->
vpp_apps_teams (vat) -> vpp_tokens (vt) and can pick a non-deterministic token
when multiple vat rows exist; change the query to assign va.country_code from a
deterministic subquery that selects a single token per (adam_id, platform) using
an ORDER BY vt.id ASC LIMIT 1 (same pattern as GetVPPTokenOwningAppInCountry)
and set va.country_code = (that subquery) only when the subquery returns a
non-NULL country_code so the backfill is deterministic.
In `@server/mdm/apple/vpp/backfill.go`:
- Around line 55-83: The current loop spawns an unbounded goroutine per token
(iterating over needsBackfill) causing a burst of GetConfig and
UpdateVPPTokenCountryCode calls; change it to a bounded-concurrency worker pool
or semaphore pattern (e.g., create a buffered channel semaphore or worker
goroutines) and acquire the token before launching each work unit so at most N
concurrent workers run; inside each worker call GetConfig(token.Token) and
ds.UpdateVPPTokenCountryCode(ctx, token.ID, cfg.CountryCode) and update filled
under filledMu, and ensure wg is used to wait for all work to finish (increment
wg for each scheduled work unit, not for each potential goroutine spawn).
---
Outside diff comments:
In `@server/datastore/mysql/vpp.go`:
- Around line 1432-1461: The update currently binds a nil countryCode which will
set country_code to NULL when the payload omits it; instead preserve the
existing value by either changing the UPDATE to use COALESCE(?, country_code)
for the country_code assignment or by fetching the current token row and passing
its country when tok.CountryCode is empty; update the code around
vppTokenDataToVppTokenDB, the countryCode variable, and the ExecContext call
(stmt / ds.writer(ctx).ExecContext) so an omitted tok.CountryCode does not
overwrite the stored country.
---
Nitpick comments:
In `@cmd/fleet/serve.go`:
- Around line 1392-1395: The one-shot backfill (vpp.BackfillLegacyCountries(ctx,
ds, logger)) should not be fire-and-forget; call it synchronously and wait for
it to complete (and handle/propagate any error) before registering or starting
the VPP refresh cron/scheduler so the first scheduled refresh never sees
partially populated country_code values; replace the goroutine call (go
vpp.BackfillLegacyCountries(...)) with a blocking call that returns/handles
errors, then only after it finishes invoke the code that registers/starts the
VPP refresh schedule.
In `@frontend/pages/SoftwarePage/components/forms/SoftwareVppForm/_styles.scss`:
- Around line 70-76: Replace the fixed pixel cap on the list's height to use a
viewport-relative limit so the list won't overflow smaller dialogs: update the
&__list rule (the selector with max-height: 718px) to use a min() between 718px
and a vh value (for example min(718px, 80vh)) so the list respects both the
design max and the viewport height.
In `@server/datastore/mysql/vpp.go`:
- Around line 1473-1506: Both UpdateVPPTokenCountryCode and
UpdateVPPAppCountryCode currently only reject empty strings; add validation at
the start of each function to normalize the incoming countryCode to lowercase
and enforce an ISO-2 shape (two ASCII letters a–z). If normalization yields a
string not matching exactly two letters, return a context-wrapped validation
error (similar to ctxerr.New) before executing the SQL. Implement the check
using a simple length and alphabetic test or a small regexp and apply the
normalized value in the ExecContext call so stored values are always lowercase
two-letter codes.
🪄 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: 67d706e7-3ac1-416a-b1f8-b6a42992603e
📥 Commits
Reviewing files that changed from the base of the PR and between ef9d932 and 7d6fa56877bc9656e9a8b5da95c882002fc9bb98.
📒 Files selected for processing (20)
changes/43846-vpp-app-store-non-us-abmcmd/fleet/serve.goee/server/service/vpp.gofrontend/interfaces/mdm.tsfrontend/pages/SoftwarePage/components/forms/SoftwareVppForm/_styles.scssfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/VppPage/components/VppTable/VppTableConfig.tsxserver/datastore/mysql/migrations/tables/20260428100000_AddVPPCountryCode.goserver/datastore/mysql/migrations/tables/20260428100000_AddVPPCountryCode_test.goserver/datastore/mysql/vpp.goserver/fleet/datastore.goserver/fleet/mdm.goserver/fleet/vpp.goserver/mdm/apple/apple_apps/api.goserver/mdm/apple/apple_apps/api_test.goserver/mdm/apple/vpp/api.goserver/mdm/apple/vpp/api_test.goserver/mdm/apple/vpp/backfill.goserver/mdm/apple/vpp/refresh.goserver/mdm/apple/vpp/refresh_test.goserver/mock/datastore_mock.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/datastore/mysql/schema.sql (1)
3150-3150: Constraincountry_codeformat to protect storefront-anchor integrity.Both new columns accept any
varchar(4)value. Since the contract is lowercase ISO country codes, consider enforcing it at the DB layer (e.g., length/case check) to prevent invalid data from backfill or future code paths.♻️ Suggested migration-level hardening
ALTER TABLE vpp_apps MODIFY COLUMN country_code CHAR(2) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci DEFAULT NULL, ADD CONSTRAINT chk_vpp_apps_country_code CHECK (country_code IS NULL OR (CHAR_LENGTH(country_code) = 2 AND country_code = LOWER(country_code))); ALTER TABLE vpp_tokens MODIFY COLUMN country_code CHAR(2) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci DEFAULT NULL, ADD CONSTRAINT chk_vpp_tokens_country_code CHECK (country_code IS NULL OR (CHAR_LENGTH(country_code) = 2 AND country_code = LOWER(country_code)));Based on learnings: The schema.sql file in server/datastore/mysql/ is auto-generated from migrations for use with tests, so it cannot be manually edited. Any changes must be made through migrations.
Also applies to: 3202-3202
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/datastore/mysql/schema.sql` at line 3150, The schema currently allows any varchar(4) for vpp_apps.country_code and vpp_tokens.country_code; create a new migration (not editing schema.sql) that ALTERs both tables to MODIFY country_code to CHAR(2) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci DEFAULT NULL and ADD CHECK constraints (e.g., chk_vpp_apps_country_code and chk_vpp_tokens_country_code) enforcing country_code IS NULL OR (CHAR_LENGTH(country_code)=2 AND country_code=LOWER(country_code)); ensure the migration targets the vpp_apps and vpp_tokens tables and includes safe SQL for existing null/valid values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/datastore/mysql/schema.sql`:
- Line 3150: The schema currently allows any varchar(4) for
vpp_apps.country_code and vpp_tokens.country_code; create a new migration (not
editing schema.sql) that ALTERs both tables to MODIFY country_code to CHAR(2)
CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci DEFAULT NULL and ADD CHECK
constraints (e.g., chk_vpp_apps_country_code and chk_vpp_tokens_country_code)
enforcing country_code IS NULL OR (CHAR_LENGTH(country_code)=2 AND
country_code=LOWER(country_code)); ensure the migration targets the vpp_apps and
vpp_tokens tables and includes safe SQL for existing null/valid values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a972cc22-a8da-4d2e-a759-aca0480acff8
📥 Commits
Reviewing files that changed from the base of the PR and between 7d6fa56877bc9656e9a8b5da95c882002fc9bb98 and a975342e63eae0b0e6592b08332fd4cd83dbf381.
📒 Files selected for processing (2)
frontend/pages/admin/IntegrationsPage/cards/MdmSettings/VppPage/components/EditTeamsVppModal/EditTeamsVppModal.tests.tsxserver/datastore/mysql/schema.sql
✅ Files skipped from review due to trivial changes (1)
- frontend/pages/admin/IntegrationsPage/cards/MdmSettings/VppPage/components/EditTeamsVppModal/EditTeamsVppModal.tests.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
server/mdm/apple/vpp/api_test.go (1)
93-96: Optional: assert request shape inTestGetConfigwrapper for stronger regression protection.At Line 93 and Line 116, adding common assertions for method/path/auth would catch accidental endpoint or header regressions in
GetConfig.🧪 Suggested test hardening
setupFakeServer(t, func(w http.ResponseWriter, r *http.Request) { calls++ + require.Equal(t, http.MethodGet, r.Method) + require.Equal(t, "/client/config", r.URL.Path) + require.Equal(t, "Bearer "+tt.token, r.Header.Get("Authorization")) tt.handler(w, r) })setupFakeServer(t, func(w http.ResponseWriter, r *http.Request) { calls++ + require.Equal(t, http.MethodGet, r.Method) + require.Equal(t, "/client/config", r.URL.Path) + require.Equal(t, "Bearer token", r.Header.Get("Authorization")) if calls < 2 { w.WriteHeader(http.StatusInternalServerError) fmt.Fprintln(w, `Internal Server Error`)Also applies to: 116-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/mdm/apple/vpp/api_test.go` around lines 93 - 96, Add request-shape assertions inside the TestGetConfig test wrapper by enhancing the handler passed to setupFakeServer: before calling tt.handler(w, r) assert r.Method is the expected HTTP verb, r.URL.Path matches the GetConfig endpoint, and the Authorization (or other expected auth) header is present/has the expected format; update both places where setupFakeServer is invoked (the wrapper at lines around setupFakeServer(t, func(w http.ResponseWriter, r *http.Request) { ... }) and the second occurrence) so TestGetConfig will fail if the request method/path/auth changes unexpectedly.server/mdm/apple/vpp/refresh_test.go (1)
168-179: Optional: Extract repeated mock datastore setup into a small test helper.The repeated
GetAllVPPApps/ListVPPTokens/InsertVPPAppswiring across tests is a bit noisy; a helper would reduce duplication and make scenario intent even clearer.Also applies to: 223-230, 270-288, 327-330, 353-358
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/mdm/apple/vpp/refresh_test.go` around lines 168 - 179, The test repeats wiring of mock.DataStore methods (GetAllVPPAppsFunc, ListVPPTokensFunc, InsertVPPAppsFunc) into a local ds variable; extract that into a small test helper (e.g., newMockVPPDataStore or setupMockVPPStore) that accepts the apps and tokens fixtures and returns the configured *mock.DataStore plus a pointer/slice to captured inserted apps, then replace the inline setup in tests with calls to that helper in all occurrences (the blocks using GetAllVPPAppsFunc / ListVPPTokensFunc / InsertVPPAppsFunc).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/mdm/apple/vpp/api_test.go`:
- Around line 93-96: Add request-shape assertions inside the TestGetConfig test
wrapper by enhancing the handler passed to setupFakeServer: before calling
tt.handler(w, r) assert r.Method is the expected HTTP verb, r.URL.Path matches
the GetConfig endpoint, and the Authorization (or other expected auth) header is
present/has the expected format; update both places where setupFakeServer is
invoked (the wrapper at lines around setupFakeServer(t, func(w
http.ResponseWriter, r *http.Request) { ... }) and the second occurrence) so
TestGetConfig will fail if the request method/path/auth changes unexpectedly.
In `@server/mdm/apple/vpp/refresh_test.go`:
- Around line 168-179: The test repeats wiring of mock.DataStore methods
(GetAllVPPAppsFunc, ListVPPTokensFunc, InsertVPPAppsFunc) into a local ds
variable; extract that into a small test helper (e.g., newMockVPPDataStore or
setupMockVPPStore) that accepts the apps and tokens fixtures and returns the
configured *mock.DataStore plus a pointer/slice to captured inserted apps, then
replace the inline setup in tests with calls to that helper in all occurrences
(the blocks using GetAllVPPAppsFunc / ListVPPTokensFunc / InsertVPPAppsFunc).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bce3ff0d-d503-482a-a31e-ab1b6ebc0cd4
📥 Commits
Reviewing files that changed from the base of the PR and between a975342e63eae0b0e6592b08332fd4cd83dbf381 and 07d5c3d48fdfc69d1ae08217510ab5125755a326.
📒 Files selected for processing (4)
cmd/fleetctl/fleetctl/testing_utils/testing_utils.goserver/mdm/apple/vpp/api_test.goserver/mdm/apple/vpp/refresh_test.goserver/service/integration_mdm_test.go
✅ Files skipped from review due to trivial changes (2)
- server/service/integration_mdm_test.go
- cmd/fleetctl/fleetctl/testing_utils/testing_utils.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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/vpp.go (1)
1336-1352:⚠️ Potential issue | 🟡 MinorConsider validating that
CountryCodeis non-empty before persisting the token.Lines 1348-1352 persist
clientCfg.CountryCodewithout validating it's non-empty. WhileensureVPPTokenCountryprovides a lazy fallback, if Apple consistently returns an empty country for a token, the token becomes unusable since downstream flows (e.g.,resolveAddAnchorat line 61-63) require a non-empty country.Rejecting upfront provides clearer feedback to the user during upload rather than failing later during app operations.
Suggested validation
clientCfg, err := vpp.GetConfig(string(tokenBytes)) if err != nil { // ... existing error handling } +if clientCfg.CountryCode == "" { + return nil, ctxerr.Wrap(ctx, fleet.NewInvalidArgumentError("token", "Invalid token. Could not determine storefront country from Apple Business.")) +} data := fleet.VPPTokenData{ Token: string(tokenBytes), Location: clientCfg.LocationName, CountryCode: clientCfg.CountryCode, }Also applies to: 1391-1407
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/server/service/vpp.go` around lines 1336 - 1352, The code persists clientCfg.CountryCode into fleet.VPPTokenData without ensuring it's non-empty; add a validation after calling vpp.GetConfig (in the block that constructs VPPTokenData) to check that clientCfg.CountryCode is not empty and return a fleet.NewInvalidArgumentError("token", "Missing country code in VPP token response from Apple; please provide a token with a valid country code") (or similar wrapped via ctxerr.Wrap) if it is empty; this mirrors the existing VPP error handling around vpp.ErrorResponse and complements ensureVPPTokenCountry and resolveAddAnchor by rejecting the token upload early when GetConfig returns an empty CountryCode.
♻️ Duplicate comments (1)
ee/server/service/vpp.go (1)
1009-1026:⚠️ Potential issue | 🟠 MajorAnchoring should be resolved per
(adam_id, platform), not justadam_id.Line 1009 indexes by
adamIDalone, but anchoring state is per(adam_id, platform). When the same Adam ID is provided with multiple platforms (iOS, iPadOS, macOS), only the first platform's anchor decision is used for all variants. This can cause incorrect storefront selection and missed re-anchor updates for subsequent platforms.Proposed fix: use composite key
+type appKey struct { + adamID string + platform fleet.InstallableDevicePlatform +} + func (svc *Service) getAnchoredVPPAppsMetadata(ctx context.Context, ids []fleet.VPPAppTeam, teamTokenInfo vppTokenInfo) ([]*fleet.VPPApp, []vppReAnchor, error) { // ... - byAdamID := make(map[string]*perApp) + byAppKey := make(map[appKey]*perApp) var reAnchors []vppReAnchor for _, id := range ids { - entry, ok := byAdamID[id.AdamID] + key := appKey{adamID: id.AdamID, platform: id.Platform} + entry, ok := byAppKey[key] if !ok { anchor, err := svc.resolveAddAnchor(ctx, id.AdamID, id.Platform, teamTokenInfo) if err != nil { return nil, nil, ctxerr.Wrap(ctx, err, "resolving anchor for batch vpp add") } entry = &perApp{anchor: anchor} - byAdamID[id.AdamID] = entry + byAppKey[key] = entry if anchor.reAnchor { reAnchors = append(reAnchors, vppReAnchor{AdamID: id.AdamID, Platform: id.Platform, CountryCode: anchor.anchorCountry}) } } entry.ids = append(entry.ids, id) }Also update the bundling and final loop to iterate over
byAppKey.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/server/service/vpp.go` around lines 1009 - 1026, The code currently keys the perApp map by adam_id (byAdamID) which causes resolveAddAnchor (and reAnchors collection) to be reused across different platforms for the same AdamID; change the map to use a composite key of adam_id and platform (e.g., byAppKey string) so anchoring is resolved per (adam_id, platform). Update the loop that builds the map (replace byAdamID with byAppKey, compute key from id.AdamID and id.Platform), create perApp entries per composite key, append to reAnchors when anchor.reAnchor for that composite entry, and then update any downstream bundling/final iteration to iterate over the new byAppKey map instead of byAdamID so ids grouping and re-anchor behavior are correct.
🧹 Nitpick comments (3)
server/mdm/apple/vpp/refresh.go (3)
76-80: Document or validate the same-country assumption.This assumes all platform rows sharing an
adamIDhave identicalCountryCode. While this holds per the anchoring design, a brief comment would clarify the invariant. Alternatively, a debug-time assertion could catch data inconsistencies early.for adamID, group := range appsByAdamID { + // All platform rows for an adamID share the same anchored country + // (set on first add); use the first row's value. anchored := group[0].CountryCode if anchored == "" { continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/mdm/apple/vpp/refresh.go` around lines 76 - 80, The loop over appsByAdamID assumes every platform row in group shares the same CountryCode (anchoring invariant); add a short clarifying comment near the loop and add a runtime check that validates this invariant (e.g., compare each row's CountryCode to group[0].CountryCode inside the for adamID, group := range appsByAdamID loop) and either log/debug-assert or return an error when a mismatch is found so data inconsistencies are caught early; reference the variables appsByAdamID, adamID, group and the CountryCode field when adding the comment and validation.
154-159: Re-anchoring is not atomic with the final insert.
UpdateVPPAppCountryCodecommits immediately (not in a transaction), whileInsertVPPAppsat line 177 is transactional. IfInsertVPPAppsfails after one or more re-anchoring updates succeed, the app'scountry_codewill reflect the new token's region but metadata (Name/IconURL/LatestVersion) will remain stale until the next successful refresh.This is likely acceptable for a background job since it self-heals on the next run. Noting for awareness in case stricter consistency is needed in the future.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/mdm/apple/vpp/refresh.go` around lines 154 - 159, Re-anchoring VPP app country is currently committed immediately by UpdateVPPAppCountryCode while InsertVPPApps runs in a transaction, causing possible inconsistent state if InsertVPPApps later fails; modify the flow so re-anchoring is performed within the same transactional boundary as InsertVPPApps—either by adding a transactional variant (e.g., UpdateVPPAppCountryCodeTx) that accepts the existing transaction/tx handle used by InsertVPPApps, or by deferring/persisting country_code updates until after a successful InsertVPPApps commit and applying them in the same transaction; update the call sites in refresh.go to use the transactional update method (or the combined transaction) so both re-anchoring and the final insert succeed or roll back together.
53-59: Consider logging the error for observability.The error from
GetAssetsis silently swallowed. While the "best effort" approach is reasonable for a background refresh, logging at warn/debug level would aid debugging when tokens fail to load assets.assets, err := GetAssets(ctx, tok.Token, nil) if err != nil { // Best effort: cache an empty set so we don't retry this token // repeatedly within one refresh run. + ctxerr.LogErr(ctx, err, fmt.Sprintf("GetAssets for token %d failed", tok.ID)) ownedByToken[tok.ID] = map[string]struct{}{} return ownedByToken[tok.ID] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/mdm/apple/vpp/refresh.go` around lines 53 - 59, GetAssets errors are currently swallowed; log the failure (including tok.ID and the error) before falling back to caching an empty map so failures are visible in logs. Locate the GetAssets call in refresh.go and replace the silent branch with a warning/debug log (e.g., logger.Warnf or similar used in this package) that includes tok.ID and err, then keep the existing ownedByToken[tok.ID] = map[string]struct{}{} and return to preserve the "best effort" behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ee/server/service/vpp.go`:
- Around line 92-114: The returned VPP token from
svc.ds.GetVPPTokenOwningAppInCountry is used without checking expiry; update the
handling so expired tokens are treated as missing: either (preferred) change the
datastore query (GetVPPTokenOwningAppInCountry) to add "AND v.renew_at > NOW()"
so expired tokens are never returned, or (if you cannot change the SQL) inspect
tok.RenewAt after retrieval and, if renew_at is in the past, behave like a
not-found case by returning the same vppAddAnchor value used for
fleet.IsNotFound (region=addingTeam.Country, fetchSecret=addingTeam.Secret,
anchorCountry=addingTeam.Country, reAnchor=true) and avoid using tok.Token for
subsequent fetches; keep the rest of the error wrapping logic intact.
In `@server/mdm/apple/vpp/refresh.go`:
- Around line 161-167: The comparison/assignment in the refresh loop is
overwriting stored app fields with empty values from Apple's response (via
ToVPPApps); modify the logic around
current.LatestVersion/current.Name/current.IconURL and
app.LatestVersion/app.Name/app.IconURL so you only treat a field as changed and
assign it when the incoming value is non-empty (e.g., non-empty string for Name
and IconURL and non-empty/valid LatestVersion); ensure appsToUpdate is only
appended when at least one non-empty incoming field actually updates the stored
app.
---
Outside diff comments:
In `@ee/server/service/vpp.go`:
- Around line 1336-1352: The code persists clientCfg.CountryCode into
fleet.VPPTokenData without ensuring it's non-empty; add a validation after
calling vpp.GetConfig (in the block that constructs VPPTokenData) to check that
clientCfg.CountryCode is not empty and return a
fleet.NewInvalidArgumentError("token", "Missing country code in VPP token
response from Apple; please provide a token with a valid country code") (or
similar wrapped via ctxerr.Wrap) if it is empty; this mirrors the existing VPP
error handling around vpp.ErrorResponse and complements ensureVPPTokenCountry
and resolveAddAnchor by rejecting the token upload early when GetConfig returns
an empty CountryCode.
---
Duplicate comments:
In `@ee/server/service/vpp.go`:
- Around line 1009-1026: The code currently keys the perApp map by adam_id
(byAdamID) which causes resolveAddAnchor (and reAnchors collection) to be reused
across different platforms for the same AdamID; change the map to use a
composite key of adam_id and platform (e.g., byAppKey string) so anchoring is
resolved per (adam_id, platform). Update the loop that builds the map (replace
byAdamID with byAppKey, compute key from id.AdamID and id.Platform), create
perApp entries per composite key, append to reAnchors when anchor.reAnchor for
that composite entry, and then update any downstream bundling/final iteration to
iterate over the new byAppKey map instead of byAdamID so ids grouping and
re-anchor behavior are correct.
---
Nitpick comments:
In `@server/mdm/apple/vpp/refresh.go`:
- Around line 76-80: The loop over appsByAdamID assumes every platform row in
group shares the same CountryCode (anchoring invariant); add a short clarifying
comment near the loop and add a runtime check that validates this invariant
(e.g., compare each row's CountryCode to group[0].CountryCode inside the for
adamID, group := range appsByAdamID loop) and either log/debug-assert or return
an error when a mismatch is found so data inconsistencies are caught early;
reference the variables appsByAdamID, adamID, group and the CountryCode field
when adding the comment and validation.
- Around line 154-159: Re-anchoring VPP app country is currently committed
immediately by UpdateVPPAppCountryCode while InsertVPPApps runs in a
transaction, causing possible inconsistent state if InsertVPPApps later fails;
modify the flow so re-anchoring is performed within the same transactional
boundary as InsertVPPApps—either by adding a transactional variant (e.g.,
UpdateVPPAppCountryCodeTx) that accepts the existing transaction/tx handle used
by InsertVPPApps, or by deferring/persisting country_code updates until after a
successful InsertVPPApps commit and applying them in the same transaction;
update the call sites in refresh.go to use the transactional update method (or
the combined transaction) so both re-anchoring and the final insert succeed or
roll back together.
- Around line 53-59: GetAssets errors are currently swallowed; log the failure
(including tok.ID and the error) before falling back to caching an empty map so
failures are visible in logs. Locate the GetAssets call in refresh.go and
replace the silent branch with a warning/debug log (e.g., logger.Warnf or
similar used in this package) that includes tok.ID and err, then keep the
existing ownedByToken[tok.ID] = map[string]struct{}{} and return to preserve the
"best effort" behavior.
🪄 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: 970a9b01-cf84-4c57-ac91-703a6708249f
📥 Commits
Reviewing files that changed from the base of the PR and between 07d5c3d48fdfc69d1ae08217510ab5125755a326 and 8837cf3d06ff27de7207f6d21e4dcb5dea26c3bb.
📒 Files selected for processing (7)
cmd/fleetctl/fleetctl/gitops_test.gocmd/fleetctl/fleetctl/testing_utils.gocmd/fleetctl/fleetctl/testing_utils/testing_utils.gocmd/fleetctl/integrationtest/gitops/software_test.goee/server/service/vpp.goserver/mdm/apple/vpp/refresh.goserver/service/integration_mdm_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- server/service/integration_mdm_test.go
- cmd/fleetctl/fleetctl/testing_utils/testing_utils.go
Member
Author
|
@claude review once |
Critical correctness fixes: - refresh.go: GetAssets errors no longer collapse to an empty owned-set, which previously made a transient Apple error indistinguishable from a legitimate "this token doesn't own the app" response and could trigger an incorrect cross-country re-anchor. The cross-country fallback now only fires when none of the anchored-country tokens errored. - ee/vpp.go: getVPPToken (install path) no longer routes through ensureVPPTokenCountry. Installs only need the secret, so they should not be coupled to Apple's /client/config availability. Country backfill now happens only in getVPPTokenInfo (add/refresh paths). - ee/vpp.go: getAnchoredVPPAppsMetadata resolves anchors per (adam_id, platform), not per adam_id. Different platforms of the same app can have diverged anchor histories, and the previous code silently reused the first platform's anchor for all platforms. Defensive correctness: - mysql/vpp.go: GetVPPTokenOwningAppInCountry now filters renew_at > NOW() so an expired token is treated as "no eligible token" and triggers a re-anchor instead of a metadata-fetch failure. - mysql/vpp.go: BackfillVPPAppCountriesFromTokens picks the lowest-id eligible token deterministically (correlated subquery with ORDER BY vt.id ASC LIMIT 1) instead of relying on MySQL's arbitrary join-row choice. - refresh.go: skip metadata update when Apple returns an empty Name or LatestVersion so a transiently-degraded response doesn't clobber valid stored values with blanks. - ee/vpp.go: explicit empty-country guard at upload and update time, defense in depth before persisting VPPTokenData. - backfill.go: bounded concurrency (semaphore size 8) on the parallel /client/config calls so a tenant with many tokens doesn't burst Apple at startup.
marko-lisica
force-pushed
the
43846-vpp-app-store-country-code
branch
from
April 29, 2026 16:00
66641ba to
5782eff
Compare
marko-lisica
changed the title
Member
|
@claude review once |
Member
|
@claude review once |
jkatz01
approved these changes
May 6, 2026
RachelElysia
approved these changes
May 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issue: Resolves #43846
Checklist for submitter
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
Database migrations
COLLATE utf8mb4_unicode_ci).Summary by CodeRabbit
New Features
Improvements
Summary by CodeRabbit
New Features
UI