Android certs support all idp vars#48100
Conversation
![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.
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
This PR expands Android certificate template variable support so templates can reference HOST_PLATFORM and the full set of HOST_END_USER_IDP_* variables, and updates unit tests to cover the new validation and substitution behavior.
Changes:
- Allow additional Fleet variables in certificate template subject names/SANs:
HOST_PLATFORM,HOST_END_USER_IDP_USERNAME_LOCAL_PART,HOST_END_USER_IDP_GROUPS,HOST_END_USER_IDP_DEPARTMENT,HOST_END_USER_IDP_FULL_NAME. - Refactor certificate variable substitution to reuse shared end-user fetching logic and add substitution implementations for the newly supported variables.
- Add/adjust unit tests to validate SAN variable allowlisting and to cover substitution + error cases for all supported variables.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| server/service/certificate_templates.go | Adds support for additional host/IdP variables and refactors substitution logic for certificate templates. |
| server/service/certificate_templates_test.go | Updates SAN validation tests and adds direct substitution tests for all supported variables. |
Files excluded by content exclusion policy (1)
- changes/36774-cert-template-all-idp-vars
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe PR expands Fleet's certificate template variable support. 🚥 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 |
![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/certificate_templates_test.go (1)
673-707: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: subtests mutate shared
ds.ScimUserByHostIDFuncwithout restoring it.These error-path subtests reassign
ds.ScimUserByHostIDFuncin place and never restore the original. The suite only passes because the happy-path subtests run before them; any future reordering or insertion would silently break. Consider scoping a freshds/func per subtest (ort.Cleanupto restore) to remove the ordering dependency.🤖 Prompt for 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. In `@server/service/certificate_templates_test.go` around lines 673 - 707, The test subtests are mutating the shared ds.ScimUserByHostIDFunc without restoring it between test cases, creating an ordering dependency. To fix this, either create a fresh ds instance at the beginning of each subtest (the "missing IDP user returns error", "missing groups returns error", "missing department returns error", and "missing full name returns error" subtests) or use t.Cleanup to save and restore the original ds.ScimUserByHostIDFunc value after each subtest completes. This ensures that each subtest is isolated and test execution order does not affect the results.
🤖 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/certificate_templates.go`:
- Around line 180-207: The IdP values (user.IdpGroups, user.Department, and
user.IdpFullName) in the replaceCertificateVariables function are being inserted
directly into certificate subject/SAN strings without escaping, which can cause
X.500 DN injection. Create or apply an RFC 4514 LDAP string encoding escape
function that converts special characters like commas to their escaped form
(e.g., comma becomes \,) before performing the substitutions. Apply this
escaping to each individual IdpGroups element before the strings.Join call, to
the user.Department value before the ReplaceAllString call, and to the fullName
value before its ReplaceAllString call, ensuring all untrusted IdP data is
properly escaped before being substituted into the certificate strings.
---
Nitpick comments:
In `@server/service/certificate_templates_test.go`:
- Around line 673-707: The test subtests are mutating the shared
ds.ScimUserByHostIDFunc without restoring it between test cases, creating an
ordering dependency. To fix this, either create a fresh ds instance at the
beginning of each subtest (the "missing IDP user returns error", "missing groups
returns error", "missing department returns error", and "missing full name
returns error" subtests) or use t.Cleanup to save and restore the original
ds.ScimUserByHostIDFunc value after each subtest completes. This ensures that
each subtest is isolated and test execution order does not affect the results.
🪄 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: e865393f-2957-4b3c-8718-6f3d1acaf457
📒 Files selected for processing (3)
changes/36774-cert-template-all-idp-varsserver/service/certificate_templates.goserver/service/certificate_templates_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #48100 +/- ##
=======================================
Coverage 67.32% 67.32%
=======================================
Files 3657 3657
Lines 231272 231331 +59
Branches 12240 12240
=======================================
+ Hits 155698 155742 +44
- Misses 61605 61614 +9
- Partials 13969 13975 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Related issue: Resolves #36774
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.
Testing
Summary by CodeRabbit