Repository: casbin/casbin
Pull request #819 (2021-06-15 17:12:35)

fix: fix the dirty data caused by LoadPolicy
Signed-off-by: Zixuan Liu <nodeces@gmail.com>

Fix: #817

Fix: #791


Patch:
diff --git a/enforcer.go b/enforcer.go
index 4a9ca6b..3ea12a4 100644
--- a/enforcer.go
+++ b/enforcer.go
@@ -17,6 +17,7 @@ package casbin
 import (
 	"errors"
 	"fmt"
+	"os"
 
 	"github.com/Knetic/govaluate"
 	"github.com/casbin/casbin/v2/effector"
@@ -29,6 +30,8 @@ import (
 	"github.com/casbin/casbin/v2/util"
 )
 
+type RoleManagerMap map[string]rbac.RoleManager
+
 // Enforcer is the main interface for authorization enforcement and policy management.
 type Enforcer struct {
 	modelPath string
@@ -39,7 +42,7 @@ type Enforcer struct {
 	adapter    persist.Adapter
 	watcher    persist.Watcher
 	dispatcher persist.Dispatcher
-	rmMap      map[string]rbac.RoleManager
+	rmMap      RoleManagerMap
 
 	enabled              bool
 	autoSave             bool
@@ -132,7 +135,21 @@ func NewEnforcer(params ...interface{}) (*Enforcer, error) {
 
 // InitWithFile initializes an enforcer with a model file and a policy file.
 func (e *Enforcer) InitWithFile(modelPath string, policyPath string) error {
-	a := fileadapter.NewAdapter(policyPath)
+	var a persist.Adapter
+
+	// When the policy path is empty, the user only passes the model path.
+	// If the policy path is not empty, we have to make sure the file exists.
+	if policyPath != "" {
+		exists, err := util.PathExists(policyPath)
+		if err != nil {
+			return err
+		}
+		if !exists {
+			return os.ErrNotExist
+		}
+		a = fileadapter.NewAdapter(policyPath)
+	}
+
 	return e.InitWithAdapter(modelPath, a)
 }
 
@@ -185,7 +202,6 @@ func (e *Enforcer) SetLogger(logger log.Logger) {
 }
 
 func (e *Enforcer) initialize() {
-	e.rmMap = map[string]rbac.RoleManager{}
 	e.eft = effector.NewDefaultEffector()
 	e.watcher = nil
 
@@ -194,7 +210,9 @@ func (e *Enforcer) initialize() {
 	e.autoBuildRoleLinks = true
 	e.autoNotifyWatcher = true
 	e.autoNotifyDispatcher = true
-	e.initRmMap()
+
+	e.rmMap = RoleManagerMap{}
+	e.initRmMap(e.model, e.rmMap)
 }
 
 // LoadModel reloads the model from the model CONF file.
@@ -271,38 +289,34 @@ func (e *Enforcer) ClearPolicy() {
 
 // LoadPolicy reloads the policy from file/database.
 func (e *Enforcer) LoadPolicy() error {
-	oldModel := e.model
-	e.model = model.NewModel()
-	e.model.SetLogger(oldModel.GetLogger())
+	newModel := model.NewModel()
+	e.model.CopyTo(&newModel)
+	newModel.ClearPolicy()
 
 	var err error
+	rmMap := RoleManagerMap{}
 	defer func() {
-		if err != nil {
-			e.model = oldModel
+		if err == nil {
+			e.model = newModel
+			e.rmMap = rmMap
 		}
 	}()
 
-	if err = e.model.LoadModelFromText(oldModel.ToText()); err != nil {
-		return err
-	}
-	if err = e.adapter.LoadPolicy(e.model); err != nil && err.Error() != "invalid file path, file path cannot be empty" {
-		return err
-	}
-
-	if err = e.model.SortPoliciesByPriority(); err != nil {
+	if err = e.adapter.LoadPolicy(newModel); err != nil {
 		return err
 	}
 
-	if err = e.clearRmMap(); err != nil {
+	if err = newModel.SortPoliciesByPriority(); err != nil {
 		return err
 	}
 
 	if e.autoBuildRoleLinks {
-		err = e.BuildRoleLinks()
+		err = e.buildRoleLinks(newModel, rmMap)
 		if err != nil {
 			return err
 		}
 	}
+
 	return nil
 }
 
@@ -324,7 +338,6 @@ func (e *Enforcer) loadFilteredPolicy(filter interface{}) error {
 		return err
 	}
 
-	e.initRmMap()
 	e.model.PrintPolicy()
 	if e.autoBuildRoleLinks {
 		err := e.BuildRoleLinks()
@@ -376,23 +389,10 @@ func (e *Enforcer) SavePolicy() error {
 	return nil
 }
 
-func (e *Enforcer) initRmMap() {
-	for ptype := range e.model["g"] {
-		if rm, ok := e.rmMap[ptype]; ok {
-			_ = rm.Clear()
-		} else {
-			e.rmMap[ptype] = defaultrolemanager.NewRoleManager(10)
-		}
-	}
-}
-
-func (e *Enforcer) clearRmMap() error {
-	for ptype := range e.model["g"] {
-		if err := e.rmMap[ptype].Clear(); err != nil {
-			return err
-		}
+func (e *Enforcer) initRmMap(model model.Model, rmMap RoleManagerMap) {
+	for ptype := range model["g"] {
+		rmMap[ptype] = defaultrolemanager.NewRoleManager(10)
 	}
-	return nil
 }
 
 // EnableEnforce changes the enforcing state of Casbin, when Casbin is disabled, all access will be allowed by the Enforce() function.
@@ -432,14 +432,12 @@ func (e *Enforcer) EnableAutoBuildRoleLinks(autoBuildRoleLinks bool) {
 
 // BuildRoleLinks manually rebuild the role inheritance relations.
 func (e *Enforcer) BuildRoleLinks() error {
-	for _, rm := range e.rmMap {
-		err := rm.Clear()
-		if err != nil {
-			return err
-		}
-	}
+	return e.buildRoleLinks(e.model, e.rmMap)
+}
 
-	return e.model.BuildRoleLinks(e.rmMap)
+func (e *Enforcer) buildRoleLinks(model model.Model, rmMap RoleManagerMap) error {
+	e.initRmMap(model, rmMap)
+	return model.BuildRoleLinks(rmMap)
 }
 
 // BuildIncrementalRoleLinks provides incremental build the role inheritance relations.
diff --git a/model/assertion.go b/model/assertion.go
index 66a8155..b439937 100644
--- a/model/assertion.go
+++ b/model/assertion.go
@@ -36,6 +36,10 @@ type Assertion struct {
 	priorityIndex int
 }
 
+func (ast *Assertion) copyTo(dest *Assertion) {
+	*dest = *ast
+}
+
 func (ast *Assertion) buildIncrementalRoleLinks(rm rbac.RoleManager, op PolicyOp, rules [][]string) error {
 	ast.RM = rm
 	count := strings.Count(ast.Value, "_")
diff --git a/model/model.go b/model/model.go
index 09a9fa7..f90945e 100644
--- a/model/model.go
+++ b/model/model.go
@@ -205,6 +205,18 @@ func (model Model) PrintModel() {
 	model.GetLogger().LogModel(modelInfo)
 }
 
+func (model Model) CopyTo(dest *Model) {
+	for modelKey, modelValue := range model {
+		astMap := make(AssertionMap)
+		(*dest)[modelKey] = astMap
+		for key, value := range modelValue {
+			ast := new(Assertion)
+			value.copyTo(ast)
+			astMap[key] = ast
+		}
+	}
+}
+
 func (model Model) SortPoliciesByPriority() error {
 	for ptype, assertion := range model["p"] {
 		for index, token := range assertion.Tokens {
diff --git a/rbac_api_with_domains.go b/rbac_api_with_domains.go
index 6495a55..e62649d 100644
--- a/rbac_api_with_domains.go
+++ b/rbac_api_with_domains.go
@@ -118,7 +118,7 @@ func (e *Enforcer) DeleteAllUsersByDomain(domain string) (bool, error) {
 
 // DeleteDomains would delete all associated users and roles.
 // It would delete all domains if parameter is not provided.
-func (e *Enforcer) DeleteDomains(domains ...string) (bool, error) {
+func (e *Enforcer) DeleteDomains(index int, domains ...string) (bool, error) {
 	if len(domains) == 0 {
 		e.ClearPolicy()
 		return true, nil
diff --git a/util/util.go b/util/util.go
index be1b8b2..d9cb6e8 100644
--- a/util/util.go
+++ b/util/util.go
@@ -15,6 +15,7 @@
 package util
 
 import (
+	"os"
 	"regexp"
 	"sort"
 	"strings"
@@ -197,3 +198,15 @@ func RemoveDuplicateElement(s []string) []string {
 	}
 	return result
 }
+
+func PathExists(path string) (bool, error) {
+	_, err := os.Stat(path)
+	if err == nil {
+		return true, nil
+	}
+	if os.IsExist(err) {
+		return true, nil
+	} else {
+		return false, err
+	}
+}


Test patch:
diff --git a/model/model_test.go b/model/model_test.go
index 06e59d3..7700118 100644
--- a/model/model_test.go
+++ b/model/model_test.go
@@ -15,11 +15,14 @@
 package model
 
 import (
-	"github.com/casbin/casbin/v2/config"
+	"fmt"
 	"io/ioutil"
 	"path/filepath"
+	"reflect"
 	"strings"
 	"testing"
+
+	"github.com/casbin/casbin/v2/config"
 )
 
 var (
@@ -122,3 +125,26 @@ func TestModel_AddDef(t *testing.T) {
 		t.Errorf("empty assertion value should not be added")
 	}
 }
+
+func TestModel_CopyTo(t *testing.T) {
+	a := NewModel()
+	a["p"] = make(AssertionMap)
+	a["p"]["p"] = new(Assertion)
+	a["p"]["p"].Policy = [][]string{{"1"}, {"2"}}
+
+	b := NewModel()
+	a.CopyTo(&b)
+
+	if fmt.Sprintf("%p", a["p"]) == fmt.Sprintf("%p", b["p"]) {
+		t.Fatal(`the memory address of a["p"] and b["p"] should not be equal`)
+	}
+
+	if fmt.Sprintf("%p", a["p"]["p"]) == fmt.Sprintf("%p", b["p"]["p"]) {
+		t.Fatal(`the memory address of a["p"]["p"] and b["p"]["p"] should not be equal`)
+	}
+
+	a["p"]["p"].Policy = nil
+	if reflect.DeepEqual(a["p"]["p"], b["p"]["p"]) {
+		t.Fatal(`the a["p"]["p"] and b["p"]["p"] should not be equal`)
+	}
+}