Conditional access host bypass - #38542
Merged
Merged
Conversation
dantecatalfamo
had a problem deploying
to
Docker Hub
January 20, 2026 21:16 — with 
GitHub Actions
Error
GitHub Actions
Error
dantecatalfamo
had a problem deploying
to
Docker Hub
January 20, 2026 21:17 — with
GitHub Actions
Failure
GitHub Actions
Failure
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #38542 +/- ##
========================================
Coverage 66.07% 66.08%
========================================
Files 2415 2417 +2
Lines 192800 192932 +132
Branches 8536 8431 -105
========================================
+ Hits 127399 127504 +105
- Misses 53832 53850 +18
- Partials 11569 11578 +9
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:
|
dantecatalfamo
temporarily deployed
to
Docker Hub
January 20, 2026 22:00 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
dantecatalfamo
temporarily deployed
to
Docker Hub
January 20, 2026 22:16 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
dantecatalfamo
marked this pull request as ready for review
January 20, 2026 22:17
dantecatalfamo
marked this pull request as draft
January 20, 2026 22:17
dantecatalfamo
temporarily deployed
to
Docker Hub
January 21, 2026 19:45 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
dantecatalfamo
temporarily deployed
to
Docker Hub
January 21, 2026 20:28 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
dantecatalfamo
temporarily deployed
to
Docker Hub
January 21, 2026 21:06 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
dantecatalfamo
marked this pull request as ready for review
January 21, 2026 21:16
dantecatalfamo
temporarily deployed
to
Docker Hub
January 21, 2026 21:23 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
✅ Actions performedFull review triggered. |
Contributor
WalkthroughImplements a host conditional access bypass mechanism enabling devices to request temporary bypass of failing policies, stored in a new database table. The bypass is checked and consumed during IDP session establishment, allowing sessions to proceed if a valid bypass exists. Changes
Sequence Diagram(s)sequenceDiagram
participant Device
participant Service as EE Service
participant Datastore
participant IDP
participant Session
rect rgba(100, 150, 200, 0.5)
Note over Device,IDP: Bypass Request Flow
Device->>Service: POST /bypass_conditional_access
Service->>Datastore: ConditionalAccessBypassDevice(hostID)
Datastore->>Datastore: INSERT/UPSERT bypass timestamp
Service->>Service: Log activity (HostBypassedConditionalAccess)
Service-->>Device: 200 OK
end
rect rgba(100, 200, 150, 0.5)
Note over IDP,Session: Policy Failure & Bypass Consumption
Device->>IDP: Authenticate (GetSession)
IDP->>IDP: Evaluate conditional access policies
alt Policy Fails
IDP->>Datastore: ConditionalAccessConsumeBypass(hostID)
alt Bypass Exists
Datastore->>Datastore: DELETE bypass (transaction)
Datastore-->>IDP: Returns bypassed_at timestamp
IDP->>Session: Allow session (bypass consumed)
Session-->>Device: Session established
else No Bypass
Datastore-->>IDP: Returns nil
IDP-->>Device: Redirect to remediation URL
end
else Policy Passes
IDP->>Session: Allow session
Session-->>Device: Session established
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/service/appconfig.go (1)
1072-1110: Guard bypass-change detection against unset input.
oktaBypassChangedusesnewAppConfig.ConditionalAccess.BypassDisabled.Valueeven when that field isn’t set, which can flip fromtrue → falseby zero-value and cause unnecessary bypass clears and activity spam. Consider checking.Setor comparing against the finalappConfigvalue instead.🛠️ Suggested fix
- oktaBypassChanged = oldAppConfig.ConditionalAccess.BypassDisabled.Value != newAppConfig.ConditionalAccess.BypassDisabled.Value + if newAppConfig.ConditionalAccess.BypassDisabled.Set { + oktaBypassChanged = oldAppConfig.ConditionalAccess.BypassDisabled.Value != appConfig.ConditionalAccess.BypassDisabled.Value + }
🤖 Fix all issues with AI agents
In `@ee/server/service/condaccess/idp.go`:
- Around line 392-415: The bypass consumption block must be gated by the same
bypass-enabled flag used by BypassConditionalAccess so disabled bypasses are
never honored; update the code around the ConditionalAccessConsumeBypass call to
first check the bypass feature flag (i.e. call the same function or config check
used by BypassConditionalAccess) and only call
p.ds.ConditionalAccessConsumeBypass(ctx, host.ID) and allow access when
bypassedAt != nil if that flag is true, otherwise skip consumption and redirect
to hostRemediationUrl as the non-bypass path does now.
In `@server/service/devices.go`:
- Around line 500-505: In bypassConditionalAccessEndpoint, the error branch
currently returns listDevicePoliciesResponse but should return
bypassConditionalAccessResponse; change the return to
bypassConditionalAccessResponse{Err: err}, nil so the endpoint always returns
the correct response shape (refer to bypassConditionalAccessEndpoint and the
bypassConditionalAccessResponse type when making the change).
In `@server/service/integration_enterprise_test.go`:
- Around line 23408-23413: The test is not asserting the response body because
s.DoRawNoAuth is used and does not unmarshal into bypassResp, so
require.Nil(bypassResp.Err) is ineffective; change the call to use s.DoJSON (or
otherwise decode the HTTP response) when calling the POST to
"/api/v1/fleet/device/test-conditional-access-bypass-token/bypass_conditional_access"
so the response is unmarshaled into the bypassConditionalAccessResponse variable
(bypassResp) and then assert on bypassResp.Err; apply the same replacement for
the other occurrence around lines 23466-23469.
- Around line 23395-23406: The cleanup function leaves Okta's bypass_disabled
flag set, causing state leakage into other tests; update
clearOktaConditionalAccess (or the TestConditionalAccessBypass cleanup) to reset
bypass_disabled to its default. Locate clearOktaConditionalAccess and modify the
request body sent there to include bypass_disabled: nil (or explicitly
bypass_disabled: false) so the flag is cleared, or alternatively capture the
original bypass_disabled value at test start in TestConditionalAccessBypass and
restore it in the t.Cleanup closure to ensure test isolation.
🧹 Nitpick comments (3)
ee/server/service/devices.go (1)
118-126: Optional: keep a non-empty IdP name in activities.If
IdpFullNameis empty, the activity ends up blank. Consider falling back toIdpUserNameor the default label.♻️ Suggested tweak
- idpFullName := "An end user" - endUsers, err := fleet.GetEndUsers(ctx, svc.ds, host.ID) + idpFullName := "An end user" + endUsers, err := fleet.GetEndUsers(ctx, svc.ds, host.ID) if err != nil { return ctxerr.Wrap(ctx, err, "getting end users for bypass activity") } - if len(endUsers) > 0 { - idpFullName = endUsers[0].IdpFullName - } + if len(endUsers) > 0 { + if endUsers[0].IdpFullName != "" { + idpFullName = endUsers[0].IdpFullName + } else if endUsers[0].IdpUserName != "" { + idpFullName = endUsers[0].IdpUserName + } + }server/fleet/datastore.go (1)
828-836: LGTM! Interface methods for conditional access bypass are well-defined.The three new methods properly scope operations:
ConditionalAccessBypassDeviceandConditionalAccessConsumeBypasscorrectly usehostIDto target specific hostsConditionalAccessClearBypassesintentionally clears all bypasses (for the policy modification use case per PR objectives)The
(*time.Time, error)return type appropriately distinguishes between "no bypass exists" (nil) and "bypass was created at this time".Minor nit: The comment on line 830-831 has slightly redundant wording ("consumes the bypass checks and consumes"). Consider rewording for clarity:
📝 Suggested comment clarification (optional)
- // ConditionalAccessConsumeBypass consumes the bypass checks and consumes any conditional access - // bypass a device has. If a bypass is present, it will return the time the bypass was enabled. + // ConditionalAccessConsumeBypass checks for and consumes any conditional access bypass a device + // has. If a bypass is present, it deletes it and returns the time the bypass was enabled. // If a bypass is not present, it will return nil.server/datastore/mysql/schema.sql (1)
563-571: App-level cleanup forhost_conditional_access.host_idalready exists; add FK constraint as defense-in-depth enhancement.App-level cleanup is already in place—
host_conditional_accessrecords are explicitly deleted during host deletion (verified in the hosts deletion flow and tested intestConditionalAccessBypassDeletedWithHost). However, adding aFOREIGN KEY ... ON DELETE CASCADEconstraint would provide database-level enforcement and protect against deletion paths that bypass app-level cleanup.🔧 Proposed FK (apply via migration, then regenerate schema)
CREATE TABLE `host_conditional_access` ( `id` int unsigned NOT NULL AUTO_INCREMENT, `host_id` int unsigned NOT NULL, `bypassed_at` timestamp NULL DEFAULT NULL, `created_at` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP, `updated_at` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, PRIMARY KEY (`id`), - UNIQUE KEY `idx_host_conditional_access_host_id` (`host_id`) + UNIQUE KEY `idx_host_conditional_access_host_id` (`host_id`), + CONSTRAINT `fk_host_conditional_access_host_id` FOREIGN KEY (`host_id`) REFERENCES `hosts` (`id`) ON DELETE CASCADE ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci;
Contributor
WalkthroughThis pull request implements a conditional access bypass feature for hosts. It introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant Device as Device<br/>(Enrolled Host)
participant Service as Service<br/>(Devices Endpoint)
participant DS as Datastore
participant IdP as IdP<br/>(Okta/Azure)
participant Session as Session<br/>Generator
Device->>Service: POST /bypass_conditional_access<br/>(device token)
Service->>Service: Extract host from token
Service->>Service: Validate license
Service->>DS: ConditionalAccessBypassDevice(host_id)
DS->>DS: INSERT/UPDATE bypass timestamp
DS-->>Service: OK
Service->>Service: Create activity log
Service-->>Device: 200 OK
Note over Device,Session: Later: Device authenticates via IdP
Device->>IdP: SAML authentication request
IdP->>IdP: Verify credentials
IdP->>Service: GetSession request (post-auth)
Service->>DS: ConditionalAccessConsumeBypass(host_id)
DS->>DS: BEGIN transaction
DS->>DS: SELECT bypass timestamp<br/>(FOR UPDATE SKIP LOCKED)
DS->>DS: DELETE bypass record
DS-->>DS: COMMIT
DS-->>Service: timestamp (or nil)
alt Bypass exists
Service->>Service: Log: bypass consumed
Service->>Session: Proceed to session generation
Session-->>IdP: Valid session
else No bypass
Service->>Service: Redirect to remediation
Service-->>IdP: Remediation URL
end
IdP-->>Device: SAML response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 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 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@server/service/devices.go`:
- Around line 500-512: In bypassConditionalAccessEndpoint, the error branch
returns the wrong response type (listDevicePoliciesResponse) which causes an
incorrect JSON structure; change the returned value in the missing-host error
case to bypassConditionalAccessResponse{Err: err} so both error branches in
bypassConditionalAccessEndpoint consistently return
bypassConditionalAccessResponse, keeping the function signature and existing
ctxerr.Wrap usage unchanged.
In `@server/service/integration_enterprise_test.go`:
- Around line 23409-23412: The test declares bypassResp and then calls
s.DoRawNoAuth(...) which does not unmarshal into bypassResp, so require.Nil(t,
bypassResp.Err) is meaningless; change the call to use s.DoJSON("POST",
"/api/v1/fleet/device/test-conditional-access-bypass-token/bypass_conditional_access",
nil, http.StatusOK, &bypassResp) so the response is decoded into bypassResp
before asserting require.Nil(t, bypassResp.Err), or alternatively remove the
require.Nil assertion and keep only the status-code check if you don't need to
inspect bypassResp.
- Around line 23486-23489: The test resets bypassResp to its zero value and then
calls s.DoRawNoAuth which does not populate it, so require.Nil(t,
bypassResp.Err) merely checks the zero value; change the test to let the HTTP
helper populate bypassResp and then assert on it: call s.DoRawNoAuth (or s.DoRaw
if that supports a destination) with &bypassResp as the response destination so
the JSON body from the POST to
"/api/v1/fleet/device/test-conditional-access-bypass-token/bypass_conditional_access"
is unmarshaled into bypassResp, then assert require.Nil(t, bypassResp.Err);
alternatively, if DoRawNoAuth cannot accept a destination, capture the response
body from s.DoRawNoAuth and json.Unmarshal into bypassResp before asserting.
🧹 Nitpick comments (1)
server/service/integration_enterprise_test.go (1)
23467-23468: Same issue:DoRawNoAuthdoesn't verify response content.This call also uses
DoRawNoAuthwithout decoding the response. If the intent is to only verify the HTTP status code (200 OK), this is fine. However, for consistency with other bypass calls in this test (e.g., line 23522), consider usingDoJSONto decode and verify the response.
getvictor
reviewed
Jan 22, 2026
getvictor
left a comment
getvictor
left a comment
Member
There was a problem hiding this comment.
This is a decent size change. Thanks for getting it working.
I made several comments. Some of the code feels AI-generated, since it doesn't follow our conventions. If so, please make sure to review the code carefully. Putting some of our conventions into agents.md or CLAUDE.md may help.
dantecatalfamo
temporarily deployed
to
Docker Hub
January 23, 2026 21:21 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
dantecatalfamo
had a problem deploying
to
Docker Hub
January 23, 2026 21:57 — with
GitHub Actions
Failure
GitHub Actions
Failure
dantecatalfamo
temporarily deployed
to
Docker Hub
January 23, 2026 22:10 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
dantecatalfamo
had a problem deploying
to
Docker Hub
January 26, 2026 21:05 — with
GitHub Actions
Error
GitHub Actions
Error
dantecatalfamo
had a problem deploying
to
Docker Hub
January 26, 2026 21:08 — with
GitHub Actions
Error
GitHub Actions
Error
This reverts commit 297aa15.
dantecatalfamo
temporarily deployed
to
Docker Hub
January 26, 2026 21:12 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Member
Author
|
Accidentally included a little bit of js from another branch |
dantecatalfamo
temporarily deployed
to
Docker Hub
January 26, 2026 21:25 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
dantecatalfamo
temporarily deployed
to
Docker Hub
January 26, 2026 21:39 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Member
Author
|
@getvictor Should be good to go now |
getvictor
approved these changes
Jan 26, 2026
2 tasks
3 tasks
8 tasks
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
3 participants
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 #37280
Also included a fix for the
conditional_access_enabledpolicy info because it's required for the upcoming frontend.SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements)Testing
Database migrations
COLLATE utf8mb4_unicode_ci).Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.