fix(handlers): verify handler (#3956)

When an anonymous user tries to access a forbidden resource with no subject, we should response with 403.

Fixes #3084

5 files changed, 13 insertions, 13 deletions

diff --git a/internal/authorization/authorizer.go b/internal/authorization/authorizer.go
index 3b6e75f74..d08ec3a5b 100644
--- a/internal/authorization/authorizer.go
+++ b/internal/authorization/authorizer.go
@@ -54,7 +54,7 @@ func (p Authorizer) IsSecondFactorEnabled() bool {
 }
 
 // GetRequiredLevel retrieve the required level of authorization to access the object.
-func (p Authorizer) GetRequiredLevel(subject Subject, object Object) Level {
+func (p Authorizer) GetRequiredLevel(subject Subject, object Object) (bool, Level) {
 	logger := logging.Logger()
 
 	logger.Debugf("Check authorization of subject %s and object %s (method %s).",
@@ -64,7 +64,7 @@ func (p Authorizer) GetRequiredLevel(subject Subject, object Object) Level {
 		if rule.IsMatch(subject, object) {
 			logger.Tracef(traceFmtACLHitMiss, "HIT", rule.Position, subject.String(), object.String(), object.Method)
 
-			return rule.Policy
+			return len(rule.Subjects) > 0, rule.Policy
 		}
 
 		logger.Tracef(traceFmtACLHitMiss, "MISS", rule.Position, subject.String(), object.String(), object.Method)
@@ -73,7 +73,7 @@ func (p Authorizer) GetRequiredLevel(subject Subject, object Object) Level {
 	logger.Debugf("No matching rule for subject %s and url %s... Applying default policy.",
 		subject.String(), object.String())
 
-	return p.defaultPolicy
+	return false, p.defaultPolicy
 }
 
 // GetRuleMatchResults iterates through the rules and produces a list of RuleMatchResult provided a subject and object.
diff --git a/internal/authorization/authorizer_test.go b/internal/authorization/authorizer_test.go
index c4e71ceb8..264d940c2 100644
--- a/internal/authorization/authorizer_test.go
+++ b/internal/authorization/authorizer_test.go
@@ -36,7 +36,7 @@ func (s *AuthorizerTester) CheckAuthorizations(t *testing.T, subject Subject, re
 
 	object := NewObject(targetURL, method)
 
-	level := s.GetRequiredLevel(subject, object)
+	_, level := s.GetRequiredLevel(subject, object)
 
 	assert.Equal(t, expectedLevel, level)
 }
diff --git a/internal/handlers/handler_verify.go b/internal/handlers/handler_verify.go
index 266d82350..a78d699ca 100644
--- a/internal/handlers/handler_verify.go
+++ b/internal/handlers/handler_verify.go
@@ -56,7 +56,7 @@ func parseBasicAuth(header []byte, auth string) (username, password string, err
 // isTargetURLAuthorized check whether the given user is authorized to access the resource.
 func isTargetURLAuthorized(authorizer *authorization.Authorizer, targetURL url.URL,
 	username string, userGroups []string, clientIP net.IP, method []byte, authLevel authentication.Level) authorizationMatching {
-	level := authorizer.GetRequiredLevel(
+	hasSubject, level := authorizer.GetRequiredLevel(
 		authorization.Subject{
 			Username: username,
 			Groups:   userGroups,
@@ -67,13 +67,12 @@ func isTargetURLAuthorized(authorizer *authorization.Authorizer, targetURL url.U
 	switch {
 	case level == authorization.Bypass:
 		return Authorized
-	case level == authorization.Denied && username != "":
+	case level == authorization.Denied && (username != "" || !hasSubject):
 		// If the user is not anonymous, it means that we went through
 		// all the rules related to that user and knowing who he is we can
 		// deduce the access is forbidden
-		// For anonymous users though, we cannot be sure that she
-		// could not be granted the rights to access the resource. Consequently
-		// for anonymous users we send Unauthorized instead of Forbidden.
+		// For anonymous users though, we check that the matched rule has no subject
+		// if matched rule has not subject then this rule applies to all users including anonymous.
 		return Forbidden
 	case level == authorization.OneFactor && authLevel >= authentication.OneFactor,
 		level == authorization.TwoFactor && authLevel >= authentication.TwoFactor:
diff --git a/internal/handlers/handler_verify_test.go b/internal/handlers/handler_verify_test.go
index 0472b5669..6ae6e2932 100644
--- a/internal/handlers/handler_verify_test.go
+++ b/internal/handlers/handler_verify_test.go
@@ -140,7 +140,7 @@ func TestShouldCheckAuthorizationMatching(t *testing.T) {
 		{"two_factor", authentication.OneFactor, NotAuthorized},
 		{"two_factor", authentication.TwoFactor, Authorized},
 
-		{"deny", authentication.NotAuthenticated, NotAuthorized},
+		{"deny", authentication.NotAuthenticated, Forbidden},
 		{"deny", authentication.OneFactor, Forbidden},
 		{"deny", authentication.TwoFactor, Forbidden},
 	}
@@ -508,11 +508,12 @@ func (p Pair) String() string {
 
 func TestShouldVerifyAuthorizationsUsingSessionCookie(t *testing.T) {
 	testCases := []Pair{
-		{"https://test.example.com", "", nil, authentication.NotAuthenticated, 401},
+		// should apply default policy.
+		{"https://test.example.com", "", nil, authentication.NotAuthenticated, 403},
 		{"https://bypass.example.com", "", nil, authentication.NotAuthenticated, 200},
 		{"https://one-factor.example.com", "", nil, authentication.NotAuthenticated, 401},
 		{"https://two-factor.example.com", "", nil, authentication.NotAuthenticated, 401},
-		{"https://deny.example.com", "", nil, authentication.NotAuthenticated, 401},
+		{"https://deny.example.com", "", nil, authentication.NotAuthenticated, 403},
 
 		{"https://test.example.com", "john", []string{"john.doe@example.com"}, authentication.OneFactor, 403},
 		{"https://bypass.example.com", "john", []string{"john.doe@example.com"}, authentication.OneFactor, 200},
diff --git a/internal/handlers/response.go b/internal/handlers/response.go
index e1a9d970e..83f5de59d 100644
--- a/internal/handlers/response.go
+++ b/internal/handlers/response.go
@@ -87,7 +87,7 @@ func Handle1FAResponse(ctx *middlewares.AutheliaCtx, targetURI, requestMethod st
 		return
 	}
 
-	requiredLevel := ctx.Providers.Authorizer.GetRequiredLevel(
+	_, requiredLevel := ctx.Providers.Authorizer.GetRequiredLevel(
 		authorization.Subject{
 			Username: username,
 			Groups:   groups,