Repository navigation
Serve custom DDM activations to devices - #50421
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #50421 +/- ##
==========================================
+ Coverage 68.34% 68.36% +0.01%
==========================================
Files 3953 3954 +1
Lines 253180 253644 +464
Branches 13530 13530
==========================================
+ Hits 173031 173398 +367
- Misses 64678 64728 +50
- Partials 15471 15518 +47
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:
|
5f23eb0 to
5bc4b34
Compare
e85b0a6 to
e2217c7
Compare
e2217c7 to
3d0b76d
Compare
3d0b76d to
38e43c5
Compare
38e43c5 to
c1cfa58
Compare
c1cfa58 to
967d83e
Compare
|
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:
WalkthroughApple DDM declarations now carry activation timestamps, identifiers, declaration types, and activation content. Effective declaration tokens include activation timestamps. Datastore queries load and persist activation metadata and resolve stored or generated activations by host and scope. The service separates configuration and management declarations, expands activation variables, validates endpoint types, and maps predicate and activation status results. Reconciliation reinstalls declarations when activation metadata changes. Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/fleet/apple_mdm_test.go (1)
717-750: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case that covers the activation timestamp.
Every call passes
nilforactivationUpdatedAt, so the new branch inEffectiveDDMTokenis never exercised here. The comment on Line 719 also still lists onlyvariables_updated_atandassets_updated_at.
server/service/apple_mdm_ddm_test.gocovers SQL-vs-Go parity end to end, so this is a unit-level gap only.♻️ Proposed test additions
const staticToken = "abc123" vars := time.Date(2026, 7, 9, 12, 0, 0, 0, time.UTC) assets := time.Date(2026, 7, 10, 8, 30, 0, 0, time.UTC) + activation := time.Date(2026, 7, 11, 9, 15, 0, 0, time.UTC) // md5Hex mirrors the hashing done by EffectiveDDMToken so we can assert the // exact concatenation order, which MUST match the SQL token computation in - // MDMAppleDDMDeclarationsToken: HEX(token) + variables_updated_at + assets_updated_at. + // MDMAppleDDMDeclarationsToken: + // HEX(token) + variables_updated_at + assets_updated_at + activation_updated_at.t.Run("different asset timestamps yield different tokens", func(t *testing.T) { later := assets.Add(time.Second) require.NotEqual(t, EffectiveDDMToken(staticToken, nil, &assets, nil), EffectiveDDMToken(staticToken, nil, &later, nil)) }) + + t.Run("only activation", func(t *testing.T) { + got := EffectiveDDMToken(staticToken, nil, nil, &activation) + require.Equal(t, md5Hex(staticToken, activation.Format(layout)), got) + require.NotEqual(t, staticToken, got) + }) + + t.Run("all three, order is static+vars+assets+activation", func(t *testing.T) { + require.Equal(t, + md5Hex(staticToken, vars.Format(layout), assets.Format(layout), activation.Format(layout)), + EffectiveDDMToken(staticToken, &vars, &assets, &activation)) + })🤖 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/fleet/apple_mdm_test.go` around lines 717 - 750, Add unit coverage for the activationUpdatedAt branch in EffectiveDDMToken by updating the md5Hex documentation and expected concatenation order to include activation_updated_at, then add cases for activation-only and combined timestamp inputs, including a differing activation timestamp producing a different token.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/mdm/apple/reconcile.go`:
- Around line 250-253: Update the existing-declaration reconcile logic near the
ActivationUpdatedAt handling to detect when a custom activation is cleared, and
mark the affected host declaration as needing resync or bump its host token.
Ensure this path updates the relevant resync/token state so the host starts a
new DDM session after setMDMAppleDDMActivationDB removes the activation.
---
Nitpick comments:
In `@server/fleet/apple_mdm_test.go`:
- Around line 717-750: Add unit coverage for the activationUpdatedAt branch in
EffectiveDDMToken by updating the md5Hex documentation and expected
concatenation order to include activation_updated_at, then add cases for
activation-only and combined timestamp inputs, including a differing activation
timestamp producing a different token.
🪄 Autofix
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 Plus
Run ID: 60a454bc-202e-452c-a1b7-cf1c569a548f
📒 Files selected for processing (10)
server/datastore/mysql/apple_mdm.goserver/datastore/mysql/apple_mdm_batched.goserver/fleet/apple_mdm.goserver/fleet/apple_mdm_test.goserver/fleet/datastore.goserver/mdm/apple/reconcile.goserver/mock/datastore_mock.goserver/service/apple_mdm.goserver/service/apple_mdm_ddm_test.goserver/service/integration_mdm_ddm_test.go
|
@coderabbitai full review |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/service/apple_mdm_ddm_test.go (1)
705-716: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the activation identifier that this declaration delivers.
Line 706 creates
configIdent + ".custom". The declaration at lines 707-712 has no custom activation. It therefore uses the synthesized identifierdecl.DeclarationUUID + ".activation".This fixture does not represent a device report for the declaration under test. Use the synthesized identifier, or persist a custom activation with the
.customidentifier.Proposed fix
- activationIdent := configIdent + ".custom" decl, err := ds.NewMDMAppleDeclaration(ctx, &fleet.MDMAppleDeclaration{ Name: "PredDecl-" + suffix, Identifier: configIdent, RawJSON: []byte(`{"Type":"com.apple.configuration.test","Identifier":"` + configIdent + `","Payload":{"Enabled":true}}`), Scope: fleet.PayloadScopeSystem, }, nil) require.NoError(t, err) + activationIdent := decl.DeclarationUUID + ".activation" token := insertHostDeclaration(t, hostUUID, decl.DeclarationUUID, "pending", "install", decl.Identifier)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/service/apple_mdm_ddm_test.go` around lines 705 - 716, Update the test fixture around NewMDMAppleDeclaration and c.report so activationIdent matches the declaration’s actual activation identifier: use the synthesized decl.DeclarationUUID + ".activation" unless the declaration is changed to persist a custom ".custom" activation. Ensure the reported identifier represents the activation delivered by the declaration under test.
🧹 Nitpick comments (1)
server/mdm/apple/reconcile_test.go (1)
598-659: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for a newer activation timestamp.
The test does not cover the
d.ActivationUpdatedAtnewer-than-host branch inComputeDeclarationDeltas. Add a case where the desired timestamp advances. Assert that it creates an install row and stores the newerActivationUpdatedAt.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/mdm/apple/reconcile_test.go` around lines 598 - 659, Add a subtest near the existing activation cases that sets the desired declaration’s ActivationUpdatedAt to a timestamp newer than the host’s value, then calls ComputeDeclarationDeltas and asserts the device is changed, exactly one install row is produced, and rows[0].ActivationUpdatedAt equals the newer desired timestamp.
🤖 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.
Outside diff comments:
In `@server/service/apple_mdm_ddm_test.go`:
- Around line 705-716: Update the test fixture around NewMDMAppleDeclaration and
c.report so activationIdent matches the declaration’s actual activation
identifier: use the synthesized decl.DeclarationUUID + ".activation" unless the
declaration is changed to persist a custom ".custom" activation. Ensure the
reported identifier represents the activation delivered by the declaration under
test.
---
Nitpick comments:
In `@server/mdm/apple/reconcile_test.go`:
- Around line 598-659: Add a subtest near the existing activation cases that
sets the desired declaration’s ActivationUpdatedAt to a timestamp newer than the
host’s value, then calls ComputeDeclarationDeltas and asserts the device is
changed, exactly one install row is produced, and rows[0].ActivationUpdatedAt
equals the newer desired timestamp.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 391ecb42-3937-453e-9790-262dadca4655
📒 Files selected for processing (3)
server/mdm/apple/reconcile.goserver/mdm/apple/reconcile_test.goserver/service/apple_mdm_ddm_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- server/mdm/apple/reconcile.go
There was a problem hiding this comment.
Pull request overview
This PR extends Fleet’s Apple DDM sync pipeline to serve custom activations and management declarations to devices, ensuring activations are advertised/fetched by their own identifiers, that activation changes trigger re-sync via tokens, and that status reporting correctly reflects predicate outcomes.
Changes:
- Serve custom activation JSON (with predicate preserved) via a dedicated activation endpoint, expanding secrets and host/Fleet variables at delivery time.
- Add
activation_updated_atinto per-host effective tokens and the manifest-level token so activation-only edits trigger re-sync. - Route
com.apple.management.*declarations under the manifest’sManagementsection with a dedicated endpoint and updated status grading (including predicate correlation via activation identifiers).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/apple_mdm.go | Manifest generation now includes Management and activation identifiers; activation/config endpoints updated; status mapping updated for predicates and management declarations. |
| server/datastore/mysql/apple_mdm.go | SQL updated to fold activation_updated_at into token computation; declaration-items query includes activation metadata; new activation fetch query added. |
| server/datastore/mysql/apple_mdm_batched.go | Reconciler input now loads activation timestamps and marks declarations as depending on activation Fleet vars. |
| server/mdm/apple/reconcile.go | Reconciler now triggers installs on activation timestamp changes/removal and persists activation_updated_at on host declaration rows. |
| server/fleet/apple_mdm.go | Data model updated: new activation_updated_at, activation delivery struct, token function signature extended, generated activation suffix constant added. |
| server/fleet/datastore.go | Datastore interface extended with MDMAppleDDMActivationResponse. |
| server/mock/datastore_mock.go | Datastore mock extended to support MDMAppleDDMActivationResponse. |
| server/service/apple_mdm_ddm_test.go | Adds coverage for token agreement, custom activation serving/scoping, management routing/guard, and predicate status mapping. |
| server/mdm/apple/reconcile_test.go | Adds tests ensuring activation changes/removal trigger reconcile deltas appropriately. |
| server/fleet/apple_mdm_test.go | Updates EffectiveDDMToken tests and HostDeclaration.Equal tests for the new activation timestamp field. |
| server/service/integration_mdm_ddm_test.go | Updates integration test expectations for the expanded EffectiveDDMToken signature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
MagnusHJensen
left a comment
MagnusHJensen
left a comment
There was a problem hiding this comment.
A couple of things to look at here to further perfect it, but else overall looking good.
|
Thanks for another round @MagnusHJensen I have addressed the copilot and your feedback points. Can you take a look, please? |
MagnusHJensen
left a comment
MagnusHJensen
left a comment
There was a problem hiding this comment.
We currently still have two issues in the entire flow.
IDP variable resend triggers does not actually check on activation variable association and therefore does not stamp the host declaration row for resend in the reconciler to fire a new IDP variable updated profile. Method triggerResendProfilesUsingVariables
Host vitals resend is also not accounting for activations. Method resendMDMProfilesForCustomHostVital
Sorry for doing these in chunks. There is a lot of mental modal to keep here and various paths to exercise so I'm doing them bit by bit when I find them.
|
No worries @MagnusHJensen I am able to build the context and build the mental model myself, so all good there. I have addressed the feedback, can you take a look, please? |
Can you clarify? I'm unsure what this means in response to my message |
|
Sorry it was not clear. The response was for -
As in, you don't need to be sorry 😅 The reviews are helping me even if they are in chunks I have made the changes as per your review. But the review in turn is helping me to look at the entry points that I missed earlier like IdP changes, host vital changes. |
Makes the stored activations from #49970 actually reach hosts. The manifest advertises a custom activation's own identifier instead of the generated one, and the fetch serves the stored JSON with its predicate. Secrets and host variables are expanded at delivery like configurations. Generated activations are now named after the declaration UUID rather than its identifier, so an admin-chosen identifier can't collide with one Fleet invents. That means one lookup instead of a match-then-trim fallback. Note this differs from the CoS wording of "byte-for-byte identical to today" — following Magnus's Slack instead, same as we did for base64. activation_updated_at folds into the token in all three places it's computed (SQL, EffectiveDDMToken, and the manifest builder), so editing a predicate re-syncs even though the declaration itself didn't change. Sourced from GREATEST(uploaded_at, secrets_updated_at) so a secret re-stamp counts too. Kept variables on variables_updated_at rather than folding them in here, so each signal stays independently comparable and the reconciler stays idempotent. Management declarations are served under the manifest's Management section with their own endpoint, and report status. Predicate outcomes map per the Figma dev note: Info.Predicate is verified with an explanation, Error.ActivationFailed is failed.
The status report query didn't select activation_updated_at, so the token Fleet computed for matching never included it while the one it served did. Every status report was silently dropped and profiles sat in verifying forever. Checked the other three EffectiveDDMToken call sites too, they were fine. Apple splits a predicate outcome across two arrays: the activation reports Info.Predicate, the configuration it gates reports Error.ActivationFailed. We only read the configuration, so a host the predicate simply excluded came back as failed. Now correlated via the activation identifier Apple puts in the failure details. Test payloads are the real reports a macOS 26 host sent rather than hand-written ones, since both bugs came from guessing at the shape.
A null document would otherwise leave it nil and panic on write.
The delta only looked for a newer activation timestamp, so deleting one left the host serving the old activation and its predicate indefinitely. Unlike an asset reference, the activation lives in its own table, so the declaration's token doesn't change to catch it.
Apple documents that a management declaration's active state is always false and not part of the activation process, so grading it like a configuration left every one of them stuck on verifying. Found testing com.apple.management.organization-info on a real Mac.
- Grade configurations and management declarations in separate passes, and fail a management declaration that reports errors while still unknown - Serve a custom activation under its own token, so editing the declaration or an asset it references no longer re-syncs the activation - Include management declarations when resolving referenced assets - Preflight the activation for Fleet variables and scan it for host vitals - Fix swapped declaration_name/declaration_identifier and add the missing scope on the host declaration insert
The manifest carried the activation's identifier, token and body as extra columns on every declaration row, which made the activation's variables get checked against the declaration's variables_updated_at. A variable that lived only in the activation was therefore never detected, so it was never preflighted and its value never re-synced. Custom activations now come from their own query, and the manifest is built in three passes: installable declarations, activations, then configurations and management.
The activation's own token only covers its content and secrets, so a new variable value moved the declaration's token but not the activation's -- the host re-fetched the configuration and kept the stale activation. Fold the declaration's variables_updated_at into it, on both the manifest and delivery sides. The IdP and host vital resend triggers had the same blind spot: neither looked at an activation's variables, so a value change never marked the declaration for resend.
bec6f0b to
f645f84
Compare
JordanMontgomery
left a comment
JordanMontgomery
left a comment
There was a problem hiding this comment.
I'm not really finding any issues and I think this is good to go. I was most worried about the reconciler/generated activation identifier changes but digging in I think they look good
Related issue: Resolves #49971
Serves the custom activations stored by #49970 to devices. Stacked on #50280.
activation_updated_atfolds into the token, so editing only a predicate re-syncs the host even though the declaration's content didn't change.Managementsection, have their own endpoint, and report status.Info.Predicate→ verified with an explanation,Error.ActivationFailed→ failed.Three things to look at
Generated activations are renamed to
<declaration uuid>.activation. Removes the collision between an admin-chosen identifier and one Fleet generates, and collapses the fetch into a single lookup rather than match-then-trim. Note this differs from the CoS wording of "byte-for-byte identical to today".Existing hosts will see their activations renamed once. The sync token doesn't move for the rename alone — activation identifiers aren't part of it, confirmed in
MDMAppleDDMDeclarationsToken— so it happens lazily on the next real DDM change rather than all at once.activation_updated_atis not themax()the issue describes. The issue asks formax(activation uploaded_at, latest variable change). Variable changes stay onvariables_updated_at; this isGREATEST(uploaded_at, secrets_updated_at). Same outcome, sinceEffectiveDDMTokenhashes all the timestamps together, but each signal stays independently comparable, which is what keeps the reconciler idempotent.The token is computed in three places, not two. SQL in
MDMAppleDDMDeclarationsToken, Go inEffectiveDDMToken, and a third independent copy building the manifest-level token inhandleDeclarationItems. All three now carry the new term in the same order.ActivationUpdatedAtFoldsIntoTokenasserts the SQL and Go agree with it populated — they only agreed for the nil case before.Checklist for submitter
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
Tested on an ADE-enrolled Mac (macOS 26.5.1) against a local server: the custom activation is advertised and fetched by its own identifier, an activation-only edit re-syncs the device even though the declaration is unchanged, a true predicate enforces the policy on the host, and a false one comes back verified with the explanation.
That turned up two bugs, both fixed here and both invisible to the test suite:
MDMAppleStoreDDMStatusReportcomputed the matching token withactivation_updated_atbut never selected the column, so it was always nil. Every status report was silently dropped and profiles sat in verifying forever.Info.Predicate, the configuration it gates reportsError.ActivationFailed. Reading only the configuration made a host the predicate excluded look failed. Now correlated via the activation identifier in the failure details.The test payloads for the second one are the real reports the Mac sent, not hand-written.
Note for anyone testing in the UI: the OS settings tooltip for a verified profile is hardcoded to "The host applied the setting. Fleet verified." and doesn't read
detailyet, so a predicate-excluded profile reads wrong there. Backend stores the right message; rendering it is #49973.New cases in
apple_mdm_ddm_test.go: the token agreement above; a custom activation advertised and served with its predicate, with the generated name no longer resolving for it; a declaration without one still getting the synthesized activation; management routing plus the endpoint type guard; the three predicate status outcomes; and a host outside a declaration's scope getting an empty manifest and an error fetching the activation by name.13 existing assertions moved from
<identifier>.activationto<uuid>.activation.The
GeneratedActivationStillServedWhenNoCustomOnecase caught a real bug on its first run: theLEFT JOINreturns NULL for declarations without a custom activation, whichjson.RawMessagecan't scan, so every non-custom activation fetch would have 500'd. That's the no-regression path.Summary by CodeRabbit
New Features
Bug Fixes