Deleting/adding Android certs to host on team transfer#37616
Deleting/adding Android certs to host on team transfer#37616
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #37616 +/- ##
==========================================
+ Coverage 65.82% 65.84% +0.02%
==========================================
Files 2367 2366 -1
Lines 187665 187663 -2
Branches 8012 7900 -112
==========================================
+ Hits 123523 123564 +41
+ Misses 52845 52796 -49
- Partials 11297 11303 +6
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:
|
getvictor
had a problem deploying
to
Docker Hub
GitHub Actions
Failure
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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: 0
🧹 Nitpick comments (3)
server/mdm/android/android.go (1)
62-66: Consider using typed enums (or at least constants) forStatusandOperation.
AgentCertificateTemplate.Statusand.Operationare plain strings, while the rest of the codebase already has strongly-typedMDMDeliveryStatus/MDMOperationType. Wiring these fields from those types (or exposing them as those types withstringJSON tags) would reduce the chance of drifting/invalid values being sent to the Android agent.server/worker/software_worker.go (1)
470-521: Cert template update order is good; consider transactional grouping for failure safety.Running
SetHostCertificateTemplatesToPendingRemoveForHostand thenCreatePendingCertificateTemplatesForNewHostper host before building the managed config is logically sound for team transfers and keeps the agent config aligned with cert templates. The only robustness gap is that these two datastore calls are independent: if the second fails after the first succeeds, the host ends up with templates marked for removal but no new pending ones for the destination team.If/when feasible, consider wrapping these two operations in a single DB transaction (or adding a compensating path) in the datastore implementation so the host’s certificate state transitions atomically.
server/datastore/mysql/host_certificate_templates_test.go (1)
1032-1082: Template-scoped removal test now encodesfailed-row deletion semantics; update docs accordingly.The updated
"deletes pending and failed rows and updates others to pending remove"test clearly asserts that:
- Rows in
pendingorfailedstatus are deleted for the given template ID.- Remaining rows (e.g., delivered/verified) are flipped to
status=pendingandoperation_type=remove.That behavior is reasonable, but it’s now stricter than the brief Datastore comment that only mentions deleting
pendingrows and updating “all other rows.” When you touch the implementation next, consider aligning theSetHostCertificateTemplatesToPendingRemovedoc comment inserver/fleet/datastore.gowith this test so callers knowfailedrows are removed rather than transitioned.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
server/datastore/mysql/host_certificate_templates.goserver/datastore/mysql/host_certificate_templates_test.goserver/fleet/certificate_templates.goserver/fleet/datastore.goserver/mdm/android/android.goserver/mdm/android/service/profiles_test.goserver/mdm/android/service/service.goserver/mock/datastore_mock.goserver/service/integration_android_certificate_templates_test.goserver/worker/software_worker.goserver/worker/software_worker_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
When reviewing SQL queries that are added or modified, ensure that appropriate filtering criteria are applied—especially when a query is intended to return data for a specific entity (e.g., a single host). Check for missing WHERE clauses or incorrect filtering that could lead to incorrect or non-deterministic results (e.g., returning the first row instead of the correct one). Flag any queries that may return unintended results due to lack of precise scoping.
Files:
server/mdm/android/service/profiles_test.goserver/worker/software_worker_test.goserver/datastore/mysql/host_certificate_templates_test.goserver/service/integration_android_certificate_templates_test.goserver/mock/datastore_mock.goserver/datastore/mysql/host_certificate_templates.goserver/fleet/certificate_templates.goserver/fleet/datastore.goserver/mdm/android/android.goserver/mdm/android/service/service.goserver/worker/software_worker.go
🧠 Learnings (5)
📚 Learning: 2025-10-03T18:16:11.482Z
Learnt from: MagnusHJensen
Repo: fleetdm/fleet PR: 33805
File: server/service/integration_mdm_test.go:1248-1251
Timestamp: 2025-10-03T18:16:11.482Z
Learning: In server/service/integration_mdm_test.go, the helper createAppleMobileHostThenEnrollMDM(platform string) is exclusively for iOS/iPadOS hosts (mobile). Do not flag macOS model/behavior issues based on changes within this helper; macOS provisioning uses different helpers such as createHostThenEnrollMDM.
Applied to files:
server/mdm/android/service/profiles_test.goserver/worker/software_worker_test.goserver/service/integration_android_certificate_templates_test.goserver/mdm/android/service/service.goserver/worker/software_worker.go
📚 Learning: 2025-12-11T23:02:52.191Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 36978
File: server/mdm/android/service/profiles.go:441-449
Timestamp: 2025-12-11T23:02:52.191Z
Learning: In server/mdm/android/service, Go allows assigning an interface value of type fleet.Datastore to a field ds of type fleet.Datastore because the underlying dynamic type implements both the embedded AndroidDatastore (via fleet.Datastore embedding AndroidDatastore). When reviewing code in profiles.go and related files, verify assignments between interfaces respect embedding and ensure the concrete type actually implements all required interfaces. This pattern is valid wherever fleet.Datastore embeds AndroidDatastore and the receiver expects fleetDS or similar fields; prefer keeping interface boundaries clear and rely on the dynamic type to satisfy both interfaces.
Applied to files:
server/mdm/android/service/profiles_test.goserver/mdm/android/service/service.go
📚 Learning: 2025-07-08T16:11:49.555Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 30589
File: ee/server/service/hostidentity/depot/depot.go:115-115
Timestamp: 2025-07-08T16:11:49.555Z
Learning: In ee/server/service/hostidentity/depot/depot.go, the error from result.RowsAffected() is intentionally ignored because the information is only used for logging purposes, not for critical program logic.
Applied to files:
server/datastore/mysql/host_certificate_templates_test.go
📚 Learning: 2025-07-08T16:06:54.576Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 30589
File: ee/server/service/hostidentity/depot/depot.go:104-119
Timestamp: 2025-07-08T16:06:54.576Z
Learning: In ee/server/service/hostidentity/depot/depot.go, the security concern where shared challenges allow certificate revocation (lines 104-119) is a known issue that will be addressed in a later feature, not an immediate concern to fix.
Applied to files:
server/datastore/mysql/host_certificate_templates_test.go
📚 Learning: 2025-11-26T18:58:18.865Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 36139
File: android/app/src/main/java/com/fleetdm/agent/scep/ScepClientImpl.kt:75-76
Timestamp: 2025-11-26T18:58:18.865Z
Learning: In Fleet's Android MDM agent SCEP implementation (android/app/src/main/java/com/fleetdm/agent/scep/ScepClientImpl.kt), OptimisticCertificateVerifier is intentionally used because: (1) SCEP URL is provided by authenticated MDM server, (2) challenge password authenticates enrollment, (3) enterprise SCEP servers use internal CAs not in system trust stores, (4) enrolled certificate is validated when used.
Applied to files:
server/service/integration_android_certificate_templates_test.go
🧬 Code graph analysis (8)
server/mdm/android/service/profiles_test.go (3)
server/mdm/android/android.go (2)
AgentManagedConfiguration(55-60)AgentCertificateTemplate(62-66)server/fleet/certificate_templates.go (5)
CertificateTemplateDelivering(51-51)CertificateTemplateStatus(47-47)CertificateTemplateVerified(54-54)CertificateTemplateDelivered(52-52)CertificateTemplateFailed(53-53)server/fleet/mdm.go (2)
MDMOperationTypeInstall(531-531)MDMOperationType(528-528)
server/worker/software_worker_test.go (1)
server/mock/datastore_mock.go (2)
SetHostCertificateTemplatesToPendingRemoveForHostFunc(1702-1702)CreatePendingCertificateTemplatesForNewHostFunc(1540-1540)
server/datastore/mysql/host_certificate_templates_test.go (2)
server/fleet/datastore.go (1)
Datastore(50-2622)server/fleet/certificate_templates.go (5)
CertificateTemplate(10-15)CertificateTemplatePending(50-50)CertificateTemplateDelivered(52-52)CertificateTemplateFailed(53-53)CertificateTemplateDelivering(51-51)
server/service/integration_android_certificate_templates_test.go (6)
server/ptr/ptr.go (2)
T(76-78)String(10-12)server/fleet/certificate_authorities.go (2)
CertificateAuthority(61-96)CATypeCustomSCEPProxy(49-49)server/fleet/hosts.go (2)
Host(276-405)AndroidHost(455-458)server/datastore/mysql/testing_utils.go (1)
ExecAdhocSQL(420-424)server/fleet/certificate_templates.go (6)
CertificateTemplateStatus(47-47)CertificateTemplatePending(50-50)CertificateTemplateDelivering(51-51)CertificateTemplateDelivered(52-52)CertificateTemplateVerified(54-54)CertificateTemplateFailed(53-53)server/fleet/mdm.go (3)
MDMOperationType(528-528)MDMOperationTypeInstall(531-531)MDMOperationTypeRemove(532-532)
server/datastore/mysql/host_certificate_templates.go (3)
server/fleet/certificate_templates.go (4)
CertificateTemplatePending(50-50)HostCertificateTemplateForDelivery(73-77)CertificateTemplateDelivering(51-51)CertificateTemplateFailed(53-53)server/fleet/mdm.go (2)
MDMOperationTypeInstall(531-531)MDMOperationTypeRemove(532-532)server/contexts/ctxerr/ctxerr.go (1)
Wrap(199-202)
server/fleet/certificate_templates.go (1)
server/fleet/mdm.go (1)
MDMOperationType(528-528)
server/mdm/android/service/service.go (2)
server/mdm/android/android.go (2)
AgentCertificateTemplate(62-66)AgentManagedConfiguration(55-60)server/fleet/certificate_templates.go (1)
HostCertificateTemplateForDelivery(73-77)
server/worker/software_worker.go (2)
server/ptr/ptr.go (1)
ValOrZero(82-90)server/fleet/datastore.go (1)
Datastore(50-2622)
🔇 Additional comments (16)
server/fleet/datastore.go (1)
2604-2606: Host-scoped removal API shape looks appropriate.
SetHostCertificateTemplatesToPendingRemoveForHostcleanly mirrors the existing template-scoped variant and gives the worker/service layers a focused primitive for handling per-host team transfers. The interface surface and signature look good.server/datastore/mysql/host_certificate_templates_test.go (3)
19-36: Adding the host-scoped test case wiring is correct.Registering
testSetHostCertificateTemplatesToPendingRemoveForHostinTestHostCertificateTemplatesmatches the new datastore API and keeps all related tests under the same umbrella. No issues here.
827-845: Extra assertions onTemplatesstrengthen delivery state coverage.The new
require.Len(t, certTemplates.Templates, 2)checks (for both calls toGetAndTransitionCertificateTemplatesToDelivering) ensure the richer per-template delivery payload stays in sync withDeliveringTemplateIDs. This is a nice sanity check around the new structure.
1142-1236: Host-scoped removal test covers the right edge cases and looks correct.The new
testSetHostCertificateTemplatesToPendingRemoveForHostverifies all the important behaviors:
- For the target host:
pendingandfailedinstall rows are deleted.deliveredinstall rows are converted topending/remove.delivering/removerows are left untouched (removal already in progress).- For a different host:
- Rows are unaffected.
This matches the intended semantics for team transfers and ensures you don’t interfere with in-flight removals or other hosts. The setup and assertions are tight and self-contained; no issues found.
server/worker/software_worker_test.go (1)
68-73: Mock wiring for new datastore methods is sufficient and non-intrusive.Providing no-op implementations for
SetHostCertificateTemplatesToPendingRemoveForHostFuncandCreatePendingCertificateTemplatesForNewHostFunckeeps the test focused on Fleet Agent preservation while satisfying the new worker dependencies. This is an appropriate level of mocking.server/mdm/android/service/service.go (3)
1007-1017: LGTM!The nil checks for
StatusandOperationTypebefore dereferencing are correctly implemented, preventing potential panics when these optional fields are not set.
1243-1244: LGTM!The refactored
buildHostConfigcorrectly accepts pre-fetched templates and maps them toAgentCertificateTemplatewith the appropriate Status and Operation fields. The direct type conversions are valid sinceHostCertificateTemplateForDeliveryuses value types (not pointers) for these fields.Also applies to: 1259-1264
1291-1291: LGTM!The call sites are correctly updated to pass the full
Templatesslice tobuildHostConfig, aligning with the new signature that enables per-template status and operation tracking.Also applies to: 1305-1305
server/mdm/android/service/profiles_test.go (2)
919-929: LGTM!The test correctly validates that certificate templates sent to the Android Management API have the expected
Status("delivering") andOperation("install") values. This aligns with the state machine where templates transition from "pending" to "delivering" before the API call.
1220-1237: LGTM!The test comprehensively validates per-template status and operation tracking across all lifecycle states (verified, delivered, delivering, failed, pending). The inline helper
assertCertTemplateimproves readability while maintaining clear assertions. The comment explaining the pending → delivering transition is helpful documentation.server/mock/datastore_mock.go (1)
1702-1703: Mock wiring for new host-scoped certificate template method is consistent and correctThe added function type, DataStore fields, and method implementation follow the existing
SetHostCertificateTemplatesToPendingRemove*pattern (lock, mark*Invoked, delegate to*Func). This should integrate cleanly with the updatedfleet.Datastoreinterface and existing tests that depend on this mock.Also applies to: 4227-4229, 10111-10116
server/fleet/certificate_templates.go (1)
71-88: LGTM! Enhanced delivery tracking with per-template status.The new
HostCertificateTemplateForDeliverystructure and replacement ofOtherTemplateIDswith theTemplatesslice provides better granularity for tracking individual template status and operations during delivery workflows.server/datastore/mysql/host_certificate_templates.go (2)
336-364: LGTM! Correct population of Templates with per-item status.The logic correctly populates the
Templatesslice for all template statuses (pending, delivering, delivered, verified, failed) with their appropriate post-transition status and operation type.
519-550: LGTM! Host-scoped removal method with proper SQL filtering.The new
SetHostCertificateTemplatesToPendingRemoveForHostmethod correctly implements host-scoped template removal preparation. Both SQL queries are properly filtered withhost_uuid = ?to ensure they only affect the specified host's templates.server/service/integration_android_certificate_templates_test.go (2)
119-172: LGTM! Well-structured test helpers.The
createTestCertificateAuthorityandcreateEnrolledAndroidHosthelper methods are clean, reusable, and properly set up test fixtures for Android host certificate template testing.
712-1034: LGTM! Comprehensive team transfer test coverage.The
TestCertificateTemplateTeamTransfertest provides excellent coverage of certificate template behavior during team transfers:
- All status/operation combinations (9 cases): Verifies that pending/failed install records are deleted while others are marked for removal, and remove operations remain unchanged
- Host without certs → team with certs: Confirms new pending templates are created
- Host with certs → team with different certs: Validates old templates are marked for removal and new ones are added
The test structure is clear, uses database helpers appropriately, and validates both expected deletions and state transitions.
…certs-teams # Conflicts: # server/datastore/mysql/schema.sql
|
@coderabbitai full review |
| return ds.withRetryTxx(ctx, func(tx sqlx.ExtContext) error { | ||
| // Delete rows with status in (pending, failed) and operation_type=install | ||
| // These certificates were never successfully installed on the device | ||
| deleteStmt := fmt.Sprintf(` |
There was a problem hiding this comment.
what's the advantage to the delete as opposed to updating the existing row's UUID/operation_type?
There was a problem hiding this comment.
If status is pending, that means it has not been sent to the device yet. Failed means (I'm assuming), that we are in the terminal state and the device does NOT have the cert, so there is nothing to delete on the device. Deleting the row here is a simplification so we don't have to deal with it anymore.
Are you suggesting to do something better?
There was a problem hiding this comment.
No that makes sense to me
<!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #37580 This migration is needed for the larger unreleased bug PR: #37616 Doing the migration separately to merge quickly and minimize merge conflicts. (cherry picked from commit fa9c868)
<!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #37580 Resolves unreleased 4.79 bug and needs to be cherry picked. Also includes fixes from manually going through the test plan at: [#30876](#30876) # Checklist for submitter ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually For unreleased bug fixes in a release candidate, one of: - [x] Confirmed that the fix is not expected to adversely impact load test results <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **New Features** * Per-template versioning and explicit operation/status fields for host certificate templates; delivery payloads now include per-template details. * **Bug Fixes** * Removal preparation broadened to also clear failed entries and handle per-host removals; delivery/transition ordering adjusted to avoid race conditions. * **Tests** * Extensive tests added for team-transfer flows, per-host removal/preparation, and end-to-end Android certificate template scenarios. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
<!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #30876 Cherry picks: | Order | Commit | PR | Description | |-------|------------|--------|--------------------------------------------------------| | 1 | 155cd56 | #37763 | Add uuid column to host_certificate_templates | | 2 | c9221b3 | #37502 | Set android host cert statuses on gitops delete | | 3 | b41effa | #37616 | Deleting/adding Android certs to host on team transfer | | 4 | a91bb56 | #37770 | Point to com.fleetdm.agent Android agent by default | --------- Co-authored-by: Tim Lee <timlee@fleetdm.com>
2 participants
Related issue: Resolves #37580
Resolves unreleased 4.79 bug and needs to be cherry picked. Also includes fixes from manually going through the test plan at: #30876
Checklist for submitter
Testing
For unreleased bug fixes in a release candidate, one of:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.