Repository navigation
Refresh rejected controller credentials before reenrollment - #241
Conversation
Thanks, I checked both PRs. #233 would avoid this for token-enrolled devices, since they use certificate authentication without OIDC revalidation. That could be useful for our MDM deployment, but OIDC-enrolled devices retain the existing behavior. #236 changes initial enrollment and explicitly leaves existing connection, refresh, and renewal behavior unchanged. So I think #241 is still needed for OIDC-enrolled devices: when the controller rejects an access token, the daemon currently deletes the saved identity without first attempting a silent refresh. This PR adds that recovery attempt while preserving re-enrollment if refresh is rejected or the refreshed credentials still fail. |
|
I'm trying to think if it is worth separating out OIDC from enrollment. i.e. device only enroll with tokens, users enroll with OIDC. I'm still figuring out the tradeoffs here, but if we go in that direction, would that be a problem for this PR? |
|
That separation makes sense to me. We use Intune, so enrolling the device independently and handling user OIDC sessions separately sounds like a good fit. I’m happy to adapt or supersede this PR as that design takes shape. My immediate concern is having something reliable to roll out to our org. Could we merge this bounded recovery fix for the current OIDC flow while the broader enrollment changes are being worked out? It preserves re-enrollment when refresh is rejected or refreshed credentials still fail, and the regression tests cover those paths |
`main` has failed `Test and lint` since a33f11e. #241 added `auth_recovery_tests.rs`, which calls `run_session` with 10 arguments, and #231 added an `open_browser: bool` parameter to `run_session`. Each passed CI on its own; together they don't compile: ``` error[E0061]: this function takes 11 arguments but 10 arguments were supplied --> crates/agent/src/remote/auth_recovery_tests.rs:205:13 ``` This passes `false`. The flag only decides whether `oidc::enroll` launches a browser, so it doesn't change what these tests check, and a test should never open one. The test module is Linux-only (`cfg(all(test, target_os = "linux"))`), so the break doesn't show up in a macOS build. It also turns `Test and lint` red on every open PR, since PR checks build against `main`.
A controller stream can reject an access token while the device still has usable refresh credentials. The daemon currently treats that
Unauthenticatedresponse as a rejected device identity: it removes the saved enrollment and starts interactive sign-in again.Refresh once and reconnect before removing the identity. The refresh is forced even when the local token deadline has not elapsed. Rotated credentials are persisted, and transient refresh failures retain the enrollment and use the existing retry backoff. If the refresh grant is rejected, or the controller still rejects the refreshed credentials, the existing re-enrollment behavior remains. An authenticated stream resets the recovery allowance for a later token expiry.
Validation:
cargo fmt --all -- --checkpasses.cargo clippy --locked -p agentdesktop-agent --lib --tests -- -D warningspasses.These session tests use Linux's temporary file-backed secret store. A native macOS sleep/wake validation and installation of a signed client build remain pending.
Closes #245.