Rewrite ACL logic

in order to

- allow explicity deny for anon access
- allow multiple rule-sets to apply to the same request

Signed-off-by: Knut Ahlers <knut@ahlers.me>

acl.go

@@ -9,34 +9,145 @@ import (
 	"github.com/Luzifer/go_helpers/v2/str"
 )
 
-const groupAnonymous = "@_anonymous"
+const (
+	groupAnonymous     = "@_anonymous"
+	groupAuthenticated = "@_authenticated"
+)
 
-type aclRule struct {
+type (
-	Field       string  `yaml:"field"`
+	acl struct {
-	Invert      bool    `yaml:"invert"`
+		RuleSets []aclRuleSet `yaml:"rule_sets"`
-	IsPresent   *bool   `yaml:"present"`
+	}
-	MatchRegex  *string `yaml:"regexp"`
+
-	MatchString *string `yaml:"equals"`
+	aclRule struct {
+		Field       string  `yaml:"field"`
+		Invert      bool    `yaml:"invert"`
+		IsPresent   *bool   `yaml:"present"`
+		MatchRegex  *string `yaml:"regexp"`
+		MatchString *string `yaml:"equals"`
+	}
+
+	aclAccessResult uint
+
+	aclRuleSet struct {
+		Rules []aclRule `yaml:"rules"`
+
+		Allow []string `yaml:"allow"`
+		Deny  []string `yaml:"deny"`
+	}
+)
+
+const (
+	accessDunno aclAccessResult = iota
+	accessAllow
+	accessDeny
+)
+
+// --- ACL
+
+func (a acl) HasAccess(user string, groups []string, r *http.Request) bool {
+	var (
+		collectionAllow = map[string]bool{}
+		collectionDeny  = map[string]bool{}
+	)
+
+	for _, rs := range a.RuleSets {
+		if !rs.AppliesToRequest(r) {
+			continue
+		}
+
+		// Collect the allows from all matching rulesets
+		for _, a := range rs.Allow {
+			collectionAllow[a] = true
+		}
+
+		// Collect the denies from all matching rulesets
+		for _, d := range rs.Deny {
+			collectionDeny[d] = true
+		}
+	}
+
+	// Form lists from the collections
+	var allowed, denied []string
+
+	for k := range collectionAllow {
+		allowed = append(allowed, k)
+	}
+	for k := range collectionDeny {
+		denied = append(denied, k)
+	}
+
+	return a.checkAccess(user, groups, allowed, denied)
 }
 
-func (a aclRule) Validate() error {
+func (a acl) Validate() error {
-	if a.Field == "" {
+	for i, r := range a.RuleSets {
-		return fmt.Errorf("Field is not set")
+		if err := r.Validate(); err != nil {
-	}
+			return fmt.Errorf("RuleSet on position %d is invalid: %s", i+1, err)
-
-	if a.IsPresent == nil && a.MatchRegex == nil && a.MatchString == nil {
-		return fmt.Errorf("No matcher (present, regexp, equals) is set")
-	}
-
-	if a.MatchRegex != nil {
-		if _, err := regexp.Compile(*a.MatchRegex); err != nil {
-			return fmt.Errorf("Regexp is invalid: %s", err)
 		}
 	}
 
 	return nil
 }
 
+func (a acl) checkAccess(user string, groups, allowed, denied []string) bool {
+	if !str.StringInSlice(user, []string{"", "\x00"}) {
+		// The user is set to a non-anon user, we add the pseudo-group
+		// authenticated to the groups list the user has
+		groups = append(groups, groupAuthenticated)
+	} else {
+		// The user did match anon, therefore we set the pseudo-group
+		// for anonymous to be used in group matching
+		groups = []string{groupAnonymous}
+	}
+
+	// Quoting the documentation here:
+	// "There is a simple logic: Users before groups, denies before allows."
+
+	// Lets check the user
+	if str.StringInSlice(user, denied) {
+		// Explicit deny on the user, they're out!
+		return false
+	}
+
+	if str.StringInSlice(user, allowed) {
+		// Explicit allow on the user, they're in!
+		return true
+	}
+
+	// The user yielded no result, lets check the groups
+	for _, group := range groups {
+		if str.StringInSlice(a.fixGroupName(group), denied) {
+			// The group is denied access
+			return false
+		}
+
+		if str.StringInSlice(a.fixGroupName(group), allowed) {
+			// The group is allowed access
+			return true
+		}
+	}
+
+	// We found no match for the user and/or group. Last chance is
+	// no ruleset denied anonymous access and at least one ruleset
+	// enabled anonymous access
+	if !str.StringInSlice(groupAnonymous, denied) && str.StringInSlice(groupAnonymous, allowed) {
+		return true
+	}
+
+	// We found neither a user nor a group with access or deny config
+	// so we fall back to the default: No access.
+	return false
+}
+
+func (acl) fixGroupName(group string) string {
+	return "@" + strings.TrimLeft(group, "@")
+}
+
+// --- ACL Rule
+
+// AppliesToFields checks whether the given rule conditions matches
+// the given fields
 func (a aclRule) AppliesToFields(fields map[string]string) bool {
 	var field, value string
 
@@ -91,19 +202,50 @@ func (a aclRule) AppliesToFields(fields map[string]string) bool {
 	return true
 }
 
-type aclAccessResult uint
+func (a aclRule) Validate() error {
+	if a.Field == "" {
+		return fmt.Errorf("field is not set")
+	}
 
-const (
+	if a.IsPresent == nil && a.MatchRegex == nil && a.MatchString == nil {
-	accessDunno aclAccessResult = iota
+		return fmt.Errorf("no matcher (present, regexp, equals) is set")
-	accessAllow
+	}
-	accessDeny
-)
 
-type aclRuleSet struct {
+	if a.MatchRegex != nil {
-	Rules []aclRule `yaml:"rules"`
+		if _, err := regexp.Compile(*a.MatchRegex); err != nil {
+			return fmt.Errorf("regexp is invalid: %s", err)
+		}
+	}
 
-	Allow []string `yaml:"allow"`
+	return nil
-	Deny  []string `yaml:"deny"`
+}
+
+// --- ACL Rule Set
+
+// AppliesToRequest checks whether every rule in the aclRuleSet
+// matches the http.Request. If not this rule-set must not be applied
+// to the given request
+func (a aclRuleSet) AppliesToRequest(r *http.Request) bool {
+	fields := a.buildFieldSet(r)
+
+	for _, rule := range a.Rules {
+		if !rule.AppliesToFields(fields) {
+			// At least one rule does not match the request
+			return false
+		}
+	}
+
+	return true
+}
+
+func (a aclRuleSet) Validate() error {
+	for i, r := range a.Rules {
+		if err := r.Validate(); err != nil {
+			return fmt.Errorf("rule on position %d is invalid: %s", i+1, err)
+		}
+	}
+
+	return nil
 }
 
 func (a aclRuleSet) buildFieldSet(r *http.Request) map[string]string {
@@ -115,86 +257,3 @@ func (a aclRuleSet) buildFieldSet(r *http.Request) map[string]string {
 
 	return result
 }
-
-func (a aclRuleSet) HasAccess(user string, groups []string, r *http.Request) aclAccessResult {
-	fields := a.buildFieldSet(r)
-
-	for _, rule := range a.Rules {
-		if !rule.AppliesToFields(fields) {
-			// At least one rule does not match the request
-			return accessDunno
-		}
-	}
-
-	// All rules do apply to this request, we can judge
-
-	if str.StringInSlice(user, a.Deny) {
-		// Explicit deny, final result
-		return accessDeny
-	}
-
-	if str.StringInSlice(user, a.Allow) {
-		// Explicit allow, final result
-		return accessAllow
-	}
-
-	for _, group := range groups {
-		if str.StringInSlice("@"+group, a.Deny) {
-			// Deny through group, final result
-			return accessDeny
-		}
-
-		if str.StringInSlice("@"+group, a.Allow) {
-			// Allow through group, final result
-			return accessAllow
-		}
-	}
-
-	// Allow special group @_authenticated if any non-anonymous user is set
-	if str.StringInSlice("@_authenticated", a.Allow) && !str.StringInSlice(user, []string{"", "\x00"}) {
-		return accessAllow
-	}
-
-	if str.StringInSlice(groupAnonymous, a.Allow) {
-		return accessAllow
-	}
-
-	// Neither user nor group are handled
-	return accessDunno
-}
-
-func (a aclRuleSet) Validate() error {
-	for i, r := range a.Rules {
-		if err := r.Validate(); err != nil {
-			return fmt.Errorf("Rule on position %d is invalid: %s", i+1, err)
-		}
-	}
-
-	return nil
-}
-
-type acl struct {
-	RuleSets []aclRuleSet `yaml:"rule_sets"`
-}
-
-func (a acl) Validate() error {
-	for i, r := range a.RuleSets {
-		if err := r.Validate(); err != nil {
-			return fmt.Errorf("RuleSet on position %d is invalid: %s", i+1, err)
-		}
-	}
-
-	return nil
-}
-
-func (a acl) HasAccess(user string, groups []string, r *http.Request) bool {
-	result := accessDunno
-
-	for _, rs := range a.RuleSets {
-		if intermediateResult := rs.HasAccess(user, groups, r); intermediateResult > result {
-			result = intermediateResult
-		}
-	}
-
-	return result == accessAllow
-}

acl_test.go

@@ -3,11 +3,14 @@ package main
 import (
 	"net/http"
 	"testing"
+
+	"github.com/stretchr/testify/assert"
 )
 
 var (
 	aclTestUser   = "test"
 	aclTestGroups = []string{"group_a", "group_b"}
+	ptrBoolTrue   = func(v bool) *bool { return &v }(true)
 )
 
 func aclTestRequest(headers map[string]string) *http.Request {
@@ -49,18 +52,14 @@ func TestRuleSetMatcher(t *testing.T) {
 		"field_c": "expected",
 	}
 
-	if r.HasAccess(aclTestUser, aclTestGroups, aclTestRequest(fields)) != accessAllow {
+	assert.True(t, r.AppliesToRequest(aclTestRequest(fields)))
-		t.Error("Access was denied")
-	}
 
 	delete(fields, "field_c")
-	if r.HasAccess(aclTestUser, aclTestGroups, aclTestRequest(fields)) != accessDunno {
+	assert.False(t, r.AppliesToRequest(aclTestRequest(fields)))
-		t.Error("Access was not unknown")
-	}
 }
 
 func TestGroupAuthenticated(t *testing.T) {
-	r := aclRuleSet{
+	a := acl{RuleSets: []aclRuleSet{{
 		Rules: []aclRule{
 			{
 				Field:       "field_a",
@@ -68,31 +67,21 @@ func TestGroupAuthenticated(t *testing.T) {
 			},
 		},
 		Allow: []string{"@_authenticated"},
-	}
+	}}}
 	fields := map[string]string{
 		"field_a": "expected",
 	}
 
-	if r.HasAccess(aclTestUser, aclTestGroups, aclTestRequest(fields)) != accessAllow {
+	assert.True(t, a.HasAccess(aclTestUser, aclTestGroups, aclTestRequest(fields)))
-		t.Error("Access was denied")
+	assert.False(t, a.HasAccess("\x00", nil, aclTestRequest(fields)), "access to anon user")
-	}
+	assert.False(t, a.HasAccess("", nil, aclTestRequest(fields)), "access to empty user")
 
-	if r.HasAccess("\x00", nil, aclTestRequest(fields)) == accessAllow {
+	a.RuleSets[0].Allow = []string{"testgroup"}
-		t.Error("Access was allowed to unauth-user")
+	assert.False(t, a.HasAccess(aclTestUser, aclTestGroups, aclTestRequest(fields)))
-	}
-
-	if r.HasAccess("", nil, aclTestRequest(fields)) == accessAllow {
-		t.Error("Access was allowed to empty user")
-	}
-
-	r.Allow = []string{"testgroup"}
-	if r.HasAccess(aclTestUser, aclTestGroups, aclTestRequest(fields)) == accessAllow {
-		t.Error("Access was allowed")
-	}
 }
 
 func TestAnonymousAccess(t *testing.T) {
-	r := aclRuleSet{
+	a := acl{RuleSets: []aclRuleSet{{
 		Rules: []aclRule{
 			{
 				Field:       "field_a",
@@ -100,22 +89,40 @@ func TestAnonymousAccess(t *testing.T) {
 			},
 		},
 		Allow: []string{groupAnonymous},
-	}
+	}}}
 	fields := map[string]string{
 		"field_a": "expected",
 	}
 
-	if r.HasAccess(aclTestUser, aclTestGroups, aclTestRequest(fields)) != accessAllow {
+	assert.True(t, a.HasAccess(aclTestUser, aclTestGroups, aclTestRequest(fields)))
-		t.Error("Access was denied")
+	assert.True(t, a.HasAccess("", nil, aclTestRequest(fields)), "access to empty user")
+	assert.True(t, a.HasAccess("\x00", nil, aclTestRequest(fields)), "access to anon user")
+}
+
+func TestAnonymousAccessExplicitDeny(t *testing.T) {
+	a := acl{
+		RuleSets: []aclRuleSet{
+			{
+				Rules: []aclRule{{Field: "field_a", IsPresent: ptrBoolTrue}},
+				Allow: []string{groupAnonymous},
+			},
+			{
+				Rules: []aclRule{{Field: "field_b", IsPresent: ptrBoolTrue}},
+				Allow: []string{"somerandomuser"},
+				Deny:  []string{groupAnonymous},
+			},
+		},
 	}
 
-	if r.HasAccess("", nil, aclTestRequest(fields)) != accessAllow {
+	assert.True(t,
-		t.Error("Access without user was denied")
+		a.HasAccess(aclTestUser, aclTestGroups, aclTestRequest(map[string]string{"field_a": ""})),
-	}
+		"anon access with only allowed field should be possible")
-
+	assert.False(t,
-	if r.HasAccess("\x00", nil, aclTestRequest(fields)) != accessAllow {
+		a.HasAccess(aclTestUser, aclTestGroups, aclTestRequest(map[string]string{"field_b": ""})),
-		t.Error("Access with special unauth-user was denied")
+		"anon access with only denied field should not be possible")
-	}
+	assert.False(t,
+		a.HasAccess(aclTestUser, aclTestGroups, aclTestRequest(map[string]string{"field_a": "", "field_b": ""})),
+		"anon access with one allowed and one denied field should not be possible")
 }
 
 func TestInvertedRegexMatcher(t *testing.T) {

go.mod

@@ -15,6 +15,7 @@ require (
 	github.com/pkg/errors v0.9.1
 	github.com/pquerna/otp v1.4.0
 	github.com/sirupsen/logrus v1.9.3
+	github.com/stretchr/testify v1.8.4
 	golang.org/x/crypto v0.11.0
 	golang.org/x/oauth2 v0.10.0
 	google.golang.org/api v0.134.0
@@ -26,6 +27,7 @@ require (
 	cloud.google.com/go/compute v1.23.0 // indirect
 	cloud.google.com/go/compute/metadata v0.2.3 // indirect
 	github.com/boombuler/barcode v1.0.1 // indirect
+	github.com/davecgh/go-spew v1.1.1 // indirect
 	github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect
 	github.com/golang/protobuf v1.5.3 // indirect
 	github.com/google/s2a-go v0.1.4 // indirect
@@ -33,6 +35,7 @@ require (
 	github.com/googleapis/enterprise-certificate-proxy v0.2.5 // indirect
 	github.com/googleapis/gax-go/v2 v2.12.0 // indirect
 	github.com/gorilla/securecookie v1.1.1 // indirect
+	github.com/pmezard/go-difflib v1.0.0 // indirect
 	github.com/pquerna/cachecontrol v0.2.0 // indirect
 	github.com/spf13/pflag v1.0.5 // indirect
 	go.opencensus.io v0.24.0 // indirect
@@ -46,4 +49,5 @@ require (
 	gopkg.in/asn1-ber.v1 v1.0.0-20181015200546-f715ec2f112d // indirect
 	gopkg.in/square/go-jose.v2 v2.6.0 // indirect
 	gopkg.in/validator.v2 v2.0.1 // indirect
+	gopkg.in/yaml.v3 v3.0.1 // indirect
 )

go.sum

@@ -121,6 +121,7 @@ github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/
 github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU=
 github.com/stretchr/testify v1.8.1/go.mod h1:w2LPCIKwWwSfY2zedu0+kehJoqGctiVI29o6fzry7u4=
 github.com/stretchr/testify v1.8.4 h1:CcVxjf3Q8PM0mHUKJCdn+eZZtm5yQwehR5yeSVQQcUk=
+github.com/stretchr/testify v1.8.4/go.mod h1:sz/lmYIOXD/1dqDmKjjqLyZ2RngseejIcXlSw2iwfAo=
 github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY=
 go.opencensus.io v0.24.0 h1:y73uSU6J157QMP2kn2r30vwW1A2W2WFwSCGnAVxeaD0=
 go.opencensus.io v0.24.0/go.mod h1:vNK8G9p7aAivkbmorf4v+7Hgx+Zs0yY+0fOtgBfjQKo=