UI: Allow conditional access bypass per-policy - #39667
Conversation
GitHub Actions
Inactive
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #39667 +/- ##
==========================================
- Coverage 66.21% 66.21% -0.01%
==========================================
Files 2436 2436
Lines 194968 194982 +14
Branches 8594 8600 +6
==========================================
Hits 129100 129100
- Misses 54157 54171 +14
Partials 11711 11711
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:
|
WalkthroughThis PR implements the frontend UI for per-policy conditional access bypass configuration. It adds a new 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 unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/pages/policies/ManagePoliciesPage/ManagePoliciesPage.tsx (1)
777-788:⚠️ Potential issue | 🔴 CriticalBug:
responses.concat(...)doesn't mutateresponses— policy updates are never awaited.
Array.prototype.concatreturns a new array; it does not modifyresponsesin place. The promises from thechangedPolicies.map(...)call are silently discarded, soPromise.all(responses)on Line 788 never awaits the policy updates. This means:
- The success flash may appear before policies are actually saved.
- Errors from policy updates are silently swallowed.
Note: the same pre-existing bug exists at Line 710 in
onUpdateCalendarEvents.🐛 Proposed fix
- responses.concat( - changedPolicies.map((changedPolicy) => { + responses.push( + ...changedPolicies.map((changedPolicy) => { return teamPoliciesAPI.update(changedPolicy.id, { conditional_access_enabled: changedPolicy.conditional_access_enabled, conditional_access_bypass_enabled: changedPolicy.conditional_access_bypass_enabled, team_id: teamIdForApi, }); }) );And for the pre-existing calendar events bug at Line 710:
- responses.concat( - formData.changedPolicies.map((changedPolicy) => { + responses.push( + ...formData.changedPolicies.map((changedPolicy) => { return teamPoliciesAPI.update(changedPolicy.id, { calendar_events_enabled: changedPolicy.calendar_events_enabled, team_id: teamIdForApi, }); }) );frontend/pages/admin/IntegrationsPage/cards/ConditionalAccess/ConditionalAccess.tsx (1)
348-376:⚠️ Potential issue | 🟠 MajorAdd
microsoft_entra_connection_configuredto the update payload inhandleSaveBypassSettings.The Okta deletion handler (visible in the search results) explicitly includes both
microsoft_entra_tenant_idandmicrosoft_entra_connection_configuredwhen updating the config. However,handleSaveBypassSettingsonly includesmicrosoft_entra_tenant_idand omitsmicrosoft_entra_connection_configured.Since the backend saves the entire
ConditionalAccessSettingsobject as JSON andmicrosoft_entra_connection_configuredis a plainboolfield (not using optjson for partial updates), omitting it will cause it to be reset tofalseduring the update, inadvertently breaking an active Microsoft Entra integration.To match the pattern in the delete handler and preserve the Entra connection state, add:
microsoft_entra_connection_configured: config?.conditional_access?.microsoft_entra_connection_configured || false,
🤖 Fix all issues with AI agents
In
`@frontend/pages/admin/IntegrationsPage/cards/ConditionalAccess/ConditionalAccess.tsx`:
- Around line 465-479: In ConditionalAccess.tsx fix two minor UI text issues:
inside the TooltipWrapper for Microsoft Entra ensure there is a space between
the closing <b> tag and the entraTenantId expression so the tooltip renders
"Tenant ID: {entraTenantId}" with a space, and make the trailing sentence
consistent by adding a period to the Entra status text so the phrase rendered by
the entraContent JSX ("Microsoft Entra ... conditional access connected.")
matches the Okta variant; locate the TooltipWrapper and the entraContent JSX to
apply these changes.
In
`@frontend/pages/policies/ManagePoliciesPage/components/ConditionalAccessModal/ConditionalAccessModal.tsx`:
- Around line 85-121: renderItemRow currently always renders a clickable
span/Checkbox even when the parent list is disabled via
disableList={!formData.enabled}; update renderItemRow to accept or read the
disabled state (from the prop passed to PoliciesPaginatedList or
formData.enabled) and early-return null when disabled OR render a
non-interactive Checkbox by setting its disabled prop and removing/guarding the
span onClick stopPropagation so it cannot be clicked; update the call site where
renderItemRow is passed to PoliciesPaginatedList to pass the disabled flag
through (or reference formData.enabled inside renderItemRow) and ensure the
Checkbox uses conditional prop conditional_access_bypass_enabled only for
display when disabled.
🧹 Nitpick comments (1)
frontend/pages/hosts/details/DeviceUserPage/DeviceUserPage.tests.tsx (1)
609-627: Missing test forconditional_access_bypass_enabled: false.There's a test verifying the button is hidden when
conditional_access_enabledis false, but no equivalent test whenconditional_access_bypass_enabledis false. SinceDeviceUserPage.tsxnow gates on both fields, consider adding a test to verifyResolve lateris hidden when a policy hasconditional_access_bypass_enabled: false.
GitHub Actions
Inactive
2 participants
Related issue: Resolves #39000
Summary by CodeRabbit