From 5c4edf2f4d4822126bea1a9014b7fed1c2a8b2bf Mon Sep 17 00:00:00 2001 From: Philipp Staiger Date: Thu, 25 Jun 2020 10:22:42 +0200 Subject: [PATCH] [FEATURE] Support for subject combinations in ACLs (#1142) --- docs/configuration/access-control.md | 8 ++++- internal/authorization/authorizer.go | 6 ++-- internal/authorization/authorizer_test.go | 23 +++++++++--- internal/authorization/subject_matcher.go | 36 ++++++++++--------- .../configuration/schema/access_control.go | 20 ++++++----- internal/mocks/mock_authelia_ctx.go | 4 +-- 6 files changed, 63 insertions(+), 34 deletions(-) diff --git a/docs/configuration/access-control.md b/docs/configuration/access-control.md index 1de322eb..6bdd2f08 100644 --- a/docs/configuration/access-control.md +++ b/docs/configuration/access-control.md @@ -70,6 +70,11 @@ For a user with unique identifier `john`, the subject should be `user:john` and uniquely identified by `developers`, the subject should be `group:developers`. Similar to resources and domains you can define multiple subjects in a single rule. +If you want a combination of subjects to be matched at once, you can specify a list of subjects like +`- ["group:developers", "group:admins"]`. Make sure to preceed it by a list key `-`. +In summary, the first level of subjects are evaluated using a logical `OR`, whereas the second level +by a logical `AND`. + ## Networks A list of network ranges can be specified in a rule in order to apply different policies when @@ -128,6 +133,7 @@ access_control: - domain: dev.example.com resources: - "^/users/john/.*$" - subject: "user:john" + subject: + - ["group:dev", "user:john"] policy: two_factor ``` diff --git a/internal/authorization/authorizer.go b/internal/authorization/authorizer.go index 9f354102..cc81683a 100644 --- a/internal/authorization/authorizer.go +++ b/internal/authorization/authorizer.go @@ -138,8 +138,10 @@ func (p *Authorizer) IsURLMatchingRuleWithGroupSubjects(requestURL url.URL) (has for _, rule := range p.configuration.Rules { if isDomainMatching(requestURL.Hostname(), rule.Domains) && isPathMatching(requestURL.Path, rule.Resources) { for _, subjectRule := range rule.Subjects { - if strings.HasPrefix(subjectRule, groupPrefix) { - return true + for _, subject := range subjectRule { + if strings.HasPrefix(subject, groupPrefix) { + return true + } } } } diff --git a/internal/authorization/authorizer_test.go b/internal/authorization/authorizer_test.go index a465c3fe..19d22606 100644 --- a/internal/authorization/authorizer_test.go +++ b/internal/authorization/authorizer_test.go @@ -166,7 +166,7 @@ func (s *AuthorizerSuite) TestShouldCheckRulePrecedence() { WithRule(schema.ACLRule{ Domains: []string{"protected.example.com"}, Policy: "bypass", - Subjects: []string{"user:john"}, + Subjects: [][]string{{"user:john"}}, }). WithRule(schema.ACLRule{ Domains: []string{"protected.example.com"}, @@ -189,7 +189,7 @@ func (s *AuthorizerSuite) TestShouldCheckUserMatching() { WithRule(schema.ACLRule{ Domains: []string{"protected.example.com"}, Policy: "bypass", - Subjects: []string{"user:john"}, + Subjects: [][]string{{"user:john"}}, }). Build() @@ -203,7 +203,7 @@ func (s *AuthorizerSuite) TestShouldCheckGroupMatching() { WithRule(schema.ACLRule{ Domains: []string{"protected.example.com"}, Policy: "bypass", - Subjects: []string{"group:admins"}, + Subjects: [][]string{{"group:admins"}}, }). Build() @@ -217,7 +217,7 @@ func (s *AuthorizerSuite) TestShouldCheckSubjectsMatching() { WithRule(schema.ACLRule{ Domains: []string{"protected.example.com"}, Policy: "bypass", - Subjects: []string{"group:admins", "user:bob"}, + Subjects: [][]string{{"group:admins"}, {"user:bob"}}, }). Build() @@ -226,6 +226,21 @@ func (s *AuthorizerSuite) TestShouldCheckSubjectsMatching() { tester.CheckAuthorizations(s.T(), AnonymousUser, "https://protected.example.com/", Denied) } +func (s *AuthorizerSuite) TestShouldCheckMultipleSubjectsMatching() { + tester := NewAuthorizerBuilder(). + WithDefaultPolicy("deny"). + WithRule(schema.ACLRule{ + Domains: []string{"protected.example.com"}, + Policy: "bypass", + Subjects: [][]string{{"group:admins", "user:bob"}, {"group:admins", "group:dev"}}, + }). + Build() + + tester.CheckAuthorizations(s.T(), John, "https://protected.example.com/", Bypass) + tester.CheckAuthorizations(s.T(), Bob, "https://protected.example.com/", Denied) + tester.CheckAuthorizations(s.T(), AnonymousUser, "https://protected.example.com/", Denied) +} + func (s *AuthorizerSuite) TestShouldCheckIPMatching() { tester := NewAuthorizerBuilder(). WithDefaultPolicy("deny"). diff --git a/internal/authorization/subject_matcher.go b/internal/authorization/subject_matcher.go index d47093b9..0e2d3b63 100644 --- a/internal/authorization/subject_matcher.go +++ b/internal/authorization/subject_matcher.go @@ -6,25 +6,29 @@ import ( "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 +func isSubjectMatching(subject Subject, subjectRule []string) bool { + for _, ruleSubject := range subjectRule { + // If no subject is provided in the rule, we match any user. + if ruleSubject == "" { + continue } - } - if strings.HasPrefix(subjectRule, groupPrefix) { - group := strings.Trim(subjectRule[len(groupPrefix):], " ") - if utils.IsStringInSlice(group, subject.Groups) { - return true + if strings.HasPrefix(ruleSubject, userPrefix) { + user := strings.Trim(ruleSubject[len(userPrefix):], " ") + if user == subject.Username { + continue + } } + + if strings.HasPrefix(ruleSubject, groupPrefix) { + group := strings.Trim(ruleSubject[len(groupPrefix):], " ") + if utils.IsStringInSlice(group, subject.Groups) { + continue + } + } + + return false } - return false + return true } diff --git a/internal/configuration/schema/access_control.go b/internal/configuration/schema/access_control.go index 6db5b8ce..09c91cc2 100644 --- a/internal/configuration/schema/access_control.go +++ b/internal/configuration/schema/access_control.go @@ -6,13 +6,13 @@ import ( "strings" ) -// ACLRule represent one ACL rule "weak" coerces a single value into string slice. +// ACLRule represents one ACL rule entry; "weak" coerces a single value into slice. type ACLRule struct { - Domains []string `mapstructure:"domain,weak"` - Policy string `mapstructure:"policy"` - Subjects []string `mapstructure:"subject,weak"` - Networks []string `mapstructure:"networks"` - Resources []string `mapstructure:"resources"` + Domains []string `mapstructure:"domain,weak"` + Policy string `mapstructure:"policy"` + Subjects [][]string `mapstructure:"subject,weak"` + Networks []string `mapstructure:"networks"` + Resources []string `mapstructure:"resources"` } // IsPolicyValid check if policy is valid. @@ -41,9 +41,11 @@ func (r *ACLRule) Validate(validator *StructValidator) { validator.Push(fmt.Errorf("A policy must either be 'deny', 'two_factor', 'one_factor' or 'bypass'")) } - for i, subject := range r.Subjects { - if !IsSubjectValid(subject) { - validator.Push(fmt.Errorf("Subject %d must start with 'user:' or 'group:'", i)) + for i, subjectRule := range r.Subjects { + for j, subject := range subjectRule { + if !IsSubjectValid(subject) { + validator.Push(fmt.Errorf("Subject %d-%d must start with 'user:' or 'group:'", i, j)) + } } } diff --git a/internal/mocks/mock_authelia_ctx.go b/internal/mocks/mock_authelia_ctx.go index 08cfb653..c3835211 100644 --- a/internal/mocks/mock_authelia_ctx.go +++ b/internal/mocks/mock_authelia_ctx.go @@ -85,11 +85,11 @@ func NewMockAutheliaCtx(t *testing.T) *MockAutheliaCtx { }, { Domains: []string{"admin.example.com"}, Policy: "two_factor", - Subjects: []string{"group:admin"}, + Subjects: [][]string{{"group:admin"}}, }, { Domains: []string{"grafana.example.com"}, Policy: "two_factor", - Subjects: []string{"group:grafana"}, + Subjects: [][]string{{"group:grafana"}}, }} providers := middlewares.Providers{}