BYOF: Add support for unique token ADUE#47407
Open
MagnusHJensen wants to merge 7 commits into
Open
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #47407 +/- ##
==========================================
- Coverage 67.19% 67.18% -0.02%
==========================================
Files 3274 3306 +32
Lines 227975 228221 +246
Branches 11746 11749 +3
==========================================
+ Hits 153195 153335 +140
- Misses 60965 61046 +81
- Partials 13815 13840 +25
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
There was a problem hiding this comment.
Pull request overview
Adds per-ABM-token (unique token) support to Apple Account Driven User Enrollment (BYOD), enabling enrollments to be associated with a specific ABM token (and its default BYOD team) while preserving legacy URL behavior. This introduces ADUE enrollment challenges for single-use profile retrieval and updates service discovery to publish tokenized URLs per ABM token, with frontend routing to support Apple’s fixed SSO callback URL constraints.
Changes:
- Add tokenized account-driven enrollment + service discovery routes and pass the token through the SSO initiator.
- Introduce ADUE enrollment challenges (create/fetch/consume/cleanup) and use them in MDMSSOCallback, profile retrieval, and TokenUpdate team assignment.
- Update integration/unit tests and frontend routing to cover tokenized and legacy flows.
Reviewed changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/testing_client_test.go | Updates test helper initiator to include token; modifies SSO test gating. |
| server/service/integration_mdm_test.go | Extends ADUE integration tests for tokenized + legacy URL and BYOD default team behavior. |
| server/service/integration_mdm_setup_experience_test.go | Updates setup experience BYOD iOS test to use tokenized SSO initiator. |
| server/service/handler.go | Adds tokenized account-driven enroll route and tokenized service discovery handler registration. |
| server/service/apple_mdm.go | Adds token extraction to ADUE enroll decode, updates account-driven enroll endpoint flow, TokenUpdate challenge usage, and per-token service discovery assignment. |
| server/service/apple_mdm_test.go | Updates TokenUpdate unit test to use ADUE challenge lookup (vs direct bearer UUID). |
| server/mock/service/service_mock.go | Updates service mock method signature to accept enrollmentToken. |
| server/mock/datastore_mock.go | Adds datastore mock methods for ABM unique token lookup and ADUE challenges CRUD/cleanup. |
| server/mdm/apple/apple_mdm.go | Adds tokenized path constants and extracts push cert topic helper. |
| server/fleet/service.go | Updates Service interface for GetMDMAccountDrivenEnrollmentSSOURL to accept enrollmentToken. |
| server/fleet/mdm.go | Adds ABM BYOD default team + enrollment URL token fields; adds URL-safe 32-byte token generator. |
| server/fleet/datastore.go | Adds datastore methods for ABM unique token lookup and ADUE challenges. |
| server/fleet/cron_schedules.go | Adds cron schedule name for ADUE challenge cleanup. |
| server/fleet/apple_mdm.go | Defines ADUE challenge expiration and struct. |
| server/datastore/mysql/apple_mdm.go | Implements ABM unique token lookup and ADUE challenge insert/get/consume/cleanup. |
| server/datastore/mysql/apple_mdm_test.go | Adds mysql datastore tests for ABM unique token lookup and ADUE challenges. |
| frontend/router/index.tsx | Adds route for /mdm/apple/account_driven_enroll/sso/:token. |
| frontend/pages/MDMAppleSSOPage/MDMAppleSSOPage.tsx | Builds initiator with token from route param when present. |
| ee/server/service/mdm.go | Parses tokenized initiator for account-driven enrollment and issues ADUE challenges in callback. |
| ee/server/service/apple_mdm.go | Implements EE account-driven enrollment SSO URL construction and challenge-backed profile generation/consumption. |
| cmd/fleet/serve.go | Registers new cron schedule for ADUE challenge cleanup. |
| cmd/fleet/cron.go | Adds the ADUE challenge cleanup cron schedule implementation. |
| changes/30871-default-byod-fleet | Release note entry for default BYOD fleet support. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
MagnusHJensen
had a problem deploying
to
Docker Hub
GitHub Actions
Failure
MagnusHJensen
force-pushed
the
45598-unique-adue-token
branch
from
7f57e41 to
424bc5e
Compare
Contributor
WalkthroughAdds per-ABM-token Apple account-driven enrollment: new ADUE challenge datastore APIs (insert/get/consume/cleanup), ABM token fields for enrollment URL token and BYOD default team, token-aware SSO initiation/callback and enrollment endpoints, MDMPushCertTopic helper, daily cron to remove expired challenges, frontend/router updates for tokened SSO, mock/service adjustments, and expanded unit/integration tests validating tokened vs legacy enrollment and single-use challenge behavior. Possibly related PRs
🚥 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/datastore/mysql/apple_mdm.go (1)
5833-5839:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
byod_default_team_idis only half-wired.
getABMTokennow readsabt.byod_default_team_id, but the same file still omits that column fromListABMTokens,InsertABMToken, andSaveABMToken. That means the new BYOD default-team assignment won't round-trip through create/update flows and won't be visible in list responses.Also applies to: 5934-5938
🤖 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/datastore/mysql/apple_mdm.go` around lines 5833 - 5839, ListABMTokens, InsertABMToken, and SaveABMToken are missing the byod_default_team_id column and associated select/params so the BYOD default-team value read by getABMToken cannot round-trip; update the SQL in ListABMTokens to include abt.byod_default_team_id (and COALESCE(t4.name, :no_team) as byod_team in the SELECT joins), and update InsertABMToken and SaveABMToken to include byod_default_team_id in their INSERT/UPDATE column lists and bind parameters, and ensure the Go structs/scan/order of fields used by those functions match the new column position so inserts, updates and list responses carry the BYOD default-team correctly.
🧹 Nitpick comments (3)
frontend/pages/MDMAppleSSOPage/MDMAppleSSOPage.tsx (1)
25-35: 💤 Low valueConsider validating the token before interpolation.
While the token will ultimately be validated by the backend, adding a simple frontend check (e.g., ensuring
params.tokenis a non-empty string) would provide earlier feedback and prevent sending a malformed initiator likeaccount_driven_enroll:undefinedif the token is somehow missing or invalid at the route level.💡 Optional validation check
if (pathname.startsWith("/mdm/apple/account_driven_enroll/sso")) { // While I acknowledge startsWith for route matching is a bit brittle // I couldn't find a better way, since the pathname is the actual resolved value and not the placeholder route. - if (params.token) { + if (params.token && typeof params.token === "string" && params.token.length > 0) { query.initiator = `account_driven_enroll:${params.token}`; } else { query.initiator = "account_driven_enroll"; }🤖 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 `@frontend/pages/MDMAppleSSOPage/MDMAppleSSOPage.tsx` around lines 25 - 35, Validate params.token before interpolating into query.initiator: in the block where pathname.startsWith("/mdm/apple/account_driven_enroll/sso") is checked, ensure params.token is a non-empty string (e.g., typeof params.token === "string" && params.token.trim().length > 0) before setting query.initiator = `account_driven_enroll:${params.token}`; if the token is missing/invalid, fall back to query.initiator = "account_driven_enroll" (or another safe default) to avoid producing values like account_driven_enroll:undefined.server/service/handler.go (1)
1318-1324: ⚡ Quick winPrefer using the path constant with placeholder replacement for maintainability.
The tokenized enrollment URL is constructed by manually concatenating
fullMDMEnrollmentURL + "/" + token(line 1321), which assumes the relationship betweenAccountDrivenEnrollPathand the tokenized path. However, the test expectation (from integration_mdm_test.go:21116) and the registered route both referenceapple_mdm.AccountDrivenEnrollTokenPathwith a{token}placeholder. For consistency and to avoid breakage if path constants evolve, construct the tokenized URL by replacing the placeholder in the constant.♻️ Proposed fix to use path constant with placeholder replacement
func registerMDMServiceDiscovery( mux *http.ServeMux, logger *slog.Logger, serverURLPrefix string, fleetConfig config.FleetConfig, ) error { serviceDiscoveryLogger := logger.With("component", "mdm-apple-service-discovery") - fullMDMEnrollmentURL := fmt.Sprintf("%s%s", serverURLPrefix, apple_mdm.AccountDrivenEnrollPath) serviceDiscoveryHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { var mdmEnrollmentURL string token := r.PathValue("token") if token != "" { - mdmEnrollmentURL = fmt.Sprintf("%s/%s", fullMDMEnrollmentURL, url.PathEscape(token)) + tokenPath := strings.Replace(apple_mdm.AccountDrivenEnrollTokenPath, "{token}", url.PathEscape(token), 1) + mdmEnrollmentURL = serverURLPrefix + tokenPath } else { - mdmEnrollmentURL = fullMDMEnrollmentURL + mdmEnrollmentURL = serverURLPrefix + apple_mdm.AccountDrivenEnrollPath }🤖 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/handler.go` around lines 1318 - 1324, Replace the manual concatenation that builds mdmEnrollmentURL with code that substitutes the "{token}" placeholder in the path constant instead of appending "/"+token: when token != "" compute the tokenized path by replacing "{token}" in apple_mdm.AccountDrivenEnrollTokenPath with url.PathEscape(token) (use strings.Replace or ReplaceAll), then combine that replaced path with fullMDMEnrollmentURL (ensuring no duplicate slashes, e.g., trim/right or format) and assign to mdmEnrollmentURL; leave the token=="" branch to use fullMDMEnrollmentURL.server/service/integration_mdm_test.go (1)
16233-16264: 💤 Low valueConsider adding explicit validation that all devices were found.
The switch statement correctly validates the three enrolled devices and their team assignments. However, if a device UUID doesn't match for any reason,
iPhoneHostIDandiPadHostIDwould remain nil, causing a panic when dereferenced later rather than a clear assertion failure.Consider adding explicit checks after the loop to improve test failure diagnostics:
require.NotNil(t, iPhoneHostID, "iPhone host not found in listing") require.NotNil(t, iPadHostID, "iPad host not found in listing")This would make test failures more immediately understandable, though the current implementation will still catch issues.
🤖 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/integration_mdm_test.go` around lines 16233 - 16264, After iterating listHostsRes.Hosts and assigning iPhoneHostID and iPadHostID inside the switch, add explicit assertions to verify both pointers are set (e.g., require.NotNil(t, iPhoneHostID, "iPhone host not found in listing") and require.NotNil(t, iPadHostID, "iPad host not found in listing")) immediately after the loop so missing-device cases fail with a clear test error; locate the check after the for _, host := range listHostsRes.Hosts loop where iPhoneHostID and iPadHostID are assigned.
🤖 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 `@ee/server/service/apple_mdm.go`:
- Around line 34-40: The code currently treats a nil enrollChallenge from
svc.ds.ConsumeADUEEnrollmentChallenge as an internal error (ctxerr.New) which
yields a 5xx; change that to return a client-facing authentication/validation
error (HTTP 4xx) so Apple sees a terminal auth failure: replace the
ctxerr.New(...) used when enrollChallenge == nil with the appropriate ctxerr
helper that signals an authentication/validation failure (e.g.,
ctxerr.NewUnauthenticated or ctxerr.NewValidation if available) and keep the
message "invalid enrollment reference" so the behavior of
ConsumeADUEEnrollmentChallenge and the enrollChallenge nil check is preserved
but surfaced as a 4xx client error rather than an internal server error.
In `@ee/server/service/mdm.go`:
- Around line 967-975: The callback currently treats an empty
account-driven-enroll suffix as a token and calls
svc.ds.GetABMTokenByUniqueToken on an empty string, causing an error; update the
block that uses strings.CutPrefix on ssoRequestData.Initiator (comparing to
fleet.SSOInitiatorAccountDrivenEnroll+":") to only call
svc.ds.GetABMTokenByUniqueToken and set abmTokenID when uniqueToken != ""
(non-empty), so the empty-suffix case is treated as the legacy flow the same way
InitiateMDMSSO does.
In `@server/datastore/mysql/apple_mdm.go`:
- Around line 7571-7576: The public GetADUEEnrollmentChallenge currently calls
getADUEEnrollmentChallenge via ds.reader but that helper includes a FOR UPDATE
lock; remove the locking read from the reader path by creating a non-locking
reader helper (e.g., getADUEEnrollmentChallenge without FOR UPDATE) and have
GetADUEEnrollmentChallenge call that; then create a separate tx-only locking
helper (e.g., getADUEEnrollmentChallengeForUpdate or reuse
getADUEEnrollmentChallenge but only from writer/transaction code) that includes
`FOR UPDATE` and is invoked only by ConsumeADUEEnrollmentChallenge (or other
writer functions) while passing a writer/transaction (not ds.reader); ensure the
reader helper uses the same SELECT fields but omits `FOR UPDATE` and that the
locking helper is only used inside a transaction taken from the writer pool so
locks behave correctly.
- Around line 7547-7564: InsertADUEEnrollmentChallenge currently sets expires_at
= time.Now().Add(expiration) which combined with the cleanup (lines around
7625-7627) yields retention = expiration + 24h; change the insert to set
expireAt = time.Now().Add(24*time.Hour) (ignore or validate the passed
expiration but enforce 24h per ADUE contract) so rows always expire 24 hours
from creation, and keep the cleanup logic as-is; update any function comment or
validation around InsertADUEEnrollmentChallenge to reflect that expires_at is
fixed to 24 hours.
In `@server/service/apple_mdm.go`:
- Around line 3877-3904: The BYOD team assignment happens after
EnqueueSetupExperienceItems is called, causing work to be queued against the
wrong team; fix by resolving the ADUE challenge/team prior to any enqueueing:
call svc.ds.GetADUEEnrollmentChallenge(...) and (if enrollChallenge.ABMTokenID
!= nil) fetch svc.ds.GetABMTokenByID(...) and determine
abmToken.BYODDefaultTeamID and, if non-nil, call svc.ds.AddHostsToTeam(...) or
set info.TeamID to the BYOD team before invoking
EnqueueSetupExperienceItems(..., info.TeamID); alternatively, after
AddHostsToTeam reload the effective team (e.g., refresh info.TeamID from the
datastore) before queuing setup/VPP work to ensure the enqueue uses the updated
team.
- Around line 6902-6912: The current handling in the block that calls
depSvc.GetMDMAppleServiceDiscoveryDetails returns from the parent function on
IsServiceDiscoveryNotSupported and on the default error branch, which aborts
processing remaining ABM tokens; change those returns into non-fatal handling so
the loop continues: for godep.IsServiceDiscoveryNotSupported(err) log the info
(logger.InfoContext) and continue processing the next token instead of return,
and for the default error branch replace the ctxerr.Wrap return with logging the
wrapped error (or logger.ErrorContext with ctxerr.Wrap(ctx, err, "...")) and
continue; keep the existing behavior for IsServiceDiscoveryNotFound. Ensure you
modify the code around depSvc.GetMDMAppleServiceDiscoveryDetails to use continue
rather than return so later tokens are still processed.
---
Outside diff comments:
In `@server/datastore/mysql/apple_mdm.go`:
- Around line 5833-5839: ListABMTokens, InsertABMToken, and SaveABMToken are
missing the byod_default_team_id column and associated select/params so the BYOD
default-team value read by getABMToken cannot round-trip; update the SQL in
ListABMTokens to include abt.byod_default_team_id (and COALESCE(t4.name,
:no_team) as byod_team in the SELECT joins), and update InsertABMToken and
SaveABMToken to include byod_default_team_id in their INSERT/UPDATE column lists
and bind parameters, and ensure the Go structs/scan/order of fields used by
those functions match the new column position so inserts, updates and list
responses carry the BYOD default-team correctly.
---
Nitpick comments:
In `@frontend/pages/MDMAppleSSOPage/MDMAppleSSOPage.tsx`:
- Around line 25-35: Validate params.token before interpolating into
query.initiator: in the block where
pathname.startsWith("/mdm/apple/account_driven_enroll/sso") is checked, ensure
params.token is a non-empty string (e.g., typeof params.token === "string" &&
params.token.trim().length > 0) before setting query.initiator =
`account_driven_enroll:${params.token}`; if the token is missing/invalid, fall
back to query.initiator = "account_driven_enroll" (or another safe default) to
avoid producing values like account_driven_enroll:undefined.
In `@server/service/handler.go`:
- Around line 1318-1324: Replace the manual concatenation that builds
mdmEnrollmentURL with code that substitutes the "{token}" placeholder in the
path constant instead of appending "/"+token: when token != "" compute the
tokenized path by replacing "{token}" in apple_mdm.AccountDrivenEnrollTokenPath
with url.PathEscape(token) (use strings.Replace or ReplaceAll), then combine
that replaced path with fullMDMEnrollmentURL (ensuring no duplicate slashes,
e.g., trim/right or format) and assign to mdmEnrollmentURL; leave the token==""
branch to use fullMDMEnrollmentURL.
In `@server/service/integration_mdm_test.go`:
- Around line 16233-16264: After iterating listHostsRes.Hosts and assigning
iPhoneHostID and iPadHostID inside the switch, add explicit assertions to verify
both pointers are set (e.g., require.NotNil(t, iPhoneHostID, "iPhone host not
found in listing") and require.NotNil(t, iPadHostID, "iPad host not found in
listing")) immediately after the loop so missing-device cases fail with a clear
test error; locate the check after the for _, host := range listHostsRes.Hosts
loop where iPhoneHostID and iPadHostID are assigned.
🪄 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: e52d3a56-4c83-4aa5-8fea-2ace33c9045b
📒 Files selected for processing (25)
changes/30871-default-byod-fleetcmd/fleet/cron.gocmd/fleet/serve.goee/server/service/apple_mdm.goee/server/service/mdm.gofrontend/pages/MDMAppleSSOPage/MDMAppleSSOPage.tsxfrontend/router/index.tsxpkg/mdm/mdmtest/apple.goserver/datastore/mysql/apple_mdm.goserver/datastore/mysql/apple_mdm_test.goserver/fleet/apple_mdm.goserver/fleet/cron_schedules.goserver/fleet/datastore.goserver/fleet/mdm.goserver/fleet/service.goserver/mdm/apple/apple_mdm.goserver/mock/datastore_mock.goserver/mock/service/service_mock.goserver/service/apple_mdm.goserver/service/apple_mdm_test.goserver/service/handler.goserver/service/integration_mdm_lifecycle_test.goserver/service/integration_mdm_setup_experience_test.goserver/service/integration_mdm_test.goserver/service/testing_client_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 82853b92-2d97-47d1-9f4b-b3b9f659caed
📒 Files selected for processing (8)
ee/server/service/apple_mdm.goee/server/service/mdm.goserver/datastore/mysql/apple_mdm.goserver/mdm/lifecycle/lifecycle.goserver/service/apple_mdm.goserver/service/handler.goserver/service/integration_mdm_test.goserver/service/testing_client_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- ee/server/service/mdm.go
- ee/server/service/apple_mdm.go
- server/service/integration_mdm_test.go
- server/service/apple_mdm.go
Comment on lines
+603
to
+605
| /* if _, ok := os.LookupEnv("SAML_IDP_TEST"); !ok { | ||
| t.Skip("SSO tests are disabled") | ||
| } | ||
| } */ |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find all SSO test helper methods and their SAML_IDP_TEST check usage
# Search for SSO test helper methods
echo "=== SSO test helper method definitions ==="
rg -n "func.*SSO.*\(.*testing\.T" server/service/testing_client_test.go
echo ""
echo "=== SAML_IDP_TEST checks ==="
rg -n -B2 -A2 "SAML_IDP_TEST" server/service/testing_client_test.go
echo ""
echo "=== Calls to loginSSOUserWithBody and LoginOTAEnrollSSOUser ==="
rg -n "loginSSOUserWithBody|LoginOTAEnrollSSOUser" server/service/Repository: fleetdm/fleet
Length of output: 1675
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="server/service/testing_client_test.go"
sed -n '430,520p' "$file" | cat -n
echo "----"
sed -n '520,620p' "$file" | cat -n
echo "----"
sed -n '620,720p' "$file" | cat -nRepository: fleetdm/fleet
Length of output: 11710
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "SAML_IDP_TEST" -S .Repository: fleetdm/fleet
Length of output: 846
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "testSAMLIDPSSOURL|testSAMLIDP" server/service/testing_client_test.go server/service/integration_sso_test.goRepository: fleetdm/fleet
Length of output: 1651
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "LoginMDMSSOUser\(|LoginAccountDrivenEnrollUser\(" server/serviceRepository: fleetdm/fleet
Length of output: 1593
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show nearest SAML_IDP_TEST gating around call sites in integration_mdm_test.go
file="server/service/integration_mdm_test.go"
rg -n "SAML_IDP_TEST" "$file" || true
echo "---- around 6482 ----"
sed -n '6450,6560p' "$file" | cat -n
echo "---- around 6772 ----"
sed -n '6720,6900p' "$file" | cat -n
echo "---- around 7186 ----"
sed -n '7130,7230p' "$file" | cat -n
echo "---- around 865 in integration_mdm_lifecycle_test.go ----"
file2="server/service/integration_mdm_lifecycle_test.go"
sed -n '820,920p' "$file2" | cat -n
echo "---- around 5166 in integration_mdm_setup_experience_test.go ----"
file3="server/service/integration_mdm_setup_experience_test.go"
sed -n '5130,5205p' "$file3" | cat -nRepository: fleetdm/fleet
Length of output: 26902
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "SAML_IDP_TEST" server/service/integration_mdm* server/service/*sso* server/service/integration* || trueRepository: fleetdm/fleet
Length of output: 243
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "SSO tests are disabled|setUpMDMSSO|FLEET_SAML_IDP_HTTP_PORT" server/service/testing_client_test.go server/service/integration_mdm* server/service/integration_sso_test.goRepository: fleetdm/fleet
Length of output: 1490
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="server/service/integration_mdm_test.go"
# show around setUpMDMSSO definition
sed -n '7240,7405p' "$file" | cat -nRepository: fleetdm/fleet
Length of output: 7073
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="server/service/testing_client_test.go"
sed -n '1,120p' "$file" | cat -nRepository: fleetdm/fleet
Length of output: 4278
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "simplesaml|SAML_IDP_HTTP_PORT|SimpleSAML|saml2/idp|SSOService.php" -S . | head -n 200Repository: fleetdm/fleet
Length of output: 9257
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "LookupEnv\\(\"SAML|Getenv\\(\"SAML|SAML_IDP_TEST|SAML_IDP_" server/service/integration_mdm* server/service/integration_mdm_* || trueRepository: fleetdm/fleet
Length of output: 39
Inconsistent SAML_IDP_TEST gating across SSO test helpers.
server/service/testing_client_test.go has the SAML_IDP_TEST skip block commented out in loginSSOUserWithBody (around lines 603-605), but it’s active in LoginOTAEnrollSSOUser (486-488) and in loginSSOUserIDPInitiated (662-663). Since LoginMDMSSOUser and LoginAccountDrivenEnrollUser both call loginSSOUserWithBody, those MDM SSO/BYOD flows aren’t skipped when SAML_IDP_TEST is unset, while the OTA enrollment helper is.
Make gating consistent (either re-enable the skip in loginSSOUserWithBody or document why MDM/account-driven flows must run even without SAML_IDP_TEST).
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 #45598
Needs #47161 for tests to pass.
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
Summary by CodeRabbit
New Features
Bug Fixes & Improvements
Tests