Repository: casbin/casbin
Pull request #1041 (2022-06-26 11:01:36)

feat: add FieldIndexMap and constant
# ChangLog
Some APIs (mostly RBAC APIs) need to obtain the index of variables such as sub, obj, dom, priority, etc. in policy_define. So this PR did the following work
1. add ``FieldIndexMap`` to store index.
2. add ``GetFieldIndex`` and ``SetFieldIndex``, try to get the index with ``sub, obj, dom, priority`` as the default definition named values. If the user does not use the default value, then ``SetFieldIndex`` should be used to set the index

Some other work is adding constants, putting some constants together instead of using strings directly in the code.

related: https://github.com/casbin/casbin/issues/1015#issuecomment-1166218883 #1015 
Signed-off-by: tangyang9464 <tangyang9464@163.com>

casbin-bot:  @closetool @sagilio please review

Patch:
diff --git a/constant/constants.go b/constant/constants.go
new file mode 100644
index 0000000..7a454ae
--- /dev/null
+++ b/constant/constants.go
@@ -0,0 +1,30 @@
+// Copyright 2022 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 constant
+
+const (
+	DomainIndex   = "dom"
+	SubjectIndex  = "sub"
+	ObjectIndex   = "obj"
+	PriorityIndex = "priority"
+)
+
+const (
+	AllowOverrideEffect   = "some(where (p_eft == allow))"
+	DenyOverrideEffect    = "!some(where (p_eft == deny))"
+	AllowAndDenyEffect    = "some(where (p_eft == allow)) && !some(where (p_eft == deny))"
+	PriorityEffect        = "priority(p_eft) || deny"
+	SubjectPriorityEffect = "subjectPriority(p_eft) || deny"
+)
diff --git a/effector/default_effector.go b/effector/default_effector.go
index de5dc37..feb083a 100644
--- a/effector/default_effector.go
+++ b/effector/default_effector.go
@@ -14,7 +14,11 @@
 
 package effector
 
-import "errors"
+import (
+	"errors"
+
+	"github.com/casbin/casbin/v2/constant"
+)
 
 // DefaultEffector is default effector for Casbin.
 type DefaultEffector struct {
@@ -32,7 +36,7 @@ func (e *DefaultEffector) MergeEffects(expr string, effects []Effect, matches []
 	explainIndex := -1
 
 	switch expr {
-	case "some(where (p_eft == allow))":
+	case constant.AllowOverrideEffect:
 		if matches[policyIndex] == 0 {
 			break
 		}
@@ -42,7 +46,7 @@ func (e *DefaultEffector) MergeEffects(expr string, effects []Effect, matches []
 			explainIndex = policyIndex
 			break
 		}
-	case "!some(where (p_eft == deny))":
+	case constant.DenyOverrideEffect:
 		// only check the current policyIndex
 		if matches[policyIndex] != 0 && effects[policyIndex] == Deny {
 			result = Deny
@@ -53,7 +57,7 @@ func (e *DefaultEffector) MergeEffects(expr string, effects []Effect, matches []
 		if policyIndex == policyLength-1 {
 			result = Allow
 		}
-	case "some(where (p_eft == allow)) && !some(where (p_eft == deny))":
+	case constant.AllowAndDenyEffect:
 		// short-circuit if matched deny rule
 		if matches[policyIndex] != 0 && effects[policyIndex] == Deny {
 			result = Deny
@@ -80,7 +84,7 @@ func (e *DefaultEffector) MergeEffects(expr string, effects []Effect, matches []
 				break
 			}
 		}
-	case "priority(p_eft) || deny", "subjectPriority(p_eft) || deny":
+	case constant.PriorityEffect, constant.SubjectPriorityEffect:
 		// reverse merge, short-circuit may be earlier
 		for i := len(effects) - 1; i >= 0; i-- {
 			if matches[i] == 0 {
diff --git a/internal_api.go b/internal_api.go
index 7dc879f..8f84006 100644
--- a/internal_api.go
+++ b/internal_api.go
@@ -15,8 +15,6 @@
 package casbin
 
 import (
-	"fmt"
-
 	Err "github.com/casbin/casbin/v2/errors"
 	"github.com/casbin/casbin/v2/model"
 	"github.com/casbin/casbin/v2/persist"
@@ -373,15 +371,11 @@ func (e *Enforcer) updateFilteredPolicies(sec string, ptype string, newRules [][
 	return ruleChanged, nil
 }
 
-func (e *Enforcer) getDomainIndex(ptype string) int {
-	p := e.model["p"][ptype]
-	pattern := fmt.Sprintf("%s_dom", ptype)
-	index := len(p.Tokens)
-	for i, token := range p.Tokens {
-		if token == pattern {
-			index = i
-			break
-		}
-	}
-	return index
+func (e *Enforcer) GetFieldIndex(ptype string, field string) (int, error) {
+	return e.model.GetFieldIndex(ptype, field)
+}
+
+func (e *Enforcer) SetFieldIndex(ptype string, field string, index int) {
+	assertion := e.model["p"][ptype]
+	assertion.FieldIndexMap[field] = index
 }
diff --git a/model/assertion.go b/model/assertion.go
index 99b3811..054dd0f 100644
--- a/model/assertion.go
+++ b/model/assertion.go
@@ -25,15 +25,15 @@ import (
 // Assertion represents an expression in a section of the model.
 // For example: r = sub, obj, act
 type Assertion struct {
-	Key       string
-	Value     string
-	Tokens    []string
-	Policy    [][]string
-	PolicyMap map[string]int
-	RM        rbac.RoleManager
-
-	logger        log.Logger
-	priorityIndex int
+	Key           string
+	Value         string
+	Tokens        []string
+	Policy        [][]string
+	PolicyMap     map[string]int
+	RM            rbac.RoleManager
+	FieldIndexMap map[string]int
+
+	logger log.Logger
 }
 
 func (ast *Assertion) buildIncrementalRoleLinks(rm rbac.RoleManager, op PolicyOp, rules [][]string) error {
@@ -93,10 +93,6 @@ func (ast *Assertion) setLogger(logger log.Logger) {
 	ast.logger = logger
 }
 
-func (ast *Assertion) initPriorityIndex() {
-	ast.priorityIndex = -1
-}
-
 func (ast *Assertion) copy() *Assertion {
 	tokens := append([]string(nil), ast.Tokens...)
 	policy := make([][]string, len(ast.Policy))
@@ -115,7 +111,7 @@ func (ast *Assertion) copy() *Assertion {
 		PolicyMap:     policyMap,
 		Tokens:        tokens,
 		Policy:        policy,
-		priorityIndex: ast.priorityIndex,
+		FieldIndexMap: ast.FieldIndexMap,
 	}
 
 	return newAst
diff --git a/model/model.go b/model/model.go
index 2141677..50ec866 100644
--- a/model/model.go
+++ b/model/model.go
@@ -24,6 +24,7 @@ import (
 	"strings"
 
 	"github.com/casbin/casbin/v2/config"
+	"github.com/casbin/casbin/v2/constant"
 	"github.com/casbin/casbin/v2/log"
 	"github.com/casbin/casbin/v2/util"
 )
@@ -63,8 +64,8 @@ func (model Model) AddDef(sec string, key string, value string) bool {
 	ast.Key = key
 	ast.Value = value
 	ast.PolicyMap = make(map[string]int)
+	ast.FieldIndexMap = make(map[string]int)
 	ast.setLogger(model.GetLogger())
-	ast.initPriorityIndex()
 
 	if sec == "r" || sec == "p" {
 		ast.Tokens = strings.Split(ast.Value, ",")
@@ -216,17 +217,14 @@ func (model Model) PrintModel() {
 }
 
 func (model Model) SortPoliciesBySubjectHierarchy() error {
-	if model["e"]["e"].Value != "subjectPriority(p_eft) || deny" {
+	if model["e"]["e"].Value != constant.SubjectPriorityEffect {
 		return nil
 	}
 	subIndex := 0
-	domainIndex := -1
 	for ptype, assertion := range model["p"] {
-		for index, token := range assertion.Tokens {
-			if token == fmt.Sprintf("%s_dom", ptype) {
-				domainIndex = index
-				break
-			}
+		domainIndex, err := model.GetFieldIndex(ptype, constant.DomainIndex)
+		if err != nil {
+			domainIndex = -1
 		}
 		policies := assertion.Policy
 		subjectHierarchyMap, err := getSubjectHierarchyMap(model["g"]["g"].Policy)
@@ -308,22 +306,17 @@ func getNameWithDomain(domain string, name string) string {
 
 func (model Model) SortPoliciesByPriority() error {
 	for ptype, assertion := range model["p"] {
-		for index, token := range assertion.Tokens {
-			if token == fmt.Sprintf("%s_priority", ptype) {
-				assertion.priorityIndex = index
-				break
-			}
-		}
-		if assertion.priorityIndex == -1 {
+		priorityIndex, err := model.GetFieldIndex(ptype, constant.PriorityIndex)
+		if err != nil {
 			continue
 		}
 		policies := assertion.Policy
 		sort.SliceStable(policies, func(i, j int) bool {
-			p1, err := strconv.Atoi(policies[i][assertion.priorityIndex])
+			p1, err := strconv.Atoi(policies[i][priorityIndex])
 			if err != nil {
 				return true
 			}
-			p2, err := strconv.Atoi(policies[j][assertion.priorityIndex])
+			p2, err := strconv.Atoi(policies[j][priorityIndex])
 			if err != nil {
 				return true
 			}
@@ -389,3 +382,23 @@ func (model Model) Copy() Model {
 	newModel.SetLogger(model.GetLogger())
 	return newModel
 }
+
+func (model Model) GetFieldIndex(ptype string, field string) (int, error) {
+	assertion := model["p"][ptype]
+	if index, ok := assertion.FieldIndexMap[field]; ok {
+		return index, nil
+	}
+	pattern := fmt.Sprintf("%s_"+field, ptype)
+	index := -1
+	for i, token := range assertion.Tokens {
+		if token == pattern {
+			index = i
+			break
+		}
+	}
+	if index == -1 {
+		return index, fmt.Errorf(field + " index is not set, please use enforcer.SetFieldIndex() to set index")
+	}
+	assertion.FieldIndexMap[field] = index
+	return index, nil
+}
diff --git a/model/policy.go b/model/policy.go
index e0e645b..c02e092 100644
--- a/model/policy.go
+++ b/model/policy.go
@@ -18,6 +18,7 @@ import (
 	"strconv"
 	"strings"
 
+	"github.com/casbin/casbin/v2/constant"
 	"github.com/casbin/casbin/v2/rbac"
 	"github.com/casbin/casbin/v2/util"
 )
@@ -148,11 +149,15 @@ func (model Model) AddPolicy(sec string, ptype string, rule []string) {
 	assertion.Policy = append(assertion.Policy, rule)
 	assertion.PolicyMap[strings.Join(rule, DefaultSep)] = len(model[sec][ptype].Policy) - 1
 
-	if sec == "p" && assertion.priorityIndex >= 0 {
-		if idxInsert, err := strconv.Atoi(rule[assertion.priorityIndex]); err == nil {
+	hasPriority := false
+	if _, ok := assertion.FieldIndexMap[constant.PriorityIndex]; ok {
+		hasPriority = true
+	}
+	if sec == "p" && hasPriority {
+		if idxInsert, err := strconv.Atoi(rule[assertion.FieldIndexMap[constant.PriorityIndex]]); err == nil {
 			i := len(assertion.Policy) - 1
 			for ; i > 0; i-- {
-				idx, err := strconv.Atoi(assertion.Policy[i-1][assertion.priorityIndex])
+				idx, err := strconv.Atoi(assertion.Policy[i-1][assertion.FieldIndexMap[constant.PriorityIndex]])
 				if err != nil {
 					break
 				}
diff --git a/rbac_api.go b/rbac_api.go
index 370af88..b366a26 100644
--- a/rbac_api.go
+++ b/rbac_api.go
@@ -15,6 +15,7 @@
 package casbin
 
 import (
+	"github.com/casbin/casbin/v2/constant"
 	"github.com/casbin/casbin/v2/errors"
 	"github.com/casbin/casbin/v2/util"
 )
@@ -99,7 +100,11 @@ func (e *Enforcer) DeleteUser(user string) (bool, error) {
 		return res1, err
 	}
 
-	res2, err := e.RemoveFilteredPolicy(0, user)
+	subIndex, err := e.GetFieldIndex("p", constant.SubjectIndex)
+	if err != nil {
+		return false, err
+	}
+	res2, err := e.RemoveFilteredPolicy(subIndex, user)
 	return res1 || res2, err
 }
 
@@ -112,7 +117,11 @@ func (e *Enforcer) DeleteRole(role string) (bool, error) {
 		return res1, err
 	}
 
-	res2, err := e.RemoveFilteredPolicy(0, role)
+	subIndex, err := e.GetFieldIndex("p", constant.SubjectIndex)
+	if err != nil {
+		return false, err
+	}
+	res2, err := e.RemoveFilteredPolicy(subIndex, role)
 	return res1 || res2, err
 }
 
@@ -147,7 +156,11 @@ func (e *Enforcer) DeletePermissionForUser(user string, permission ...string) (b
 // DeletePermissionsForUser deletes permissions for a user or role.
 // Returns false if the user or role does not have any permissions (aka not affected).
 func (e *Enforcer) DeletePermissionsForUser(user string) (bool, error) {
-	return e.RemoveFilteredPolicy(0, user)
+	subIndex, err := e.GetFieldIndex("p", constant.SubjectIndex)
+	if err != nil {
+		return false, err
+	}
+	return e.RemoveFilteredPolicy(subIndex, user)
 }
 
 // GetPermissionsForUser gets permissions for a user or role.
@@ -163,13 +176,18 @@ func (e *Enforcer) GetNamedPermissionsForUser(ptype string, user string, domain
 			continue
 		}
 		args := make([]string, len(assertion.Tokens))
-		args[0] = user
+		subIndex, err := e.GetFieldIndex("p", constant.SubjectIndex)
+		if err != nil {
+			subIndex = 0
+		}
+		args[subIndex] = user
 
 		if len(domain) > 0 {
-			index := e.getDomainIndex(ptype)
-			if index < len(assertion.Tokens) {
-				args[index] = domain[0]
+			index, err := e.GetFieldIndex(ptype, constant.DomainIndex)
+			if err != nil {
+				return permission
 			}
+			args[index] = domain[0]
 		}
 		perm := e.GetFilteredNamedPolicy(ptype, 0, args...)
 		permission = append(permission, perm...)
diff --git a/rbac_api_with_domains.go b/rbac_api_with_domains.go
index 874d07f..04944f3 100644
--- a/rbac_api_with_domains.go
+++ b/rbac_api_with_domains.go
@@ -14,6 +14,8 @@
 
 package casbin
 
+import "github.com/casbin/casbin/v2/constant"
+
 // GetUsersForRoleInDomain gets the users that has a role inside a domain. Add by Gordon
 func (e *Enforcer) GetUsersForRoleInDomain(name string, domain string) []string {
 	res, _ := e.model["g"]["g"].RM.GetUsers(name, domain)
@@ -28,13 +30,7 @@ func (e *Enforcer) GetRolesForUserInDomain(name string, domain string) []string
 
 // GetPermissionsForUserInDomain gets permissions for a user or role inside a domain.
 func (e *Enforcer) GetPermissionsForUserInDomain(user string, domain string) [][]string {
-	var res [][]string
-	users, _ := e.GetRolesForUser(user, domain)
-	users = append(users, user)
-	for _, singleUser := range users {
-		policy := e.GetFilteredPolicy(0, singleUser, domain)
-		res = append(res, policy...)
-	}
+	res, _ := e.GetImplicitPermissionsForUser(user, domain)
 	return res
 }
 
@@ -72,7 +68,10 @@ func (e *Enforcer) GetAllUsersByDomain(domain string) []string {
 	g := e.model["g"]["g"]
 	p := e.model["p"]["p"]
 	users := make([]string, 0)
-	index := e.getDomainIndex("p")
+	index, err := e.GetFieldIndex("p", constant.DomainIndex)
+	if err != nil {
+		return []string{}
+	}
 
 	getUser := func(index int, policies [][]string, domain string, m map[string]struct{}) []string {
 		if len(policies) == 0 || len(policies[0]) <= index {
@@ -97,7 +96,10 @@ func (e *Enforcer) GetAllUsersByDomain(domain string) []string {
 func (e *Enforcer) DeleteAllUsersByDomain(domain string) (bool, error) {
 	g := e.model["g"]["g"]
 	p := e.model["p"]["p"]
-	index := e.getDomainIndex("p")
+	index, err := e.GetFieldIndex("p", constant.DomainIndex)
+	if err != nil {
+		return false, err
+	}
 
 	getUser := func(index int, policies [][]string, domain string) [][]string {
 		if len(policies) == 0 || len(policies[0]) <= index {


Test patch:
diff --git a/model/model_test.go b/model/model_test.go
index 5fafa76..1933fe4 100644
--- a/model/model_test.go
+++ b/model/model_test.go
@@ -21,6 +21,7 @@ import (
 	"testing"
 
 	"github.com/casbin/casbin/v2/config"
+	"github.com/casbin/casbin/v2/constant"
 )
 
 var (
@@ -140,7 +141,7 @@ func testModelToText(t *testing.T, mData, mExpected string) {
 	expected := map[string]string{
 		"r": "sub, obj, act",
 		"p": "sub, obj, act",
-		"e": "some(where (p_eft == allow))",
+		"e": constant.AllowOverrideEffect,
 		"m": mExpected,
 	}
 	addData := func(ptype string) {