Persist byod=true enroll param through IdP redirects#48808
Conversation
JordanMontgomery
had a problem deploying
to
Docker Hub
GitHub Actions
Error
JordanMontgomery
had a problem deploying
to
Docker Hub
GitHub Actions
Error
|
@coderabbitai review |
✅ Action performedReview finished.
|
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 ensures the byod=true enrollment query parameter is preserved when initiating OTA enrollment via SSO, so it survives the IdP redirect round-trip and the user returns to the correct enrollment flow.
Changes:
- Thread
byod=truethrough thecustomOriginalURLpassed intoInitiateMDMSSOin the OTA enrollment SSO initiator. - Add a focused unit test that asserts which enrollment query params are (and are not) persisted into the SSO “original URL”.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| server/service/frontend.go | Persist byod=true in the original URL used for the SSO round-trip during OTA enrollment initiation. |
| server/service/frontend_test.go | Add a table-driven test using a capturing service stub to verify byod/fully_managed query param persistence behavior. |
Files excluded by content exclusion policy (1)
- changes/48805-byod-idp-enroll
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WalkthroughThis change preserves Changes
Possibly related PRs
Related Issues: None specified Suggested labels: bug, enrollment, sso Suggested reviewers: None specified 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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/frontend.go (1)
261-270: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider consolidating repeated param-passthrough checks.
The
fully_managedandbyodblocks are structurally identical. If more enrollment params need to be persisted through IdP redirects in the future, consider a small helper (e.g., iterate over an allow-list of param names) to avoid repeating this pattern a third/fourth time.♻️ Example refactor
- if r.URL.Query().Get("fully_managed") == "true" { - requestURL += "&fully_managed=true" - } - if r.URL.Query().Get("byod") == "true" { - requestURL += "&byod=true" - } + for _, param := range []string{"fully_managed", "byod"} { + if r.URL.Query().Get(param) == "true" { + requestURL += "&" + param + "=true" + } + }🤖 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/frontend.go` around lines 261 - 270, The param passthrough logic in initiateOTAEnrollSSO is duplicated for fully_managed and byod, so consolidate it into a small reusable helper or allow-list-based loop. Update the requestURL construction in initiateOTAEnrollSSO to iterate over the supported enrollment params and append any present values once, so future params can be added without repeating the same conditional blocks.
🤖 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.
Nitpick comments:
In `@server/service/frontend.go`:
- Around line 261-270: The param passthrough logic in initiateOTAEnrollSSO is
duplicated for fully_managed and byod, so consolidate it into a small reusable
helper or allow-list-based loop. Update the requestURL construction in
initiateOTAEnrollSSO to iterate over the supported enrollment params and append
any present values once, so future params can be added without repeating the
same conditional blocks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 06e80fa7-0dc5-4677-b020-a3ab56bffd59
📒 Files selected for processing (3)
changes/48805-byod-idp-enrollserver/service/frontend.goserver/service/frontend_test.go
JordanMontgomery
temporarily deployed
to
Docker Hub
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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #48808 +/- ##
==========================================
+ Coverage 68.02% 68.04% +0.02%
==========================================
Files 3681 3684 +3
Lines 233961 234056 +95
Branches 12453 12453
==========================================
+ Hits 159146 159258 +112
+ Misses 60497 60484 -13
+ Partials 14318 14314 -4
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 #48805
Checklist for submitter
If some of the following don't apply, delete the relevant line.
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Input data is properly validated,
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.Timeouts are implemented and retries are limited to avoid infinite loops
If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes
Testing
Added/updated automated tests
Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)
QA'd all new/changed functionality manually
Summary by CodeRabbit