DDMV: Support Fleet variables in DDM - #43222
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #43222 +/- ##
========================================
Coverage 66.92% 66.92%
========================================
Files 2600 2599 -1
Lines 208713 208862 +149
Branches 9339 9309 -30
========================================
+ Hits 139671 139788 +117
- Misses 56329 56351 +22
- Partials 12713 12723 +10
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:
|
|
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:
WalkthroughAdded support for Fleet variables in Apple DDM declarations and propagated changes across service, datastore, mock, and test layers. Datastore APIs for creating/updating declarations now accept a 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/service/mdm.go (1)
2164-2174:⚠️ Potential issue | 🟠 MajorPrevent silent variable-association key collisions.
At Line 2164 the TODO is valid:
profileVarsByProfIdentifiermixes Apple keys (Identifier) and Windows keys (Name) in one map. At Line 2320 (and earlier Line 2306), same-string collisions overwrite entries silently, which can persist wrong Fleet-variable associations.💡 Proposed safeguard (fail fast on collisions)
func validateFleetVariables(ctx context.Context, ds fleet.Datastore, appConfig *fleet.AppConfig, lic *fleet.LicenseInfo, appleProfiles map[int]*fleet.MDMAppleConfigProfile, windowsProfiles map[int]*fleet.MDMWindowsConfigProfile, appleDecls map[int]*fleet.MDMAppleDeclaration, ) (map[string][]string, error) { @@ profileVarsByProfIdentifier := make(map[string][]string) + upsertVars := func(key string, vars []string) error { + if len(vars) == 0 { + return nil + } + if _, exists := profileVarsByProfIdentifier[key]; exists { + return fleet.NewInvalidArgumentError( + "profile", + fmt.Sprintf("Couldn't set profile. Duplicate Fleet variable key collision for %q across profile types.", key), + ) + } + profileVarsByProfIdentifier[key] = vars + return nil + } for _, p := range appleProfiles { profileVars, err := validateConfigProfileFleetVariables(string(p.Mobileconfig), lic, groupedCAs) if err != nil { return nil, ctxerr.Wrap(ctx, err, "validating config profile Fleet variables") } - profileVarsByProfIdentifier[p.Identifier] = profileVars + if err := upsertVars(p.Identifier, profileVars); err != nil { + return nil, ctxerr.Wrap(ctx, err, "validating config profile Fleet variables") + } } @@ - if len(windowsVars) > 0 { - profileVarsByProfIdentifier[p.Name] = windowsVars - } + if err := upsertVars(p.Name, windowsVars); err != nil { + return nil, ctxerr.Wrap(ctx, err, "validating Windows profile Fleet variables") + } } @@ - if len(declVars) > 0 { - profileVarsByProfIdentifier[p.Identifier] = declVars - } + if err := upsertVars(p.Identifier, declVars); err != nil { + return nil, ctxerr.Wrap(ctx, err, "validating declaration Fleet variables") + } } return profileVarsByProfIdentifier, nil }Also applies to: 2310-2321
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/mdm.go` around lines 2164 - 2174, The code silently overwrites variable associations because Apple profile Identifier and Windows declaration Name share the same map key; update the map building and consumption to detect and fail-fast on collisions by including the source/platform in the key or by storing source metadata and returning an error when the same key is seen from a different source. Specifically, when populating profilesVariablesByIdentifierMap (and the earlier profileVarsByProfIdentifier usage), change the logic to either prefix keys with the platform (e.g., "apple:"+identifier vs "windows:"+name) or keep a struct value that records {key, sourcePlatform} and check for existing entries with a different platform, returning an error; ensure the downstream conversion that builds profilesVariablesByIdentifier (the loop using profilesVariablesByIdentifierMap and the conversion to fleet.MDMProfileIdentifierFleetVariables) uses the updated keys/values and surfaces the collision error instead of silently overwriting.
🧹 Nitpick comments (5)
cmd/fleetctl/fleetctl/gitops_test.go (1)
5869-5871: AssertusesFleetVarsstays empty in these OS-update tests.These cases cover standard Apple/Windows OS update declarations. As written, they would still pass if those declarations started being tagged as Fleet-variable-backed, which weakens the backwards-compatibility coverage this PR is supposed to keep.
♻️ Suggested assertion
ds.SetOrUpdateMDMAppleDeclarationFunc = func(ctx context.Context, declaration *fleet.MDMAppleDeclaration, usesFleetVars []fleet.FleetVarName) (*fleet.MDMAppleDeclaration, error) { + require.Empty(t, usesFleetVars) return &fleet.MDMAppleDeclaration{DeclarationUUID: "test-uuid"}, nil }Also applies to: 6154-6156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/fleetctl/fleetctl/gitops_test.go` around lines 5869 - 5871, The mock setter SetOrUpdateMDMAppleDeclarationFunc currently ignores the usesFleetVars argument; update that mock (and the analogous SetOrUpdateMDMWindowsDeclarationFunc used later) to assert usesFleetVars is empty by returning an error or failing the test if len(usesFleetVars) != 0, e.g., check the usesFleetVars slice at the start of SetOrUpdateMDMAppleDeclarationFunc and fail the test when it's non-empty so the OS-update tests ensure no Fleet-variable-backed flags are introduced.cmd/fleetctl/integrationtest/gitops/gitops_integration_test.go (1)
226-264: Add a no-variable control case.This only proves the free-tier apply fails; it doesn't prove the failure is caused by Fleet-variable usage specifically. A sibling case using the same DDM declaration with a literal value would make this test much more precise and avoid false positives from unrelated DDM rejection.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/fleetctl/integrationtest/gitops/gitops_integration_test.go` around lines 226 - 264, The test only verifies that applying a DDM declaration fails on free-tier but doesn't prove Fleet-variable usage is the cause; add a sibling case in TestFleetGitopsDDMFleetVarsRequiresPremium that writes or uses a second decl file (e.g., create declLiteralFile similar to declFile but with Payload.Value set to a literal string instead of "$FLEET_VAR_HOST_HARDWARE_SERIAL") and call fleetctl.RunAppNoChecks with that literal declaration (using the same fleetctlConfig and globalFile setup) and assert it does not fail with "missing or invalid license" (or assert no error), keeping the original Fleet-variable run asserting the license error; reference declFile, declLiteralFile, TestFleetGitopsDDMFleetVarsRequiresPremium and fleetctl.RunAppNoChecks to locate where to add the new case.server/service/apple_mdm_test.go (1)
6348-6350:makeDeclcurrently stringifies JSON array inputs, reducing test fidelity.Line 6349 uses
%q, so values like["$FLEET_VAR_..."]become a single JSON string, not an array. That makes the “multiple supported variables” and “all supported variables” cases less representative.♻️ Suggested test helper adjustment
- makeDecl := func(value string) string { - return fmt.Sprintf(`{"Type": "com.apple.configuration.management.test", "Identifier": "com.example.test", "Payload": {"Value": %q}}`, value) - } + makeDeclString := func(value string) string { + return fmt.Sprintf(`{"Type":"com.apple.configuration.management.test","Identifier":"com.example.test","Payload":{"Value":%q}}`, value) + } + makeDeclRaw := func(rawValue string) string { + return fmt.Sprintf(`{"Type":"com.apple.configuration.management.test","Identifier":"com.example.test","Payload":{"Value":%s}}`, rawValue) + } @@ - vars, err := validateDeclarationFleetVariables( - makeDecl(`["$FLEET_VAR_HOST_HARDWARE_SERIAL", "$FLEET_VAR_HOST_END_USER_IDP_USERNAME"]`), premiumLic) + vars, err := validateDeclarationFleetVariables( + makeDeclRaw(`["$FLEET_VAR_HOST_HARDWARE_SERIAL", "$FLEET_VAR_HOST_END_USER_IDP_USERNAME"]`), premiumLic) @@ - vars, err := validateDeclarationFleetVariables( - makeDecl("["+strings.Join(jsonVars, ", ")+"]"), premiumLic) + vars, err := validateDeclarationFleetVariables( + makeDeclRaw("["+strings.Join(jsonVars, ", ")+"]"), premiumLic)Also applies to: 6371-6373, 6384-6386
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/apple_mdm_test.go` around lines 6348 - 6350, The test helper makeDecl is currently using fmt.Sprintf with %q which JSON-escapes arrays (e.g. ["$FLEET_VAR_..."]) into a single string, breaking tests that expect actual JSON arrays; update makeDecl to insert the raw value without quoting (use %s) or better, build the payload via encoding/json (marshal a struct or map) so that inputs like ["$FLEET_VAR_..."] remain arrays; update the other occurrences noted (the similar helpers at the other locations) to use the same approach so the “multiple supported variables” and “all supported variables” cases get real JSON arrays rather than stringified arrays.cmd/fleetctl/fleetctl/apply_test.go (1)
758-761: Consider assertingusesFleetVarsin at least one path.Line 758 correctly includes the new parameter; adding a targeted assertion here would strengthen regression coverage for variable propagation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/fleetctl/fleetctl/apply_test.go` around lines 758 - 761, Add an assertion that the new parameter usesFleetVars is propagated correctly inside the mock implementation assigned to SetOrUpdateMDMAppleDeclarationFunc: in the test path where you expect certain FleetVarName values, check usesFleetVars contains (or equals) the expected slice before returning the declaration (e.g., via a test helper or t.Fatalf/t.Helper), so the mock validates variable propagation for SetOrUpdateMDMAppleDeclarationFunc.server/service/integration_mdm_ddm_test.go (1)
1744-1746: Consider adding assertion formdmDevice3after team declaration change.After uploading the new IdP username declaration to the team (Line 1730-1731), the test verifies
mdmDevice1(team member) gets DDM sync andmdmDevice2(global) doesn't. However,mdmDevice3is also on the same team and should receive the new declaration—yet there's no assertion for it here.Either add
checkDDMSync(mdmDevice3)if host3 should also receive the command, orcheckNoCommands(mdmDevice3)with a comment explaining why it wouldn't.💡 Suggested addition
// Host1 gets DDM sync (declaration set changed) checkDDMSync(mdmDevice1) checkNoCommands(mdmDevice2) + checkDDMSync(mdmDevice3) // host3 is also on the team and should receive the new declaration🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/integration_mdm_ddm_test.go` around lines 1744 - 1746, The test updated the team declaration but only asserts on mdmDevice1 and mdmDevice2; add an assertion for mdmDevice3 to reflect expected behavior after the team IdP username declaration change: either call checkDDMSync(mdmDevice3) if host3 is on the same team and should receive the DDM sync, or call checkNoCommands(mdmDevice3) with a brief comment explaining why it should not receive commands; place this new assertion alongside the existing checkDDMSync(mdmDevice1)/checkNoCommands(mdmDevice2) lines to keep the intent clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/datastore/mysql/apple_mdm.go`:
- Around line 5673-5675: The MDMAppleDDMDeclarationsToken upsert currently
preserves a stale hmad.variables_updated_at value when a declaration stops using
Fleet variables; update the upsert that writes hmad.variables_updated_at so it
explicitly clears that column (set to NULL) when the declaration no longer uses
Fleet variables (use a CASE/conditional or COALESCE with a check like "WHEN not
using variables THEN NULL ELSE <timestamp> END"), and keep the token-hash
expression (MD5(CONCAT(... GROUP_CONCAT(CONCAT(HEX(mad.token),
IFNULL(hmad.variables_updated_at, '')) ...)))) as-is so NULL becomes an empty
string for hashing; apply the same fix to the analogous upsert block referenced
in the review (lines around the second occurrence 5847-5852).
In `@server/service/apple_mdm.go`:
- Around line 1146-1149: The handler markDeclarationFailed should not rotate the
variables_updated_at timestamp when a substitution/fetch-time failure occurs;
instead of creating varsUpdatedAt = time.Now().UTC() and passing its pointer to
SetHostMDMAppleDeclarationStatus, pass nil for the varsUpdatedAt parameter so
the existing timestamp is preserved; update the call sites in
markDeclarationFailed (and any similar failure paths) to call
svc.ds.SetHostMDMAppleDeclarationStatus(ctx, hostUUID, d.DeclarationUUID,
&status, detail, nil) while keeping status and detail updates unchanged.
- Around line 917-920: The current code always allocates an empty slice for
varNames from declVars which breaks the repository convention of using nil to
indicate "no Fleet variables referenced"; update the varNames construction near
declVars so that varNames remains nil when len(declVars)==0 and only initialize
(make and append) when len(declVars)>0, and apply the same nil-vs-empty-slice
change for the other occurrence referenced in the comment (the call to
batchSetProfileVariableAssociationsDB / usesFleetVars). Ensure you keep the same
element conversion fleet.FleetVarName(v) when populating the slice.
---
Outside diff comments:
In `@server/service/mdm.go`:
- Around line 2164-2174: The code silently overwrites variable associations
because Apple profile Identifier and Windows declaration Name share the same map
key; update the map building and consumption to detect and fail-fast on
collisions by including the source/platform in the key or by storing source
metadata and returning an error when the same key is seen from a different
source. Specifically, when populating profilesVariablesByIdentifierMap (and the
earlier profileVarsByProfIdentifier usage), change the logic to either prefix
keys with the platform (e.g., "apple:"+identifier vs "windows:"+name) or keep a
struct value that records {key, sourcePlatform} and check for existing entries
with a different platform, returning an error; ensure the downstream conversion
that builds profilesVariablesByIdentifier (the loop using
profilesVariablesByIdentifierMap and the conversion to
fleet.MDMProfileIdentifierFleetVariables) uses the updated keys/values and
surfaces the collision error instead of silently overwriting.
---
Nitpick comments:
In `@cmd/fleetctl/fleetctl/apply_test.go`:
- Around line 758-761: Add an assertion that the new parameter usesFleetVars is
propagated correctly inside the mock implementation assigned to
SetOrUpdateMDMAppleDeclarationFunc: in the test path where you expect certain
FleetVarName values, check usesFleetVars contains (or equals) the expected slice
before returning the declaration (e.g., via a test helper or t.Fatalf/t.Helper),
so the mock validates variable propagation for
SetOrUpdateMDMAppleDeclarationFunc.
In `@cmd/fleetctl/fleetctl/gitops_test.go`:
- Around line 5869-5871: The mock setter SetOrUpdateMDMAppleDeclarationFunc
currently ignores the usesFleetVars argument; update that mock (and the
analogous SetOrUpdateMDMWindowsDeclarationFunc used later) to assert
usesFleetVars is empty by returning an error or failing the test if
len(usesFleetVars) != 0, e.g., check the usesFleetVars slice at the start of
SetOrUpdateMDMAppleDeclarationFunc and fail the test when it's non-empty so the
OS-update tests ensure no Fleet-variable-backed flags are introduced.
In `@cmd/fleetctl/integrationtest/gitops/gitops_integration_test.go`:
- Around line 226-264: The test only verifies that applying a DDM declaration
fails on free-tier but doesn't prove Fleet-variable usage is the cause; add a
sibling case in TestFleetGitopsDDMFleetVarsRequiresPremium that writes or uses a
second decl file (e.g., create declLiteralFile similar to declFile but with
Payload.Value set to a literal string instead of
"$FLEET_VAR_HOST_HARDWARE_SERIAL") and call fleetctl.RunAppNoChecks with that
literal declaration (using the same fleetctlConfig and globalFile setup) and
assert it does not fail with "missing or invalid license" (or assert no error),
keeping the original Fleet-variable run asserting the license error; reference
declFile, declLiteralFile, TestFleetGitopsDDMFleetVarsRequiresPremium and
fleetctl.RunAppNoChecks to locate where to add the new case.
In `@server/service/apple_mdm_test.go`:
- Around line 6348-6350: The test helper makeDecl is currently using fmt.Sprintf
with %q which JSON-escapes arrays (e.g. ["$FLEET_VAR_..."]) into a single
string, breaking tests that expect actual JSON arrays; update makeDecl to insert
the raw value without quoting (use %s) or better, build the payload via
encoding/json (marshal a struct or map) so that inputs like ["$FLEET_VAR_..."]
remain arrays; update the other occurrences noted (the similar helpers at the
other locations) to use the same approach so the “multiple supported variables”
and “all supported variables” cases get real JSON arrays rather than stringified
arrays.
In `@server/service/integration_mdm_ddm_test.go`:
- Around line 1744-1746: The test updated the team declaration but only asserts
on mdmDevice1 and mdmDevice2; add an assertion for mdmDevice3 to reflect
expected behavior after the team IdP username declaration change: either call
checkDDMSync(mdmDevice3) if host3 is on the same team and should receive the DDM
sync, or call checkNoCommands(mdmDevice3) with a brief comment explaining why it
should not receive commands; place this new assertion alongside the existing
checkDDMSync(mdmDevice1)/checkNoCommands(mdmDevice2) lines to keep the intent
clear.
🪄 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: cc6c11fe-1212-4b26-8404-a1b733896c6d
📒 Files selected for processing (32)
changes/43222-support-fleet-variables-in-ddmcmd/fleetctl/fleetctl/apply_deprecated_test.gocmd/fleetctl/fleetctl/apply_test.gocmd/fleetctl/fleetctl/get_test.gocmd/fleetctl/fleetctl/gitops_test.gocmd/fleetctl/fleetctl/testing_utils.gocmd/fleetctl/fleetctl/testing_utils/testing_utils.gocmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_test.gocmd/fleetctl/integrationtest/gitops/gitops_integration_test.goee/server/service/mdm.goee/server/service/mdm_external_test.goserver/datastore/mysql/apple_mdm.goserver/datastore/mysql/apple_mdm_ddm_test.goserver/datastore/mysql/apple_mdm_test.goserver/datastore/mysql/hosts_test.goserver/datastore/mysql/mdm.goserver/datastore/mysql/microsoft_mdm.goserver/datastore/mysql/microsoft_mdm_test.goserver/datastore/mysql/scim.goserver/datastore/mysql/secret_variables_test.goserver/datastore/mysql/teams_test.goserver/fleet/apple_mdm.goserver/fleet/apple_mdm_test.goserver/fleet/datastore.goserver/fleet/mdm.goserver/mock/datastore_mock.goserver/service/apple_mdm.goserver/service/apple_mdm_ddm_test.goserver/service/apple_mdm_test.goserver/service/integration_core_test.goserver/service/integration_mdm_ddm_test.goserver/service/mdm.go
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
server/service/apple_mdm.go:6203-6213— When host variable values change (e.g. via SCIM sync), the overall DeclarationsToken correctly incorporates variables_updated_at, causing the device to re-fetch declaration-items. However, each declaration's individual ServerToken in that manifest is always HEX(mad.token)—the static content hash—which never changes when only variable values change. Per Apple's DDM protocol, a device re-downloads a declaration only when its per-declaration ServerToken differs from its cached value; since the token is unchanged, the device uses its cached content and the updated variable values are never delivered to the device.Extended reasoning...
The bug in brief
Apple DDM uses a two-level token comparison. First the device compares the overall DeclarationsToken from the /declaration-items endpoint; if changed, it fetches the manifest. Then, for every declaration listed in the manifest, the device compares the per-declaration ServerToken with its locally cached value. Only declarations whose ServerToken has changed are re-downloaded. If the per-declaration ServerToken is unchanged, the device silently reuses its cached copy—no new network request is made.
The specific code path
In handleDeclarationItems (server/service/apple_mdm.go ~6203-6213), Fleet builds the overall DeclarationsToken by concatenating HEX(mad.token) + variables_updated_at for each declaration and MD5-hashing the result. This means the overall token correctly changes whenever variable values are recomputed. However, when the function builds the configurations/activations slice returned to the device, each entry is populated as d.ServerToken = HEX(mad.token)—the static content hash only—without incorporating variablesUpdatedAt. The struct comment even documents this limitation: "ServerToken field is just for the static token of the DDM."
Why existing code doesn't prevent it
The overall DeclarationsToken change will successfully wake the device and cause it to re-fetch the /declaration-items manifest. At that point the device sees the same per-declaration ServerToken as before (because the content hash HEX(mad.token) has not changed—only the Fleet variable values changed). The device therefore concludes it already has the latest version of every declaration and downloads nothing new. The device never requests the /declaration/configuration/ endpoint, so Fleet's correctly-rendered variable substitution is never served.
Impact
Any Fleet variable that can change after initial enrollment—HOST_END_USER_IDP_USERNAME, HOST_END_USER_IDP_USERNAME_LOCAL_PART, HOST_END_USER_IDP_GROUPS, HOST_END_USER_IDP_DEPARTMENT, HOST_END_USER_IDP_FULLNAME—will silently deliver stale values to the device even after triggerResendProfilesUsingVariables resets the declaration status and setVariablesUpdatedAtForDeclarations records a new timestamp. The feature appears to work (DDM sync is triggered, Fleet marks the declaration as installed) but the device configuration is wrong.
Why the test misses this
TestAppleDDMFleetVariables in server/service/integration_mdm_ddm_test.go directly calls s.Do("GET", "/mdm/apple/declarative/.../declaration/configuration/...", ...) to verify variable substitution. This bypasses the device-side two-level token comparison entirely—the test never exercises the path where a device decides whether to re-download a declaration. A correct end-to-end test would need to simulate the device's token-comparison logic: (1) GET declaration-items, (2) compare per-declaration ServerTokens against a cached set, (3) only fetch declarations with changed tokens.
How to fix
The per-declaration ServerToken written into the manifest should be derived from both the content hash and the variables_updated_at timestamp, for example: d.ServerToken = HEX(md5(HEX(mad.token) + IFNULL(variables_updated_at, ''))). This mirrors exactly what is already done for the overall DeclarationsToken. A simpler alternative is to concatenate them as a string (e.g. HEX(mad.token)+"_"+variables_updated_at.Unix()) before returning the declaration item.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/service/integration_mdm_ddm_test.go`:
- Around line 1446-1450: The test currently maps declarations by declaration
token (declsByToken) but the behavior under test depends on per-host ServerToken
changes driven by host_mdm_apple_declarations.variables_updated_at; update the
assertions to capture and compare the host-specific ServerToken values rather
than the declaration/token-level values: locate where declsByToken is built and
where dbDeclUUID, dbDeclSerial, dbDeclPlain are referenced and instead fetch the
ServerToken for the specific host (e.g., via the host's ServerToken field or by
querying the host_mdm_apple_declarations entry), then assert that the
ServerToken for the variable-backed declarations (dbDeclUUID, dbDeclSerial)
changes after variable resend while the ServerToken (or Plain.json) for
dbDeclPlain remains stable; apply the same change to the other occurrences
around the blocks originally at lines 1466-1470 and 1638-1642.
- Around line 1607-1616: The test currently bypasses the resend-selection logic
by directly setting host_mdm_apple_declarations.status = NULL via
mysql.ExecAdhocSQL; instead invoke the actual resend-selection code so the
junction/query path is exercised—replace the direct UPDATE with a call to the
function or SQL used by the code under test (e.g.,
triggerResendProfilesUsingVariables(ctx, ...) or the same SELECT/INSERT/UPDATE
query that marks declarations for resend) so that the logic which decides which
declarations to mark pending runs (and keep references to
host_mdm_apple_declarations.status, dbDeclUUID.DeclarationUUID,
dbDeclSerial.DeclarationUUID to locate the same rows).
- Around line 1396-1411: The tests currently only cover declWithUUID,
declWithSerial and declPlain which are JSON-safe; add a new test case (e.g.,
declWithEscaped or declWithSpecialChars) that injects a Fleet variable
containing characters that require JSON string escaping (quotes, backslashes,
newlines) and assert the substituted declaration embeds the escaped string (not
the raw unescaped value). Locate the declaration payloads near
declWithUUID/declWithSerial/declPlain and add the new payload and corresponding
assertions where other declarations are validated (also add the similar escaping
case noted for lines 1472-1490) so the code path that performs variable
substitution is verified to produce JSON-string-escaped output.
🪄 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: de34b3f2-a069-497e-bab8-f39f4dbd0b2c
📒 Files selected for processing (1)
server/service/integration_mdm_ddm_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
server/service/integration_mdm_ddm_test.go (3)
1396-1411:⚠️ Potential issue | 🟠 MajorAdd one escaping-sensitive substitution case.
These fixtures only exercise UUID/serial substitutions, which are already JSON-safe. A regression in JSON string-escaping for quotes, backslashes, or newlines would still pass this test, even though that escaping is part of the feature contract.
Also applies to: 1460-1479, 1704-1716
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/integration_mdm_ddm_test.go` around lines 1396 - 1411, The test only covers JSON-safe substitutions (declWithUUID, declWithSerial, declPlain); add an escaping-sensitive substitution case that exercises JSON string-escaping (e.g., a declaration payload that references a variable containing quotes, backslashes or newlines) so the code path that emits escaped JSON strings is validated; update the fixtures near declWithUUID/declWithSerial/declPlain (and the similar blocks at the other mentioned locations) to include a declaration using the escaping-sensitive variable substitution and assert the resulting served payload contains properly escaped sequences.
1608-1617:⚠️ Potential issue | 🟠 MajorThis still bypasses the resend-selection path under test.
Setting
host_mdm_apple_declarations.status = NULLonly checks reconciliation after rows are already pending. It never exercises the logic that decides which declarations should be resent when variables change, so the new junction/query path can regress without failing here.Also applies to: 1679-1687
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/integration_mdm_ddm_test.go` around lines 1608 - 1617, The test is bypassing the resend-selection logic by directly setting host_mdm_apple_declarations.status = NULL; instead, simulate a variable value change or invoke the real selection path: call triggerResendProfilesUsingVariables (or the public function that handles variable-change selection) or perform the same update to the variables table that triggerResendProfilesUsingVariables watches so the code path that decides which declarations to resend is exercised; update the test at the block referencing host_mdm_apple_declarations (and the similar block at 1679-1687) to trigger the selection logic instead of directly nulling status.
1495-1499:⚠️ Potential issue | 🟠 MajorAssert per-host
ServerTokenmovement, not just the collection token.The assertions here prove
DeclarationsTokenchanged, but they still don’t verify thatcom.fleet.var.uuidandcom.fleet.var.serialgot new host-specificServerTokens whilecom.fleet.plainstayed stable. A regression inEffectiveDDMTokenordeclaration-itemscould slip through.Suggested assertion shape
+ r, err = mdmDevice1.DeclarativeManagement("declaration-items") + require.NoError(t, err) + beforeItems := parseDeclarationItemsResp(t, r) + beforeTokensByID := map[string]string{} + for _, d := range beforeItems.Declarations.Configurations { + beforeTokensByID[d.Identifier] = d.ServerToken + } + // variables_updated_at for variable declarations was updated (newer) varsUpdatedUUIDAfterChange := getHostDeclVarsUpdatedAt(t, host1.UUID, dbDeclUUID.DeclarationUUID) require.NotNil(t, varsUpdatedUUIDAfterChange) assert.True(t, varsUpdatedUUIDAfterChange.After(*varsUpdatedUUID), "variables_updated_at should be newer after variable change, got %v vs original %v", varsUpdatedUUIDAfterChange, varsUpdatedUUID) @@ r, err = mdmDevice1.DeclarativeManagement("tokens") require.NoError(t, err) tokens = parseTokensResp(t, r) assert.NotEqual(t, lastSyncDeclToken, tokens.SyncTokens.DeclarationsToken) + + r, err = mdmDevice1.DeclarativeManagement("declaration-items") + require.NoError(t, err) + afterItems := parseDeclarationItemsResp(t, r) + afterTokensByID := map[string]string{} + for _, d := range afterItems.Declarations.Configurations { + afterTokensByID[d.Identifier] = d.ServerToken + } + assert.NotEqual(t, beforeTokensByID["com.fleet.var.uuid"], afterTokensByID["com.fleet.var.uuid"]) + assert.NotEqual(t, beforeTokensByID["com.fleet.var.serial"], afterTokensByID["com.fleet.var.serial"]) + assert.Equal(t, beforeTokensByID["com.fleet.plain"], afterTokensByID["com.fleet.plain"])Also applies to: 1625-1643
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/integration_mdm_ddm_test.go` around lines 1495 - 1499, The test currently only checks that the collection-level DeclarationsToken changed; update the assertions after calling mdmDevice1.DeclarativeManagement (and mirror the same change for the similar block around lines 1625-1643) to verify per-host ServerToken movement: parse the itemsResp (from parseDeclarationItemsResp) and assert that the ServerToken for the declarations with keys "com.fleet.var.uuid" and "com.fleet.var.serial" increased/changed compared to the values stored in declsByToken for that host, while the ServerToken for "com.fleet.plain" remains equal to its previous value; use the existing lastSyncDeclToken/declsByToken fixtures to compare prior vs current host-specific ServerToken values rather than only checking the top-level DeclarationsToken.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/service/apple_mdm.go`:
- Around line 6303-6317: The JSON unmarshal failure after
replaceDeclarationFleetVariables isn't being handled as a host-level declaration
failure, so parse errors return an error to the device instead of marking the
declaration failed for that host; update the error path after json.Unmarshal (in
the same block that handles replaceDeclarationFleetVariables) to call
svc.markDeclarationFailed(ctx, hostUUID, d, err.Error()) and return nil, nil if
markDeclarationFailed succeeds (wrap and return the markDeclarationFailed error
if it fails), mirroring the existing handling for
replaceDeclarationFleetVariables; reference replaceDeclarationFleetVariables,
json.Unmarshal, and svc.markDeclarationFailed to locate the code to change.
In `@server/service/integration_mdm_ddm_test.go`:
- Around line 1795-1799: The test only asserts variables_updated_at is non-nil
for VarUUID but does not confirm VarSerial's timestamp remained unchanged;
update the test to record the pre-update variables_updated_at for the VarSerial
declaration (use getHostDeclVarsUpdatedAt for VarSerial.json for host1 and
host3), perform the VarUUID update, then assert the post-update
variables_updated_at for VarSerial equals the previously recorded timestamps
(i.e., unchanged). Apply the same change to the analogous assertions around the
VarSerial/VarUUID check at the later block (the 1845-1848 area).
---
Duplicate comments:
In `@server/service/integration_mdm_ddm_test.go`:
- Around line 1396-1411: The test only covers JSON-safe substitutions
(declWithUUID, declWithSerial, declPlain); add an escaping-sensitive
substitution case that exercises JSON string-escaping (e.g., a declaration
payload that references a variable containing quotes, backslashes or newlines)
so the code path that emits escaped JSON strings is validated; update the
fixtures near declWithUUID/declWithSerial/declPlain (and the similar blocks at
the other mentioned locations) to include a declaration using the
escaping-sensitive variable substitution and assert the resulting served payload
contains properly escaped sequences.
- Around line 1608-1617: The test is bypassing the resend-selection logic by
directly setting host_mdm_apple_declarations.status = NULL; instead, simulate a
variable value change or invoke the real selection path: call
triggerResendProfilesUsingVariables (or the public function that handles
variable-change selection) or perform the same update to the variables table
that triggerResendProfilesUsingVariables watches so the code path that decides
which declarations to resend is exercised; update the test at the block
referencing host_mdm_apple_declarations (and the similar block at 1679-1687) to
trigger the selection logic instead of directly nulling status.
- Around line 1495-1499: The test currently only checks that the
collection-level DeclarationsToken changed; update the assertions after calling
mdmDevice1.DeclarativeManagement (and mirror the same change for the similar
block around lines 1625-1643) to verify per-host ServerToken movement: parse the
itemsResp (from parseDeclarationItemsResp) and assert that the ServerToken for
the declarations with keys "com.fleet.var.uuid" and "com.fleet.var.serial"
increased/changed compared to the values stored in declsByToken for that host,
while the ServerToken for "com.fleet.plain" remains equal to its previous value;
use the existing lastSyncDeclToken/declsByToken fixtures to compare prior vs
current host-specific ServerToken values rather than only checking the top-level
DeclarationsToken.
🪄 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: fea1d28d-1545-42de-8c82-a4de7e28f69e
📒 Files selected for processing (4)
server/datastore/mysql/apple_mdm.goserver/fleet/apple_mdm.goserver/service/apple_mdm.goserver/service/integration_mdm_ddm_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- server/datastore/mysql/apple_mdm.go
|
Note that there's one bug found in QA that I will fix in a follow-up PR : we can't return an empty payload in the /declaration/configuration/ endpoint, the host doesn't like it and it replies with a failure that overrides the failed error that we want to surface (E.g. host has no IdP user). Instead, this check should be done in the activation phase, and not send the DDM profile to activate it it can't be delivered. It's already a big PR with most of it working properly, so I'll address that in a follow-up. |
JordanMontgomery
left a comment
JordanMontgomery
left a comment
There was a problem hiding this comment.
This looks good. The one thing that might be worth adding(perhaps in the followup PR already opened) could be a specific, explicit test that EffectiveDDMToken matches the mysql representation for a given value, though the existing tests implicitly check that.
<!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #43047 Follow-up to #43222 # Checklist for submitter - [x] Input data is properly validated, `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 - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually See #42960 (comment) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved Apple MDM declaration handling: declarations with unresolved per-device variables are now attempted per host, marked failed when resolution fails, and omitted from device configuration/activation manifests. * Declarations that fail resolution still factor into declaration token computation to keep token behavior consistent. * **Tests** * Updated tests to reflect per-device resolution failures and adjusted validation flow. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Related issue: Resolves #43047
Checklist for submitter
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Input data is properly validated,
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
Added/updated automated tests
Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)
QA'd all new/changed functionality manually
See Support Fleet variables in declaration (DDM) profiles #42960 (comment) and subsequent comments.
Summary by CodeRabbit
New Features
Bug Fixes
Tests