diff --git a/detector/effect_conflict_detector.go b/detector/effect_conflict_detector.go new file mode 100644 index 00000000..4aa494a4 --- /dev/null +++ b/detector/effect_conflict_detector.go @@ -0,0 +1,174 @@ +// Copyright 2025 The casbin Authors. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package detector + +import ( + "fmt" + "strings" + + "github.com/casbin/casbin/v3/model" + "github.com/casbin/casbin/v3/rbac" +) + +// ModelDetector defines the interface for detectors that need access to both the model and role manager. +type ModelDetector interface { + // CheckModel checks whether the current status contains logical errors. + // param: m Model instance + // param: rm RoleManager instance + // return: If an error is found, return a descriptive error; otherwise return nil. + CheckModel(m model.Model, rm rbac.RoleManager) error +} + +// EffectConflictDetector detects conflicts between user policies and role policies. +// It identifies cases where a user is explicitly allowed/denied to do something, +// but their role has the opposite effect for the same action. +// +// Note: In Casbin, explicit user policies override role policies, so such conflicts +// are not errors but might indicate policy design issues that should be reviewed. +// This detector is opt-in and not enabled by default. +// +// Example conflict: +// p, alice, data2, write, deny +// p, admin, data2, write, allow +// g, alice, admin +// Here alice is explicitly denied but her role allows it - this might be intentional +// (to override the role permission) or it might be a mistake. +type EffectConflictDetector struct{} + +// NewEffectConflictDetector creates a new instance of EffectConflictDetector. +// +// Usage example: +// e, _ := casbin.NewEnforcer("model.conf", "policy.csv") +// e.SetDetectors([]detector.Detector{ +// detector.NewDefaultDetector(), +// detector.NewEffectConflictDetector(), +// }) +// err := e.RunDetections() +func NewEffectConflictDetector() *EffectConflictDetector { + return &EffectConflictDetector{} +} + +// CheckModel checks for effect conflicts between user and role policies. +func (d *EffectConflictDetector) CheckModel(m model.Model, rm rbac.RoleManager) error { + if m == nil { + return fmt.Errorf("model cannot be nil") + } + if rm == nil { + return fmt.Errorf("role manager cannot be nil") + } + + // Get all policies + policies, err := m.GetPolicy("p", "p") + if err != nil { + return err + } + + // Get all role assignments + roles, err := m.GetPolicy("g", "g") + if err != nil { + // If no role assignments, no conflicts possible + return nil + } + + // Build a map of user -> roles + userRoles := make(map[string][]string) + for _, role := range roles { + if len(role) < 2 { + continue + } + user := role[0] + roleName := role[1] + userRoles[user] = append(userRoles[user], roleName) + } + + // Build a map of (subject, object, action) -> effect + policyEffects := make(map[string]string) + for _, policy := range policies { + if len(policy) < 3 { + continue + } + subject := policy[0] + object := policy[1] + action := policy[2] + effect := "allow" // Default effect if not specified + if len(policy) >= 4 { + effect = policy[3] + } + + key := makePolicyKey(subject, object, action) + policyEffects[key] = effect + } + + // Check for conflicts + for user, roleList := range userRoles { + for _, roleName := range roleList { + // Check all policy combinations + for policyKey, effect := range policyEffects { + parts := strings.Split(policyKey, policyKeySeparator) + if len(parts) != 3 { + continue + } + subject := parts[0] + object := parts[1] + action := parts[2] + + // Check if this is a user policy + if subject == user { + // Check if any role has opposite effect + roleKey := makePolicyKey(roleName, object, action) + if roleEffect, exists := policyEffects[roleKey]; exists { + if err := checkEffectConflict(user, roleName, object, action, effect, roleEffect); err != nil { + return err + } + } + } else if subject == roleName { + // Check if user has opposite effect + userKey := makePolicyKey(user, object, action) + if userEffect, exists := policyEffects[userKey]; exists { + if err := checkEffectConflict(user, roleName, object, action, userEffect, effect); err != nil { + return err + } + } + } + } + } + } + + return nil +} + +const policyKeySeparator = ":" + +// makePolicyKey creates a consistent key for a policy. +func makePolicyKey(subject, object, action string) string { + return fmt.Sprintf("%s%s%s%s%s", subject, policyKeySeparator, object, policyKeySeparator, action) +} + +// checkEffectConflict checks if two effects conflict and returns an error if they do. +func checkEffectConflict(user, role, object, action, userEffect, roleEffect string) error { + if (userEffect == "allow" && roleEffect == "deny") || + (userEffect == "deny" && roleEffect == "allow") { + return fmt.Errorf( + "effect conflict detected: user '%s' has '%s' effect for (%s, %s), "+ + "but role '%s' has '%s' effect for the same action", + user, userEffect, object, action, role, roleEffect) + } + return nil +} + +// Check implements the Detector interface by returning an error indicating this detector needs model access. +func (d *EffectConflictDetector) Check(rm rbac.RoleManager) error { + return fmt.Errorf("EffectConflictDetector requires both model and role manager access. This detector should be used through CheckModel() method or the enforcer's RunDetections() method") +} diff --git a/detector/effect_conflict_detector_test.go b/detector/effect_conflict_detector_test.go new file mode 100644 index 00000000..785cdd29 --- /dev/null +++ b/detector/effect_conflict_detector_test.go @@ -0,0 +1,283 @@ +// Copyright 2025 The casbin Authors. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package detector + +import ( + "strings" + "testing" + + "github.com/casbin/casbin/v3/model" + defaultrolemanager "github.com/casbin/casbin/v3/rbac/default-role-manager" +) + +func TestEffectConflictDetector_NilModel(t *testing.T) { + detector := NewEffectConflictDetector() + rm := defaultrolemanager.NewRoleManagerImpl(10) + + err := detector.CheckModel(nil, rm) + if err == nil { + t.Error("Expected error for nil model, but got nil") + } else if !strings.Contains(err.Error(), "model cannot be nil") { + t.Errorf("Expected error message to contain 'model cannot be nil', got: %s", err.Error()) + } +} + +func TestEffectConflictDetector_NilRoleManager(t *testing.T) { + detector := NewEffectConflictDetector() + m := model.Model{} + + err := detector.CheckModel(m, nil) + if err == nil { + t.Error("Expected error for nil role manager, but got nil") + } else if !strings.Contains(err.Error(), "role manager cannot be nil") { + t.Errorf("Expected error message to contain 'role manager cannot be nil', got: %s", err.Error()) + } +} + +func TestEffectConflictDetector_NoConflict(t *testing.T) { + detector := NewEffectConflictDetector() + rm := defaultrolemanager.NewRoleManagerImpl(10) + + // Create a simple model + m := model.Model{} + m.AddDef("p", "p", "sub, obj, act, eft") + m.AddDef("g", "g", "_, _") + + // Add policies - no conflicts + _ = m.AddPolicy("p", "p", []string{"alice", "data1", "read", "allow"}) + _ = m.AddPolicy("p", "p", []string{"data2_admin", "data2", "write", "allow"}) + + // Add role assignment + _ = m.AddPolicy("g", "g", []string{"alice", "data2_admin"}) + + err := detector.CheckModel(m, rm) + if err != nil { + t.Errorf("Expected no conflict, but got error: %v", err) + } +} + +func TestEffectConflictDetector_UserAllowRoleDeny(t *testing.T) { + detector := NewEffectConflictDetector() + rm := defaultrolemanager.NewRoleManagerImpl(10) + + // Create a model + m := model.Model{} + m.AddDef("p", "p", "sub, obj, act, eft") + m.AddDef("g", "g", "_, _") + + // alice is allowed to write to data2 + _ = m.AddPolicy("p", "p", []string{"alice", "data2", "write", "allow"}) + // data2_admin is denied to write to data2 + _ = m.AddPolicy("p", "p", []string{"data2_admin", "data2", "write", "deny"}) + + // alice has role data2_admin + _ = m.AddPolicy("g", "g", []string{"alice", "data2_admin"}) + + err := detector.CheckModel(m, rm) + if err == nil { + t.Error("Expected conflict detection error, but got nil") + } else { + errMsg := err.Error() + if !strings.Contains(errMsg, "effect conflict detected") { + t.Errorf("Expected error message to contain 'effect conflict detected', got: %s", errMsg) + } + if !strings.Contains(errMsg, "alice") { + t.Errorf("Expected error message to contain 'alice', got: %s", errMsg) + } + if !strings.Contains(errMsg, "data2_admin") { + t.Errorf("Expected error message to contain 'data2_admin', got: %s", errMsg) + } + } +} + +func TestEffectConflictDetector_UserDenyRoleAllow(t *testing.T) { + detector := NewEffectConflictDetector() + rm := defaultrolemanager.NewRoleManagerImpl(10) + + // Create a model + m := model.Model{} + m.AddDef("p", "p", "sub, obj, act, eft") + m.AddDef("g", "g", "_, _") + + // alice is denied to read data1 + _ = m.AddPolicy("p", "p", []string{"alice", "data1", "read", "deny"}) + // admin is allowed to read data1 + _ = m.AddPolicy("p", "p", []string{"admin", "data1", "read", "allow"}) + + // alice has role admin + _ = m.AddPolicy("g", "g", []string{"alice", "admin"}) + + err := detector.CheckModel(m, rm) + if err == nil { + t.Error("Expected conflict detection error, but got nil") + } else { + errMsg := err.Error() + if !strings.Contains(errMsg, "effect conflict detected") { + t.Errorf("Expected error message to contain 'effect conflict detected', got: %s", errMsg) + } + if !strings.Contains(errMsg, "alice") { + t.Errorf("Expected error message to contain 'alice', got: %s", errMsg) + } + if !strings.Contains(errMsg, "admin") { + t.Errorf("Expected error message to contain 'admin', got: %s", errMsg) + } + } +} + +func TestEffectConflictDetector_NoRoles(t *testing.T) { + detector := NewEffectConflictDetector() + rm := defaultrolemanager.NewRoleManagerImpl(10) + + // Create a model + m := model.Model{} + m.AddDef("p", "p", "sub, obj, act, eft") + m.AddDef("g", "g", "_, _") + + // Add policies but no role assignments + _ = m.AddPolicy("p", "p", []string{"alice", "data1", "read", "allow"}) + _ = m.AddPolicy("p", "p", []string{"bob", "data2", "write", "deny"}) + + err := detector.CheckModel(m, rm) + if err != nil { + t.Errorf("Expected no conflict when there are no role assignments, but got error: %v", err) + } +} + +func TestEffectConflictDetector_SameEffect(t *testing.T) { + detector := NewEffectConflictDetector() + rm := defaultrolemanager.NewRoleManagerImpl(10) + + // Create a model + m := model.Model{} + m.AddDef("p", "p", "sub, obj, act, eft") + m.AddDef("g", "g", "_, _") + + // Both alice and admin are allowed to read data1 + _ = m.AddPolicy("p", "p", []string{"alice", "data1", "read", "allow"}) + _ = m.AddPolicy("p", "p", []string{"admin", "data1", "read", "allow"}) + + // alice has role admin + _ = m.AddPolicy("g", "g", []string{"alice", "admin"}) + + err := detector.CheckModel(m, rm) + if err != nil { + t.Errorf("Expected no conflict when effects are the same, but got error: %v", err) + } +} + +func TestEffectConflictDetector_MultipleRoles(t *testing.T) { + detector := NewEffectConflictDetector() + rm := defaultrolemanager.NewRoleManagerImpl(10) + + // Create a model + m := model.Model{} + m.AddDef("p", "p", "sub, obj, act, eft") + m.AddDef("g", "g", "_, _") + + // alice is allowed to write to data1 + _ = m.AddPolicy("p", "p", []string{"alice", "data1", "write", "allow"}) + // admin is allowed to write to data1 + _ = m.AddPolicy("p", "p", []string{"admin", "data1", "write", "allow"}) + // moderator is denied to write to data1 + _ = m.AddPolicy("p", "p", []string{"moderator", "data1", "write", "deny"}) + + // alice has both admin and moderator roles + _ = m.AddPolicy("g", "g", []string{"alice", "admin"}) + _ = m.AddPolicy("g", "g", []string{"alice", "moderator"}) + + err := detector.CheckModel(m, rm) + if err == nil { + t.Error("Expected conflict detection error with multiple roles, but got nil") + } else { + errMsg := err.Error() + if !strings.Contains(errMsg, "effect conflict detected") { + t.Errorf("Expected error message to contain 'effect conflict detected', got: %s", errMsg) + } + } +} + +func TestEffectConflictDetector_DefaultEffect(t *testing.T) { + detector := NewEffectConflictDetector() + rm := defaultrolemanager.NewRoleManagerImpl(10) + + // Create a model + m := model.Model{} + m.AddDef("p", "p", "sub, obj, act") + m.AddDef("g", "g", "_, _") + + // alice has default effect (allow) + _ = m.AddPolicy("p", "p", []string{"alice", "data1", "read"}) + // admin is denied + _ = m.AddPolicy("p", "p", []string{"admin", "data1", "read", "deny"}) + + // alice has role admin + _ = m.AddPolicy("g", "g", []string{"alice", "admin"}) + + err := detector.CheckModel(m, rm) + if err == nil { + t.Error("Expected conflict detection error with default effect, but got nil") + } else { + errMsg := err.Error() + if !strings.Contains(errMsg, "effect conflict detected") { + t.Errorf("Expected error message to contain 'effect conflict detected', got: %s", errMsg) + } + } +} + +func TestEffectConflictDetector_DifferentActions(t *testing.T) { + detector := NewEffectConflictDetector() + rm := defaultrolemanager.NewRoleManagerImpl(10) + + // Create a model + m := model.Model{} + m.AddDef("p", "p", "sub, obj, act, eft") + m.AddDef("g", "g", "_, _") + + // alice is allowed to read data1 + _ = m.AddPolicy("p", "p", []string{"alice", "data1", "read", "allow"}) + // admin is denied to write data1 (different action) + _ = m.AddPolicy("p", "p", []string{"admin", "data1", "write", "deny"}) + + // alice has role admin + _ = m.AddPolicy("g", "g", []string{"alice", "admin"}) + + err := detector.CheckModel(m, rm) + if err != nil { + t.Errorf("Expected no conflict for different actions, but got error: %v", err) + } +} + +func TestEffectConflictDetector_DifferentObjects(t *testing.T) { + detector := NewEffectConflictDetector() + rm := defaultrolemanager.NewRoleManagerImpl(10) + + // Create a model + m := model.Model{} + m.AddDef("p", "p", "sub, obj, act, eft") + m.AddDef("g", "g", "_, _") + + // alice is allowed to read data1 + _ = m.AddPolicy("p", "p", []string{"alice", "data1", "read", "allow"}) + // admin is denied to read data2 (different object) + _ = m.AddPolicy("p", "p", []string{"admin", "data2", "read", "deny"}) + + // alice has role admin + _ = m.AddPolicy("g", "g", []string{"alice", "admin"}) + + err := detector.CheckModel(m, rm) + if err != nil { + t.Errorf("Expected no conflict for different objects, but got error: %v", err) + } +} diff --git a/enforcer.go b/enforcer.go index ff8c5431..c8600e79 100644 --- a/enforcer.go +++ b/enforcer.go @@ -316,14 +316,22 @@ func (e *Enforcer) RunDetections() error { // Run detectors on all role managers for _, rm := range e.rmMap { for _, d := range e.detectors { - err := d.Check(rm) - // Skip if the role manager doesn't support the required iteration or is not initialized - if err != nil && (strings.Contains(err.Error(), "does not support Range iteration") || - strings.Contains(err.Error(), "not properly initialized")) { - continue - } - if err != nil { - return err + // Check if this is a ModelDetector + if md, ok := d.(detector.ModelDetector); ok { + err := md.CheckModel(e.model, rm) + if err != nil { + return err + } + } else { + err := d.Check(rm) + // Skip if the role manager doesn't support the required iteration or is not initialized + if err != nil && (strings.Contains(err.Error(), "does not support Range iteration") || + strings.Contains(err.Error(), "not properly initialized")) { + continue + } + if err != nil { + return err + } } } } @@ -331,14 +339,22 @@ func (e *Enforcer) RunDetections() error { // Run detectors on all conditional role managers for _, crm := range e.condRmMap { for _, d := range e.detectors { - err := d.Check(crm) - // Skip if the role manager doesn't support the required iteration or is not initialized - if err != nil && (strings.Contains(err.Error(), "does not support Range iteration") || - strings.Contains(err.Error(), "not properly initialized")) { - continue - } - if err != nil { - return err + // Check if this is a ModelDetector + if md, ok := d.(detector.ModelDetector); ok { + err := md.CheckModel(e.model, crm) + if err != nil { + return err + } + } else { + err := d.Check(crm) + // Skip if the role manager doesn't support the required iteration or is not initialized + if err != nil && (strings.Contains(err.Error(), "does not support Range iteration") || + strings.Contains(err.Error(), "not properly initialized")) { + continue + } + if err != nil { + return err + } } } } diff --git a/enforcer_test.go b/enforcer_test.go index bc8f8b4b..b8ff8a14 100644 --- a/enforcer_test.go +++ b/enforcer_test.go @@ -801,3 +801,31 @@ func TestEnforcerSetDetectors(t *testing.T) { t.Errorf("Expected no error with multiple detectors, but got: %v", err) } } + +func TestEnforcerWithEffectConflictDetector(t *testing.T) { + // Test that effect conflict detector can be enabled and detects conflicts + e, err := NewEnforcer("examples/rbac_with_deny_model.conf", "examples/rbac_with_deny_policy.csv") + + // Without the detector, should work fine + if err != nil { + t.Errorf("Expected no error without effect conflict detector, but got: %v", err) + return + } + + // Now set the effect conflict detector + e.SetDetectors([]detector.Detector{ + detector.NewDefaultDetector(), + detector.NewEffectConflictDetector(), + }) + + // Run detections should now find the conflict + err = e.RunDetections() + if err == nil { + t.Error("Expected error for effect conflict in policy, but got nil") + } else { + errMsg := err.Error() + if !strings.Contains(errMsg, "effect conflict detected") { + t.Errorf("Expected error message to contain 'effect conflict detected', got: %s", errMsg) + } + } +} diff --git a/examples/rbac_user_allow_role_deny_policy.csv b/examples/rbac_user_allow_role_deny_policy.csv new file mode 100644 index 00000000..e9fab04d --- /dev/null +++ b/examples/rbac_user_allow_role_deny_policy.csv @@ -0,0 +1,4 @@ +p, alice, data1, read, allow +p, admin, data1, read, deny + +g, alice, admin \ No newline at end of file diff --git a/examples/rbac_with_effect_conflict_policy.csv b/examples/rbac_with_effect_conflict_policy.csv new file mode 100644 index 00000000..0603db8d --- /dev/null +++ b/examples/rbac_with_effect_conflict_policy.csv @@ -0,0 +1,7 @@ +p, alice, data1, read, allow +p, bob, data2, write, allow +p, data2_admin, data2, read, allow +p, data2_admin, data2, write, allow +p, alice, data2, write, deny + +g, alice, data2_admin \ No newline at end of file