rule: Resolve matcher cache

A bug caused the rule matcher to not cache the regular expression result. Closes #291
ory · Dec 17, 2019 · 1c3070f · 1c3070f
1 parent a178031
commit 1c3070f
Show file tree

Hide file tree

Showing 11 changed files with 75 additions and 56 deletions.
diff --git a/api/decision_test.go b/api/decision_test.go
@@ -59,14 +59,14 @@ func TestDecisionAPI(t *testing.T) {
 	defer ts.Close()
 
 	ruleNoOpAuthenticator := rule.Rule{
-		Match:          rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/authn-noop/<[0-9]+>"},
+		Match:          &rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/authn-noop/<[0-9]+>"},
 		Authenticators: []rule.RuleHandler{{Handler: "noop"}},
 		Authorizer:     rule.RuleHandler{Handler: "allow"},
 		Mutators:       []rule.RuleHandler{{Handler: "noop"}},
 		Upstream:       rule.Upstream{URL: ""},
 	}
 	ruleNoOpAuthenticatorModifyUpstream := rule.Rule{
-		Match:          rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/strip-path/authn-noop/<[0-9]+>"},
+		Match:          &rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/strip-path/authn-noop/<[0-9]+>"},
 		Authenticators: []rule.RuleHandler{{Handler: "noop"}},
 		Authorizer:     rule.RuleHandler{Handler: "allow"},
 		Mutators:       []rule.RuleHandler{{Handler: "noop"}},
@@ -118,7 +118,7 @@ func TestDecisionAPI(t *testing.T) {
 			d:   "should fail because no authorizer was configured",
 			url: ts.URL + "/decisions" + "/authn-anon/authz-none/cred-none/1234",
 			rules: []rule.Rule{{
-				Match:          rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-none/cred-none/<[0-9]+>"},
+				Match:          &rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-none/cred-none/<[0-9]+>"},
 				Authenticators: []rule.RuleHandler{{Handler: "anonymous"}},
 				Upstream:       rule.Upstream{URL: ""},
 			}},
@@ -131,7 +131,7 @@ func TestDecisionAPI(t *testing.T) {
 			d:   "should fail because no mutator was configured",
 			url: ts.URL + "/decisions" + "/authn-anon/authz-allow/cred-none/1234",
 			rules: []rule.Rule{{
-				Match:          rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-allow/cred-none/<[0-9]+>"},
+				Match:          &rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-allow/cred-none/<[0-9]+>"},
 				Authenticators: []rule.RuleHandler{{Handler: "anonymous"}},
 				Authorizer:     rule.RuleHandler{Handler: "allow"},
 				Upstream:       rule.Upstream{URL: ""},
@@ -142,7 +142,7 @@ func TestDecisionAPI(t *testing.T) {
 			d:   "should pass with anonymous and everything else set to noop",
 			url: ts.URL + "/decisions" + "/authn-anon/authz-allow/cred-noop/1234",
 			rules: []rule.Rule{{
-				Match:          rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-allow/cred-noop/<[0-9]+>"},
+				Match:          &rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-allow/cred-noop/<[0-9]+>"},
 				Authenticators: []rule.RuleHandler{{Handler: "anonymous"}},
 				Authorizer:     rule.RuleHandler{Handler: "allow"},
 				Mutators:       []rule.RuleHandler{{Handler: "noop"}},
@@ -155,7 +155,7 @@ func TestDecisionAPI(t *testing.T) {
 			d:   "should fail when authorizer fails",
 			url: ts.URL + "/decisions" + "/authn-anon/authz-deny/cred-noop/1234",
 			rules: []rule.Rule{{
-				Match:          rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-deny/cred-noop/<[0-9]+>"},
+				Match:          &rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-deny/cred-noop/<[0-9]+>"},
 				Authenticators: []rule.RuleHandler{{Handler: "anonymous"}},
 				Authorizer:     rule.RuleHandler{Handler: "deny"},
 				Mutators:       []rule.RuleHandler{{Handler: "noop"}},
@@ -167,7 +167,7 @@ func TestDecisionAPI(t *testing.T) {
 			d:   "should fail when authenticator fails",
 			url: ts.URL + "/decisions" + "/authn-broken/authz-none/cred-none/1234",
 			rules: []rule.Rule{{
-				Match:          rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/authn-broken/authz-none/cred-none/<[0-9]+>"},
+				Match:          &rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/authn-broken/authz-none/cred-none/<[0-9]+>"},
 				Authenticators: []rule.RuleHandler{{Handler: "unauthorized"}},
 				Upstream:       rule.Upstream{URL: ""},
 			}},
@@ -177,7 +177,7 @@ func TestDecisionAPI(t *testing.T) {
 			d:   "should fail when mutator fails",
 			url: ts.URL + "/decisions" + "/authn-anonymous/authz-allow/cred-broken/1234",
 			rules: []rule.Rule{{
-				Match:          rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/authn-anonymous/authz-allow/cred-broken/<[0-9]+>"},
+				Match:          &rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/authn-anonymous/authz-allow/cred-broken/<[0-9]+>"},
 				Authenticators: []rule.RuleHandler{{Handler: "anonymous"}},
 				Authorizer:     rule.RuleHandler{Handler: "allow"},
 				Mutators:       []rule.RuleHandler{{Handler: "broken"}},
@@ -189,7 +189,7 @@ func TestDecisionAPI(t *testing.T) {
 			d:   "should fail when one of the mutators fails",
 			url: ts.URL + "/decisions" + "/authn-anonymous/authz-allow/cred-broken/1234",
 			rules: []rule.Rule{{
-				Match:          rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/authn-anonymous/authz-allow/cred-broken/<[0-9]+>"},
+				Match:          &rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/authn-anonymous/authz-allow/cred-broken/<[0-9]+>"},
 				Authenticators: []rule.RuleHandler{{Handler: "anonymous"}},
 				Authorizer:     rule.RuleHandler{Handler: "allow"},
 				Mutators:       []rule.RuleHandler{{Handler: "noop"}, {Handler: "broken"}},

diff --git a/api/rule_test.go b/api/rule_test.go
@@ -59,7 +59,7 @@ func TestHandler(t *testing.T) {
 	rules := []rule.Rule{
 		{
 			ID: "foo1",
-			Match: rule.RuleMatch{
+			Match: &rule.RuleMatch{
 				URL:     "https://localhost:1234/<foo|bar>",
 				Methods: []string{"POST"},
 			},
@@ -75,7 +75,7 @@ func TestHandler(t *testing.T) {
 		},
 		{
 			ID: "foo2",
-			Match: rule.RuleMatch{
+			Match: &rule.RuleMatch{
 				URL:     "https://localhost:34/<baz|bar>",
 				Methods: []string{"GET"},
 			},

diff --git a/pipeline/authz/keto_engine_acp_ory_test.go b/pipeline/authz/keto_engine_acp_ory_test.go
@@ -70,7 +70,7 @@ func TestAuthorizerKetoWarden(t *testing.T) {
 		{
 			config: []byte(`{ "required_action": "action", "required_resource": "resource" }`),
 			rule: &rule.Rule{
-				Match: rule.RuleMatch{
+				Match: &rule.RuleMatch{
 					Methods: []string{"POST"},
 					URL:     "https://localhost/",
 				},
@@ -82,7 +82,7 @@ func TestAuthorizerKetoWarden(t *testing.T) {
 		{
 			config: []byte(`{ "required_action": "action", "required_resource": "resource", "flavor": "regex" }`),
 			rule: &rule.Rule{
-				Match: rule.RuleMatch{
+				Match: &rule.RuleMatch{
 					Methods: []string{"POST"},
 					URL:     "https://localhost/",
 				},
@@ -99,7 +99,7 @@ func TestAuthorizerKetoWarden(t *testing.T) {
 		{
 			config: []byte(`{ "required_action": "action", "required_resource": "resource", "flavor": "exact" }`),
 			rule: &rule.Rule{
-				Match: rule.RuleMatch{
+				Match: &rule.RuleMatch{
 					Methods: []string{"POST"},
 					URL:     "https://localhost/",
 				},
@@ -119,7 +119,7 @@ func TestAuthorizerKetoWarden(t *testing.T) {
 		{
 			config: []byte(`{ "required_action": "action:$1:$2", "required_resource": "resource:$1:$2" }`),
 			rule: &rule.Rule{
-				Match: rule.RuleMatch{
+				Match: &rule.RuleMatch{
 					Methods: []string{"POST"},
 					URL:     "https://localhost/api/users/<[0-9]+>/<[a-z]+>",
 				},
@@ -145,7 +145,7 @@ func TestAuthorizerKetoWarden(t *testing.T) {
 		{
 			config: []byte(`{ "required_action": "action:$1:$2", "required_resource": "resource:$1:$2", "subject": "{{ .Extra.name }}" }`),
 			rule: &rule.Rule{
-				Match: rule.RuleMatch{
+				Match: &rule.RuleMatch{
 					Methods: []string{"POST"},
 					URL:     "https://localhost/api/users/<[0-9]+>/<[a-z]+>",
 				},
@@ -171,7 +171,7 @@ func TestAuthorizerKetoWarden(t *testing.T) {
 		{
 			config: []byte(`{ "required_action": "action:$1:$2", "required_resource": "resource:$1:$2", "subject": "{{ .Extra.name }}" }`),
 			rule: &rule.Rule{
-				Match: rule.RuleMatch{
+				Match: &rule.RuleMatch{
 					Methods: []string{"POST"},
 					URL:     "https://localhost/api/users/<[0-9]+>/<[a-z]+>",
 				},

diff --git a/proxy/proxy_test.go b/proxy/proxy_test.go
@@ -93,14 +93,14 @@ func TestProxy(t *testing.T) {
 	viper.Set(configuration.ViperKeyMutatorNoopIsEnabled, true)
 
 	ruleNoOpAuthenticator := rule.Rule{
-		Match:          rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/authn-noop/<[0-9]+>"},
+		Match:          &rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/authn-noop/<[0-9]+>"},
 		Authenticators: []rule.RuleHandler{{Handler: "noop"}},
 		Authorizer:     rule.RuleHandler{Handler: "allow"},
 		Mutators:       []rule.RuleHandler{{Handler: "noop"}},
 		Upstream:       rule.Upstream{URL: backend.URL},
 	}
 	ruleNoOpAuthenticatorModifyUpstream := rule.Rule{
-		Match:          rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/strip-path/authn-noop/<[0-9]+>"},
+		Match:          &rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/strip-path/authn-noop/<[0-9]+>"},
 		Authenticators: []rule.RuleHandler{{Handler: "noop"}},
 		Authorizer:     rule.RuleHandler{Handler: "allow"},
 		Mutators:       []rule.RuleHandler{{Handler: "noop"}},
@@ -164,7 +164,7 @@ func TestProxy(t *testing.T) {
 			d:   "should fail because no authorizer was configured",
 			url: ts.URL + "/authn-anon/authz-none/cred-none/1234",
 			rules: []rule.Rule{{
-				Match:          rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-none/cred-none/<[0-9]+>"},
+				Match:          &rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-none/cred-none/<[0-9]+>"},
 				Authenticators: []rule.RuleHandler{{Handler: "anonymous"}},
 				Upstream:       rule.Upstream{URL: backend.URL},
 			}},
@@ -177,7 +177,7 @@ func TestProxy(t *testing.T) {
 			d:   "should fail because no credentials issuer was configured",
 			url: ts.URL + "/authn-anon/authz-allow/cred-none/1234",
 			rules: []rule.Rule{{
-				Match:          rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-allow/cred-none/<[0-9]+>"},
+				Match:          &rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-allow/cred-none/<[0-9]+>"},
 				Authenticators: []rule.RuleHandler{{Handler: "anonymous"}},
 				Authorizer:     rule.RuleHandler{Handler: "allow"},
 				Upstream:       rule.Upstream{URL: backend.URL},
@@ -188,7 +188,7 @@ func TestProxy(t *testing.T) {
 			d:   "should pass with anonymous and everything else set to noop",
 			url: ts.URL + "/authn-anon/authz-allow/cred-noop/1234",
 			rules: []rule.Rule{{
-				Match:          rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-allow/cred-noop/<[0-9]+>"},
+				Match:          &rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-allow/cred-noop/<[0-9]+>"},
 				Authenticators: []rule.RuleHandler{{Handler: "anonymous"}},
 				Authorizer:     rule.RuleHandler{Handler: "allow"},
 				Mutators:       []rule.RuleHandler{{Handler: "noop"}},
@@ -205,7 +205,7 @@ func TestProxy(t *testing.T) {
 			d:   "should fail when authorizer fails",
 			url: ts.URL + "/authn-anon/authz-deny/cred-noop/1234",
 			rules: []rule.Rule{{
-				Match:          rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-deny/cred-noop/<[0-9]+>"},
+				Match:          &rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-deny/cred-noop/<[0-9]+>"},
 				Authenticators: []rule.RuleHandler{{Handler: "anonymous"}},
 				Authorizer:     rule.RuleHandler{Handler: "deny"},
 				Mutators:       []rule.RuleHandler{{Handler: "noop"}},
@@ -217,7 +217,7 @@ func TestProxy(t *testing.T) {
 			d:   "should fail when authenticator fails",
 			url: ts.URL + "/authn-broken/authz-none/cred-none/1234",
 			rules: []rule.Rule{{
-				Match:          rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/authn-broken/authz-none/cred-none/<[0-9]+>"},
+				Match:          &rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/authn-broken/authz-none/cred-none/<[0-9]+>"},
 				Authenticators: []rule.RuleHandler{{Handler: "unauthorized"}},
 				Upstream:       rule.Upstream{URL: backend.URL},
 			}},
@@ -227,7 +227,7 @@ func TestProxy(t *testing.T) {
 			d:   "should fail because no mutator was configured",
 			url: ts.URL + "/authn-anon/authz-deny/cred-noop/1234",
 			rules: []rule.Rule{{
-				Match:          rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-deny/cred-noop/<[0-9]+>"},
+				Match:          &rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-deny/cred-noop/<[0-9]+>"},
 				Authenticators: []rule.RuleHandler{{Handler: "anonymous"}},
 				Authorizer:     rule.RuleHandler{Handler: "allow"},
 				Upstream:       rule.Upstream{URL: backend.URL},
@@ -238,7 +238,7 @@ func TestProxy(t *testing.T) {
 			d:   "should fail when one of the mutators fails",
 			url: ts.URL + "/authn-anon/authz-deny/cred-noop/1234",
 			rules: []rule.Rule{{
-				Match:          rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-deny/cred-noop/<[0-9]+>"},
+				Match:          &rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-deny/cred-noop/<[0-9]+>"},
 				Authenticators: []rule.RuleHandler{{Handler: "anonymous"}},
 				Authorizer:     rule.RuleHandler{Handler: "allow"},
 				Mutators:       []rule.RuleHandler{{Handler: "noop"}, {Handler: "broken"}},
@@ -250,7 +250,7 @@ func TestProxy(t *testing.T) {
 			d:   "should fail when credentials issuer fails",
 			url: ts.URL + "/authn-anonymous/authz-allow/cred-broken/1234",
 			rules: []rule.Rule{{
-				Match:          rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/authn-anonymous/authz-allow/cred-broken/<[0-9]+>"},
+				Match:          &rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/authn-anonymous/authz-allow/cred-broken/<[0-9]+>"},
 				Authenticators: []rule.RuleHandler{{Handler: "anonymous"}},
 				Authorizer:     rule.RuleHandler{Handler: "allow"},
 				Mutators:       []rule.RuleHandler{{Handler: "broken"}},

diff --git a/rule/matcher_test.go b/rule/matcher_test.go
@@ -33,7 +33,7 @@ import (
 var testRules = []Rule{
 	{
 		ID:             "foo1",
-		Match:          RuleMatch{URL: "https://localhost:1234/<foo|bar>", Methods: []string{"POST"}},
+		Match:          &RuleMatch{URL: "https://localhost:1234/<foo|bar>", Methods: []string{"POST"}},
 		Description:    "Create users rule",
 		Authorizer:     RuleHandler{Handler: "allow", Config: []byte(`{"type":"any"}`)},
 		Authenticators: []RuleHandler{{Handler: "anonymous", Config: []byte(`{"name":"anonymous1"}`)}},
@@ -42,7 +42,7 @@ var testRules = []Rule{
 	},
 	{
 		ID:             "foo2",
-		Match:          RuleMatch{URL: "https://localhost:34/<baz|bar>", Methods: []string{"GET"}},
+		Match:          &RuleMatch{URL: "https://localhost:34/<baz|bar>", Methods: []string{"GET"}},
 		Description:    "Get users rule",
 		Authorizer:     RuleHandler{Handler: "deny", Config: []byte(`{"type":"any"}`)},
 		Authenticators: []RuleHandler{{Handler: "oauth2_introspection", Config: []byte(`{"name":"anonymous1"}`)}},
@@ -51,7 +51,7 @@ var testRules = []Rule{
 	},
 	{
 		ID:             "foo3",
-		Match:          RuleMatch{URL: "https://localhost:343/<baz|bar>", Methods: []string{"GET"}},
+		Match:          &RuleMatch{URL: "https://localhost:343/<baz|bar>", Methods: []string{"GET"}},
 		Description:    "Get users rule",
 		Authorizer:     RuleHandler{Handler: "deny"},
 		Authenticators: []RuleHandler{{Handler: "oauth2_introspection"}},
@@ -100,6 +100,15 @@ func TestMatcher(t *testing.T) {
 				testMatcher(t, matcher, "DELETE", "https://localhost:1234/foo", true, nil)
 			})
 
+			t.Run("case=cache", func(t *testing.T) {
+				r, err := matcher.Match(context.Background(), "GET", mustParseURL(t, "https://localhost:34/baz"))
+				require.NoError(t, err)
+				got, err := matcher.Get(context.Background(), r.ID)
+				require.NoError(t, err)
+				assert.NotEmpty(t, got.Match.compiledURL)
+				assert.NotEmpty(t, got.Match.compiledURLChecksum)
+			})
+
 			require.NoError(t, matcher.Set(context.Background(), testRules[1:]))
 
 			t.Run("case=updated", func(t *testing.T) {

diff --git a/rule/repository_memory.go b/rule/repository_memory.go
@@ -105,14 +105,16 @@ func (m *RepositoryMemory) Set(ctx context.Context, rules []Rule) error {
 }
 
 func (m *RepositoryMemory) Match(ctx context.Context, method string, u *url.URL) (*Rule, error) {
-	m.RLock()
-	defer m.RUnlock()
+	m.Lock()
+	defer m.Unlock()
 
 	var rules []Rule
-	for _, rule := range m.rules {
-		if err := rule.IsMatching(method, u); err == nil {
-			rules = append(rules, rule)
+	for k := range m.rules {
+		r := &m.rules[k]
+		if err := r.IsMatching(method, u); err == nil {
+			rules = append(rules, *r)
 		}
+		m.rules[k] = *r
 	}
 
 	if len(rules) == 0 {

diff --git a/rule/rule.go b/rule/rule.go
@@ -78,7 +78,7 @@ type Rule struct {
 	Description string `json:"description"`
 
 	// Match defines the URL that this rule should match.
-	Match RuleMatch `json:"match"`
+	Match *RuleMatch `json:"match"`
 
 	// Authenticators is a list of authentication handlers that will try and authenticate the provided credentials.
 	// Authenticators are checked iteratively from index 0 to n and if the first authenticator to return a positive
@@ -118,7 +118,7 @@ var _ json.Unmarshaler = new(Rule)
 
 func NewRule() *Rule {
 	return &Rule{
-		Match:          RuleMatch{},
+		Match:          &RuleMatch{},
 		Authenticators: []RuleHandler{},
 		Mutators:       []RuleHandler{},
 	}
@@ -129,7 +129,7 @@ func (r *Rule) UnmarshalJSON(raw []byte) error {
 		ID             string        `json:"id"`
 		Version        string        `json:"version"`
 		Description    string        `json:"description"`
-		Match          RuleMatch     `json:"match"`
+		Match          *RuleMatch    `json:"match"`
 		Authenticators []RuleHandler `json:"authenticators"`
 		Authorizer     RuleHandler   `json:"authorizer"`
 		Mutators       []RuleHandler `json:"mutators"`
@@ -176,11 +176,11 @@ func (r *Rule) CompileURL() (*regexp.Regexp, error) {
 	m := r.Match
 	c := crc32.ChecksumIEEE([]byte(m.URL))
 	if m.compiledURL == nil || c != m.compiledURLChecksum {
-		r, err := compiler.CompileRegex(m.URL, '<', '>')
+		regex, err := compiler.CompileRegex(m.URL, '<', '>')
 		if err != nil {
 			return nil, errors.Wrap(err, "Unable to compile URL matcher")
 		}
-		m.compiledURL = r
+		m.compiledURL = regex
 		m.compiledURLChecksum = c
 	}