feat: add support for deny by default access controls#852
Open
steveiliop56 wants to merge 2 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughAuthorization logic is moved from AuthService to AccessControlsService, which now reads ACLS from model.Config (policy: allow/deny), implements user/group/IP/path predicates, and is wired in bootstrap, proxy controller, and tests. ChangesACL Logic Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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 docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
dosubot
Bot
added
the
size:L
![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
🧹 Nitpick comments (2)
internal/controller/proxy_controller_test.go (1)
25-25: ⚡ Quick winAdd tests for the new deny/block policy.
The shared
cfgfromtest.CreateTestConfigs(t)doesn't setAuth.ACLS.Policy, so the entire deny-by-default code path introduced in this PR is never executed byTestProxyController. The existing happy-path/path-allow/ip-bypass/user-allow cases all bypasspolicyResulteither via explicit filters or via the bypass/auth-disabled short circuits.Consider adding at least:
- A test where
cfg.Auth.ACLS.Policy = "block"and a request hits a domain with no matchingAppsentry — assert it's denied at the IP allow step.- A test where
cfg.Auth.ACLS.Policy = "block"and a request hits a domain with an explicitIP.Allow(orUsers.Allow) — assert it succeeds.- A test with an invalid policy string (e.g.,
"deny","") — assert it logs a warning and behaves as the documented default ("allow"). This will also catch the fallback regression flagged inNewAccessControlsService.Also applies to: 367-367
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/proxy_controller_test.go` at line 25, The tests for TestProxyController never exercise the new deny-by-default policy because test.CreateTestConfigs(t) doesn't set cfg.Auth.ACLS.Policy; add new unit tests that set cfg.Auth.ACLS.Policy = "block" and exercise the code paths around policyResult: (1) a request to a domain with no matching Apps entry should be denied at the IP allow step, (2) a request to a domain with an explicit IP.Allow or Users.Allow should succeed, and (3) a test with an invalid policy string (e.g., "deny" or "") should assert a warning is logged and behavior falls back to documented default ("allow") to catch the NewAccessControlsService fallback regression. Ensure tests reference and assert on the same controller flow exercised by TestProxyController so policyResult is evaluated rather than bypassed.internal/controller/proxy_controller.go (1)
53-72: ⚡ Quick winRemove unused
authfield and constructor parameter fromProxyController.After the refactor, all authorization checks in
proxyHandlerdelegate tocontroller.acls(IP bypass, auth-enabled status, user allowed, groups). Theauthfield on the struct and correspondingauthparameter inNewProxyControllerare never referenced. Remove them to keep the dependency surface minimal and make intent clearer. Update the wiring ininternal/bootstrap/router_bootstrap.go:47and test setup accordingly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/proxy_controller.go` around lines 53 - 72, The ProxyController struct and constructor still include an unused auth dependency: remove the auth field from the ProxyController struct and remove the auth parameter (type *service.AuthService) from NewProxyController signature and its struct literal, then update all call sites that invoke NewProxyController to stop passing the auth argument (including bootstrap/router wiring and tests) so the code compiles with the reduced dependency surface; ensure any imports or test fixtures related only to auth are cleaned up.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/model/config.go`:
- Around line 230-232: The ACLSConfig description string is inconsistent and has
a typo: change "deny-by-defaut" to "deny-by-default" and make the allowed values
consistent with accessControlPolicyFromString by either (preferred) switching
the internal parser/constants in access_controls_service.go (and any related
constants) to accept the YAML value "deny" instead of "block", or conversely
update this description to list "allow and block"; specifically modify
ACLSConfig (type ACLSConfig) description text and update
accessControlPolicyFromString (and its associated policy constants) so the
accepted policy string matches the user-facing value ("deny") throughout the
codebase.
In `@internal/service/access_controls_service.go`:
- Around line 213-215: The current append usage can mutate the shared backing
arrays for service.config.Auth.IP.Block/Allow; to fix, create new slices before
concatenation so you never write into the shared slice: allocate a fresh slice
(e.g., with make or by starting from a nil-typed slice) and copy or append the
global entries then the per-app entries to produce blockedIps and allowedIPs;
update the code that constructs blockedIps and allowedIPs (the lines using
append with service.config.Auth.IP.Block and acls.IP.Block, and similarly for
.Allow) to use the safe allocation/copy approach (or slices.Concat if you target
Go ≥1.22).
- Around line 41-68: NewAccessControlsService sets service.policy incorrectly
when accessControlPolicyFromString returns ok==false: it first assigns
service.policy = PolicyAllow then later logs and overwrites service.policy with
the empty/local variable policy, leaving an invalid value. Fix by using the
validated policy variable (or set policy = PolicyAllow when !ok) before any
logging and before assigning into service.policy; specifically adjust
NewAccessControlsService to set policy = PolicyAllow when
accessControlPolicyFromString returns ok==false (and keep the warning log), then
use that policy for the subsequent if/else logging and final service.policy
assignment so service.policy and the logged policy remain consistent
(references: NewAccessControlsService, accessControlPolicyFromString,
service.policy, policy, PolicyAllow).
- Around line 106-125: IsUserAllowed currently treats an existing ACL with empty
users filters as allowing access because utils.CheckFilter("", username) returns
true; change IsUserAllowed (and similarly handle the OAuth whitelist if desired)
to detect when both acls.Users.Block and acls.Users.Allow are empty and route
that case through service.policyResult(true) instead of calling
utils.CheckFilter, so the global policy (policyResult) is respected when an ACL
exists but contains no user filters; use the symbols IsUserAllowed,
acls.Users.Block, acls.Users.Allow, utils.CheckFilter, and service.policyResult
to locate and modify the logic.
---
Nitpick comments:
In `@internal/controller/proxy_controller_test.go`:
- Line 25: The tests for TestProxyController never exercise the new
deny-by-default policy because test.CreateTestConfigs(t) doesn't set
cfg.Auth.ACLS.Policy; add new unit tests that set cfg.Auth.ACLS.Policy = "block"
and exercise the code paths around policyResult: (1) a request to a domain with
no matching Apps entry should be denied at the IP allow step, (2) a request to a
domain with an explicit IP.Allow or Users.Allow should succeed, and (3) a test
with an invalid policy string (e.g., "deny" or "") should assert a warning is
logged and behavior falls back to documented default ("allow") to catch the
NewAccessControlsService fallback regression. Ensure tests reference and assert
on the same controller flow exercised by TestProxyController so policyResult is
evaluated rather than bypassed.
In `@internal/controller/proxy_controller.go`:
- Around line 53-72: The ProxyController struct and constructor still include an
unused auth dependency: remove the auth field from the ProxyController struct
and remove the auth parameter (type *service.AuthService) from
NewProxyController signature and its struct literal, then update all call sites
that invoke NewProxyController to stop passing the auth argument (including
bootstrap/router wiring and tests) so the code compiles with the reduced
dependency surface; ensure any imports or test fixtures related only to auth are
cleaned up.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d27ab4c6-72fd-4514-9274-045ba1cf85e8
📒 Files selected for processing (7)
internal/bootstrap/service_bootstrap.gointernal/controller/proxy_controller.gointernal/controller/proxy_controller_test.gointernal/model/config.gointernal/service/access_controls_service.gointernal/service/auth_service.gointernal/test/test.go
💤 Files with no reviewable changes (1)
- internal/service/auth_service.go
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/service/access_controls_service.go (1)
71-82: 💤 Low valuePrefer the explicit local-copy pattern when storing a pointer to the loop value.
appAcls = &configtakes the address of the range loop variable. Although the immediatebreakmakes this functionally safe, the project convention is to materialize an explicit local copy to make intent obvious and avoid loop-variable capture confusion.♻️ Proposed refactor
var appAcls *model.App for app, config := range service.config.Apps { + appConfig := config if config.Config.Domain == domain { service.log.App.Debug().Str("name", app).Msg("Found matching container by domain") - appAcls = &config + appAcls = &appConfig break // If we find a match by domain, we can stop searching } if strings.SplitN(domain, ".", 2)[0] == app { service.log.App.Debug().Str("name", app).Msg("Found matching container by app name") - appAcls = &config + appAcls = &appConfig break // If we find a match by app name, we can stop searching } }Based on learnings: "prefer the explicit local copy pattern (e.g.,
result := config; return &result) rather than returning&<range-variable>directly … the project prefers the explicit copy for clarity and to avoid reviewer confusion about loop-variable capture semantics."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/service/access_controls_service.go` around lines 71 - 82, The code takes the address of the range loop variable (config) when assigning appAcls (appAcls = &config); change this to the explicit local-copy pattern to avoid loop-variable capture confusion: create a new local variable (e.g., cfg := config) and assign appAcls = &cfg instead for both matches (the domain check and the app-name check) while keeping the break logic and using the existing service.config.Apps, appAcls, and config identifiers to locate the spots to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/service/access_controls_service.go`:
- Around line 173-205: The Path.Block branch in
AccessControlsService.IsAuthEnabled currently uses a negation (if
!regex.MatchString(uri) { return false }) which inverts the intended behavior;
remove the negation so that matching the block regex disables auth (i.e., if
regex.MatchString(uri) { return false }). Also avoid compiling regexes on every
call to IsAuthEnabled: precompile and cache the regex objects for
acls.Path.Block and acls.Path.Allow (either during service initialization or by
adding compiled fields on the model.App/ACL struct) and use those cached
*regexp.Regexp instances in IsAuthEnabled; preserve existing error logging via
service.log.App.Error() when compilation fails during initialization or when
setting the ACL.
---
Nitpick comments:
In `@internal/service/access_controls_service.go`:
- Around line 71-82: The code takes the address of the range loop variable
(config) when assigning appAcls (appAcls = &config); change this to the explicit
local-copy pattern to avoid loop-variable capture confusion: create a new local
variable (e.g., cfg := config) and assign appAcls = &cfg instead for both
matches (the domain check and the app-name check) while keeping the break logic
and using the existing service.config.Apps, appAcls, and config identifiers to
locate the spots to change.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a7e25ccc-eb0b-4e59-b3b4-8d0fc39ae11f
📒 Files selected for processing (3)
internal/model/config.gointernal/service/access_controls_service.gointernal/test/test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/test/test.go
- internal/model/config.go
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
1 participant
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.
Solves #850
Need help on this PR? Tag
@codesmithwith what you need.Summary by CodeRabbit
New Features
Refactor
Tests