Clear Android cert records on unenroll. - #42920
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Pull request overview
Clears Android host certificate template tracking records when an Android device is unenrolled, preventing stale certificate template statuses from persisting across re-enrollment (Resolves #42600).
Changes:
- Delete
host_certificate_templatesrows during Android unenroll (single-host and bulk unenroll paths). - Extend datastore + integration tests to assert certificate template records are removed on unenroll.
- Add a changelog entry describing the bug fix.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| server/datastore/mysql/android.go | Deletes Android host certificate template rows when hosts are unenrolled (single + bulk paths). |
| server/datastore/mysql/android_test.go | Adds/extends tests to verify cert template rows are removed on unenroll. |
| server/service/integration_android_certificate_templates_test.go | Updates integration scenario to cover verified template rows being cleared on unenroll. |
| changes/42600-android-cert-templates-cleared-on-reenroll | User-visible changelog entry for the fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WalkthroughThis PR fixes a bug where Android host certificate template records persisted in the database after device unenrollment, resulting in stale certificate statuses on re-enrollment. Changes include adding deletion logic for Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/service/integration_android_certificate_templates_test.go (1)
688-693: Consider relaxing the post-unenroll assertion to “nil OR empty.”The behavior under test is “no profiles remain”; asserting strictly
nilmay be brittle if response serialization later returns[].💡 Optional assertion tweak
- require.Nil(t, getHostResp.Host.MDM.Profiles, "All certificate template records should be cleared on unenroll") + require.True(t, + getHostResp.Host.MDM.Profiles == nil || len(*getHostResp.Host.MDM.Profiles) == 0, + "All certificate template records should be cleared on unenroll", + )🤖 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 688 - 693, The test currently asserts require.Nil(t, getHostResp.Host.MDM.Profiles) which is brittle; change the assertion to accept either nil or an empty slice so the test validates "no profiles remain" regardless of JSON serialization. Update the assertion around getHostResp.Host.MDM.Profiles (in the getHostResponse usage after s.DoJSON) to check that either getHostResp.Host.MDM.Profiles == nil OR len(getHostResp.Host.MDM.Profiles) == 0 (e.g., using require.Truef or require.Empty) so the test passes for both nil and [] outcomes.
🤖 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/android.go`:
- Around line 424-438: The deletion of host_certificate_templates is currently
executed using hostUUID without validating it; add a guard to ensure hostUUID is
non-empty (and return/unwrap a contextual error) before calling tx.ExecContext
for the DELETE to avoid accidental mass-deletes. Locate the hostUUID variable
retrieval and, after sqlx.GetContext populates hostUUID (and after the call to
ds.deleteMDMOSCustomSettingsForHost), validate hostUUID is not the empty string
(or otherwise corrupted) and return a ctxerr.Wrap error if it is; only then call
tx.ExecContext(`DELETE FROM host_certificate_templates WHERE host_uuid = ?`,
hostUUID).
---
Nitpick comments:
In `@server/service/integration_android_certificate_templates_test.go`:
- Around line 688-693: The test currently asserts require.Nil(t,
getHostResp.Host.MDM.Profiles) which is brittle; change the assertion to accept
either nil or an empty slice so the test validates "no profiles remain"
regardless of JSON serialization. Update the assertion around
getHostResp.Host.MDM.Profiles (in the getHostResponse usage after s.DoJSON) to
check that either getHostResp.Host.MDM.Profiles == nil OR
len(getHostResp.Host.MDM.Profiles) == 0 (e.g., using require.Truef or
require.Empty) so the test passes for both nil and [] outcomes.
🪄 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: e507c1f3-3112-428b-8a86-5677e9bb755e
📒 Files selected for processing (4)
changes/42600-android-cert-templates-cleared-on-reenrollserver/datastore/mysql/android.goserver/datastore/mysql/android_test.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.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #42920 +/- ##
=======================================
Coverage 66.81% 66.81%
=======================================
Files 2543 2543
Lines 204301 204341 +40
Branches 9237 9237
=======================================
+ Hits 136510 136538 +28
- Misses 55414 55421 +7
- Partials 12377 12382 +5
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:
|
Related issue: Resolves #42600
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