From ca85992ac6dabafd8410a8928c01ebb8edaf6d7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20Nu=C3=B1ez?= <10672208+mind-ar@users.noreply.github.com> Date: Sun, 4 Sep 2022 19:21:30 -0300 Subject: [PATCH] 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 --- internal/authorization/authorizer.go | 6 +++--- internal/authorization/authorizer_test.go | 2 +- internal/handlers/handler_verify.go | 9 ++++----- internal/handlers/handler_verify_test.go | 7 ++++--- internal/handlers/response.go | 2 +- 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,