Add header auth and access restrictions to reverse proxy#583
Closed
Add header auth and access restrictions to reverse proxy#583
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds header-based HTTP auth, CIDR/country access-restrictions, and subdivision-aware region text to reverse-proxy types, UI modals, and event rendering; updates target/timeouts, target selection, and related badges/hover UI across reverse-proxy components. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Modal as ReverseProxyModal
participant HeaderModal as AuthHeaderModal
participant AccessSection as AccessRestrictionsSection
participant API as API
User->>Modal: Open reverse-proxy config
User->>Modal: Open Header Auth
Modal->>HeaderModal: open(currentHeaders)
HeaderModal->>HeaderModal: validate drafts
User->>HeaderModal: Save
HeaderModal->>Modal: onSave(HeaderAuthConfig[])
Modal->>Modal: update header_auths state
User->>Modal: Open Access tab
Modal->>AccessSection: render(currentRestrictions)
User->>AccessSection: edit CIDR/country rows
AccessSection->>Modal: onChange(AccessRestrictions)
Modal->>Modal: update accessRestrictions state
User->>Modal: Submit
Modal->>API: POST/PUT payload with header_auths & access_restrictions
API-->>Modal: OK
sequenceDiagram
participant Viewer as User
participant AuthCell as ReverseProxyAuthCell
participant Badge as L4AccessBadge
participant Modal as ReverseProxyModal
Viewer->>AuthCell: View auth cell
AuthCell->>AuthCell: compute totalEnabled (including header_auths)
alt totalEnabled > 1
AuthCell->>Viewer: show stacked badges
else only header auth
AuthCell->>Viewer: show Header Auth badge
end
alt access_restrictions present
AuthCell->>Badge: render Access Control badge
Viewer->>Badge: hover
Badge->>Viewer: show allowed/blocked lists
Viewer->>Badge: click Configure
Badge->>Modal: open Access tab
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/modules/reverse-proxy/AccessRestrictionsSection.tsx (1)
42-50: CIDR validation shows errors but doesn't block submission.Invalid CIDR entries display an error message but are still propagated to the parent via
onChange. Consider whether invalid entries should be filtered out or if validation should occur at the form submission level.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/reverse-proxy/AccessRestrictionsSection.tsx` around lines 42 - 50, The CIDR check currently sets an error via setErrors when raw fails cidr.isValidCIDR(raw) but still allows the invalid value to be propagated via onChange; update the change handler so that when cidr.isValidCIDR(raw) is false you do not call props.onChange (or you call onChange with the values list filtered to exclude that id/raw), and when valid you clear the error and call props.onChange with the updated valid CIDR list; locate the block using raw, id, setErrors and cidr.isValidCIDR in AccessRestrictionsSection.tsx and adjust the logic around the onChange invocation to only pass valid CIDRs (or explicitly remove invalid entries) while preserving the existing setErrors behavior.src/modules/reverse-proxy/ReverseProxyModal.tsx (1)
1174-1198: Consider adding a "Learn more" link for the Access Control tab.The modal footer provides documentation links for the "targets", "auth", and "settings" tabs, but the new "access" tab doesn't have a corresponding link. If documentation exists for access restrictions, consider adding it for consistency.
Would you like me to help draft the code for adding the access control documentation link once the docs URL is available?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/reverse-proxy/ReverseProxyModal.tsx` around lines 1174 - 1198, Add a footer entry for the "access" tab similar to the existing blocks for "auth" and "settings": when tab === "access" render a Paragraph containing an InlineLink to the access docs (use a new constant like REVERSE_PROXY_ACCESS_DOCS_LINK) and include the ExternalLinkIcon; follow the same className ("text-sm mt-auto") and target={"_blank"} patterns used in the "auth" and "settings" blocks so the modal footer remains consistent across tabs and the link is only shown for the "access" tab.
🤖 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/reverse-proxy/AccessRestrictionsSection.tsx`:
- Around line 105-108: CountryList's local rows state (rows, setRows created in
CountryList via useState) is initialized from the values prop and will go stale
when values changes; update CountryList the same way as CidrList by making rows
reflect prop updates: initialize rows once from values but add a useEffect that
watches values and reconstructs setRows(values.map(v => ({ id: nextRowId++,
value: v }))) when values changes (ensuring nextRowId progression is preserved)
so the component stays in sync with the values prop.
- Around line 25-28: CidrList currently initializes internal rows state from the
values prop only on mount (see CidrList, rows, setRows, nextRowId), so it
becomes stale when the parent supplies new values; fix by syncing prop changes:
either update the parent to force remount with a stable unique key (e.g.,
key={`allowed-${reverseProxy.id}`} when rendering CidrList) or add a useEffect
inside CidrList that watches values and resets rows via setRows(values.map(v =>
({ id: nextRowId++, value: v }))) whenever values changes, ensuring the
displayed rows always reflect the latest prop.
In `@src/modules/reverse-proxy/auth/AuthHeaderModal.tsx`:
- Around line 143-145: The drafts state (useState initializer) only runs on
mount so when currentHeaders changes between modal opens the component remains
stale; update the component to synchronize drafts with currentHeaders by either
adding a key prop from the parent to force remount or adding a useEffect inside
AuthHeaderModal that watches currentHeaders and calls
setDrafts(currentHeaders.length > 0 ? currentHeaders.map(configToDraft) :
[newDraft()]) to reset drafts; reference the drafts/setDrafts state and
functions configToDraft and newDraft when implementing the fix.
---
Nitpick comments:
In `@src/modules/reverse-proxy/AccessRestrictionsSection.tsx`:
- Around line 42-50: The CIDR check currently sets an error via setErrors when
raw fails cidr.isValidCIDR(raw) but still allows the invalid value to be
propagated via onChange; update the change handler so that when
cidr.isValidCIDR(raw) is false you do not call props.onChange (or you call
onChange with the values list filtered to exclude that id/raw), and when valid
you clear the error and call props.onChange with the updated valid CIDR list;
locate the block using raw, id, setErrors and cidr.isValidCIDR in
AccessRestrictionsSection.tsx and adjust the logic around the onChange
invocation to only pass valid CIDRs (or explicitly remove invalid entries) while
preserving the existing setErrors behavior.
In `@src/modules/reverse-proxy/ReverseProxyModal.tsx`:
- Around line 1174-1198: Add a footer entry for the "access" tab similar to the
existing blocks for "auth" and "settings": when tab === "access" render a
Paragraph containing an InlineLink to the access docs (use a new constant like
REVERSE_PROXY_ACCESS_DOCS_LINK) and include the ExternalLinkIcon; follow the
same className ("text-sm mt-auto") and target={"_blank"} patterns used in the
"auth" and "settings" blocks so the modal footer remains consistent across tabs
and the link is only shown for the "access" tab.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9fb78a0f-ca19-4e7f-a330-4b50e1b58b3e
📒 Files selected for processing (10)
src/contexts/CountryProvider.tsxsrc/interfaces/ReverseProxy.tssrc/modules/reverse-proxy/AccessRestrictionsSection.tsxsrc/modules/reverse-proxy/ReverseProxyModal.tsxsrc/modules/reverse-proxy/auth/AuthHeaderModal.tsxsrc/modules/reverse-proxy/events/ReverseProxyEventsAuthMethodCell.tsxsrc/modules/reverse-proxy/events/ReverseProxyEventsLocationIpCell.tsxsrc/modules/reverse-proxy/table/ReverseProxyAuthCell.tsxsrc/modules/reverse-proxy/targets/ReverseProxyTargetModal.tsxsrc/modules/reverse-proxy/targets/useReverseProxyTargetOptions.ts
💤 Files with no reviewable changes (1)
- src/modules/reverse-proxy/targets/useReverseProxyTargetOptions.ts
Contributor
|
overall it looks good. The quick overview about auth and restrictions could be better I think. If I have restrictions but no auth it is not really detectable from the main view if just says 'none' (you need to know the difference between the 2 no tags) |
lixmal
force-pushed
the
feature/reverse-proxy-auth
branch
from
a48e071 to
0844507
Compare
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.
Add UI for configuring header-based authentication, geo/IP access restrictions, and session idle timeouts on reverse proxy services. Includes auth header modal, access restrictions section, and updated event display for auth/location metadata.
Depends on netbirdio/netbird#5587
Summary by CodeRabbit
New Features
Improvements