feat: add support for Casbin v3 with new middleware and configuration options#1930
feat: add support for Casbin v3 with new middleware and configuration options#1930PedroDBFlores wants to merge 4 commits into
Conversation
PedroDBFlores
requested review from
ReneWerner87,
Copilot,
efectn,
gaby and
sixcolors
and removed request for
a team
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces Casbin v3 support by adding the NewV3 constructor and ConfigV3 structure, while refactoring the Middleware struct to store specific function pointers for enforcement and role lookup. Review feedback identified critical nil pointer dereference risks in both New and NewV3 when using default configurations, as the initialization logic currently bypasses enforcer setup. Additionally, a correction was requested to mark the Casbin v3 dependency as a direct requirement in the go.mod file.
| if len(config) < 1 { | ||
| return ConfigV3Default, nil | ||
| } | ||
|
|
||
| cfg := config[0] |
There was a problem hiding this comment.
The configDefaultV3 function returns early when no configuration is provided, which skips the initialization of the Casbin enforcer and other default values. This will lead to a nil pointer dereference in NewV3 when it attempts to capture the enforcer's methods.
| if len(config) < 1 { | |
| return ConfigV3Default, nil | |
| } | |
| cfg := config[0] | |
| cfg := ConfigV3Default | |
| if len(config) > 0 { | |
| cfg = config[0] | |
| } | |
| enforce: cfg.Enforcer.Enforce, | ||
| getRolesForUser: cfg.Enforcer.GetRolesForUser, |
There was a problem hiding this comment.
Accessing cfg.Enforcer.Enforce here will cause a nil pointer dereference if New() is called without arguments. This is because configDefault (in config.go) returns early and does not initialize the enforcer when no config is passed. While this issue existed in the previous version, the refactoring now causes a panic at startup instead of when the middleware is first used. Please ensure configDefault is also updated to handle the default case correctly.
| require ( | ||
| github.com/andybalholm/brotli v1.2.1 // indirect | ||
| github.com/bmatcuk/doublestar/v4 v4.10.0 // indirect | ||
| github.com/casbin/casbin/v3 v3.10.0 // indirect |
There was a problem hiding this comment.
github.com/casbin/casbin/v3 is a direct dependency of this package (imported in config_v3.go) and should not be marked as // indirect.
| github.com/casbin/casbin/v3 v3.10.0 // indirect | |
| github.com/casbin/casbin/v3 v3.10.0 |
There was a problem hiding this comment.
Pull request overview
Adds Casbin v3 support to the v3/casbin Fiber middleware by introducing a v3-specific configuration/constructor and refactoring middleware internals to work with either v2 or v3 enforcers via function pointers.
Changes:
- Added
ConfigV3+ defaulting helper (configDefaultV3) for Casbin v3 configuration. - Added
NewV3constructor and refactoredMiddlewareto store resolved handler/enforcer function pointers (v2 or v3). - Added Casbin v3 dependency entries and a new v3-focused test suite.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| v3/casbin/go.sum | Adds checksums for Casbin v3 dependency. |
| v3/casbin/go.mod | Adds Casbin v3 module requirement. |
| v3/casbin/config.go | Clarifies that Config.Enforcer refers to Casbin v2 (comment update). |
| v3/casbin/config_v3.go | Introduces v3 configuration struct, defaults, and defaulting logic. |
| v3/casbin/casbin.go | Refactors middleware to version-agnostic function pointers and adds NewV3. |
| v3/casbin/casbin_v3_test.go | Adds tests covering middleware behavior when using a Casbin v3 enforcer. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if len(config) < 1 { | ||
| return ConfigV3Default, nil | ||
| } | ||
|
|
||
| cfg := config[0] | ||
|
|
| // Enforcer is a Casbin v2 enforcer. If you want to use your own enforcer. | ||
| // Optional. Default: nil (one is created from ModelFilePath and PolicyAdapter). | ||
| Enforcer *casbin.Enforcer |
| require ( | ||
| github.com/andybalholm/brotli v1.2.1 // indirect | ||
| github.com/bmatcuk/doublestar/v4 v4.10.0 // indirect | ||
| github.com/casbin/casbin/v3 v3.10.0 // indirect | ||
| github.com/casbin/govaluate v1.10.0 // indirect |
📝 WalkthroughWalkthroughThis PR refactors the Casbin middleware to use dependency injection and adds support for Casbin v3. The Middleware struct now stores injected function handlers for authorization logic instead of a config reference, allowing both v2 (via ChangesCasbin v3 Middleware Support with Dependency Injection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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: 1
🤖 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 `@v3/casbin/config_v3.go`:
- Around line 47-50: configDefaultV3 currently returns ConfigV3Default with a
nil Enforcer when no args are passed; update configDefaultV3 to construct and
assign a working default enforcer to ConfigV3Default.Enforcer before returning.
Locate configDefaultV3 and ensure you instantiate the default enforcer (using
the project's enforcer constructor/helper, e.g. NewEnforcer or NewEnforcerV3)
and set ConfigV3Default.Enforcer = <constructed enforcer>, handling any
construction error and returning it instead of nil so callers always receive a
ConfigV3 with a non-nil Enforcer.
🪄 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: 4c3e8534-0734-4d75-8429-91ba2c7d7704
⛔ Files ignored due to path filters (2)
v3/casbin/go.modis excluded by!**/*.modv3/casbin/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (4)
v3/casbin/casbin.gov3/casbin/casbin_v3_test.gov3/casbin/config.gov3/casbin/config_v3.go
📜 Review details
🔇 Additional comments (5)
v3/casbin/casbin_v3_test.go (1)
1-404: LGTM!v3/casbin/casbin.go (2)
45-50: Downstream effect of nil-enforcer default path.This wiring issue is covered by the root-cause comment in
configDefaultV3; fixing that resolves this path as well.
9-37: LGTM!Also applies to: 54-156
v3/casbin/config_v3.go (1)
1-46: LGTM!Also applies to: 52-84
v3/casbin/config.go (1)
20-21: LGTM!
| func configDefaultV3(config ...ConfigV3) (ConfigV3, error) { | ||
| if len(config) < 1 { | ||
| return ConfigV3Default, nil | ||
| } |
There was a problem hiding this comment.
Create the default v3 enforcer in the no-arg path.
When no config is passed, this returns ConfigV3Default as-is, which leaves Enforcer nil. That contradicts the documented default behavior and can cause nil-enforcer failures later in middleware execution.
💡 Proposed fix
func configDefaultV3(config ...ConfigV3) (ConfigV3, error) {
- if len(config) < 1 {
- return ConfigV3Default, nil
- }
-
- cfg := config[0]
+ cfg := ConfigV3Default
+ if len(config) > 0 {
+ cfg = config[0]
+ }
if cfg.Enforcer == nil {
if cfg.ModelFilePath == "" {
cfg.ModelFilePath = ConfigV3Default.ModelFilePath📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func configDefaultV3(config ...ConfigV3) (ConfigV3, error) { | |
| if len(config) < 1 { | |
| return ConfigV3Default, nil | |
| } | |
| func configDefaultV3(config ...ConfigV3) (ConfigV3, error) { | |
| cfg := ConfigV3Default | |
| if len(config) > 0 { | |
| cfg = config[0] | |
| } | |
| if cfg.Enforcer == nil { | |
| if cfg.ModelFilePath == "" { | |
| cfg.ModelFilePath = ConfigV3Default.ModelFilePath |
🤖 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 `@v3/casbin/config_v3.go` around lines 47 - 50, configDefaultV3 currently
returns ConfigV3Default with a nil Enforcer when no args are passed; update
configDefaultV3 to construct and assign a working default enforcer to
ConfigV3Default.Enforcer before returning. Locate configDefaultV3 and ensure you
instantiate the default enforcer (using the project's enforcer
constructor/helper, e.g. NewEnforcer or NewEnforcerV3) and set
ConfigV3Default.Enforcer = <constructed enforcer>, handling any construction
error and returning it instead of nil so callers always receive a ConfigV3 with
a non-nil Enforcer.
There was a problem hiding this comment.
I kept the logic the same as the v2 config, in order to maintain coherence between implementations
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
cat v3/casbin/config.go
Length of output: 2480
@PedroDBFlores, confirmed — the v2 configDefault has the exact same pattern: it returns ConfigDefault (with a nil Enforcer) immediately when no args are provided, so your v3 implementation is consistent with it.
That said, the nil-enforcer risk on the no-arg path is technically shared by both v2 and v3. If you'd ever like to address it, fixing both together in a follow-up would be the natural place to do so. Happy to open a tracking issue for that if you'd like. Otherwise, I'll defer to your judgment here.
🐇
There was a problem hiding this comment.
♻️ Duplicate comments (1)
v3/casbin/config_v3.go (1)
49-51:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winconfigDefaultV3 returns nil Enforcer when called with no arguments.
When no config is provided, the function returns
ConfigV3Defaultas-is, bypassing enforcer creation at lines 56-71. SinceConfigV3Default.Enforceris uninitialized (nil), downstream middleware will panic on any enforcer method call.The fix is to merge both paths so the Enforcer-nil check always runs, creating a default enforcer from
ModelFilePathandPolicyAdapter.🔧 Proposed fix
func configDefaultV3(config ...ConfigV3) (ConfigV3, error) { - // Return default config if nothing provided - if len(config) < 1 { - return ConfigV3Default, nil - } - - // Override default config - cfg := config[0] + cfg := ConfigV3Default + if len(config) > 0 { + cfg = config[0] + } if cfg.Enforcer == nil { if cfg.ModelFilePath == "" {🤖 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 `@v3/casbin/config_v3.go` around lines 49 - 51, The function configDefaultV3 returns ConfigV3Default directly when called with no args, leaving ConfigV3Default.Enforcer nil; modify configDefaultV3 so both the empty-config and non-empty-config paths converge before the Enforcer nil check: always check cfg.Enforcer and if it's nil create and assign a new Enforcer using cfg.ModelFilePath and cfg.PolicyAdapter (the same logic currently in the Enforcer-nil branch around the create-enforcer code), ensuring ConfigV3Default is not returned with a nil Enforcer.
🤖 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.
Duplicate comments:
In `@v3/casbin/config_v3.go`:
- Around line 49-51: The function configDefaultV3 returns ConfigV3Default
directly when called with no args, leaving ConfigV3Default.Enforcer nil; modify
configDefaultV3 so both the empty-config and non-empty-config paths converge
before the Enforcer nil check: always check cfg.Enforcer and if it's nil create
and assign a new Enforcer using cfg.ModelFilePath and cfg.PolicyAdapter (the
same logic currently in the Enforcer-nil branch around the create-enforcer
code), ensuring ConfigV3Default is not returned with a nil Enforcer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ff6fc50c-e8b9-46d0-a2f0-cbcc62249188
⛔ Files ignored due to path filters (1)
v3/casbin/go.modis excluded by!**/*.mod
📒 Files selected for processing (1)
v3/casbin/config_v3.go
This pull request adds support for Casbin v3 to the Fiber Casbin middleware, allowing users to choose between Casbin v2 and v3 enforcers. The main changes include the introduction of a new configuration struct and constructor for v3 and internal refactoring to generalize the middleware logic.
Casbin v3 Support:
ConfigV3struct inconfig_v3.goto support Casbin v3 enforcers, including default values and a helper functionconfigDefaultV3to initialize the configuration.NewV3constructor incasbin.goto create middleware instances using Casbin v3 enforcers.Middleware Refactoring:
Middlewarestruct to store handler functions and enforcer methods directly, abstracting over v2 and v3 enforcer differences.Documentation and Dependency Updates:
config.goto clarify thatEnforcerrefers specifically to Casbin v2, and added Casbin v3 as a dependency ingo.mod.Summary by CodeRabbit
New Features
Refactor
Tests