proxy: Authenticator noop should not bypass

Closes #97 Signed-off-by: arekkas <[email protected]>
ory · Aug 9, 2018 · 6f8ab4f · 6f8ab4f
1 parent 078542a
commit 6f8ab4f
Show file tree

Hide file tree

Showing 7 changed files with 40 additions and 24 deletions.
diff --git a/UPGRADE.md b/UPGRADE.md
@@ -16,6 +16,15 @@ before finalizing the upgrade process.
 
 <!-- END doctoc generated TOC please keep comment here to allow auto update -->
 
+## 1.0.0-beta.8
+
+### `noop` authenticator no longer bypasses authorizers/credentials issuers
+
+The `noop` authenticator is now very similar to `anonymous` with the difference that no anonymous subject is being
+set.
+
+Previously, the `noop` authenticator bypassed the authorizer and credential issuers. This patch changes that.
+
 ## 1.0.0-beta.2
 
 This release introduces serious breaking changes. If you are upgrading, you will - unfortunately - need to

diff --git a/judge/handler_test.go b/judge/handler_test.go
@@ -34,7 +34,7 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func TestProxy(t *testing.T) {
+func TestJudge(t *testing.T) {
 	matcher := &rule.CachedMatcher{Rules: map[string]rule.Rule{}}
 	rh := proxy.NewRequestHandler(
 		nil,
@@ -50,14 +50,18 @@ func TestProxy(t *testing.T) {
 	defer ts.Close()
 
 	ruleNoOpAuthenticator := rule.Rule{
-		Match:          rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/authn-noop/<[0-9]+>"},
-		Authenticators: []rule.RuleHandler{{Handler: "noop"}},
-		Upstream:       rule.Upstream{URL: ""},
+		Match:             rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/authn-noop/<[0-9]+>"},
+		Authenticators:    []rule.RuleHandler{{Handler: "noop"}},
+		Authorizer:        rule.RuleHandler{Handler: proxy.NewAuthorizerAllow().GetID()},
+		CredentialsIssuer: rule.RuleHandler{Handler: proxy.NewCredentialsIssuerNoOp().GetID()},
+		Upstream:          rule.Upstream{URL: ""},
 	}
 	ruleNoOpAuthenticatorModifyUpstream := rule.Rule{
-		Match:          rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/strip-path/authn-noop/<[0-9]+>"},
-		Authenticators: []rule.RuleHandler{{Handler: "noop"}},
-		Upstream:       rule.Upstream{URL: "", StripPath: "/strip-path/", PreserveHost: true},
+		Match:             rule.RuleMatch{Methods: []string{"GET"}, URL: ts.URL + "/strip-path/authn-noop/<[0-9]+>"},
+		Authenticators:    []rule.RuleHandler{{Handler: "noop"}},
+		Authorizer:        rule.RuleHandler{Handler: proxy.NewAuthorizerAllow().GetID()},
+		CredentialsIssuer: rule.RuleHandler{Handler: proxy.NewCredentialsIssuerNoOp().GetID()},
+		Upstream:          rule.Upstream{URL: "", StripPath: "/strip-path/", PreserveHost: true},
 	}
 
 	for k, tc := range []struct {

diff --git a/proxy/authenticator_noop.go b/proxy/authenticator_noop.go
@@ -5,7 +5,6 @@ import (
 	"net/http"
 
 	"github.com/ory/oathkeeper/rule"
-	"github.com/pkg/errors"
 )
 
 type AuthenticatorNoOp struct{}
@@ -19,5 +18,5 @@ func (a *AuthenticatorNoOp) GetID() string {
 }
 
 func (a *AuthenticatorNoOp) Authenticate(r *http.Request, config json.RawMessage, rl *rule.Rule) (*AuthenticationSession, error) {
-	return nil, errors.WithStack(ErrAuthenticatorBypassed)
+	return &AuthenticationSession{Subject: ""}, nil
 }
diff --git a/proxy/authenticator_noop_test.go b/proxy/authenticator_noop_test.go
@@ -32,5 +32,5 @@ func TestAuthenticatorNoop(t *testing.T) {
 	assert.NotEmpty(t, NewAuthenticatorNoOp().GetID())
 
 	_, err := NewAuthenticatorNoOp().Authenticate(nil, nil, nil)
-	require.Error(t, err)
+	require.NoError(t, err)
 }
diff --git a/proxy/proxy_test.go b/proxy/proxy_test.go
@@ -87,14 +87,18 @@ func TestProxy(t *testing.T) {
 	defer proxy.Close()
 
 	ruleNoOpAuthenticator := rule.Rule{
-		Match:          rule.RuleMatch{Methods: []string{"GET"}, URL: proxy.URL + "/authn-noop/<[0-9]+>"},
-		Authenticators: []rule.RuleHandler{{Handler: "noop"}},
-		Upstream:       rule.Upstream{URL: backend.URL},
+		Match:             rule.RuleMatch{Methods: []string{"GET"}, URL: proxy.URL + "/authn-noop/<[0-9]+>"},
+		Authenticators:    []rule.RuleHandler{{Handler: "noop"}},
+		Authorizer:        rule.RuleHandler{Handler: "allow"},
+		CredentialsIssuer: rule.RuleHandler{Handler: "noop"},
+		Upstream:          rule.Upstream{URL: backend.URL},
 	}
 	ruleNoOpAuthenticatorModifyUpstream := rule.Rule{
-		Match:          rule.RuleMatch{Methods: []string{"GET"}, URL: proxy.URL + "/strip-path/authn-noop/<[0-9]+>"},
-		Authenticators: []rule.RuleHandler{{Handler: "noop"}},
-		Upstream:       rule.Upstream{URL: backend.URL, StripPath: "/strip-path/", PreserveHost: true},
+		Match:             rule.RuleMatch{Methods: []string{"GET"}, URL: proxy.URL + "/strip-path/authn-noop/<[0-9]+>"},
+		Authenticators:    []rule.RuleHandler{{Handler: "noop"}},
+		Authorizer:        rule.RuleHandler{Handler: "allow"},
+		CredentialsIssuer: rule.RuleHandler{Handler: "noop"},
+		Upstream:          rule.Upstream{URL: backend.URL, StripPath: "/strip-path/", PreserveHost: true},
 	}
 
 	//acceptRuleStripHost := rule.Rule{MatchesMethods: []string{"GET"}, MatchesURLCompiled: mustCompileRegex(t, proxy.URL+"/users/<[0-9]+>"), Mode: "pass_through_accept", Upstream: rule.Upstream{URLParsed: u, StripPath: "/users/", PreserveHost: true}}

diff --git a/proxy/request_handler.go b/proxy/request_handler.go
@@ -102,10 +102,10 @@ func (d *RequestHandler) HandleRequest(r *http.Request, rl *rule.Rule) error {
 			case ErrAuthenticatorNotResponsible.Error():
 				// The authentication handler is not responsible for handling this request, skip to the next handler
 				break
-			case ErrAuthenticatorBypassed.Error():
-				// The authentication handler says that no further authentication/authorization is required, and the request should
-				// be forwarded to its final destination.
-				return nil
+			//case ErrAuthenticatorBypassed.Error():
+			// The authentication handler says that no further authentication/authorization is required, and the request should
+			// be forwarded to its final destination.
+			//return nil
 			default:
 				d.Logger.WithError(err).
 					WithField("granted", false).

diff --git a/proxy/request_handler_test.go b/proxy/request_handler_test.go
@@ -73,7 +73,7 @@ func TestRequestHandler(t *testing.T) {
 		{
 			expectErr: true,
 			r:         newTestRequest(t, "http://localhost"),
-			j:         NewRequestHandler(nil, []Authenticator{NewAuthenticatorNoOp()}, []Authorizer{}, []CredentialsIssuer{}),
+			j:         NewRequestHandler(nil, []Authenticator{NewAuthenticatorNoOp()}, []Authorizer{NewAuthorizerAllow()}, []CredentialsIssuer{NewCredentialsIssuerNoOp()}),
 			rule: rule.Rule{
 				Authenticators:    []rule.RuleHandler{},
 				Authorizer:        rule.RuleHandler{},
@@ -83,11 +83,11 @@ func TestRequestHandler(t *testing.T) {
 		{
 			expectErr: false,
 			r:         newTestRequest(t, "http://localhost"),
-			j:         NewRequestHandler(nil, []Authenticator{NewAuthenticatorNoOp()}, []Authorizer{}, []CredentialsIssuer{}),
+			j:         NewRequestHandler(nil, []Authenticator{NewAuthenticatorNoOp()}, []Authorizer{NewAuthorizerAllow()}, []CredentialsIssuer{NewCredentialsIssuerNoOp()}),
 			rule: rule.Rule{
 				Authenticators:    []rule.RuleHandler{{Handler: NewAuthenticatorNoOp().GetID()}},
-				Authorizer:        rule.RuleHandler{},
-				CredentialsIssuer: rule.RuleHandler{},
+				Authorizer:        rule.RuleHandler{Handler: NewAuthorizerAllow().GetID()},
+				CredentialsIssuer: rule.RuleHandler{Handler: NewCredentialsIssuerNoOp().GetID()},
 			},
 		},
 		{