Skip to content

Integration tests for non-proxied cert renewal#45663

Draft
mostlikelee wants to merge 2 commits into
40639-cert-renewfrom
pr-2.5b-integration-tests
Draft

Integration tests for non-proxied cert renewal#45663
mostlikelee wants to merge 2 commits into
40639-cert-renewfrom
pr-2.5b-integration-tests

Conversation

@mostlikelee
Copy link
Copy Markdown
Contributor

@mostlikelee mostlikelee commented May 15, 2026

Related issue: Resolves #45250

Summary by CodeRabbit

Release Notes

  • Tests
    • Added comprehensive integration tests for MDM certificate renewal and SCEP profile uploads to validate renewal variable handling and proxy configuration compatibility.

Review Change Stack

Adds end-to-end coverage for the public profile-upload API surface
under Decision 2.6 (marker is opt-in). Four tests:

- TestACMEProfileUploadAcceptsAllMarkerPlacements: 4-case matrix
  (preferred/legacy/no-marker × OU/CN placement) confirms acceptance
  across the board for com.apple.security.acme profiles.
- TestRawSCEPProfileUploadAcceptsAllMarkerPlacements: same matrix for
  com.apple.security.scep payloads without Fleet proxy variables.
- TestConditionalAccessProfileUploadsCleanly: renders Fleet's generated
  Conditional Access SCEP profile and confirms clean upload via custom
  OS settings.
- TestWindowsSCEPProfilePreferredVariableAccepted: covers PR #45237's
  pre-existing-surface back-compat (NDES / Custom SCEP proxy SCEP
  accept the preferred variable name).
@mostlikelee
Copy link
Copy Markdown
Contributor Author

@coderabbitai full review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 15, 2026

✅ Actions performed

Full review triggered.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 15, 2026

Walkthrough

This PR adds a new integration test file covering non-proxied certificate renewal profile uploads to the MDM /api/v1/fleet/mdm/profiles/batch endpoint. The tests verify ACME and raw SCEP profile uploads accept renewal variable markers in multiple positions (OU vs. CN, preferred vs. legacy names), validate Fleet-generated Conditional Access SCEP profiles upload cleanly, and ensure Windows NDES/custom SCEP proxy endpoints accept the preferred renewal variable. A test SCEP server and custom proxy certificate authority are instantiated to validate the Windows scenario end-to-end.

Possibly related issues

  • #45584: Windows non-proxied SCEP renewal-variable handling integration tests directly address the Windows SCEP validator gap by testing acceptance of the preferred $FLEET_VAR_CERTIFICATE_RENEWAL_ID.
  • #45629: New integration tests verify ACME/raw SCEP/Windows profiles accept preferred/legacy/no renewal markers, corresponding to the revert of ACME/non-proxied SCEP validators and regression testing of marker-optional behavior.
  • #45580: New integration tests exercise Fleet-generated Conditional Access SCEP profile uploads by executing the template into a buffer, directly relating to the proposed template change to add the renewal OU marker.

Possibly related PRs

  • fleetdm/fleet#45237: Windows NDES/custom SCEP profile validation for the preferred $FLEET_VAR_CERTIFICATE_RENEWAL_ID variable is directly exercised by the new Windows proxy integration test.
  • fleetdm/fleet#45643: New ACME and non-proxied SCEP renewal-profile upload tests align with that PR's revert making the renewal-ID marker opt-in and updating Apple payload validator tests.
  • fleetdm/fleet#45364: New integration tests for non-proxied ACME/raw SCEP uploads exercise the validation behavior added in apple_mdm.go for renewal-variable/marker handling.
🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description is incomplete and does not follow the required template structure with necessary sections for testing, validation, and database considerations. Complete the description using the provided template: add the checklist items for Changes files, input validation, testing (automated tests and QA), and any other applicable sections. Ensure all required boxes are addressed.
Linked Issues check ❓ Inconclusive While the PR adds four integration test cases covering ACME, raw SCEP, and Windows SCEP profile uploads, it does not fully satisfy issue #45250's requirements for cron-job and INSERT-path testing. Clarify status of RenewMDMManagedCertificates cron and UpdateHostCertificates INSERT validation tests, or formally defer those to QA/future work with explicit documentation in the PR description or linked issue.
✅ Passed checks (3 passed)
Check name Status Explanation
Out of Scope Changes check ✅ Passed All changes are focused on adding integration tests for non-proxied certificate renewal profile uploads, directly aligned with issue #45250's stated scope.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check ✅ Passed The title 'Integration tests for non-proxied cert renewal' directly and concisely summarizes the main change: adding integration tests for non-proxied certificate renewal functionality.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch pr-2.5b-integration-tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@server/service/integration_mdm_non_proxied_cert_renewal_test.go`:
- Around line 193-197: The replacement using bytes.ReplaceAll on
windowsDeviceSCEPProfileForRenewalTest into preferred may silently no-op; add
assertions that the legacy token exists before replacement and that the new
token exists after replacement. Specifically, assert
bytes.Contains(windowsDeviceSCEPProfileForRenewalTest,
[]byte("$FLEET_VAR_SCEP_RENEWAL_ID")) before calling bytes.ReplaceAll, then
assert bytes.Contains(preferred, []byte("$FLEET_VAR_CERTIFICATE_RENEWAL_ID"))
(and optionally assert !bytes.Contains(preferred,
[]byte("$FLEET_VAR_SCEP_RENEWAL_ID"))) after to ensure the swap actually
occurred prior to the upload step.
🪄 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: 0019fb44-e33d-4b89-8cf0-f4cb08088b24

📥 Commits

Reviewing files that changed from the base of the PR and between 888f0a2 and fba3f31.

📒 Files selected for processing (1)
  • server/service/integration_mdm_non_proxied_cert_renewal_test.go

Comment thread server/service/integration_mdm_non_proxied_cert_renewal_test.go
@codecov
Copy link
Copy Markdown

codecov Bot commented May 15, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (40639-cert-renew@888f0a2). Learn more about missing BASE report.

Additional details and impacted files
@@                 Coverage Diff                 @@
##             40639-cert-renew   #45663   +/-   ##
===================================================
  Coverage                    ?   66.76%           
===================================================
  Files                       ?     2746           
  Lines                       ?   219484           
  Branches                    ?    10831           
===================================================
  Hits                        ?   146538           
  Misses                      ?    59707           
  Partials                    ?    13239           
Flag Coverage Δ
backend 68.59% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

bytes.ReplaceAll silently no-ops if the legacy token isn't in the
fixture. Assert before-and-after so a future fixture change can't
mask the swap failing.
@mostlikelee mostlikelee changed the title Integration tests for non-proxied cert renewal (PR 2.5b) Integration tests for non-proxied cert renewal May 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant