fix: cleanup acl logic to match stable one

internal/service/access_controls_rules.go

@@ -9,6 +9,12 @@ import (
 	"github.com/tinyauthapp/tinyauth/internal/utils/logger"
 )
 
+// For LDAP and OAuth groups and IP allow/deny, we default to allow even with a deny policy.
+// This is because we can't force the user to use groups in LDAP and OAuth if they would like to use
+// a deny policy. As for IP checks, we can't reliably get the client IP (most of Tinyauth instances are
+// behind a Docker bridge network) so to make it easier for users to use a deny policy without
+// issues with IPs we allow by default.
+
 type RuleName string
 
 const (
@@ -25,7 +31,11 @@ type UserAllowedRule struct {
 }
 
 func (rule *UserAllowedRule) Evaluate(ctx *ACLContext) Effect {
-	if ctx.ACLs == nil || ctx.UserContext == nil {
+	if ctx.UserContext == nil {
+		return EffectDeny
+	}
+
+	if ctx.ACLs == nil {
 		return EffectAbstain
 	}
 
@@ -34,7 +44,7 @@ func (rule *UserAllowedRule) Evaluate(ctx *ACLContext) Effect {
 		match, err := utils.CheckFilter(ctx.ACLs.OAuth.Whitelist, ctx.UserContext.OAuth.Email)
 		if err != nil {
 			rule.Log.App.Warn().Err(err).Str("item", ctx.UserContext.OAuth.Email).Msg("Invalid entry in OAuth whitelist")
-			return EffectAbstain
+			return EffectDeny
 		}
 		if match {
 			rule.Log.App.Debug().Str("email", ctx.UserContext.OAuth.Email).Msg("User is in OAuth whitelist, allowing access")
@@ -48,7 +58,7 @@ func (rule *UserAllowedRule) Evaluate(ctx *ACLContext) Effect {
 		match, err := utils.CheckFilter(ctx.ACLs.Users.Block, ctx.UserContext.GetUsername())
 		if err != nil {
 			rule.Log.App.Warn().Err(err).Str("item", ctx.UserContext.GetUsername()).Msg("Invalid entry in users block list")
-			return EffectAbstain
+			return EffectDeny
 		}
 		if match {
 			rule.Log.App.Debug().Str("username", ctx.UserContext.GetUsername()).Msg("User is in users block list, denying access")
@@ -62,8 +72,11 @@ func (rule *UserAllowedRule) Evaluate(ctx *ACLContext) Effect {
 	match, err := utils.CheckFilter(ctx.ACLs.Users.Allow, ctx.UserContext.GetUsername())
 
 	if err != nil {
+		if err == utils.ErrFilterEmpty {
+			return EffectAbstain
+		}
 		rule.Log.App.Warn().Err(err).Str("item", ctx.UserContext.GetUsername()).Msg("Invalid entry in users allow list")
-		return EffectAbstain
+		return EffectDeny
 	}
 
 	if match {
@@ -80,13 +93,22 @@ type OAuthGroupRule struct {
 }
 
 func (rule *OAuthGroupRule) Evaluate(ctx *ACLContext) Effect {
-	if ctx.ACLs == nil || ctx.UserContext == nil {
+	if ctx.UserContext == nil {
-		return EffectAbstain
+		return EffectDeny
+	}
+
+	if ctx.ACLs == nil {
+		return EffectAllow
 	}
 
 	if !ctx.UserContext.IsOAuth() {
 		rule.Log.App.Debug().Msg("User is not an OAuth user, skipping OAuth group check")
-		return EffectAbstain
+		return EffectAllow
+	}
+
+	if len(ctx.ACLs.OAuth.Groups) == 0 {
+		rule.Log.App.Debug().Msg("No OAuth groups specified in ACLs, allowing access")
+		return EffectAllow
 	}
 
 	if _, ok := model.OverrideProviders[ctx.UserContext.OAuth.ID]; ok {
@@ -97,7 +119,8 @@ func (rule *OAuthGroupRule) Evaluate(ctx *ACLContext) Effect {
 	for _, group := range ctx.UserContext.OAuth.Groups {
 		match, err := utils.CheckFilter(ctx.ACLs.OAuth.Groups, strings.TrimSpace(group))
 		if err != nil {
-			return EffectAbstain
+			rule.Log.App.Warn().Err(err).Str("item", group).Msg("Invalid entry in OAuth groups ACL")
+			return EffectDeny
 		}
 		if match {
 			rule.Log.App.Trace().Str("group", group).Str("required", ctx.ACLs.OAuth.Groups).Msg("User group matched, allowing access")
@@ -114,19 +137,29 @@ type LDAPGroupRule struct {
 }
 
 func (rule *LDAPGroupRule) Evaluate(ctx *ACLContext) Effect {
-	if ctx == nil || ctx.UserContext == nil || ctx.ACLs == nil {
+	if ctx.UserContext == nil {
-		return EffectAbstain
+		return EffectDeny
+	}
+
+	if ctx.ACLs == nil {
+		return EffectAllow
 	}
 
 	if !ctx.UserContext.IsLDAP() {
 		rule.Log.App.Debug().Msg("User is not an LDAP user, skipping LDAP group check")
-		return EffectAbstain
+		return EffectAllow
+	}
+
+	if len(ctx.ACLs.LDAP.Groups) == 0 {
+		rule.Log.App.Debug().Msg("No LDAP groups specified in ACLs, allowing access")
+		return EffectAllow
 	}
 
 	for _, group := range ctx.UserContext.LDAP.Groups {
 		match, err := utils.CheckFilter(ctx.ACLs.LDAP.Groups, strings.TrimSpace(group))
 		if err != nil {
-			return EffectAbstain
+			rule.Log.App.Warn().Err(err).Str("item", group).Msg("Invalid entry in LDAP groups ACL")
+			return EffectDeny
 		}
 		if match {
 			rule.Log.App.Trace().Str("group", group).Str("required", ctx.ACLs.LDAP.Groups).Msg("User group matched, allowing access")

internal/service/access_controls_rules_test.go

@@ -21,6 +21,16 @@ func TestUserAllowedRule(t *testing.T) {
 		ctx      *ACLContext
 		expected Effect
 	}{
+		{
+			name: "denies when user context is nil",
+			ctx: &ACLContext{
+				ACLs: &model.App{
+					OAuth: model.AppOAuth{Whitelist: "alice"},
+				},
+				UserContext: nil,
+			},
+			expected: EffectDeny,
+		},
 		{
 			name: "abstains when ACLs are nil",
 			ctx: &ACLContext{
@@ -34,16 +44,6 @@ func TestUserAllowedRule(t *testing.T) {
 			},
 			expected: EffectAbstain,
 		},
-		{
-			name: "abstains when user context is nil",
-			ctx: &ACLContext{
-				ACLs: &model.App{
-					OAuth: model.AppOAuth{Whitelist: "alice"},
-				},
-				UserContext: nil,
-			},
-			expected: EffectAbstain,
-		},
 		{
 			name: "allows OAuth user when email matches whitelist",
 			ctx: &ACLContext{
@@ -78,7 +78,7 @@ func TestUserAllowedRule(t *testing.T) {
 			expected: EffectDeny,
 		},
 		{
-			name: "abstains for OAuth user when whitelist filter is invalid",
+			name: "denies for OAuth user when whitelist filter is invalid",
 			ctx: &ACLContext{
 				ACLs: &model.App{
 					OAuth: model.AppOAuth{Whitelist: "/[/"},
@@ -90,7 +90,7 @@ func TestUserAllowedRule(t *testing.T) {
 					},
 				},
 			},
-			expected: EffectAbstain,
+			expected: EffectDeny,
 		},
 		{
 			name: "denies local user when username matches block list",
@@ -123,7 +123,7 @@ func TestUserAllowedRule(t *testing.T) {
 			expected: EffectAllow,
 		},
 		{
-			name: "abstains when block list filter is invalid",
+			name: "denies when block list filter is invalid",
 			ctx: &ACLContext{
 				ACLs: &model.App{
 					Users: model.AppUsers{Block: "/[/"},
@@ -135,6 +135,21 @@ func TestUserAllowedRule(t *testing.T) {
 					},
 				},
 			},
+			expected: EffectDeny,
+		},
+		{
+			name: "abstains when allow list is empty",
+			ctx: &ACLContext{
+				ACLs: &model.App{
+					Users: model.AppUsers{Allow: ""},
+				},
+				UserContext: &model.UserContext{
+					Provider: model.ProviderLocal,
+					Local: &model.LocalContext{
+						BaseContext: model.BaseContext{Username: "alice"},
+					},
+				},
+			},
 			expected: EffectAbstain,
 		},
 		{
@@ -168,7 +183,7 @@ func TestUserAllowedRule(t *testing.T) {
 			expected: EffectDeny,
 		},
 		{
-			name: "abstains when allow list filter is invalid",
+			name: "denies when allow list filter is invalid",
 			ctx: &ACLContext{
 				ACLs: &model.App{
 					Users: model.AppUsers{Allow: "/[/"},
@@ -180,7 +195,7 @@ func TestUserAllowedRule(t *testing.T) {
 					},
 				},
 			},
-			expected: EffectAbstain,
+			expected: EffectDeny,
 		},
 	}
 
@@ -203,7 +218,17 @@ func TestOAuthGroupRule(t *testing.T) {
 		expected Effect
 	}{
 		{
-			name: "abstains when ACLs are nil",
+			name: "denies when user context is nil",
+			ctx: &ACLContext{
+				ACLs: &model.App{
+					OAuth: model.AppOAuth{Whitelist: "alice"},
+				},
+				UserContext: nil,
+			},
+			expected: EffectDeny,
+		},
+		{
+			name: "allows when ACLs are nil",
 			ctx: &ACLContext{
 				ACLs: nil,
 				UserContext: &model.UserContext{
@@ -213,20 +238,10 @@ func TestOAuthGroupRule(t *testing.T) {
 					},
 				},
 			},
-			expected: EffectAbstain,
+			expected: EffectAllow,
 		},
 		{
-			name: "abstains when user context is nil",
+			name: "allows when user is not OAuth",
-			ctx: &ACLContext{
-				ACLs: &model.App{
-					OAuth: model.AppOAuth{Whitelist: "alice"},
-				},
-				UserContext: nil,
-			},
-			expected: EffectAbstain,
-		},
-		{
-			name: "abstains when user is not OAuth",
 			ctx: &ACLContext{
 				ACLs: &model.App{
 					OAuth: model.AppOAuth{Groups: "admins"},
@@ -238,7 +253,22 @@ func TestOAuthGroupRule(t *testing.T) {
 					},
 				},
 			},
-			expected: EffectAbstain,
+			expected: EffectAllow,
+		},
+		{
+			name: "allows when group filter is empty",
+			ctx: &ACLContext{
+				ACLs: &model.App{
+					OAuth: model.AppOAuth{Groups: ""},
+				},
+				UserContext: &model.UserContext{
+					Provider: model.ProviderOAuth,
+					OAuth: &model.OAuthContext{
+						BaseContext: model.BaseContext{Username: "alice"},
+					},
+				},
+			},
+			expected: EffectAllow,
 		},
 		{
 			name: "allows when provider is an override provider regardless of groups",
@@ -305,7 +335,7 @@ func TestOAuthGroupRule(t *testing.T) {
 			expected: EffectDeny,
 		},
 		{
-			name: "abstains when groups filter is invalid",
+			name: "denies when groups filter is invalid",
 			ctx: &ACLContext{
 				ACLs: &model.App{
 					OAuth: model.AppOAuth{Groups: "/[/"},
@@ -318,7 +348,7 @@ func TestOAuthGroupRule(t *testing.T) {
 					},
 				},
 			},
-			expected: EffectAbstain,
+			expected: EffectDeny,
 		},
 	}
 
@@ -341,22 +371,30 @@ func TestLDAPGroupRule(t *testing.T) {
 		expected Effect
 	}{
 		{
-			name:     "abstains when context is nil",
+			name: "denies when user context is nil",
-			ctx:      nil,
-			expected: EffectAbstain,
-		},
-		{
-			name: "abstains when user context is nil",
 			ctx: &ACLContext{
 				ACLs: &model.App{
 					OAuth: model.AppOAuth{Whitelist: "alice"},
 				},
 				UserContext: nil,
 			},
-			expected: EffectAbstain,
+			expected: EffectDeny,
 		},
 		{
-			name: "abstains when user is not LDAP",
+			name: "allows when acls are nil",
+			ctx: &ACLContext{
+				ACLs: nil,
+				UserContext: &model.UserContext{
+					Provider: model.ProviderLocal,
+					Local: &model.LocalContext{
+						BaseContext: model.BaseContext{Username: "alice"},
+					},
+				},
+			},
+			expected: EffectAllow,
+		},
+		{
+			name: "allows when user is not LDAP",
 			ctx: &ACLContext{
 				ACLs: &model.App{
 					LDAP: model.AppLDAP{Groups: "admins"},
@@ -368,7 +406,22 @@ func TestLDAPGroupRule(t *testing.T) {
 					},
 				},
 			},
-			expected: EffectAbstain,
+			expected: EffectAllow,
+		},
+		{
+			name: "allows when group filter is empty",
+			ctx: &ACLContext{
+				ACLs: &model.App{
+					LDAP: model.AppLDAP{Groups: ""},
+				},
+				UserContext: &model.UserContext{
+					Provider: model.ProviderLDAP,
+					LDAP: &model.LDAPContext{
+						BaseContext: model.BaseContext{Username: "alice"},
+					},
+				},
+			},
+			expected: EffectAllow,
 		},
 		{
 			name: "allows LDAP user when a group matches",
@@ -416,7 +469,7 @@ func TestLDAPGroupRule(t *testing.T) {
 			expected: EffectDeny,
 		},
 		{
-			name: "abstains when groups filter is invalid",
+			name: "denies when groups filter is invalid",
 			ctx: &ACLContext{
 				ACLs: &model.App{
 					LDAP: model.AppLDAP{Groups: "/[/"},
@@ -428,7 +481,7 @@ func TestLDAPGroupRule(t *testing.T) {
 					},
 				},
 			},
-			expected: EffectAbstain,
+			expected: EffectDeny,
 		},
 	}