fix: look up pattern tokens by group name in KeyMatch4 and KeyGet2/3 by arshilkxwork · Pull Request #1767 · apache/casbin · GitHub
Skip to content

fix: look up pattern tokens by group name in KeyMatch4 and KeyGet2/3 - #1767

Open
arshilkxwork wants to merge 1 commit into
apache:masterfrom
arshilkxwork:fix/keymatch4-keyget-capture-groups
Open

arshilkxwork wants to merge 1 commit into
apache:masterfrom
arshilkxwork:fix/keymatch4-keyget-capture-groups

Conversation

@arshilkxwork

Copy link
Copy Markdown

KeyMatch4, KeyGet2 and KeyGet3 read the value of the n-th pattern variable from capture group n+1. That only holds when the pattern has no capture groups of its own. With one, the indices shift:

  • KeyMatch4("/a/42", "/(a|b)/{id}") panics with KeyMatch4: number of tokens is not equal to number of values. Enforce recovers it and returns a panic: error with a stack trace. When util.KeyMatch4 is registered with AddNamedMatchingFunc, role lookups run outside that recover: after adding g2, /(book|pen)/{id}/copy/{id}, book_group, GetNamedImplicitRolesForUser("g2", "/book/1/copy/1") panics the caller. This is the same role-manager path 0a73027 covered for invalid patterns.
  • KeyGet2("/a/x/42", "/(a|b)/x/:id", "id") returns "a" instead of "42", and KeyGet3 does the same with {id}. No error is raised. A group that comes after the variable (/:id/(a|b)) happens to work today.

Each pattern variable now compiles to a named group, (?P<casbinToken0>[^/]+) and so on, and compilePattern stores the submatch index of each one using SubexpIndex. keyGet and keyMatch4 read values through those indices. The token/value count check in keyMatch4 is removed, since the two can no longer disagree.

Parse errors from these three functions quote the rewritten regexp, so they now show the group names. TestKeyGet2Func pins that text and is updated to `^/(?P<casbinToken0>[^/]+)/(using$`.

KeyGet2/KeyGet3 also switch from FindAllStringSubmatch to FindStringSubmatch. The pattern is anchored, so only the first match was ever used. On my machine BenchmarkKeyGet2 goes from 171 to 134 ns/op (3 allocs to 1), BenchmarkKeyGet3 from 217 to 200 ns/op, and BenchmarkKeyMatch4 is unchanged.

Testing:

  • New cases in TestKeyGet2, TestKeyGet3 and TestKeyMatch4 with groups before, between and after the variables. On master the KeyGet cases return the wrong segment and TestKeyMatch4 panics.
  • New TestPatternWithCaptureGroupInPolicy registers util.KeyMatch4 as the g2 matching function with /(book|pen)/{id}/copy/{id} and checks Enforce and GetNamedImplicitRolesForUser. On master Enforce returns the panic error and GetNamedImplicitRolesForUser panics.
  • go test -race ./... passes, and golangci-lint v2.15.0 reports no issues.

KeyMatch4, KeyGet2 and KeyGet3 assumed the n-th pattern variable was
capture group n+1. A policy pattern with a capture group of its own
breaks that: "/(a|b)/{id}" made KeyMatch4 panic with "number of tokens
is not equal to number of values", and KeyGet2("/a/x/42", "/(a|b)/x/:id",
"id") returned "a" instead of "42". With KeyMatch4 registered as a
role-manager matching function, role lookups such as
GetNamedImplicitRolesForUser hit the panic outside Enforce's recover.

Each variable now compiles to a named group (?P<casbinTokenN>...), and
compilePattern records its submatch index with SubexpIndex. The count
check that panicked is removed because the two can no longer disagree.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant