bitflicker64 commented on code in PR #1760:
URL: https://github.com/apache/casbin/pull/1760#discussion_r3952153373
##########
rbac_api.go:
##########
@@ -197,7 +197,7 @@ func (e *Enforcer) GetNamedPermissionsForUser(ptype string,
user string, domain
continue
}
args := make([]string, len(assertion.Tokens))
- subIndex, err := e.GetFieldIndex("p", constant.SubjectIndex)
+ subIndex, err := e.GetFieldIndex(ptype, constant.SubjectIndex)
if err != nil {
subIndex = 0
Review Comment:
Pre-existing, not blocking: the `subIndex = 0` fallback here still swallows
a missing `<ptype>_sub` token (with no `SetFieldIndex` call), while the domain
lookup a few lines down returns the error instead. Filtering on column 0 by
accident would reproduce the same "other users' rows" symptom this PR fixes.
Might be worth returning the error in a follow-up so the two lookups behave the
same way.
##########
rbac_api_test.go:
##########
@@ -892,3 +893,55 @@ func TestGetImplicitObjectPatternsForUser(t *testing.T) {
// Test case 5: non-existent action
testGetImplicitObjectPatternsForUser(t, e, "admin", "domain1",
"non_existent", []string{})
}
+
+func TestNamedPermissionsForUserWithDifferentColumnOrder(t *testing.T) {
+ text := `
+[request_definition]
+r = sub, dom, obj, act
+
+[policy_definition]
+p = sub, dom, obj, act
+p2 = dom, sub, obj, act
+
+[role_definition]
+g = _, _, _
+
+[policy_effect]
+e = some(where (p.eft == allow))
+
+[matchers]
+m = g(r.sub, p.sub, r.dom) && r.dom == p.dom && r.obj == p.obj && r.act ==
p.act
+`
+ m, err := model.NewModelFromString(text)
+ if err != nil {
+ t.Fatal(err)
+ }
+ e, err := NewEnforcer(m)
+ if err != nil {
+ t.Fatal(err)
+ }
+
+ if _, err = e.AddNamedPolicy("p2", "tenant1", "alice", "report1",
"read"); err != nil {
+ t.Fatal(err)
+ }
+ if _, err = e.AddNamedPolicy("p2", "tenant1", "bob", "report2",
"read"); err != nil {
+ t.Fatal(err)
+ }
+
+ got, err := e.GetNamedPermissionsForUser("p2", "alice")
Review Comment:
Nit: `testGetNamedPermissionsForUser` at the top of this file already wraps
this call plus the `Array2DEquals` check, so the two assertions could each
become a one-liner. Fine as is if you would rather keep the test self-contained.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
