Added variables in Android configuration profiles - #47750
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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR adds Possibly related issues
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #47750 +/- ##
========================================
Coverage 67.23% 67.23%
========================================
Files 3634 3637 +3
Lines 229815 230072 +257
Branches 11967 11928 -39
========================================
+ Hits 154517 154695 +178
- Misses 61423 61482 +59
- Partials 13875 13895 +20
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Adds support for $FLEET_VAR_HOST_* variable usage in Android configuration profiles (and Android managed app configurations), including validation, per-host substitution during delivery, and database tracking of which variables are used by Android profiles.
Changes:
- Validate/allow-list Fleet variables in Android configuration profiles and Android app configurations, including rejecting unsupported variables.
- Substitute supported Fleet variables per-host when delivering Android profiles and when applying Android managed app configurations.
- Track Android profile ↔ Fleet variable associations in
mdm_configuration_profile_variablesvia a newandroid_profile_uuidcolumn + FK/unique index, with migration + tests.
Reviewed changes
Copilot reviewed 16 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/worker/software_worker.go | Adds per-host substitution for Android app managed configurations (including a per-host policy application path). |
| server/service/mdm.go | Includes Android profiles in Fleet variable discovery/tracking during BatchSetMDMProfiles. |
| server/service/mdm_test.go | Updates batch profile tests to reflect Android variable validation behavior. |
| server/service/integration_android_software_test.go | Adds integration coverage for supported vs unsupported variables in Android app configs (single + batch). |
| server/mdm/profiles/profile_variables.go | Refactors IDP variable resolution into a reusable resolver for non-XML contexts. |
| server/mdm/profiles/android_appconfig.go | Implements JSON-safe Fleet variable substitution and host/IDP resolution for Android JSON configs. |
| server/mdm/profiles/android_appconfig_test.go | Unit tests for Android JSON variable substitution + escaping behavior. |
| server/mdm/android/service/profiles.go | Substitutes Fleet variables per-host before merging/sending Android profiles; surfaces delivery failures on unresolvable vars. |
| server/fleet/android.go | Validates supported variables and enforces “variables must be inside JSON string values” for Android profiles. |
| server/fleet/android_test.go | Adds tests for Android app config validation and Android profile variable validation. |
| server/datastore/mysql/schema.sql | Updates schema for Android profile variable tracking column/index/FK + migration status. |
| server/datastore/mysql/migrations/tables/20260616222258_AddAndroidProfileVariableTracking.go | Migration adding android_profile_uuid to variable association table with constraints. |
| server/datastore/mysql/migrations/tables/20260616222258_AddAndroidProfileVariableTracking_test.go | Migration test verifying column, uniqueness, check constraint, and cascade delete. |
| server/datastore/mysql/mdm.go | Enables variable association batching for Android profiles (uses new column). |
| server/datastore/mysql/android.go | Expands Android host lite query to include platform + hardware serial for substitution. |
| ee/server/service/vpp.go | Validates Android app configuration payloads during batch association. |
Files excluded by content exclusion policy (2)
- changes/41968-android-profile-fleet-variables
- changes/45353-android-var-software-configs
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for hostUUID := range hosts { | ||
| h, ok := hostByUUID[hostUUID] | ||
| if !ok { | ||
| continue // host may have been deleted since the job was queued | ||
| } |
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.
Files excluded by content exclusion policy (1)
- changes/41968-android-profile-fleet-variables
![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/android/service/profiles.go (1)
682-726: 💤 Low valueMinor optimization opportunity:
AndroidHostLiteByHostUUIDmay be called twice for the same host.When profiles contain variables (line 703) and the policy subsequently changes (line 456),
AndroidHostLiteByHostUUIDis invoked twice for the same host within a singlesendHostProfilescall.This is functionally correct, but if you want to avoid the duplicate lookup, you could refactor to fetch the host data once at the top of
sendHostProfilesand reuse it. Given that this is a lightweight lookup and only occurs when both conditions are true, this is a low-priority refinement.🤖 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/android/service/profiles.go` around lines 682 - 726, The `substituteProfileVarsForHost` function calls `AndroidHostLiteByHostUUID` to fetch the Android host data, but this lookup is also performed elsewhere in the `sendHostProfiles` function, resulting in a duplicate call for the same host. To optimize this, refactor `sendHostProfiles` to fetch the Android host data once at the beginning of the function and pass the result to `substituteProfileVarsForHost` as an additional parameter instead of having it perform its own lookup. Update the function signature of `substituteProfileVarsForHost` to accept the pre-fetched Android host object and use it directly, eliminating the duplicate `AndroidHostLiteByHostUUID` call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@server/datastore/mysql/migrations/tables/20260616222258_AddAndroidProfileVariableTracking_test.go`:
- Around line 45-46: The INSERT statement in the test is using an invalid
apple_profile_uuid value 'a-fake' which causes a foreign key constraint failure
before the CHECK constraint ck_mdm_configuration_profile_variables_exactly_one
can be validated. To properly test the CHECK constraint, replace the 'a-fake'
value with either a valid apple_profile_uuid that exists in the database or NULL
(depending on what the constraint actually requires), so the FK constraint
passes and the actual CHECK constraint validation is what gets tested.
In `@server/fleet/android.go`:
- Around line 136-141: The current validation using stringVars map only checks
if a variable name exists somewhere in the parsed JSON, but fails to reject
variables that appear in JSON keys even when the same variable also appears in a
string value. To fix this, modify the walkJSONForStringVars function to only
track variables found in JSON string values and exclude any variables found in
keys, or add separate validation logic to explicitly check that variables do not
appear as keys in the JSON structure before accepting them. This ensures the
"string value only" rule is properly enforced. Apply the same fix to the second
occurrence of this validation pattern at lines 147-156.
---
Nitpick comments:
In `@server/mdm/android/service/profiles.go`:
- Around line 682-726: The `substituteProfileVarsForHost` function calls
`AndroidHostLiteByHostUUID` to fetch the Android host data, but this lookup is
also performed elsewhere in the `sendHostProfiles` function, resulting in a
duplicate call for the same host. To optimize this, refactor `sendHostProfiles`
to fetch the Android host data once at the beginning of the function and pass
the result to `substituteProfileVarsForHost` as an additional parameter instead
of having it perform its own lookup. Update the function signature of
`substituteProfileVarsForHost` to accept the pre-fetched Android host object and
use it directly, eliminating the duplicate `AndroidHostLiteByHostUUID` call.
🪄 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: e19ce4dc-2ae7-476d-b023-fda830d396f4
📒 Files selected for processing (11)
changes/41968-android-profile-fleet-variablesserver/datastore/mysql/mdm.goserver/datastore/mysql/migrations/tables/20260616222258_AddAndroidProfileVariableTracking.goserver/datastore/mysql/migrations/tables/20260616222258_AddAndroidProfileVariableTracking_test.goserver/datastore/mysql/schema.sqlserver/fleet/android.goserver/fleet/android_test.goserver/mdm/android/service/profiles.goserver/service/mdm.goserver/service/mdm_test.goserver/worker/software_worker.go
getvictor
left a comment
getvictor
left a comment
There was a problem hiding this comment.
Looks good. Let a couple non-blocking comments.
| hostProfilesContents, varSubErr := substituteProfileVarsForHost(ctx, r.DS, hostUUID, profilesContents) | ||
| if varSubErr != nil { | ||
| if !errors.Is(varSubErr, profiles.ErrUnresolvableAndroidAppConfigVar) { | ||
| return nil, ctxerr.Wrapf(ctx, varSubErr, "substitute fleet vars for host %s", hostUUID) |
There was a problem hiding this comment.
So if 1 profile has a bad var, then none get delivered? Is this desired/expected behavior?
There was a problem hiding this comment.
Oddly enough that's how it works right now. I would be open to changing that behavior because it's certainly strange to me as well. Thoughts?
| found := variables.Find(string(rawJSON)) | ||
| if len(found) == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| for _, name := range found { | ||
| if !slices.Contains(FleetVarsSupportedInAndroidAppConfig, FleetVarName(name)) { | ||
| return fmt.Errorf("Couldn't edit profile. Unsupported Fleet variable $FLEET_VAR_%s.", name) | ||
| } | ||
| } |
There was a problem hiding this comment.
Nit. Feels like this part (find variables and check them against a list) is generic and can be reused across the codebase.
| } | ||
|
|
||
| // walkJSONForVars recursively walks a decoded JSON value and collects fleet | ||
| // variable names found in string values and in map keys separately. |
There was a problem hiding this comment.
Will there be a doc update that for Android variables behave differently than for Apple DDM, where they can be anywhere?
There was a problem hiding this comment.
Android profile keys must be real androidmanagement.Policy field names like "name", "wifiConfigsLockdownEnabled", "maximumTimeToLock", etc. So $FLEET_VAR_HOST_* won't ever match that, not before substitution, not after.
I can definitely update the docs so it's not confusing. I'll create a docs PR.
Related issue: Resolves #41968
Checklist for submitter
If some of the following don't apply, delete the relevant line.
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
$FLEET_VAR_HOST_*) in Android configuration profiles, enabling per-host dynamic value substitution during deployment.Improvements