Add Entra device authentication admin UI#621
Add Entra device authentication admin UI#621thvevirtue wants to merge 1 commit intonetbirdio:mainfrom
Conversation
Implements the dashboard-facing half of the Entra device auth feature delivered in the backend (netbirdio/netbird#5977). Adds: - New page under /integrations/entra-device-auth with two tabs: * Configuration (singleton integration config: tenant/client/secret, issuer, audience, Intune compliance toggle, tenant-only fallback + fallback auto-groups, mapping resolution strategy, revalidation interval). * Mappings (CRUD list of Entra security group → NetBird auto-groups mappings with ephemeral, extra-DNS-labels, expires-in, priority, revoked). - Sidebar navigation entry gated on the optional entra_device_auth permission module (defaults to visible for backward compatibility). - TypeScript interfaces mirroring the backend integrationDTO / mappingDTO shapes. - Optional entra_device_auth permission field on Permissions to remain compatible with management servers that don't publish the new module yet. UI patterns follow existing setup-keys and identity-providers modules. Uses the existing useApiCall/useFetchApi hooks, PeerGroupSelector for auto-groups, FancyToggleSwitch for booleans, and notify() for operation feedback.
|
|
📝 WalkthroughWalkthroughIntroduces a new Entra Device Auth integration module for the dashboard, comprising route layouts, page components, type definitions, configuration management, mapping management with table/modal/action components, navigation entry, and icon assets for managing Microsoft Entra device authentication settings and group mappings. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
src/modules/integrations/entra-device-auth/EntraDeviceMappingModal.tsx (1)
113-130:submitstarts the API call beforenotifyis wired up.
const call = isEditing ? updateRequest.put(payload) : createRequest.post(payload);invokes the request synchronously on this line;notify({ promise: call.then(...) })then attaches handlers to the already-running promise. This works, but if any synchronous error occurs between request kickoff and thenotifycall, you'll get an unhandled rejection. Prefer passing a thunk/promise factory directly, or awaiting inside a try/catch that surfaces errors vianotify.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/integrations/entra-device-auth/EntraDeviceMappingModal.tsx` around lines 113 - 130, The submit function currently starts the HTTP call immediately by evaluating const call = isEditing ? updateRequest.put(payload) : createRequest.post(payload) before wiring notify, risking unhandled rejections; change submit to either (A) pass a promise factory to notify (i.e., provide a thunk that when invoked calls updateRequest.put(payload) or createRequest.post(payload) and then runs mutate("/integrations/entra-device-auth/mappings"), mutate("/groups") and onClose()), or (B) await the request inside a try/catch around buildRequest and surface errors via notify (use notify to report the awaited promise result and perform mutate/onClose in the success path). Reference functions/variables: submit, buildRequest, updateRequest.put, createRequest.post, notify, mutate, onClose.src/modules/integrations/entra-device-auth/EntraDeviceMappingActionCell.tsx (1)
14-25:onEditprop is declared but never consumed.
Props.onEditis part of the public type but the component only destructuresmapping. Either wire it into a hypothetical edit control here, or drop it fromPropsto keep the component surface honest.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/integrations/entra-device-auth/EntraDeviceMappingActionCell.tsx` around lines 14 - 25, The Props type declares onEdit but EntraDeviceMappingActionCell only destructures mapping; update the component to either accept and use onEdit (add onEdit to the function signature and hook it up to the edit UI element in this file, e.g., the edit button or menu handler within EntraDeviceMappingActionCell) or remove onEdit from the Props type if no edit behavior is intended; locate the Props declaration and the EntraDeviceMappingActionCell function to implement the chosen change (symbols: Props, onEdit, EntraDeviceMappingActionCell, mapping).src/modules/integrations/entra-device-auth/EntraDeviceMappingsTable.tsx (1)
232-237: Modal remounts on close, likely skipping the close animation.
key={modalOpen ? (editMapping?.id ?? "new") : "closed"}changes the key whenever the modal transitions from open to closed, which forces React to unmount the currently-open instance synchronously and mount a fresh one withopen={false}. Any dismiss animation or async cleanup insideEntraDeviceMappingModalwill be bypassed. You only need a fresh instance when switching between mappings — the key can stay stable across the open → close transition.♻️ Proposed refactor
- <EntraDeviceMappingModal - open={modalOpen} - onClose={handleCloseModal} - mapping={editMapping} - key={modalOpen ? (editMapping?.id ?? "new") : "closed"} - /> + <EntraDeviceMappingModal + open={modalOpen} + onClose={handleCloseModal} + mapping={editMapping} + key={editMapping?.id ?? "new"} + />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/integrations/entra-device-auth/EntraDeviceMappingsTable.tsx` around lines 232 - 237, The modal is being force-remounted on close because the key includes modalOpen; change the key logic so it only changes when the mapping itself changes (e.g., derive key from editMapping?.id or omit key entirely) so that EntraDeviceMappingModal is not unmounted when open → closed; keep props open, onClose and mapping as-is and ensure key no longer depends on modalOpen to preserve dismiss animations and async cleanup inside EntraDeviceMappingModal.src/modules/integrations/entra-device-auth/EntraDeviceAuthConfig.tsx (1)
297-334: Toggle/select disable logic ignorescanCreateduring first-time setup.The pattern
disabled={!canUpdate && isEditing}only disables inputs in the edit flow. A user who lacks bothcreateandupdatepermissions (or has onlyread) but reaches this page via navigation can still toggle every switch and edit the select — the only thing blocking them is the submit button. It's a minor consistency nit; consider disabling inputs when the corresponding mutation isn't permitted:disabled={isEditing ? !canUpdate : !canCreate}Applies to Lines 307, 320, 333, 346, 361, and 392. Server remains source of truth, so this is purely UX polish.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/integrations/entra-device-auth/EntraDeviceAuthConfig.tsx` around lines 297 - 334, The toggle and select components (e.g., the FancyToggleSwitch instances controlling enabled, requireIntune, allowTenantOnlyFallback and their onChange handlers setEnabled, setRequireIntune, setAllowTenantOnlyFallback) currently compute disabled as {!canUpdate && isEditing}; update this to consider create permissions during initial setup by using the conditional pattern isEditing ? !canUpdate : !canCreate for the disabled prop so inputs are disabled when the user lacks create rights on first-time setup and lacks update rights when editing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/`(dashboard)/integrations/entra-device-auth/page.tsx:
- Around line 40-45: The "Integrations" breadcrumb in the Breadcrumbs component
currently links to the same page via Breadcrumbs.Item with
href="/integrations/entra-device-auth"; update that item so it doesn't point to
the current page—either remove the href prop from the parent Breadcrumbs.Item or
change it to a meaningful parent route such as "/integrations" (modify the
Breadcrumbs.Item that has label="Integrations" and
href="/integrations/entra-device-auth" accordingly).
- Around line 28-34: The page component EntraDeviceAuthPage is calling
useSearchParams() directly which in Next.js App Router requires a Suspense
boundary to avoid forcing client-side rendering; extract the tab-dependent UI
(starting at RestrictedAccess and any components that rely on queryParams or
initialTab) into a child Client Component (e.g., EntraDeviceAuthBody or
EntraDeviceAuthClient) and keep EntraDeviceAuthPage as a server component that
imports that client component, then wrap the client child in a React.Suspense
with an appropriate fallback (e.g., loading skeleton) at the page level so
useSearchParams() runs inside the Suspense boundary and the page maintains
static rendering.
In `@src/modules/integrations/entra-device-auth/EntraDeviceAuthConfig.tsx`:
- Around line 134-140: buildRequest() currently always includes
fallback_auto_groups from groups even when allowTenantOnlyFallback is false;
update buildRequest() to omit fallback_auto_groups (or send an empty array) when
allowTenantOnlyFallback is false and short-circuit any caller logic that invokes
saveFallbackGroups()—specifically, check the allowTenantOnlyFallback flag before
adding fallback_auto_groups to the returned payload in buildRequest(), and
before calling saveFallbackGroups() (or call it only when
allowTenantOnlyFallback is true) to avoid persisting or creating unused groups.
- Around line 84-104: The hydration effect currently never populates the
useGroupHelper state so fallbackGroups stays empty and saveFallbackGroups()
returns [] in buildRequest; inside the existing useEffect that reads data, call
setFallbackGroups(data.fallback_auto_groups ?? []) (or an empty array if
missing) to hydrate the group helper from server data, referencing the
useGroupHelper hook symbols (fallbackGroups, setFallbackGroups,
saveFallbackGroups) and the async payload field data.fallback_auto_groups so
subsequent buildRequest() uses the correct groups.
In `@src/modules/integrations/entra-device-auth/EntraDeviceMappingModal.tsx`:
- Around line 90-111: The buildRequest currently treats parsed days <= 0 as
"never" expiry; update the validation so zero/negative values are rejected
instead of silently ignored: adjust the isDisabled logic that currently only
checks parseInt succeeds to also treat parsed days <= 0 as invalid, and set the
UI/input to enforce a minimum (e.g., change the <Input> for expiresInDays to
include min={1} and show a validation/error message) so admins cannot submit 0
or negative numbers; keep buildRequest as-is (it may still compute expires_at
when days > 0) but rely on the tightened isDisabled/UI validation to prevent <=
0 from being submitted.
- Around line 69-75: The current initialization of expiresInDays (based on
mapping?.expires_at with diffMs <= 0 returning "") causes buildRequest to send
expires_at: null for expired mappings; change the logic so an expired mapping
does not get cleared unless the user edits the expiry: initialize expiresInDays
to a sentinel (e.g., undefined or a special value like "UNCHANGED") when
mapping.expires_at exists but is in the past, display the original timestamp or
an "Expired" read-only hint in the form, and update buildRequest to only
include/override expires_at when expiresInDays contains a user-edited numeric
value (otherwise leave expires_at as the original mapping.expires_at). Ensure
you update the state initialization (expiresInDays setter), the UI display for
expiry, and the buildRequest function to respect the untouched sentinel.
In `@src/modules/integrations/entra-device-auth/EntraDeviceMappingsTable.tsx`:
- Around line 47-60: The empty-state flashes because hasIntegration is computed
from integration?.id while the integration fetch is in-flight; update the
EntraDeviceMappingsTable to read the loading flag from the integration fetch
(e.g., const { data: integration, isLoading: isLoadingIntegration } =
useFetchApi<EntraDeviceAuth>("/integrations/entra-device-auth", true)) and
change hasIntegration to incorporate that flag (e.g., const hasIntegration =
!isLoadingIntegration && !!integration?.id) so the "Configure the integration
first" card is not shown until the integration request completes.
---
Nitpick comments:
In `@src/modules/integrations/entra-device-auth/EntraDeviceAuthConfig.tsx`:
- Around line 297-334: The toggle and select components (e.g., the
FancyToggleSwitch instances controlling enabled, requireIntune,
allowTenantOnlyFallback and their onChange handlers setEnabled,
setRequireIntune, setAllowTenantOnlyFallback) currently compute disabled as
{!canUpdate && isEditing}; update this to consider create permissions during
initial setup by using the conditional pattern isEditing ? !canUpdate :
!canCreate for the disabled prop so inputs are disabled when the user lacks
create rights on first-time setup and lacks update rights when editing.
In `@src/modules/integrations/entra-device-auth/EntraDeviceMappingActionCell.tsx`:
- Around line 14-25: The Props type declares onEdit but
EntraDeviceMappingActionCell only destructures mapping; update the component to
either accept and use onEdit (add onEdit to the function signature and hook it
up to the edit UI element in this file, e.g., the edit button or menu handler
within EntraDeviceMappingActionCell) or remove onEdit from the Props type if no
edit behavior is intended; locate the Props declaration and the
EntraDeviceMappingActionCell function to implement the chosen change (symbols:
Props, onEdit, EntraDeviceMappingActionCell, mapping).
In `@src/modules/integrations/entra-device-auth/EntraDeviceMappingModal.tsx`:
- Around line 113-130: The submit function currently starts the HTTP call
immediately by evaluating const call = isEditing ? updateRequest.put(payload) :
createRequest.post(payload) before wiring notify, risking unhandled rejections;
change submit to either (A) pass a promise factory to notify (i.e., provide a
thunk that when invoked calls updateRequest.put(payload) or
createRequest.post(payload) and then runs
mutate("/integrations/entra-device-auth/mappings"), mutate("/groups") and
onClose()), or (B) await the request inside a try/catch around buildRequest and
surface errors via notify (use notify to report the awaited promise result and
perform mutate/onClose in the success path). Reference functions/variables:
submit, buildRequest, updateRequest.put, createRequest.post, notify, mutate,
onClose.
In `@src/modules/integrations/entra-device-auth/EntraDeviceMappingsTable.tsx`:
- Around line 232-237: The modal is being force-remounted on close because the
key includes modalOpen; change the key logic so it only changes when the mapping
itself changes (e.g., derive key from editMapping?.id or omit key entirely) so
that EntraDeviceMappingModal is not unmounted when open → closed; keep props
open, onClose and mapping as-is and ensure key no longer depends on modalOpen to
preserve dismiss animations and async cleanup inside EntraDeviceMappingModal.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: faef74fd-5bf8-497e-a695-d2913d0e0e6e
📒 Files selected for processing (11)
src/app/(dashboard)/integrations/entra-device-auth/layout.tsxsrc/app/(dashboard)/integrations/entra-device-auth/page.tsxsrc/app/(dashboard)/integrations/layout.tsxsrc/assets/icons/EntraDeviceIcon.tsxsrc/interfaces/EntraDeviceAuth.tssrc/interfaces/Permission.tssrc/layouts/Navigation.tsxsrc/modules/integrations/entra-device-auth/EntraDeviceAuthConfig.tsxsrc/modules/integrations/entra-device-auth/EntraDeviceMappingActionCell.tsxsrc/modules/integrations/entra-device-auth/EntraDeviceMappingModal.tsxsrc/modules/integrations/entra-device-auth/EntraDeviceMappingsTable.tsx
| export default function EntraDeviceAuthPage() { | ||
| const { permission } = usePermissions(); | ||
| const queryParams = useSearchParams(); | ||
| const initialTab = useMemo(() => queryParams.get("tab") ?? "config", [ | ||
| queryParams, | ||
| ]); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Next.js 16 useSearchParams Suspense boundary requirement app router
💡 Result:
In Next.js 16 with the App Router, using the useSearchParams hook (a Client Component hook from next/navigation) requires wrapping the component that calls it in a Suspense boundary. This is necessary during static prerendering at build time, when no request/query parameters exist, causing the hook to suspend. Without Suspense, the entire page opts into full client-side rendering (CSR bailout), potentially leaving the page blank until JS loads, and failing the build with a "Missing Suspense boundary with useSearchParams" error. This requirement has not changed in Next.js 16; Next.js 16 updates (like async searchParams props in Server Components) do not affect useSearchParams behavior. ### Key Rules: - Works in development without Suspense (on-demand rendering). - Fails production build without Suspense for static routes. - Isolate to smallest subtree: wrap only the Client Component using the hook. ### Example Fix: tsx // Client Component using the hook 'use client'; import { useSearchParams } from 'next/navigation'; function Search({ searchParamsPromise }: { searchParamsPromise: Promise<{ q?: string }> }) { const searchParams = use(searchParamsPromise); // Or directly: const searchParams = useSearchParams; const q = searchParams.get('q'); return <input defaultValue={q || ''} />; } // Server Component Page/Layout import { Suspense } from 'react'; export default function Page({ searchParams }: { searchParams: Promise<{ q?: string }> }) { return ( <Suspense fallback={
Citations:
- 1: https://nextjs.org/docs/messages/missing-suspense-with-csr-bailout
- 2: https://nextjs.org/docs/app/api-reference/functions/use-search-params
- 3: https://nextjs.org/blog/next-16
- 4: https://nextjs.org/docs/app/guides/upgrading/version-16
- 5: https://github.com/vercel/next.js/releases/tag/v16.0.0
🏁 Script executed:
cat -n src/app/\(dashboard\)/integrations/entra-device-auth/page.tsxRepository: netbirdio/dashboard
Length of output: 3965
Wrap useSearchParams() usage in a <Suspense> boundary.
In Next.js 16 with the App Router, calling useSearchParams() requires the component to be wrapped in a <Suspense> boundary. Without it, the entire page opts into client-side rendering and will fail the build-time check. Extract the tab body content (e.g., RestrictedAccess onwards) into a child Client Component and wrap it with <Suspense fallback={...}> at the page level to maintain static rendering and satisfy the build requirement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/`(dashboard)/integrations/entra-device-auth/page.tsx around lines 28
- 34, The page component EntraDeviceAuthPage is calling useSearchParams()
directly which in Next.js App Router requires a Suspense boundary to avoid
forcing client-side rendering; extract the tab-dependent UI (starting at
RestrictedAccess and any components that rely on queryParams or initialTab) into
a child Client Component (e.g., EntraDeviceAuthBody or EntraDeviceAuthClient)
and keep EntraDeviceAuthPage as a server component that imports that client
component, then wrap the client child in a React.Suspense with an appropriate
fallback (e.g., loading skeleton) at the page level so useSearchParams() runs
inside the Suspense boundary and the page maintains static rendering.
| <Breadcrumbs> | ||
| <Breadcrumbs.Item | ||
| href="/integrations/entra-device-auth" | ||
| label="Integrations" | ||
| icon={<SettingsIcon size={13} />} | ||
| /> |
There was a problem hiding this comment.
"Integrations" breadcrumb points to the current page.
Both breadcrumb items use href="/integrations/entra-device-auth", so the parent "Integrations" crumb just re-navigates to this same page. If there's no integrations index route, either drop the href on the parent crumb or point it at a meaningful parent (e.g., /integrations or the settings landing).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/`(dashboard)/integrations/entra-device-auth/page.tsx around lines 40
- 45, The "Integrations" breadcrumb in the Breadcrumbs component currently links
to the same page via Breadcrumbs.Item with
href="/integrations/entra-device-auth"; update that item so it doesn't point to
the current page—either remove the href prop from the parent Breadcrumbs.Item or
change it to a meaningful parent route such as "/integrations" (modify the
Breadcrumbs.Item that has label="Integrations" and
href="/integrations/entra-device-auth" accordingly).
| const [fallbackGroups, setFallbackGroups, { save: saveFallbackGroups }] = | ||
| useGroupHelper({ | ||
| initial: [], | ||
| }); | ||
|
|
||
| // Hydrate from server-side config once it arrives. | ||
| useEffect(() => { | ||
| if (!data) return; | ||
| setTenantId(data.tenant_id ?? ""); | ||
| setClientId(data.client_id ?? ""); | ||
| // The server returns "********" when a secret is already stored; keep | ||
| // the UI empty so we don't accidentally echo it back as plaintext. | ||
| setClientSecret(""); | ||
| setIssuer(data.issuer ?? ""); | ||
| setAudience(data.audience ?? ""); | ||
| setEnabled(!!data.enabled); | ||
| setRequireIntune(!!data.require_intune_compliant); | ||
| setAllowTenantOnlyFallback(!!data.allow_tenant_only_fallback); | ||
| setMappingResolution(data.mapping_resolution ?? "strict_priority"); | ||
| setRevalidationInterval(data.revalidation_interval ?? ""); | ||
| }, [data]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect useGroupHelper to confirm `initial` is only consumed on mount.
fd -t f 'useGroupHelper.tsx' | xargs catRepository: netbirdio/dashboard
Length of output: 4425
🏁 Script executed:
#!/bin/bash
rg -nP -C4 'useGroupHelper\(' --type=ts --type=tsx -g '!**/EntraDeviceAuthConfig.tsx'Repository: netbirdio/dashboard
Length of output: 91
🏁 Script executed:
#!/bin/bash
# First, locate the EntraDeviceAuthConfig file and verify it exists
fd -type f 'EntraDeviceAuthConfig.tsx'Repository: netbirdio/dashboard
Length of output: 234
🏁 Script executed:
#!/bin/bash
# Search for useGroupHelper usages with correct rg syntax (typescript, not tsx)
rg -nP -C4 'useGroupHelper\(' --type=ts -g '!**/EntraDeviceAuthConfig.tsx'Repository: netbirdio/dashboard
Length of output: 18118
🏁 Script executed:
#!/bin/bash
# Inspect the EntraDeviceAuthConfig component to verify field names and structure
fd -type f 'EntraDeviceAuthConfig.tsx' | xargs head -150Repository: netbirdio/dashboard
Length of output: 234
🏁 Script executed:
#!/bin/bash
# Search for fallback_auto_groups references in the file
fd -type f 'EntraDeviceAuthConfig.tsx' | xargs grep -n 'fallback'Repository: netbirdio/dashboard
Length of output: 236
🏁 Script executed:
#!/bin/bash
# Read the EntraDeviceAuthConfig file to verify the code and field names
cat "src/modules/integrations/entra-device-auth/EntraDeviceAuthConfig.tsx"Repository: netbirdio/dashboard
Length of output: 14329
🏁 Script executed:
#!/bin/bash
# Search for references to fallback in the entire integrations folder
rg -n 'fallback' src/modules/integrations/entra-device-auth/Repository: netbirdio/dashboard
Length of output: 1214
🏁 Script executed:
#!/bin/bash
# Find the EntraDeviceAuth interface definition
rg -n 'interface EntraDeviceAuth' src/Repository: netbirdio/dashboard
Length of output: 214
🏁 Script executed:
#!/bin/bash
# Check EntraDeviceMappingModal.tsx around lines 124-141 for context
head -150 "src/modules/integrations/entra-device-auth/EntraDeviceMappingModal.tsx" | tail -50Repository: netbirdio/dashboard
Length of output: 1544
🏁 Script executed:
#!/bin/bash
# Read the EntraDeviceAuth interface definition
cat "src/interfaces/EntraDeviceAuth.ts"Repository: netbirdio/dashboard
Length of output: 2859
Hydrate fallback_auto_groups from server config in the useEffect.
The useGroupHelper hook initializes fallbackGroups to an empty array and the hydration effect does not populate it from data.fallback_auto_groups. When buildRequest() calls saveFallbackGroups(), it returns [], causing the PUT request to send fallback_auto_groups: [] and overwrite the server-side configuration. Since useGroupHelper consumes the initial prop only on mount (via useState), you must use the setter to hydrate async data:
Proposed fix
const [fallbackGroups, setFallbackGroups, { save: saveFallbackGroups }] =
useGroupHelper({
initial: [],
});
// Hydrate from server-side config once it arrives.
useEffect(() => {
if (!data) return;
setTenantId(data.tenant_id ?? "");
setClientId(data.client_id ?? "");
// The server returns "********" when a secret is already stored; keep
// the UI empty so we don't accidentally echo it back as plaintext.
setClientSecret("");
setIssuer(data.issuer ?? "");
setAudience(data.audience ?? "");
setEnabled(!!data.enabled);
setRequireIntune(!!data.require_intune_compliant);
setAllowTenantOnlyFallback(!!data.allow_tenant_only_fallback);
setMappingResolution(data.mapping_resolution ?? "strict_priority");
setRevalidationInterval(data.revalidation_interval ?? "");
+ setFallbackGroups(data.fallback_auto_groups ?? []);
}, [data, setFallbackGroups]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/integrations/entra-device-auth/EntraDeviceAuthConfig.tsx` around
lines 84 - 104, The hydration effect currently never populates the
useGroupHelper state so fallbackGroups stays empty and saveFallbackGroups()
returns [] in buildRequest; inside the existing useEffect that reads data, call
setFallbackGroups(data.fallback_auto_groups ?? []) (or an empty array if
missing) to hydrate the group helper from server data, referencing the
useGroupHelper hook symbols (fallbackGroups, setFallbackGroups,
saveFallbackGroups) and the async payload field data.fallback_auto_groups so
subsequent buildRequest() uses the correct groups.
| enabled, | ||
| require_intune_compliant: requireIntune, | ||
| allow_tenant_only_fallback: allowTenantOnlyFallback, | ||
| fallback_auto_groups: groups.map((g: Group) => g.id!).filter(Boolean), | ||
| mapping_resolution: mappingResolution, | ||
| revalidation_interval: trim(revalidationInterval) || undefined, | ||
| }; |
There was a problem hiding this comment.
fallback_auto_groups is sent even when the fallback toggle is off.
When allowTenantOnlyFallback is false, the UI hides the picker but buildRequest() still includes whatever groups are in state. Depending on backend semantics this could either persist stale selections or cause unnecessary group-create side-effects via saveFallbackGroups(). Consider omitting the field (or sending an empty array) when the toggle is off, and short-circuiting the await saveFallbackGroups() call to avoid creating groups that will never be used.
🛠️ Proposed fix
- const buildRequest = async (): Promise<EntraDeviceAuthRequest> => {
- const groups = await saveFallbackGroups();
+ const buildRequest = async (): Promise<EntraDeviceAuthRequest> => {
+ const groups = allowTenantOnlyFallback ? await saveFallbackGroups() : [];
return {
tenant_id: trim(tenantId),
client_id: trim(clientId),
...
- fallback_auto_groups: groups.map((g: Group) => g.id!).filter(Boolean),
+ fallback_auto_groups: allowTenantOnlyFallback
+ ? groups.map((g: Group) => g.id!).filter(Boolean)
+ : [],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/integrations/entra-device-auth/EntraDeviceAuthConfig.tsx` around
lines 134 - 140, buildRequest() currently always includes fallback_auto_groups
from groups even when allowTenantOnlyFallback is false; update buildRequest() to
omit fallback_auto_groups (or send an empty array) when allowTenantOnlyFallback
is false and short-circuit any caller logic that invokes
saveFallbackGroups()—specifically, check the allowTenantOnlyFallback flag before
adding fallback_auto_groups to the returned payload in buildRequest(), and
before calling saveFallbackGroups() (or call it only when
allowTenantOnlyFallback is true) to avoid persisting or creating unused groups.
| const [expiresInDays, setExpiresInDays] = useState(() => { | ||
| if (!mapping?.expires_at) return ""; | ||
| const diffMs = | ||
| new Date(mapping.expires_at).getTime() - Date.now(); | ||
| if (diffMs <= 0) return ""; | ||
| return String(Math.ceil(diffMs / (24 * 60 * 60 * 1000))); | ||
| }); |
There was a problem hiding this comment.
Editing a mapping with an already-expired expires_at silently clears the expiry on save.
When mapping.expires_at is in the past, diffMs <= 0 makes expiresInDays initialize to "", and buildRequest (Line 92-100) then sends expires_at: null — effectively re-enabling the expired mapping forever if the admin just tweaks an unrelated field. Consider either surfacing the original timestamp (read-only or as "expired") and only overwriting when the admin explicitly edits it, or preserving mapping.expires_at in the payload when the field is untouched.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/integrations/entra-device-auth/EntraDeviceMappingModal.tsx`
around lines 69 - 75, The current initialization of expiresInDays (based on
mapping?.expires_at with diffMs <= 0 returning "") causes buildRequest to send
expires_at: null for expired mappings; change the logic so an expired mapping
does not get cleared unless the user edits the expiry: initialize expiresInDays
to a sentinel (e.g., undefined or a special value like "UNCHANGED") when
mapping.expires_at exists but is in the past, display the original timestamp or
an "Expired" read-only hint in the form, and update buildRequest to only
include/override expires_at when expiresInDays contains a user-edited numeric
value (otherwise leave expires_at as the original mapping.expires_at). Ensure
you update the state initialization (expiresInDays setter), the UI display for
expiry, and the buildRequest function to respect the untouched sentinel.
| const buildRequest = async (): Promise<EntraDeviceMappingRequest> => { | ||
| const groups = await saveGroups(); | ||
| let expiresAt: string | null = null; | ||
| if (trim(expiresInDays).length > 0) { | ||
| const days = parseInt(expiresInDays, 10); | ||
| if (!isNaN(days) && days > 0) { | ||
| expiresAt = new Date( | ||
| Date.now() + days * 24 * 60 * 60 * 1000, | ||
| ).toISOString(); | ||
| } | ||
| } | ||
| return { | ||
| name: trim(name), | ||
| entra_group_id: trim(entraGroupId), | ||
| auto_groups: groups.map((g) => g.id!).filter(Boolean), | ||
| ephemeral, | ||
| allow_extra_dns_labels: allowExtraDNSLabels, | ||
| expires_at: expiresAt, | ||
| revoked, | ||
| priority: parseInt(priority || "0", 10) || 0, | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Zero/negative expiresInDays silently falls through to "never".
isDisabled only validates that expiresInDays parses as an integer, and buildRequest only writes expires_at when days > 0. Typing 0 or -5 passes validation, keeps expiresAt = null, and creates a mapping with no expiry — which is almost certainly not what the admin intended. Consider rejecting <= 0 via isDisabled and/or the <Input min={1}> validation surface.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/integrations/entra-device-auth/EntraDeviceMappingModal.tsx`
around lines 90 - 111, The buildRequest currently treats parsed days <= 0 as
"never" expiry; update the validation so zero/negative values are rejected
instead of silently ignored: adjust the isDisabled logic that currently only
checks parseInt succeeds to also treat parsed days <= 0 as invalid, and set the
UI/input to enforce a minimum (e.g., change the <Input> for expiresInDays to
include min={1} and show a validation/error message) so admins cannot submit 0
or negative numbers; keep buildRequest as-is (it may still compute expires_at
when days > 0) but rely on the tightened isDisabled/UI validation to prevent <=
0 from being submitted.
| const { data: integration } = useFetchApi<EntraDeviceAuth>( | ||
| "/integrations/entra-device-auth", | ||
| true, | ||
| ); | ||
|
|
||
| const { data: mappings, isLoading } = useFetchApi<EntraDeviceMapping[]>( | ||
| "/integrations/entra-device-auth/mappings", | ||
| true, | ||
| ); | ||
|
|
||
| const canRead = permission?.entra_device_auth?.read ?? true; | ||
| const canCreate = permission?.entra_device_auth?.create ?? true; | ||
|
|
||
| const hasIntegration = !!integration?.id; |
There was a problem hiding this comment.
Empty-state briefly flashes during initial load.
hasIntegration is derived only from integration?.id, but useFetchApi returns undefined while the request is in flight. On first mount (or after cache invalidation), the "Configure the integration first" card will render for a moment before the real integration data arrives, even when the integration does exist. Consider gating the empty state on the integration's own loading flag so users don't see an incorrect get-started card.
🛠️ Proposed fix
- const { data: integration } = useFetchApi<EntraDeviceAuth>(
+ const { data: integration, isLoading: integrationLoading } = useFetchApi<EntraDeviceAuth>(
"/integrations/entra-device-auth",
true,
);
@@
- if (!hasIntegration) {
+ if (!integrationLoading && !hasIntegration) {
return (Also applies to: 212-228
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/integrations/entra-device-auth/EntraDeviceMappingsTable.tsx`
around lines 47 - 60, The empty-state flashes because hasIntegration is computed
from integration?.id while the integration fetch is in-flight; update the
EntraDeviceMappingsTable to read the loading flag from the integration fetch
(e.g., const { data: integration, isLoading: isLoadingIntegration } =
useFetchApi<EntraDeviceAuth>("/integrations/entra-device-auth", true)) and
change hasIntegration to incorporate that flag (e.g., const hasIntegration =
!isLoadingIntegration && !!integration?.id) so the "Configure the integration
first" card is not shown until the integration request completes.
Ready for final review 🙏This is the dashboard half of the Entra device authentication feature shipped in the backend (netbirdio/netbird#5977). What it adds
Verification
PR is mergeable; |
2 participants
Describe your changes
Adds the dashboard-facing half of the Microsoft Entra device authentication integration already implemented in the management server (netbirdio/netbird#5977).
This PR only adds UI — no API contract changes are required beyond what the linked backend PR already ships.
New page
/integrations/entra-device-authA new admin page with two horizontally-tabbed panels:
Configuration — singleton integration config per account. Covers everything the backend
integrationDTOaccepts:enabled,require_intune_compliant,allow_tenant_only_fallback.fallback_auto_groupspicker (shown when tenant-only fallback is enabled).mapping_resolutionselector (strict_priority|union).revalidation_intervalas a Go-duration string.Mappings — CRUD list for Entra security group → NetBird auto-groups mappings, modelled after the Setup Keys table:
*rendered as a pill), auto-groups (MultipleGroupshover-card), priority, ephemeral, expires.PeerGroupSelector,FancyToggleSwitch, and the same "expires in days" input asSetupKeyModal.revoked: true) and delete.no_integrationin that case).Sidebar entry
New
Entra Device Authitem in the main sidebar, gated onpermission?.entra_device_auth?.read. Because the permission module is optional on the backend for backwards compatibility, the link stays visible by default and the server-side permission check remains the source of truth.TypeScript additions
src/interfaces/EntraDeviceAuth.tsmirrors the backend'sintegrationDTO,mappingDTO, andtypes.MappingResolution.src/interfaces/Permission.tsgains an optionalentra_device_auth?: Permissionfield so the UI stays compatible with management servers that don't publish the new module.Files
src/app/(dashboard)/integrations/layout.tsxsrc/app/(dashboard)/integrations/entra-device-auth/{layout.tsx,page.tsx}src/modules/integrations/entra-device-auth/{EntraDeviceAuthConfig.tsx,EntraDeviceMappingsTable.tsx,EntraDeviceMappingModal.tsx,EntraDeviceMappingActionCell.tsx}src/interfaces/EntraDeviceAuth.tssrc/assets/icons/EntraDeviceIcon.tsxsrc/interfaces/Permission.ts(optional new module)src/layouts/Navigation.tsx(sidebar wiring)Test your changes
npm run devin the dashboard.Allow tenant-only fallbackreveals/hides the fallback group picker.Mapping resolutionandRevalidation intervalround-trips correctly.name,entra_group_id,auto_groups,priority,ephemeral,allow_extra_dns_labels,expires_at,revoked).*as the Entra group ID renders the "Any device in tenant" pill.revoked: truevia PUT; Delete removes the row via DELETE.Additional information
Summary by CodeRabbit