From c578c8651d44f1e447ff1d0238a766ec97462892 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Michaud?= Date: Tue, 18 Feb 2020 23:15:09 +0100 Subject: [PATCH] [MISC] Add unit tests to authorization module and trace logs. (#638) This aims to help debug #637. --- internal/authorization/authorizer.go | 108 +++--------------- internal/authorization/authorizer_test.go | 5 - internal/authorization/domain_matcher.go | 13 +++ internal/authorization/domain_matcher_test.go | 25 ++++ internal/authorization/ip_matcher.go | 34 ++++++ internal/authorization/ip_matcher_test.go | 23 ++++ internal/authorization/path_matcher.go | 24 ++++ internal/authorization/path_matcher_test.go | 24 ++++ internal/authorization/subject_matcher.go | 29 +++++ 9 files changed, 185 insertions(+), 100 deletions(-) create mode 100644 internal/authorization/domain_matcher.go create mode 100644 internal/authorization/domain_matcher_test.go create mode 100644 internal/authorization/ip_matcher.go create mode 100644 internal/authorization/ip_matcher_test.go create mode 100644 internal/authorization/path_matcher.go create mode 100644 internal/authorization/path_matcher_test.go create mode 100644 internal/authorization/subject_matcher.go diff --git a/internal/authorization/authorizer.go b/internal/authorization/authorizer.go index 072d6b67..e6bbc5a2 100644 --- a/internal/authorization/authorizer.go +++ b/internal/authorization/authorizer.go @@ -1,12 +1,13 @@ package authorization import ( + "fmt" "net" "net/url" - "regexp" "strings" "github.com/authelia/authelia/internal/configuration/schema" + "github.com/authelia/authelia/internal/logging" ) const userPrefix = "user:" @@ -31,110 +32,22 @@ type Subject struct { IP net.IP } +func (s Subject) String() string { + return fmt.Sprintf("username=%s groups=%s ip=%s", s.Username, strings.Join(s.Groups, ","), s.IP.String()) +} + // Object object to check access control for type Object struct { Domain string Path string } -func isDomainMatching(domain string, domainRule string) bool { - if domain == domainRule { // if domain matches exactly - return true - } else if strings.HasPrefix(domainRule, "*") && strings.HasSuffix(domain, domainRule[1:]) { - // If domain pattern starts with *, it's a multi domain pattern. - return true - } - return false -} - -func isPathMatching(path string, pathRegexps []string) bool { - // If there is no regexp patterns, it means that we match any path. - if len(pathRegexps) == 0 { - return true - } - - for _, pathRegexp := range pathRegexps { - match, err := regexp.MatchString(pathRegexp, path) - if err != nil { - // TODO(c.michaud): make sure this is safe in advance to - // avoid checking this case here. - continue - } - - if match { - return true - } - } - return false -} - -func isSubjectMatching(subject Subject, subjectRule string) bool { - // If no subject is provided in the rule, we match any user. - if subjectRule == "" { - return true - } - - if strings.HasPrefix(subjectRule, userPrefix) { - user := strings.Trim(subjectRule[len(userPrefix):], " ") - if user == subject.Username { - return true - } - } - - if strings.HasPrefix(subjectRule, groupPrefix) { - group := strings.Trim(subjectRule[len(groupPrefix):], " ") - if isStringInSlice(group, subject.Groups) { - return true - } - } - return false -} - -// isIPMatching check whether user's IP is in one of the network ranges. -func isIPMatching(ip net.IP, networks []string) bool { - // If no network is provided in the rule, we match any network - if len(networks) == 0 { - return true - } - - for _, network := range networks { - if !strings.Contains(network, "/") { - if ip.String() == network { - return true - } - continue - } - _, ipNet, err := net.ParseCIDR(network) - if err != nil { - // TODO(c.michaud): make sure the rule is valid at startup to - // to such a case here. - continue - } - - if ipNet.Contains(ip) { - return true - } - } - return false -} - -func isStringInSlice(a string, list []string) bool { - for _, b := range list { - if b == a { - return true - } - } - return false -} - // selectMatchingSubjectRules take a set of rules and select only the rules matching the subject constraints. func selectMatchingSubjectRules(rules []schema.ACLRule, subject Subject) []schema.ACLRule { selectedRules := []schema.ACLRule{} for _, rule := range rules { - if isSubjectMatching(subject, rule.Subject) && - isIPMatching(subject.IP, rule.Networks) { - + if isSubjectMatching(subject, rule.Subject) && isIPMatching(subject.IP, rule.Networks) { selectedRules = append(selectedRules, rule) } } @@ -148,7 +61,6 @@ func selectMatchingObjectRules(rules []schema.ACLRule, object Object) []schema.A for _, rule := range rules { if isDomainMatching(object.Domain, rule.Domain) && isPathMatching(object.Path, rule.Resources) { - selectedRules = append(selectedRules, rule) } } @@ -177,6 +89,9 @@ func PolicyToLevel(policy string) Level { // GetRequiredLevel retrieve the required level of authorization to access the object. func (p *Authorizer) GetRequiredLevel(subject Subject, requestURL url.URL) Level { + logging.Logger().Tracef("Check authorization of subject %s and url %s.", + subject.String(), requestURL.String()) + matchingRules := selectMatchingRules(p.configuration.Rules, subject, Object{ Domain: requestURL.Hostname(), Path: requestURL.Path, @@ -185,5 +100,8 @@ func (p *Authorizer) GetRequiredLevel(subject Subject, requestURL url.URL) Level if len(matchingRules) > 0 { return PolicyToLevel(matchingRules[0].Policy) } + logging.Logger().Tracef("No matching rule for subject %s and url %s... Applying default policy.", + subject.String(), requestURL.String()) + return PolicyToLevel(p.configuration.DefaultPolicy) } diff --git a/internal/authorization/authorizer_test.go b/internal/authorization/authorizer_test.go index e373c12f..3e935421 100644 --- a/internal/authorization/authorizer_test.go +++ b/internal/authorization/authorizer_test.go @@ -65,11 +65,6 @@ func (b *AuthorizerTesterBuilder) Build() *AuthorizerTester { return NewAuthorizerTester(b.config) } -type Request struct { - subject Subject - object Object -} - var AnonymousUser = Subject{ Username: "", Groups: []string{}, diff --git a/internal/authorization/domain_matcher.go b/internal/authorization/domain_matcher.go new file mode 100644 index 00000000..80d98d0f --- /dev/null +++ b/internal/authorization/domain_matcher.go @@ -0,0 +1,13 @@ +package authorization + +import "strings" + +func isDomainMatching(domain string, domainRule string) bool { + if domain == domainRule { // if domain matches exactly + return true + } else if strings.HasPrefix(domainRule, "*.") && strings.HasSuffix(domain, domainRule[1:]) { + // If domain pattern starts with *, it's a multi domain pattern. + return true + } + return false +} diff --git a/internal/authorization/domain_matcher_test.go b/internal/authorization/domain_matcher_test.go new file mode 100644 index 00000000..7ba3b1cc --- /dev/null +++ b/internal/authorization/domain_matcher_test.go @@ -0,0 +1,25 @@ +package authorization + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestDomainMatcher(t *testing.T) { + assert.True(t, isDomainMatching("example.com", "example.com")) + + assert.False(t, isDomainMatching("example.com", "*.example.com")) + assert.True(t, isDomainMatching("abc.example.com", "*.example.com")) + assert.True(t, isDomainMatching("abc.def.example.com", "*.example.com")) + + // Character * must be followed by . to be valid. + assert.False(t, isDomainMatching("example.com", "*example.com")) + + assert.False(t, isDomainMatching("example.com", "*.example.com")) + assert.False(t, isDomainMatching("example.com", "*.exampl.com")) + + assert.False(t, isDomainMatching("example.com", "*.other.net")) + assert.False(t, isDomainMatching("example.com", "*other.net")) + assert.False(t, isDomainMatching("example.com", "other.net")) +} diff --git a/internal/authorization/ip_matcher.go b/internal/authorization/ip_matcher.go new file mode 100644 index 00000000..c5814a53 --- /dev/null +++ b/internal/authorization/ip_matcher.go @@ -0,0 +1,34 @@ +package authorization + +import ( + "net" + "strings" +) + +// isIPMatching check whether user's IP is in one of the network ranges. +func isIPMatching(ip net.IP, networks []string) bool { + // If no network is provided in the rule, we match any network + if len(networks) == 0 { + return true + } + + for _, network := range networks { + if !strings.Contains(network, "/") { + if ip.String() == network { + return true + } + continue + } + _, ipNet, err := net.ParseCIDR(network) + if err != nil { + // TODO(c.michaud): make sure the rule is valid at startup to + // to such a case here. + continue + } + + if ipNet.Contains(ip) { + return true + } + } + return false +} diff --git a/internal/authorization/ip_matcher_test.go b/internal/authorization/ip_matcher_test.go new file mode 100644 index 00000000..747bdc66 --- /dev/null +++ b/internal/authorization/ip_matcher_test.go @@ -0,0 +1,23 @@ +package authorization + +import ( + "net" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestIPMatcher(t *testing.T) { + // Default policy is 'allow all ips' if no IP is defined + assert.True(t, isIPMatching(net.ParseIP("127.0.0.1"), []string{})) + + assert.True(t, isIPMatching(net.ParseIP("127.0.0.1"), []string{"127.0.0.1"})) + assert.False(t, isIPMatching(net.ParseIP("127.1"), []string{"127.0.0.1"})) + assert.False(t, isIPMatching(net.ParseIP("not-an-ip"), []string{"127.0.0.1"})) + + assert.False(t, isIPMatching(net.ParseIP("127.0.0.1"), []string{"10.0.0.1"})) + assert.False(t, isIPMatching(net.ParseIP("127.0.0.1"), []string{"10.0.0.0/8"})) + + assert.True(t, isIPMatching(net.ParseIP("10.230.5.1"), []string{"10.0.0.0/8"})) + assert.True(t, isIPMatching(net.ParseIP("10.230.5.1"), []string{"192.168.0.0/24", "10.0.0.0/8"})) +} diff --git a/internal/authorization/path_matcher.go b/internal/authorization/path_matcher.go new file mode 100644 index 00000000..b2b3b11a --- /dev/null +++ b/internal/authorization/path_matcher.go @@ -0,0 +1,24 @@ +package authorization + +import "regexp" + +func isPathMatching(path string, pathRegexps []string) bool { + // If there is no regexp patterns, it means that we match any path. + if len(pathRegexps) == 0 { + return true + } + + for _, pathRegexp := range pathRegexps { + match, err := regexp.MatchString(pathRegexp, path) + if err != nil { + // TODO(c.michaud): make sure this is safe in advance to + // avoid checking this case here. + continue + } + + if match { + return true + } + } + return false +} diff --git a/internal/authorization/path_matcher_test.go b/internal/authorization/path_matcher_test.go new file mode 100644 index 00000000..bfcd8a54 --- /dev/null +++ b/internal/authorization/path_matcher_test.go @@ -0,0 +1,24 @@ +package authorization + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestPathMatcher(t *testing.T) { + // Matching any path if no regexp is provided + assert.True(t, isPathMatching("/", []string{})) + + assert.False(t, isPathMatching("/", []string{"^/api"})) + assert.True(t, isPathMatching("/api/test", []string{"^/api"})) + assert.False(t, isPathMatching("/api/test", []string{"^/api$"})) + assert.True(t, isPathMatching("/api", []string{"^/api$"})) + assert.True(t, isPathMatching("/api/test", []string{"^/api/?.*"})) + assert.True(t, isPathMatching("/apitest", []string{"^/api/?.*"})) + assert.True(t, isPathMatching("/api/test", []string{"^/api/.*"})) + assert.True(t, isPathMatching("/api/", []string{"^/api/.*"})) + assert.False(t, isPathMatching("/api", []string{"^/api/.*"})) + + assert.False(t, isPathMatching("/api", []string{"xyz", "^/api/.*"})) +} diff --git a/internal/authorization/subject_matcher.go b/internal/authorization/subject_matcher.go new file mode 100644 index 00000000..5161dc88 --- /dev/null +++ b/internal/authorization/subject_matcher.go @@ -0,0 +1,29 @@ +package authorization + +import ( + "strings" + + "github.com/authelia/authelia/internal/utils" +) + +func isSubjectMatching(subject Subject, subjectRule string) bool { + // If no subject is provided in the rule, we match any user. + if subjectRule == "" { + return true + } + + if strings.HasPrefix(subjectRule, userPrefix) { + user := strings.Trim(subjectRule[len(userPrefix):], " ") + if user == subject.Username { + return true + } + } + + if strings.HasPrefix(subjectRule, groupPrefix) { + group := strings.Trim(subjectRule[len(groupPrefix):], " ") + if utils.IsStringInSlice(group, subject.Groups) { + return true + } + } + return false +}