Android Wi-Fi profile withheld until cert installed on device - #42877
Merged
Conversation
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR ensures Android Wi-Fi (ONC) profiles that reference a client certificate alias are not delivered until the referenced certificate is in a terminal state on the device, and adds GitOps-time validation to catch mismatched ClientCertKeyPairAlias references.
Changes:
- Withhold Android
openNetworkConfigurationprofiles that referenceClientCertKeyPairAliasuntil the matching certificate isverified(or terminalfailed) on the host. - Add
fleetctl gitopsvalidation that ONCClientCertKeyPairAliasvalues match certificate template names defined in the same GitOps config. - Add unit/integration tests for ONC alias extraction, withholding behavior, and GitOps validation.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/mdm/android/service/profiles.go | Adds ONC/certificate withholding during Android profile reconciliation. |
| server/datastore/mysql/host_certificate_templates.go | Adds datastore query to fetch cert template statuses by name for a host. |
| server/fleet/datastore.go | Extends datastore interface with cert-status lookup by host/name. |
| server/mock/datastore_mock.go | Updates mock datastore to support new interface method. |
| server/mdm/android/onc.go | Adds minimal ONC parsing + alias extraction helpers. |
| server/mdm/android/onc_test.go | Tests ONC/profile alias extraction. |
| server/mdm/android/service/profiles_filter_test.go | Tests filtering/withholding behavior given cert statuses. |
| server/mdm/android/service/profiles_test.go | Adds reconciler-level test covering withheld→released behavior. |
| server/service/client.go | Adds GitOps validation for ONC alias ↔ certificate template name matching. |
| server/service/client_onc_validation_test.go | Tests GitOps ONC alias validation. |
| server/service/integration_android_certificate_templates_test.go | Adds end-to-end integration test for withholding until cert verified. |
| changes/42405-android-onc-after-cert | Adds user-visible change notes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
WalkthroughWithholds Android ONC profiles that reference certificates via Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/host_certificate_templates.go`:
- Around line 179-195: The query in the stmt that selects from
host_certificate_templates and builds result :=
map[string]fleet.CertificateTemplateStatus is missing a filter to only consider
install operations, causing nondeterministic overwrites when multiple rows for
the same template exist; update the SQL (the stmt used with sqlx.SelectContext)
to add a WHERE clause (or extend the existing WHERE) that restricts
hct.operation (or the operation column name used in host_certificate_templates)
to the install action so that the rows slice and the loop populating
result[r.Name] = fleet.CertificateTemplateStatus(r.Status) only reflect install
rows.
In `@server/mdm/android/service/profiles.go`:
- Around line 170-176: The code currently "fails open" when
r.DS.GetCertificateTemplateStatusesByNameForHost returns an error; instead of
proceeding you must withhold affected profiles: when
GetCertificateTemplateStatusesByNameForHost returns err, log the error and move
all candidate profiles (profilesToMerge) into withheldProfiles and clear
profilesToMerge (or return an error to abort sending) so no ONC profiles are
merged/sent; keep the normal path using
filterProfilesWithPendingCerts(certStatuses) when there is no error.
- Around line 463-466: The code currently treats errors from
android.ExtractCertAliasesFromProfileJSON(content) as "ready" by appending prof
to ready; change this to fail-closed: if err != nil then do not append prof and
instead return the error (or mark the profile as not ready) so ONC parsing
failures block delivery; only append prof to ready when err == nil and
len(aliases) > 0. Ensure you update the handling around
ExtractCertAliasesFromProfileJSON, ready, and prof accordingly.
In `@server/service/client.go`:
- Around line 3259-3263: Call validateONCCertificateReferences earlier in the
GitOps flow so ONC alias validation runs before any writes: move the invocation
out of the certificate application path and into the start of DoGitOps (or
immediately before ApplyGroup) so it executes before the zero-cert early-return
path that skips processing when android_settings.certificates is empty; ensure
validateONCCertificateReferences(config, certificates) is invoked even when the
certificates slice is empty so invalid ClientCertKeyPairAlias values are caught
and returned before any ApplyGroup or persistence occurs.
🪄 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: 1e9f0570-b601-4821-850d-8e94f86ac92f
📒 Files selected for processing (12)
changes/42405-android-onc-after-certserver/datastore/mysql/host_certificate_templates.goserver/fleet/datastore.goserver/mdm/android/onc.goserver/mdm/android/onc_test.goserver/mdm/android/service/profiles.goserver/mdm/android/service/profiles_filter_test.goserver/mdm/android/service/profiles_test.goserver/mock/datastore_mock.goserver/service/client.goserver/service/client_onc_validation_test.goserver/service/integration_android_certificate_templates_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #42877 +/- ##
==========================================
+ Coverage 66.85% 66.87% +0.01%
==========================================
Files 2581 2583 +2
Lines 206998 207056 +58
Branches 9294 9279 -15
==========================================
+ Hits 138386 138462 +76
+ Misses 56041 56022 -19
- Partials 12571 12572 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
server/mdm/android/onc.go (1)
42-53: Consider de-duplicating extracted aliases.If the same alias appears in multiple network blocks, returning unique aliases avoids redundant status checks downstream.
♻️ Proposed refactor
func ExtractCertAliasesFromONC(oncJSON json.RawMessage) ([]string, error) { var onc oncConfig if err := json.Unmarshal(oncJSON, &onc); err != nil { return nil, err } var aliases []string + seen := make(map[string]struct{}) + addAlias := func(alias string) { + if alias == "" { + return + } + if _, ok := seen[alias]; ok { + return + } + seen[alias] = struct{}{} + aliases = append(aliases, alias) + } + for _, nc := range onc.NetworkConfigurations { - if nc.WiFi != nil && nc.WiFi.EAP != nil && nc.WiFi.EAP.ClientCertKeyPairAlias != "" { - aliases = append(aliases, nc.WiFi.EAP.ClientCertKeyPairAlias) + if nc.WiFi != nil && nc.WiFi.EAP != nil { + addAlias(nc.WiFi.EAP.ClientCertKeyPairAlias) } - if nc.Ethernet != nil && nc.Ethernet.EAP != nil && nc.Ethernet.EAP.ClientCertKeyPairAlias != "" { - aliases = append(aliases, nc.Ethernet.EAP.ClientCertKeyPairAlias) + if nc.Ethernet != nil && nc.Ethernet.EAP != nil { + addAlias(nc.Ethernet.EAP.ClientCertKeyPairAlias) } - if nc.VPN != nil && nc.VPN.ClientCertKeyPairAlias != "" { - aliases = append(aliases, nc.VPN.ClientCertKeyPairAlias) + if nc.VPN != nil { + addAlias(nc.VPN.ClientCertKeyPairAlias) } } return aliases, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/mdm/android/onc.go` around lines 42 - 53, The loop that appends ClientCertKeyPairAlias values from onc.NetworkConfigurations into the aliases slice can produce duplicates; change the collection to deduplicate by using a temporary set (e.g., map[string]struct{}) keyed by the alias while iterating over nc.WiFi, nc.Ethernet, and nc.VPN ClientCertKeyPairAlias values, then build the final aliases slice from the set (or replace with a helper like uniqueStrings) so aliases contains only unique entries before returning or using it.server/service/integration_android_certificate_templates_test.go (1)
1752-1767: Strengthen “applied” assertions with policy inclusion fields.Right now “applied” is inferred from
status=pendingand detail text only. Consider assertingpolicy_request_uuidand/orincluded_in_policy_versionare non-NULL so this can’t pass on false positives.Also applies to: 1795-1803
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/integration_android_certificate_templates_test.go` around lines 1752 - 1767, The test currently infers a profile was applied by checking profileStatuses[0].Status == "pending" and the Detail text; strengthen this by querying and asserting the database fields that indicate policy inclusion—update the SELECT in the mysql.ExecAdhocSQL call (the query used to populate profileStatuses) or add a separate query against host_mdm_android_profiles to fetch policy_request_uuid and included_in_policy_version for the same host/profile, then add require.NotNil (or equivalent) assertions on those fields for the entries (e.g., the camera-policy and other profile entries) so the test verifies policy_request_uuid and/or included_in_policy_version are non-NULL instead of relying solely on Status and Detail.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@frontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingsTable.stories.tsx`:
- Around line 7-16: Lint errors come from empty async/no-op callbacks in the
story (e.g., noop and the NotificationWrapper's renderFlash, renderMultiFlash,
hideFlash), so replace their empty bodies with minimal returned values or
Storybook action calls: for noop return Promise.resolve(undefined) (or use
action('noop') if using `@storybook/addon-actions`), and have
NotificationWrapper's functions return a value (e.g., renderFlash: (msg)=>msg or
Promise.resolve) or call Storybook actions so they are non-empty and satisfy
no-empty-function lint rule; update references to noop and NotificationWrapper
accordingly.
- Line 13: The multiline object literal for the notification prop is not
Prettier-formatted; reformat the object/array literals (e.g., the notification:
{ alertType: null, isVisible: false, message: null, persistOnPageChange: false }
entry in the OSSettingsTable story) to follow Prettier's multiline wrapping
rules (one property per line, proper indentation and trailing commas where
required) so the file passes formatting checks; run Prettier or your project's
formatter on OSSettingsTable.stories.tsx and apply the same formatting to the
other reported location around line 169.
In `@server/mdm/android/service/androidmgmt/google_client.go`:
- Around line 171-173: Remove the redundant shadowing assignment inside the loop
over t.Fields(); instead of reassigning f with `f := f`, use the loop variable
directly (the for loop over t.Fields() already provides a fresh variable per
iteration in Go 1.26.1), i.e., update the loop in the code that iterates over
t.Fields() so the closure-capture mitigation (`f := f`) is removed and
subsequent uses such as the call to f.Tag.Lookup("json") reference the loop
variable directly.
In `@server/mdm/android/service/profiles_test.go`:
- Line 1310: The struct literal sets IncludedInPolicyVersion: new(1), which is
invalid because new expects a type; replace it with a pointer-to-int value
(e.g., create an int variable and take its address, use a small helper like
intPtr(1), or inline func(i int) *int { return &i }(1)) so
IncludedInPolicyVersion is a *int; update the test in profiles_test.go where the
struct with IncludedInPolicyVersion is constructed.
In `@server/mdm/android/service/pubsub_test.go`:
- Line 323: The code uses invalid Go syntax new(1)/new(2) to create integer
pointers (e.g., policyVersion := new(1)); replace these with the helper that
returns *int (e.g., policyVersion := ptr.Int(1) or ptr.Int(2)) across the test
file, updating each occurrence where an int pointer is needed so the code
compiles; look for variables like policyVersion and other test vars using
new(1)/new(2) and switch them to ptr.Int(value).
---
Nitpick comments:
In `@server/mdm/android/onc.go`:
- Around line 42-53: The loop that appends ClientCertKeyPairAlias values from
onc.NetworkConfigurations into the aliases slice can produce duplicates; change
the collection to deduplicate by using a temporary set (e.g.,
map[string]struct{}) keyed by the alias while iterating over nc.WiFi,
nc.Ethernet, and nc.VPN ClientCertKeyPairAlias values, then build the final
aliases slice from the set (or replace with a helper like uniqueStrings) so
aliases contains only unique entries before returning or using it.
In `@server/service/integration_android_certificate_templates_test.go`:
- Around line 1752-1767: The test currently infers a profile was applied by
checking profileStatuses[0].Status == "pending" and the Detail text; strengthen
this by querying and asserting the database fields that indicate policy
inclusion—update the SELECT in the mysql.ExecAdhocSQL call (the query used to
populate profileStatuses) or add a separate query against
host_mdm_android_profiles to fetch policy_request_uuid and
included_in_policy_version for the same host/profile, then add require.NotNil
(or equivalent) assertions on those fields for the entries (e.g., the
camera-policy and other profile entries) so the test verifies
policy_request_uuid and/or included_in_policy_version are non-NULL instead of
relying solely on Status and Detail.
🪄 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: 267fbdff-73d3-4a3c-833b-a4be5169fe29
📒 Files selected for processing (18)
.storybook/main.tschanges/42405-android-onc-after-certfrontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingsErrorCell/OSSettingsErrorCell.tsxfrontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingsTable.stories.tsxserver/datastore/mysql/host_certificate_templates.goserver/fleet/datastore.goserver/mdm/android/onc.goserver/mdm/android/onc_test.goserver/mdm/android/service/androidmgmt/google_client.goserver/mdm/android/service/endpoint_utils.goserver/mdm/android/service/profiles.goserver/mdm/android/service/profiles_filter_test.goserver/mdm/android/service/profiles_test.goserver/mdm/android/service/pubsub.goserver/mdm/android/service/pubsub_test.goserver/mdm/android/service/service.goserver/mock/datastore_mock.goserver/service/integration_android_certificate_templates_test.go
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/mdm/android/service/profiles.go (2)
178-234:⚠️ Potential issue | 🟠 MajorWithheld profiles not persisted on max-failure early-return path.
When
setFailCount >= maxRequestFailures(line 190), the function returns at line 234 without adding withheld profiles tobulkProfilesByUUID. These profiles lose their "Waiting for certificate..." status until the next reconcile.The
withheldProfilesslice is populated at line 182 before this check, but they're only added tobulkProfilesByUUIDat lines 394-404, which is after this early return.💡 Suggested fix
Add withheld profiles before returning on max-failure path:
} + // Persist withheld ONC profiles even on failure path + for _, prof := range withheldProfiles { + status := fleet.MDMDeliveryPending + bulkProfilesByUUID[prof.ProfileUUID] = &fleet.MDMAndroidProfilePayload{ + HostUUID: hostUUID, + Status: &status, + OperationType: fleet.MDMOperationTypeInstall, + ProfileUUID: prof.ProfileUUID, + ProfileName: prof.ProfileName, + Detail: prof.Detail, + } + } return slices.Collect(maps.Values(bulkProfilesByUUID)), nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/mdm/android/service/profiles.go` around lines 178 - 234, The early-return when setFailCount >= maxRequestFailures omits persisting withheldProfiles, causing their "Waiting for certificate..." status to be lost; before returning in that branch (the block using setFailCount, maxRequestFailures, and building bulkProfilesByUUID), iterate withheldProfiles and add each to bulkProfilesByUUID with HostUUID, OperationType (use prof.OperationType or infer install/remove), Status pointing to the waiting/certificate-pending state (same representation used later for withheld handling), ProfileUUID/ProfileName and appropriate Detail, then return as currently done so withheldProfiles are persisted even on the max-failure fast path.
340-341:⚠️ Potential issue | 🟠 MajorWithheld profiles not persisted on patchPolicy failure path.
Similar to the max-failure path, when
patchPolicyReqFailedis true (line 340), the function returns at line 341 without persisting withheld profiles. Add them before this early return as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/mdm/android/service/profiles.go` around lines 340 - 341, The early return when patchPolicyReqFailed is true skips persisting withheld profiles; locate the same persistence logic used on the "max-failure" path (the code that writes/flushes withheld profiles) and invoke it just before the return in the branch that checks patchPolicyReqFailed (the block that currently returns slices.Collect(maps.Values(bulkProfilesByUUID)), nil). Ensure you call the identical persistence routine (the function/logic that handles withheld profiles) using the same inputs (e.g., bulkProfilesByUUID or the withheld subset) so withheld profiles are saved before returning.
♻️ Duplicate comments (1)
server/mdm/android/service/profiles.go (1)
460-471:⚠️ Potential issue | 🟠 MajorParse errors bypass ONC certificate gating.
When
ExtractCertAliasesFromProfileJSONfails, the profile is excluded fromresultand will pass throughfilterProfilesWithPendingCertsas "ready" (line 484-486). This delivers potentially invalid ONC profiles.Consider withholding profiles with extraction errors or treating them as failed:
💡 Suggested approach
aliases, err := android.ExtractCertAliasesFromProfileJSON(content) if err != nil { - // Should not happen since profiles are validated on upload. - logger.ErrorContext(ctx, "failed to extract ONC cert aliases from profile", "profile.uuid", profileUUID, "err", err) - ctxerr.Handle(ctx, err) - continue + // Mark for withholding or failure - don't deliver a potentially broken ONC + return nil, ctxerr.Wrapf(ctx, err, "extract ONC cert aliases from profile %s", profileUUID) }Or mark the profile for withholding by setting a sentinel value in the map.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/mdm/android/service/profiles.go` around lines 460 - 471, When ExtractCertAliasesFromProfileJSON(profile content) returns an error, do not just skip the profile; instead mark it as withheld by inserting a sentinel value into result keyed by profileUUID (e.g., define a package-level constant like CERT_ALIAS_EXTRACTION_ERROR_SENTINEL and set result[profileUUID] = []string{CERT_ALIAS_EXTRACTION_ERROR_SENTINEL}) so downstream filterProfilesWithPendingCerts can detect and treat it as not-ready/failed. Keep the existing logger.ErrorContext and ctxerr.Handle calls, remove the continue-only behavior, and ensure the sentinel is documented so other code can check for it.
🧹 Nitpick comments (3)
server/platform/logging/testutils/test_logger.go (1)
26-28: Consider trimming trailing newline to avoid double-spacing.
slog.TextHandlerappends a newline to each log record, andt.Logalso adds its own newline. This will produce extra blank lines between log entries in test output.🔧 Optional fix to trim trailing newline
+import ( + "bytes" + "log/slog" + "testing" +) + func (w tLogWriter) Write(p []byte) (int, error) { - w.t.Log(string(p)) + w.t.Log(string(bytes.TrimRight(p, "\n"))) return len(p), nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/platform/logging/testutils/test_logger.go` around lines 26 - 28, The test logger's Write method (tLogWriter.Write) passes the raw byte slice to t.Log, which combined with slog.TextHandler's appended newline causes blank lines; update tLogWriter.Write to trim trailing newline(s) from p (e.g., remove trailing '\n' and/or '\r\n') before converting to string and calling w.t.Log, then return the original len(p) and nil as before so callers see the same byte count.server/service/integration_android_certificate_templates_test.go (1)
1778-1793: Exercise the real release path instead of forcing DB state.This test switches the certificate row to
verifiedand nullshost_mdm_android_profiles.statusdirectly, so it still passes if the service stops requeueing withheld profiles when a certificate becomes terminal. Driving the transition through the fleetd status endpoint would validate the intended end-to-end behavior and makes it easy to cover the terminalfailedpath too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/integration_android_certificate_templates_test.go` around lines 1778 - 1793, The test is directly mutating DB rows (UPDATE on host_certificate_templates and host_mdm_android_profiles via mysql.ExecAdhocSQL) instead of exercising the real release path; change it to drive the certificate state transition through the fleetd status endpoint (the same endpoint used in production to mark a certificate as verified/failed) for the host UUID used in the test so the service requeues withheld Android profiles naturally, then call s.awaitTriggerAndroidProfileSchedule(t) and assert expected behavior; also add a subcase that posts a terminal failed status via the fleetd status endpoint to cover the failed path instead of nulling host_mdm_android_profiles.status directly.server/mdm/android/service/profiles_filter_test.go (1)
48-86: Coverdeliveringanddeliveredin the blocker matrix.The contract here is “withhold until terminal”, but this table only proves
pendingblocks andverified/failedrelease. A regression that treatsdeliveringordeliveredas ready would slip through this suite.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/mdm/android/service/profiles_filter_test.go` around lines 48 - 86, The tests for filter currently assert that CertificateTemplateStatus pending blocks and verified/failed release, but they miss non-terminal states delivering and delivered; add two test cases (e.g., "delivering cert withholds ONC profile" and "delivered cert withholds ONC profile") that call oncProfile("p1", "wifi-profile", "my-cert") and pass certStatuses with "my-cert": fleet.CertificateTemplateDelivering and fleet.CertificateTemplateDelivered respectively, then assert ready is empty and withheld contains the expected Waiting for certificate "my-cert" detail; this ensures filter handles delivering/delivered like pending until a terminal status (verified/failed).
🤖 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/fleet/datastore.go`:
- Around line 2780-2782: The current contract for
RequeueWithheldONCProfilesForHost is too broad; update its implementation so the
SQL UPDATE only targets rows that are ONC installs currently waiting on a
certificate (i.e., narrow the WHERE clause to status = the certificate-waiting
sentinel such as "withheld_onc_waiting_cert" and the ONC/install kind
discriminator such as profile_type/install_kind = "ONC", plus host_uuid =
hostUUID) instead of all pending Android profiles; adjust the query and any used
constants/enum names in the function to reflect this precise scoping.
---
Outside diff comments:
In `@server/mdm/android/service/profiles.go`:
- Around line 178-234: The early-return when setFailCount >= maxRequestFailures
omits persisting withheldProfiles, causing their "Waiting for certificate..."
status to be lost; before returning in that branch (the block using
setFailCount, maxRequestFailures, and building bulkProfilesByUUID), iterate
withheldProfiles and add each to bulkProfilesByUUID with HostUUID, OperationType
(use prof.OperationType or infer install/remove), Status pointing to the
waiting/certificate-pending state (same representation used later for withheld
handling), ProfileUUID/ProfileName and appropriate Detail, then return as
currently done so withheldProfiles are persisted even on the max-failure fast
path.
- Around line 340-341: The early return when patchPolicyReqFailed is true skips
persisting withheld profiles; locate the same persistence logic used on the
"max-failure" path (the code that writes/flushes withheld profiles) and invoke
it just before the return in the branch that checks patchPolicyReqFailed (the
block that currently returns slices.Collect(maps.Values(bulkProfilesByUUID)),
nil). Ensure you call the identical persistence routine (the function/logic that
handles withheld profiles) using the same inputs (e.g., bulkProfilesByUUID or
the withheld subset) so withheld profiles are saved before returning.
---
Duplicate comments:
In `@server/mdm/android/service/profiles.go`:
- Around line 460-471: When ExtractCertAliasesFromProfileJSON(profile content)
returns an error, do not just skip the profile; instead mark it as withheld by
inserting a sentinel value into result keyed by profileUUID (e.g., define a
package-level constant like CERT_ALIAS_EXTRACTION_ERROR_SENTINEL and set
result[profileUUID] = []string{CERT_ALIAS_EXTRACTION_ERROR_SENTINEL}) so
downstream filterProfilesWithPendingCerts can detect and treat it as
not-ready/failed. Keep the existing logger.ErrorContext and ctxerr.Handle calls,
remove the continue-only behavior, and ensure the sentinel is documented so
other code can check for it.
---
Nitpick comments:
In `@server/mdm/android/service/profiles_filter_test.go`:
- Around line 48-86: The tests for filter currently assert that
CertificateTemplateStatus pending blocks and verified/failed release, but they
miss non-terminal states delivering and delivered; add two test cases (e.g.,
"delivering cert withholds ONC profile" and "delivered cert withholds ONC
profile") that call oncProfile("p1", "wifi-profile", "my-cert") and pass
certStatuses with "my-cert": fleet.CertificateTemplateDelivering and
fleet.CertificateTemplateDelivered respectively, then assert ready is empty and
withheld contains the expected Waiting for certificate "my-cert" detail; this
ensures filter handles delivering/delivered like pending until a terminal status
(verified/failed).
In `@server/platform/logging/testutils/test_logger.go`:
- Around line 26-28: The test logger's Write method (tLogWriter.Write) passes
the raw byte slice to t.Log, which combined with slog.TextHandler's appended
newline causes blank lines; update tLogWriter.Write to trim trailing newline(s)
from p (e.g., remove trailing '\n' and/or '\r\n') before converting to string
and calling w.t.Log, then return the original len(p) and nil as before so
callers see the same byte count.
In `@server/service/integration_android_certificate_templates_test.go`:
- Around line 1778-1793: The test is directly mutating DB rows (UPDATE on
host_certificate_templates and host_mdm_android_profiles via mysql.ExecAdhocSQL)
instead of exercising the real release path; change it to drive the certificate
state transition through the fleetd status endpoint (the same endpoint used in
production to mark a certificate as verified/failed) for the host UUID used in
the test so the service requeues withheld Android profiles naturally, then call
s.awaitTriggerAndroidProfileSchedule(t) and assert expected behavior; also add a
subcase that posts a terminal failed status via the fleetd status endpoint to
cover the failed path instead of nulling host_mdm_android_profiles.status
directly.
🪄 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: 2fffe80b-c02d-4922-9485-b3f3792d7342
📒 Files selected for processing (19)
.storybook/main.tschanges/42405-android-onc-after-certfrontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingsErrorCell/OSSettingsErrorCell.tsxfrontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingsTable.stories.tsxserver/datastore/mysql/host_certificate_templates.goserver/datastore/mysql/host_certificate_templates_test.goserver/fleet/datastore.goserver/fleet/host_certificate_template.goserver/mdm/android/onc.goserver/mdm/android/onc_test.goserver/mdm/android/service/androidmgmt/google_client.goserver/mdm/android/service/endpoint_utils.goserver/mdm/android/service/profiles.goserver/mdm/android/service/profiles_filter_test.goserver/mdm/android/service/profiles_test.goserver/mock/datastore_mock.goserver/platform/logging/testutils/test_logger.goserver/service/certificates.goserver/service/integration_android_certificate_templates_test.go
![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.
ksykulev
reviewed
Apr 7, 2026
| canRotateRecoveryLockPassword && (isFailed || isVerified); | ||
| const value = (isFailed && profile.detail) || DEFAULT_EMPTY_CELL_VALUE; | ||
| const value = | ||
| ((isFailed || isPending) && profile.detail) || DEFAULT_EMPTY_CELL_VALUE; |
Contributor
There was a problem hiding this comment.
Do we have to do any filtering for macos/windows here? They also use similar profile statuses and can be resent, correct?
Member
Author
There was a problem hiding this comment.
I thought about special casing Android here, but I didn't see anything in the codebase that set a detail status for pending items. So, it seemed like doing the general approach was simpler and less code.
Contributor
There was a problem hiding this comment.
also worst case it shows a stale message
| SELECT hct.host_uuid, ct.name, hct.status | ||
| FROM host_certificate_templates hct | ||
| JOIN certificate_templates ct ON ct.id = hct.certificate_template_id | ||
| WHERE hct.host_uuid IN (?) AND hct.operation_type = ? |
Contributor
There was a problem hiding this comment.
I believe MySQL will use the unique key on (host_uuid, certificate_template_id) to satisfy the host_uuid IN (?), then evaluate operation_type = ? as a post-filter on the matched rows, since there is not index on host_certificate_templates on (host_uuid, operation_type). But this feels like over engineering here, and we probably will never have any performance issues.
Member
Author
There was a problem hiding this comment.
Agreed. Should not be an issue with small number of templates per host. But we do need to have a real load test for Android hosts.
…3080) <!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #42405 Also, fixed bug with this edge case: Certificate removed while ONC profile is withheld # Checklist for submitter - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually
getvictor
marked this pull request as draft
April 7, 2026 18:31
getvictor
marked this pull request as ready for review
April 7, 2026 20:18
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.
Member
Author
|
@ksykulev This is ready for re-review. I removed the unnecessary DB calls and also added the gitops ordering change. |
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
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 #42405
Demo video: https://www.youtube.com/watch?v=F3nfFvwdj-c
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.Testing
Summary by CodeRabbit