Clear idp cookie after succesful SSO#47569
Open
ksykulev wants to merge 1 commit into
Open
Conversation
Contributor
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 updates the end-user OTA enrollment flow to prevent reusing a stale IdP identity across enrollments by clearing the BYOD IdP cookie after successful SSO, and by plumbing an IdP UUID into the Android enrollment token request when needed.
Changes:
- Clears the BYOD IdP cookie after creating an Android enrollment token (via
SetCookieson the response). - Passes an
IdpUUIDvalue through the/enrollHTML template and into the Android enrollment token fetch asidp_uuid. - Updates the Android enrollment token request decoder to accept
idp_uuidfrom the query string before falling back to the cookie.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/frontend.go | Passes IdpUUID into the enroll template and clears the BYOD IdP cookie before rendering the enroll page (needs adjustment to avoid breaking iOS/iPadOS flows). |
| server/mdm/android/service/service.go | Accepts idp_uuid query param for enrollment token requests before checking the cookie. |
| server/mdm/android/service.go | Adds SetCookies to clear the BYOD IdP cookie after successfully creating an enrollment token; introduces a cookie name constant. |
| frontend/templates/enroll-ota.html | Appends idp_uuid to the Android enrollment token fetch URL when present. |
| changes/47343-idp-cookie | User-visible change entry (content excluded from review). |
Files excluded by content exclusion policy (1)
- changes/47343-idp-cookie
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+175
to
+179
| // Clear the BYOD IdP cookie now that we are about to render the enrollment page. | ||
| var idpUUID string | ||
| if authRequired { | ||
| idpUUID = r.URL.Query().Get("enrollment_reference") | ||
| http.SetCookie(w, &http.Cookie{ |
| "google.golang.org/api/androidmanagement/v1" | ||
| ) | ||
|
|
||
| const byodIdpCookieName = "__Host-FLEETBYODIDP" |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #47569 +/- ##
==========================================
- Coverage 67.19% 67.18% -0.01%
==========================================
Files 3616 3616
Lines 229016 229048 +32
Branches 11785 11929 +144
==========================================
+ Hits 153885 153890 +5
- Misses 61303 61319 +16
- Partials 13828 13839 +11
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:
|
Contributor
WalkthroughThis change updates Android enrollment to carry an IdP UUID from the server-rendered enrollment page into the enrollment-token request. The frontend handler now passes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/mdm/android/service.go (1)
10-10: ⚡ Quick winUse the shared BYOD IdP cookie constant instead of duplicating the literal.
byodIdpCookieNameduplicates a cross-layer contract value. If the canonical cookie name changes, this cookie-clearing path can silently break.Proposed change
import ( "context" "net/http" + shared_mdm "github.com/fleetdm/fleet/v4/pkg/mdm" "google.golang.org/api/androidmanagement/v1" ) - -const byodIdpCookieName = "__Host-FLEETBYODIDP" @@ http.SetCookie(w, &http.Cookie{ - Name: byodIdpCookieName, + Name: shared_mdm.BYODIdpCookieName, Value: "", Path: "/",🤖 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/mdm/android/service.go` at line 10, The constant byodIdpCookieName is currently defined as a local literal value "__Host-FLEETBYODIDP" in this file, which duplicates the canonical definition elsewhere in the codebase. Remove the local constant definition and instead import and use the shared byodIdpCookieName constant from its canonical location (likely in a shared utilities or constants package) to ensure consistency across layers and prevent the cookie-clearing logic from breaking if the canonical cookie name is updated.
🤖 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/mdm/android/service/service.go`:
- Around line 500-506: The idp_uuid query parameter is being accepted directly
without server-side validation, allowing any caller with an enroll_secret to
bind an enrollment to any IdP UUID without fresh authentication. Remove or
significantly restrict the code block that extracts idp_uuid from the query
parameter and directly populates it in the enrollmentTokenRequest struct.
Instead, only accept idp_uuid when it is cryptographically bound to
server-trusted state, such as validation against an active session cookie or a
signed one-time token generated by the server during a prior authenticated
operation. Ensure that the enrollmentTokenRequest construction validates the
identity proof before incorporating any IdP identifier.
---
Nitpick comments:
In `@server/mdm/android/service.go`:
- Line 10: The constant byodIdpCookieName is currently defined as a local
literal value "__Host-FLEETBYODIDP" in this file, which duplicates the canonical
definition elsewhere in the codebase. Remove the local constant definition and
instead import and use the shared byodIdpCookieName constant from its canonical
location (likely in a shared utilities or constants package) to ensure
consistency across layers and prevent the cookie-clearing logic from breaking if
the canonical cookie name is updated.
🪄 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: e13046bb-cb8e-4cf9-85df-43df31ff1b85
📒 Files selected for processing (5)
changes/47343-idp-cookiefrontend/templates/enroll-ota.htmlserver/mdm/android/service.goserver/mdm/android/service/service.goserver/service/frontend.go
Comment on lines
+500
to
+506
| if idpUUID := r.URL.Query().Get("idp_uuid"); idpUUID != "" { | ||
| return &enrollmentTokenRequest{ | ||
| EnrollSecret: enrollSecret, | ||
| IdpUUID: idpUUID, | ||
| FullyManaged: fullyManaged, | ||
| }, nil | ||
| } |
Contributor
There was a problem hiding this comment.
Do not treat raw idp_uuid query input as authenticated identity proof.
This early return bypasses cookie/session validation. A caller with enroll_secret and any valid IdP UUID can mint enrollment tokens without fresh SSO and bind the enrollment to that identity. idp_uuid should be accepted only when bound to server-trusted state (e.g., validated session/cookie match or signed one-time server token).
🤖 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/mdm/android/service/service.go` around lines 500 - 506, The idp_uuid
query parameter is being accepted directly without server-side validation,
allowing any caller with an enroll_secret to bind an enrollment to any IdP UUID
without fresh authentication. Remove or significantly restrict the code block
that extracts idp_uuid from the query parameter and directly populates it in the
enrollmentTokenRequest struct. Instead, only accept idp_uuid when it is
cryptographically bound to server-trusted state, such as validation against an
active session cookie or a signed one-time token generated by the server during
a prior authenticated operation. Ensure that the enrollmentTokenRequest
construction validates the identity proof before incorporating any IdP
identifier.
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issue: Resolves #47343
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