41542 android cert resend backend#42099
Conversation
dantecatalfamo
had a problem deploying
to
Docker Hub
GitHub Actions
Error
dantecatalfamo
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
dantecatalfamo
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #42099 +/- ##
==========================================
+ Coverage 66.41% 66.51% +0.10%
==========================================
Files 2509 2517 +8
Lines 200777 202300 +1523
Branches 9048 9048
==========================================
+ Hits 133344 134565 +1221
- Misses 55370 55579 +209
- Partials 12063 12156 +93
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:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis pull request implements backend functionality to support resending Android host certificates. It adds a datastore method that updates host certificate template records by regenerating their UUID and resetting the status to pending. A corresponding service layer method handles authorization checks, retrieves template metadata for activity logging, and creates an activity record. A new HTTP POST endpoint is registered at 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 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 Tip CodeRabbit can suggest fixes for GitHub Check annotations.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
server/service/integration_android_certificate_templates_test.go (1)
1467-1473: Add a case for “template exists but not for this host.”Current negative coverage only uses non-existent IDs. Please add a case where the certificate template exists (belongs to another host/team) and assert the resend endpoint returns 404 for that host/template pair. This will lock down the no-row-updated path.
🤖 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 1467 - 1473, Add a test case where the certificate template exists but is associated with a different host/team: create a second host (e.g., otherHost) and a certificate template for that host to obtain otherCertTemplateID, then call s.DoJSON("POST", fmt.Sprintf("/api/latest/fleet/hosts/%d/certificates/%d/resend", host.ID, otherCertTemplateID), nil, http.StatusNotFound, &struct{}{}) to assert the resend endpoint returns 404 for a template that exists but not for the given host; reference certTemplateID and host.ID to place this new case alongside the existing negative tests.server/service/certificate_templates_test.go (1)
448-457: Add a failure-path test for certificate lookup errors.
ResendHostCertificateTemplatealso fails whenGetCertificateTemplateByIderrors; adding that case would complete coverage for all pre-activity failure branches and assert no activity is emitted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/certificate_templates_test.go` around lines 448 - 457, Add a new unit test that mirrors the existing "returns error when datastore fails" case but mocks datastore.GetCertificateTemplateById to return an error (e.g., errors.New("db error")) before calling svc.ResendHostCertificateTemplate(ctx, hostID, templateID), then assert that an error is returned and its message contains the db error and that opts.ActivityMock.NewActivityFuncInvoked is false to ensure no activity was emitted; reference the ResendHostCertificateTemplate method, the GetCertificateTemplateById datastore mock, and the ActivityMock.NewActivityFuncInvoked flag when implementing the test.
🤖 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/certificate_templates.go`:
- Around line 423-426: Handle the error returned by results.RowsAffected() and
preserve the not-found semantic: call affected, err := results.RowsAffected()
and if err != nil return a wrapped DB error (e.g., ctxerr.Errorf(ctx, "checking
rows affected for template %d host %d: %v", templateID, hostID, err)); otherwise
if affected == 0 return a typed not-found error (use the project’s not-found
helper, e.g., ctxerr.NotFoundf(ctx, "template %d does not exist for host %d",
templateID, hostID)) so that DB failures surface as errors and missing templates
remain a 404 path.
In `@server/service/certificates.go`:
- Around line 737-744: The code currently calls
svc.ds.GetCertificateTemplateById before proving the template belongs to the
host; to avoid exposing template existence, call
svc.ds.ResendHostCertificateTemplate(ctx, hostID, templateID) first (or replace
GetCertificateTemplateById with a host-scoped lookup like
GetHostCertificateTemplateById if available) and only fetch certificate details
via svc.ds.GetCertificateTemplateById after ResendHostCertificateTemplate
succeeds, so the template existence is validated through the host relationship
before any detailed lookup or logging.
---
Nitpick comments:
In `@server/service/certificate_templates_test.go`:
- Around line 448-457: Add a new unit test that mirrors the existing "returns
error when datastore fails" case but mocks datastore.GetCertificateTemplateById
to return an error (e.g., errors.New("db error")) before calling
svc.ResendHostCertificateTemplate(ctx, hostID, templateID), then assert that an
error is returned and its message contains the db error and that
opts.ActivityMock.NewActivityFuncInvoked is false to ensure no activity was
emitted; reference the ResendHostCertificateTemplate method, the
GetCertificateTemplateById datastore mock, and the
ActivityMock.NewActivityFuncInvoked flag when implementing the test.
In `@server/service/integration_android_certificate_templates_test.go`:
- Around line 1467-1473: Add a test case where the certificate template exists
but is associated with a different host/team: create a second host (e.g.,
otherHost) and a certificate template for that host to obtain
otherCertTemplateID, then call s.DoJSON("POST",
fmt.Sprintf("/api/latest/fleet/hosts/%d/certificates/%d/resend", host.ID,
otherCertTemplateID), nil, http.StatusNotFound, &struct{}{}) to assert the
resend endpoint returns 404 for a template that exists but not for the given
host; reference certTemplateID and host.ID to place this new case alongside the
existing negative tests.
🪄 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: 450d5a5a-6664-4a37-b05e-903e7b063099
📒 Files selected for processing (13)
changes/41542-android-cert-resend-backendserver/datastore/mysql/certificate_templates.goserver/datastore/mysql/certificate_templates_test.goserver/fleet/activities.goserver/fleet/datastore.goserver/fleet/service.goserver/mock/datastore_mock.goserver/mock/service/service_mock.goserver/service/certificate_templates_test.goserver/service/certificates.goserver/service/handler.goserver/service/hosts.goserver/service/integration_android_certificate_templates_test.go
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
dantecatalfamo
had a problem deploying
to
Docker Hub
GitHub Actions
Error
dantecatalfamo
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
| }{ | ||
| {"from verified", fleet.CertificateTemplateVerified}, | ||
| {"from delivered", fleet.CertificateTemplateDelivered}, | ||
| {"from failed", fleet.CertificateTemplateFailed}, |
There was a problem hiding this comment.
What if someone tries to resend a certificate that's already pending? Just thinking about a race condition that could occur.
Certificate is in pending status
cron starts delivery (transitions to delivering)
User calls resend (resets to pending with new UUID)
The delivery cron might have already fetched the old UUID
I forget how the UUID works in the delivery process. might not be a concern.
There was a problem hiding this comment.
Was thinking about it more, and even if it was allowed to happen, either the cron would just pick it up again next run, or mark it as delivered and then the UUID in the DB wouldn't match what's on the device but that wouldn't really matter since it's only used to re-install a cert and if it's mismatched on the first install it still accomplished the goal of installing the certificate
<!-- Add the related story/sub-task/bug number, like Resolves fleetdm#123, or remove if NA --> **Related issue:** Resolves fleetdm#41542
<!-- Add the related story/sub-task/bug number, like Resolves fleetdm#123, or remove if NA --> **Related issue:** Resolves fleetdm#41542
3 participants
Related issue: Resolves #41542
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
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
Summary by CodeRabbit