End-user authentication for Window/Linux setup experience: agent #34847
Conversation
There was a problem hiding this comment.
Implemented a new WithErrorFilter option for the retry package which allows you to change the behavior of the retry loop based on what kind of error is returned from the function being retried. This allows us to continuously retry the enroll endpoint every 30 seconds indefinitely when an "end user auth required" error is returned, regardless of the max number of attempts or backoff multiplier configured.
| func() error { | ||
| var err error | ||
| orbitNodeKey_, err = oc.enrollAndWriteNodeKeyFile() | ||
| switch { | ||
| case err == nil: | ||
| return nil | ||
| case errors.Is(err, notFoundErr{}): | ||
| // Do not retry if the endpoint does not exist. | ||
| endpointDoesNotExist = true | ||
| return nil | ||
| default: | ||
| logging.LogErrIfEnvNotSet(constant.SilenceEnrollLogErrorEnvVar, err, "enroll failed, retrying") | ||
| return err | ||
| } | ||
| return err |
There was a problem hiding this comment.
All of this logic is now moved into the WithErrorFilter() function, which was implemented so that we could retry indefinitely when waiting for the user to log in to the IdP.
| // If we get an ErrEndUserAuthRequired error, then the user | ||
| // needs to authenticate with the identity provider. | ||
| // | ||
| // Open a browser window to the sign-on page and | ||
| // then keep retrying until they authenticate. | ||
| log.Debug().Msg("enroll unauthenticated, waiting for end-user to authenticate via SSO") | ||
| if !oc.initiatedIdpAuth { | ||
| oc.initiatedIdpAuth = true | ||
| log.Debug().Msg("opening SSO window") | ||
| openWindowErr := oc.openSSOWindow() | ||
| if openWindowErr != nil { | ||
| log.Error().Err(openWindowErr).Msg("opening SSO window") | ||
| return retry.ErrorOutcomeNormalRetry | ||
| } | ||
| } | ||
| // Sleep for 20 seconds, making the total retry interval 30 seconds | ||
| time.Sleep(20 * time.Second) | ||
| return retry.ErrorOutcomeResetAttempts |
There was a problem hiding this comment.
Because we can't reliably close a browser window we've opened, for this first iteration we're just gonna open the window once per Orbit start. If the user closes it, they'll need to restart their computer to get it back. We may later implement a feature in Fleet Desktop to allow re-opening the SSO login window.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis PR implements SSO authentication during Orbit enrollment by adding error handling for end-user authentication requirements, introducing flexible error filtering for retries, registering an SSO window callback, and modifying the enrollment flow to trigger SSO authentication when needed with stateful retry management. Changes
Sequence DiagramsequenceDiagram
participant OrbitCmd as Orbit Command
participant OrbitClient as Orbit Client
participant Enroll as Enroll Endpoint
participant SSO as SSO Window
participant IdP as IdP
OrbitCmd->>OrbitClient: SetOpenSSOWindowFunc(callback)
OrbitCmd->>OrbitClient: Enrollment flow starts
OrbitClient->>Enroll: Attempt enrollment
Enroll-->>OrbitClient: 401 END_USER_AUTH_REQUIRED
OrbitClient->>OrbitClient: Detect error via filter
rect rgb(200, 220, 255)
Note over OrbitClient,SSO: First SSO attempt only
OrbitClient->>SSO: openSSOWindow() → browser callback
SSO->>IdP: Browser opens IdP auth
IdP-->>SSO: User completes auth
end
OrbitClient->>OrbitClient: Mark initiatedIdpAuth, wait 20s
OrbitClient->>OrbitClient: Reset retry attempts
OrbitClient->>Enroll: Retry enrollment (with auth context)
Enroll-->>OrbitClient: 200 OK (enrollment succeeds)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 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: 2
🧹 Nitpick comments (7)
server/service/base_client_errors.go (1)
19-19: Add a doc comment for the new exported error.Exported identifiers should have GoDoc. Add a short comment to satisfy linters and improve discoverability.
- ErrEndUserAuthRequired = errors.New("end user authentication required") + // ErrEndUserAuthRequired indicates the server requires end‑user SSO authentication before proceeding. + ErrEndUserAuthRequired = errors.New("end user authentication required")server/service/base_client.go (1)
52-60: Make 401 handling robust: parse error name and reason once.Relying on a substring in the reason is brittle. Parse the JSON and check the error name; keep a case‑insensitive check for password reset.
- errText := extractServerErrorText(response.Body) - if strings.Contains(errText, "password reset required") { + errName, errText := extractServerErrorNameReason(response.Body) + if strings.Contains(strings.ToLower(errText), "password reset required") { return ErrPasswordResetRequired } - if strings.Contains(errText, "END_USER_AUTH_REQUIRED") { + if errName == "END_USER_AUTH_REQUIRED" || strings.Contains(errText, "END_USER_AUTH_REQUIRED") { return ErrEndUserAuthRequired }pkg/retry/retry.go (1)
8-20: Document new exported retry extension points.Add brief GoDoc for the exported type, constants, and option to aid usage and linting. Logic looks good.
-type ErrorOutcome int +// ErrorOutcome tells retry.Do how to react to a returned error. +// Use with WithErrorFilter to control retry behavior. +type ErrorOutcome int @@ -const ( - ErrorOutcomeNormalRetry ErrorOutcome = iota - // Reset attempts counter and backoff. - // Useful for hijacking the retry cycle to retry - // indefinitely until a certain condition is met. - ErrorOutcomeResetAttempts - // Ignore the error and return as if successful. - ErrorOutcomeIgnore - // Bail out of the retry loop immediately. - ErrorOutcomeDoNotRetry -) +const ( + // ErrorOutcomeNormalRetry continues with the configured retry policy. + ErrorOutcomeNormalRetry ErrorOutcome = iota + // ErrorOutcomeResetAttempts resets attempts/backoff to initial values. + ErrorOutcomeResetAttempts + // ErrorOutcomeIgnore treats the error as success (no more retries). + ErrorOutcomeIgnore + // ErrorOutcomeDoNotRetry stops immediately and returns the error. + ErrorOutcomeDoNotRetry +) @@ -func WithErrorFilter(f func(error) ErrorOutcome) Option { +// WithErrorFilter sets a function that maps errors to retry outcomes. +// The filter is evaluated before max‑attempts/backoff handling. +func WithErrorFilter(f func(error) ErrorOutcome) Option { return func(c *config) { c.errorFilter = f } }Also applies to: 26-26, 55-59
pkg/retry/retry_test.go (2)
72-108: Prefer sentinel errors and errors.Is over string comparisons.Eliminates brittle string matching and makes intent clearer.
t.Run("with error filter (test ignore)", func(t *testing.T) { count := 0 + errNormal := errors.New("normal") + errReset := errors.New("reset") + errIgnore := errors.New("ignore") err := Do(func() error { count++ if count == 1 { - return errors.New("normal") + return errNormal } if count == 2 { - return errors.New("reset") + return errReset } if count == 3 { - return errors.New("ignore") + return errIgnore } return nil }, @@ - WithErrorFilter(func(err error) ErrorOutcome { - if err.Error() == "normal" { + WithErrorFilter(func(err error) ErrorOutcome { + if errors.Is(err, errNormal) { return ErrorOutcomeNormalRetry } - if err.Error() == "reset" { + if errors.Is(err, errReset) { return ErrorOutcomeResetAttempts } - if err.Error() == "ignore" { + if errors.Is(err, errIgnore) { return ErrorOutcomeIgnore } return ErrorOutcomeDoNotRetry }),
110-145: Apply the same sentinel pattern to the no‑retry test.Reduces flakiness and mirrors production usage with errors.Is.
t.Run("with error filter (test noretry)", func(t *testing.T) { count := 0 + errNormal := errors.New("normal") + errReset := errors.New("reset") + errStop := errors.New("stop") err := Do(func() error { count++ if count == 1 { - return errors.New("normal") + return errNormal } if count == 2 { - return errors.New("reset") + return errReset } if count == 3 { - return errors.New("stop") + return errStop } return nil }, @@ - WithErrorFilter(func(err error) ErrorOutcome { - if err.Error() == "normal" { + WithErrorFilter(func(err error) ErrorOutcome { + if errors.Is(err, errNormal) { return ErrorOutcomeNormalRetry } - if err.Error() == "reset" { + if errors.Is(err, errReset) { return ErrorOutcomeResetAttempts } - if err.Error() == "stop" { + if errors.Is(err, errStop) { return ErrorOutcomeDoNotRetry } return ErrorOutcomeNormalRetry }),server/service/orbit_client.go (2)
89-92: Add doc comment to exported setter; allow nil to clear.Small polish for API clarity.
-func (oc *OrbitClient) SetOpenSSOWindowFunc(f func() error) { +// SetOpenSSOWindowFunc sets the callback used to open the SSO login window. +// Pass nil to clear the callback. +func (oc *OrbitClient) SetOpenSSOWindowFunc(f func() error) { oc.openSSOWindow = f }
594-596: Prefer errors.As for not-found sentinel.errors.As is more reliable than comparing against a zero value of a struct error type.
- if errors.Is(err, notFoundErr{}) { + if errors.As(err, new(notFoundErr)) { return "", errors.New("enroll endpoint does not exist") }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
orbit/cmd/orbit/orbit.go(2 hunks)pkg/retry/retry.go(3 hunks)pkg/retry/retry_test.go(1 hunks)server/service/base_client.go(1 hunks)server/service/base_client_errors.go(1 hunks)server/service/orbit_client.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
When reviewing SQL queries that are added or modified, ensure that appropriate filtering criteria are applied—especially when a query is intended to return data for a specific entity (e.g., a single host). Check for missing WHERE clauses or incorrect filtering that could lead to incorrect or non-deterministic results (e.g., returning the first row instead of the correct one). Flag any queries that may return unintended results due to lack of precise scoping.
Files:
server/service/base_client.goserver/service/base_client_errors.gopkg/retry/retry.gopkg/retry/retry_test.goorbit/cmd/orbit/orbit.goserver/service/orbit_client.go
🔇 Additional comments (2)
pkg/retry/retry.go (1)
86-99: LGTM on integration into Do().Ordering (filter before max‑attempts/backoff) and state resets look correct.
orbit/cmd/orbit/orbit.go (1)
58-58: LGTM!The import is necessary for opening the SSO browser window in the callback function below.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #34847 +/- ##
==========================================
+ Coverage 66.04% 66.23% +0.18%
==========================================
Files 2066 2071 +5
Lines 172701 174614 +1913
Branches 6952 6952
==========================================
+ Hits 114061 115649 +1588
- Misses 48118 48335 +217
- Partials 10522 10630 +108
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Related issue: Resolves #34528
Details
This PR implements the agent changes for allowing Fleet admins to require that users authenticate with an IdP prior to having their devices set up. I'll comment on changes inline but the high-level is:
ErrEndUserAuthRequiredresponse, then it opens a window to the/mdm/ssoFleet page and retries the enroll endpoint every 30 seconds indefinitely.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](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-
changes.md#changes-files) for more information.
Will add changelog when story is one.
Testing
Added/updated automated tests
Added test for new retry logic
QA'd all new/changed functionality manually
This is kinda hard to test without the associated backend PR: #34835
fleetd/orbit/Fleet Desktop
This is compatible with all Fleet versions, since older ones won't send the new error.
runtime.GOOSis used as needed to isolate changesThis is compatible with all platforms, although it currently should only ever run on Windows and Linux since macOS devices will have end-user auth taken care of before they even download Orbit.
Testing this now.
Summary by CodeRabbit
New Features
Bug Fixes