Add Certificate Authority UI to settings and peer pages#574
Closed
zgv163 wants to merge 3 commits intonetbirdio:mainfrom
Closed
Add Certificate Authority UI to settings and peer pages#574zgv163 wants to merge 3 commits intonetbirdio:mainfrom
zgv163 wants to merge 3 commits intonetbirdio:mainfrom
Conversation
Introduce a Certificate Authority settings tab with CA initialization, wildcard toggle, rotation, and inactive CA management. Add a Certificates tab on the peer detail page showing the active certificate as a card with a collapsible history of previous certificates.
|
|
📝 WalkthroughWalkthroughAdds Certificate Authority management and peer certificate viewing/revocation: new CA-related interfaces and permission flag, UI tabs in Settings and Peer pages, a Certificate Authority admin tab, and a Peer Certificates section with init/rotate/download/wildcard and revoke flows. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant UI as "CA UI"
participant API as "Backend API"
participant Cache as "SWR Cache"
User->>Browser: Open Settings -> Certificate Authority tab
Browser->>UI: Render CA tab
UI->>API: GET /ca
API-->>UI: CA list
UI->>Cache: Mutate /ca
alt Initialize CA
User->>UI: Click Initialize
UI->>User: Show Init Modal
User->>UI: Submit init data
UI->>API: POST /ca
API-->>UI: Created CA
UI->>Cache: Mutate /ca
else Rotate CA
User->>UI: Click Rotate
UI->>User: Confirm rotation
User->>UI: Confirm
UI->>API: POST /ca/rotate
API-->>UI: Rotation complete
UI->>Cache: Mutate /ca, /ca/certificates
end
User->>Browser: Open Peer -> Certificates tab
Browser->>UI: Render PeerCertificatesSection
UI->>API: GET /ca/certificates?peer_id=...
API-->>UI: Issued certificates
UI->>Cache: Mutate /ca/certificates
User->>UI: Click Revoke
UI->>User: Confirm revoke
User->>UI: Confirm
UI->>API: POST /ca/certificates/{id}/revoke
API-->>UI: Revoked
UI->>Cache: Mutate /ca/certificates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
7 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/modules/peer/PeerCertificatesSection.tsx`:
- Around line 229-231: The fetch error path from useFetchApi in
PeerCertificatesSection.tsx is not handled, causing the UI to display "No
Certificates Issued" when the request fails; update the render logic around the
certificates, isLoading, and any error/isError return values from useFetchApi to
explicitly handle errors: check for an error (e.g., error or isError) and render
an appropriate error state or suppress the empty-state message, only showing "No
Certificates Issued" when not loading, no error, and certificates is an empty
array; apply the same change to the second fetch/render block covering the code
near lines 248-273 so both certificate lists respect the error state.
- Around line 33-35: The isCertActive predicate (function isCertActive(cert:
IssuedCertificate)) currently returns true for any certificate that is not
revoked and not expired; update it to also check the certificate's not_before
timestamp so certificates that are not yet valid are treated as inactive. Modify
isCertActive to return false when cert.not_before is in the future (compare
against Date.now()/new Date()), or reuse/create a helper like
isCertNotYetValid(cert) alongside isCertExpired(cert), ensuring the function
only returns true when !cert.revoked && !isCertExpired(cert) &&
!isCertNotYetValid(cert).
In `@src/modules/settings/CertificateAuthorityTab.tsx`:
- Around line 318-320: The code currently uses
useFetchApi<CACertificate[]>("/ca") and only reads data (cas) and isLoading
(isCAsLoading), which causes backend/network errors to be treated as “no CA
initialized”; update the fetch usage to also read the error state (e.g., error
or isError) from useFetchApi and change the rendering logic in the CA
initialization/empty-state branch (the block that renders when cas is empty) to
first check for a fetch error and render an explicit error message/UI instead of
the "no CA" empty-state; locate useFetchApi, cas, isCAsLoading and the CA
empty-state rendering (the component around the CA initialization UI) and add
the conditional rendering for error vs. empty result.
- Around line 444-457: The FancyToggleSwitch for wildcard certificates is
incorrectly gated by permission.settings.update; change the disabled prop to
gate against the Certificate Authority update permission (e.g., replace
permission.settings.update with the CA-specific permission such as
permission.certificateAuthority.update or permission.ca.update) so that the
FancyToggleSwitch (value={wildcardAllowed}, onChange={toggleWildcard}) is
enabled for users with CA update rights; locate the component usage and update
the boolean expression accordingly and run related UI/permission tests to verify
behavior.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/app/(dashboard)/peer/page.tsxsrc/app/(dashboard)/settings/page.tsxsrc/interfaces/Account.tssrc/interfaces/CertificateAuthority.tssrc/interfaces/Permission.tssrc/modules/peer/PeerCertificatesSection.tsxsrc/modules/settings/CertificateAuthorityTab.tsx
- Add error state handling for CA and certificate API calls - Fix isCertActive to reject future not_before dates - Use certificate_authority permission for wildcard toggle
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/modules/settings/CertificateAuthorityTab.tsx (2)
271-285: Add disclosure accessibility attributes for “Previous CAs”.Same disclosure UX improvement applies here:
type="button",aria-expanded, andaria-controls.Suggested fix
<button + type="button" className={ "flex items-center gap-2 text-sm text-nb-gray-400 hover:text-nb-gray-300 transition-colors" } onClick={() => setExpanded(!expanded)} + aria-expanded={expanded} + aria-controls="inactive-cas-list" > @@ - {expanded && ( - <div className={"mt-3 flex flex-col gap-2"}> + {expanded && ( + <div id="inactive-cas-list" className={"mt-3 flex flex-col gap-2"}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/settings/CertificateAuthorityTab.tsx` around lines 271 - 285, The disclosure button in CertificateAuthorityTab.tsx (the button that toggles expanded via setExpanded and displays Previous CAs ({cas.length})) lacks required accessibility attributes; change the button element to include type="button", aria-expanded={expanded}, and aria-controls pointing to a unique id (e.g., "previous-cas-panel"), and add the matching id="previous-cas-panel" on the conditional div (the container rendered when expanded) so assistive tech can associate the control with the panel.
326-328: KeepwildcardAllowedstate synchronized withaccountupdates.This local state is initialized once. If
account.settings.cert_wildcard_allowedchanges from a later fetch, the toggle can drift from source-of-truth.Suggested fix
-import React, { useMemo, useState } from "react"; +import React, { useEffect, useMemo, useState } from "react"; @@ const [wildcardAllowed, setWildcardAllowed] = useState( account.settings.cert_wildcard_allowed ?? false, ); + + useEffect(() => { + setWildcardAllowed(account.settings.cert_wildcard_allowed ?? false); + }, [account.settings.cert_wildcard_allowed]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/settings/CertificateAuthorityTab.tsx` around lines 326 - 328, The local state variable wildcardAllowed in the CertificateAuthorityTab component is only initialized once and can drift from the account source-of-truth; add a useEffect that watches account.settings.cert_wildcard_allowed (or account) and calls setWildcardAllowed(account.settings.cert_wildcard_allowed ?? false) whenever it changes so the toggle stays synchronized with account updates, keeping the existing state setter (setWildcardAllowed) for user interactions.src/modules/peer/PeerCertificatesSection.tsx (1)
194-207: Expose expanded/collapsed state for the previous-certificates disclosure.The toggle is keyboard-accessible, but adding
type="button"andaria-expanded/aria-controlsimproves SR navigation and avoids accidental form submit behavior.Suggested fix
<button + type="button" className={ "flex items-center gap-2 text-sm text-nb-gray-400 hover:text-nb-gray-300 transition-colors" } onClick={() => setExpanded(!expanded)} + aria-expanded={expanded} + aria-controls="peer-previous-certificates" > @@ - {expanded && ( - <div className={"mt-3 flex flex-col gap-2"}> + {expanded && ( + <div id="peer-previous-certificates" className={"mt-3 flex flex-col gap-2"}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/peer/PeerCertificatesSection.tsx` around lines 194 - 207, The disclosure toggle button in PeerCertificatesSection (the button using expanded, setExpanded and showing Previous certificates ({certs.length})) should be made explicit for accessibility and to avoid form submits: add type="button" to the button, add aria-expanded={expanded}, and add aria-controls pointing to the id of the panel that renders when expanded; also ensure the conditional panel (the element rendered when expanded) has a matching id (e.g., "previous-certificates-panel") so screen readers can associate the button with the collapsible content.
🤖 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/modules/peer/PeerCertificatesSection.tsx`:
- Around line 41-63: CertStatusBadge currently only checks cert.revoked and
isCertExpired, so future-dated certificates (cert.not_before) fall through to
"Active"; update CertStatusBadge to mirror isCertActive logic by adding a check
for not-yet-valid certificates (e.g., using cert.not_before or a helper like
!isCertActive(cert)) and render a distinct "Not yet valid" badge (or appropriate
variant) before returning "Active", keeping the revoked and expired checks
intact.
- Around line 80-83: The confirm call in PeerCertificatesSection.tsx uses
cert.dns_names.join(...) without guarding against undefined; update the code
that builds the confirmation description (the confirm(...) call) to compute a
safe label first (e.g., const label = cert.dns_names?.join(", ") ||
cert.serial_number) and then use that label in the description string so that
confirm(...) never calls .join on undefined.
---
Nitpick comments:
In `@src/modules/peer/PeerCertificatesSection.tsx`:
- Around line 194-207: The disclosure toggle button in PeerCertificatesSection
(the button using expanded, setExpanded and showing Previous certificates
({certs.length})) should be made explicit for accessibility and to avoid form
submits: add type="button" to the button, add aria-expanded={expanded}, and add
aria-controls pointing to the id of the panel that renders when expanded; also
ensure the conditional panel (the element rendered when expanded) has a matching
id (e.g., "previous-certificates-panel") so screen readers can associate the
button with the collapsible content.
In `@src/modules/settings/CertificateAuthorityTab.tsx`:
- Around line 271-285: The disclosure button in CertificateAuthorityTab.tsx (the
button that toggles expanded via setExpanded and displays Previous CAs
({cas.length})) lacks required accessibility attributes; change the button
element to include type="button", aria-expanded={expanded}, and aria-controls
pointing to a unique id (e.g., "previous-cas-panel"), and add the matching
id="previous-cas-panel" on the conditional div (the container rendered when
expanded) so assistive tech can associate the control with the panel.
- Around line 326-328: The local state variable wildcardAllowed in the
CertificateAuthorityTab component is only initialized once and can drift from
the account source-of-truth; add a useEffect that watches
account.settings.cert_wildcard_allowed (or account) and calls
setWildcardAllowed(account.settings.cert_wildcard_allowed ?? false) whenever it
changes so the toggle stays synchronized with account updates, keeping the
existing state setter (setWildcardAllowed) for user interactions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cdb8c2a2-c715-4898-b8ed-5856869cccc0
📒 Files selected for processing (2)
src/modules/peer/PeerCertificatesSection.tsxsrc/modules/settings/CertificateAuthorityTab.tsx
- CertStatusBadge shows "Pending" for future-dated certificates - Guard dns_names with optional chaining in revoke dialog
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/modules/peer/PeerCertificatesSection.tsx (1)
133-140: Avoid duplicating status rendering logic in the active card.This hardcoded badge can drift from
CertStatusBadgerules. Reusing the shared badge keeps status logic single-sourced.Proposed refactor
<Card.ListItem label={"Status"} tooltip={false} - value={ - <Badge variant={"green"} size={"xs"}> - Active - </Badge> - } + value={<CertStatusBadge cert={cert} />} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/peer/PeerCertificatesSection.tsx` around lines 133 - 140, The Status ListItem currently renders a hardcoded Badge inside Card.ListItem which duplicates status logic; replace the inline Badge with the shared CertStatusBadge component (used elsewhere) and pass the certificate's status value (e.g., cert.status or the same status prop used where CertStatusBadge is used) into CertStatusBadge so all status presentation is single-sourced; update the Card.ListItem value to render <CertStatusBadge status={...} /> instead of the hardcoded Badge.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/modules/peer/PeerCertificatesSection.tsx`:
- Around line 133-140: The Status ListItem currently renders a hardcoded Badge
inside Card.ListItem which duplicates status logic; replace the inline Badge
with the shared CertStatusBadge component (used elsewhere) and pass the
certificate's status value (e.g., cert.status or the same status prop used where
CertStatusBadge is used) into CertStatusBadge so all status presentation is
single-sourced; update the Card.ListItem value to render <CertStatusBadge
status={...} /> instead of the hardcoded Badge.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ead48837-cebc-4d23-848d-56370e3ab36e
📒 Files selected for processing (1)
src/modules/peer/PeerCertificatesSection.tsx
Author
|
Closing to resubmit alongside the updated core PR. Will fix error handling, permission checks, and other review findings before reopening. |
Author
|
Closing this in favor of a Let's Encrypt DNS-PERSIST-01 approach instead. The Internal CA route doesn't provide a smooth experience on iOS and Android — see updated comment on netbirdio/netbird#5479 for details. |
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
2 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.
Describe your changes
Adds the dashboard UI for the internal Certificate Authority feature introduced in netbirdio/netbird#5491.
Settings > Certificate Authority tab:
Peer > Certificates tab:
Supporting changes:
CertificateAuthority.ts— interfaces forCACertificateandIssuedCertificatePermission.ts— addedcertificate_authoritypermission fieldAccount.ts— addedcert_wildcard_allowedsettingIssue ticket number and link
Part of netbirdio/netbird#5479
Core PR: netbirdio/netbird#5491
Checklist
Summary by CodeRabbit