Don't resend pending certificates - #43820
Conversation
GitHub Actions
Inactive
![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.
WalkthroughThis PR adds validation to the 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}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/service/certificates.go (1)
784-809: Optional: the follow-upGetCertificateTemplateByIdcall is now redundant.
GetCertificateTemplateByIdForHostat line 784 already returns the template'sIDandName(via the embeddedCertificateTemplateResponse→CertificateTemplateResponseSummary), which are the only fields consumed by the activity payload below. The subsequentsvc.ds.GetCertificateTemplateById(ctx, templateID)at line 797 is an extra DB round-trip that can be eliminated.♻️ Proposed refactor
- if err := svc.ds.ResendHostCertificateTemplate(ctx, hostID, templateID); err != nil { - return ctxerr.Wrap(ctx, err, "resending certificate template") - } - - certificate, err := svc.ds.GetCertificateTemplateById(ctx, templateID) - if err != nil { - return ctxerr.Wrap(ctx, err, "getting certificate details") - } - - if err := svc.NewActivity(ctx, authz.UserFromContext(ctx), fleet.ActivityTypeResentCertificate{ - HostID: host.ID, - HostDisplayName: host.DisplayName(), - CertificateTemplateID: certificate.ID, - CertificateName: certificate.Name, - }); err != nil { + if err := svc.ds.ResendHostCertificateTemplate(ctx, hostID, templateID); err != nil { + return ctxerr.Wrap(ctx, err, "resending certificate template") + } + + if err := svc.NewActivity(ctx, authz.UserFromContext(ctx), fleet.ActivityTypeResentCertificate{ + HostID: host.ID, + HostDisplayName: host.DisplayName(), + CertificateTemplateID: template.ID, + CertificateName: template.Name, + }); err != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/certificates.go` around lines 784 - 809, Remove the redundant DB round-trip by dropping the svc.ds.GetCertificateTemplateById call and use the already-fetched template from GetCertificateTemplateByIdForHost (the local variable template) when building the ActivityTypeResentCertificate payload; i.e., after successful svc.ds.ResendHostCertificateTemplate return to creating the activity using template.ID and template.Name (and existing host.ID/host.DisplayName()), and delete the extra certificate variable and its error-handling block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/service/certificates.go`:
- Around line 784-809: Remove the redundant DB round-trip by dropping the
svc.ds.GetCertificateTemplateById call and use the already-fetched template from
GetCertificateTemplateByIdForHost (the local variable template) when building
the ActivityTypeResentCertificate payload; i.e., after successful
svc.ds.ResendHostCertificateTemplate return to creating the activity using
template.ID and template.Name (and existing host.ID/host.DisplayName()), and
delete the extra certificate variable and its error-handling block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d52052c3-ffc8-480f-8f49-33563c5305bb
📒 Files selected for processing (2)
server/service/certificate_templates_test.goserver/service/certificates.go
There was a problem hiding this comment.
Pull request overview
Prevents resending an Android host certificate template when it’s already in a pending state, returning a client error instead of resetting the delivery.
Changes:
- Add a host-specific template status lookup before resend and return HTTP 400 if status is
pending. - Add unit coverage to ensure pending templates cannot be resent (and no activity/resend is triggered).
- Import
net/httpto usehttp.StatusBadRequest.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
server/service/certificates.go |
Adds a pre-check to block resending pending certificate templates with a 400 user-message error. |
server/service/certificate_templates_test.go |
Adds/updates tests to cover the new 400 behavior and ensure no resend/activity occurs. |
Comments suppressed due to low confidence (1)
server/service/certificates.go:800
- This adds an extra datastore read (
GetCertificateTemplateByIdForHost) before the resend, and the function later fetches the certificate again viaGetCertificateTemplateByIdjust to populate activity fields. Since the host-specific template response already includes the template ID/name, consider reusingtemplatefor the activity payload to avoid the additional query.
template, err := svc.ds.GetCertificateTemplateByIdForHost(ctx, templateID, host.UUID)
if err != nil {
return ctxerr.Wrap(ctx, err, "checking host certificate template")
}
if template.Status == fleet.CertificateTemplatePending {
return fleet.NewUserMessageError(errors.New("Couldn't resend pending certificate template."), http.StatusBadRequest)
}
if err := svc.ds.ResendHostCertificateTemplate(ctx, hostID, templateID); err != nil {
return ctxerr.Wrap(ctx, err, "resending certificate template")
}
certificate, err := svc.ds.GetCertificateTemplateById(ctx, templateID)
if err != nil {
return ctxerr.Wrap(ctx, err, "getting certificate details")
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #43820 +/- ##
=======================================
Coverage 66.91% 66.91%
=======================================
Files 2600 2600
Lines 208985 208995 +10
Branches 9305 9305
=======================================
+ Hits 139846 139857 +11
+ Misses 56397 56394 -3
- Partials 12742 12744 +2
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 #37556
Fixes an issue caught in QA where a pending certificate could get resent using the API
Checklist for submitter
Testing
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests