diff --git a/internal/service/access_controls_rules.go b/internal/service/access_controls_rules.go index 3fc6d51b..98d2f1df 100644 --- a/internal/service/access_controls_rules.go +++ b/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 { - return EffectAbstain + if ctx.UserContext == nil { + 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 { - return EffectAbstain + if ctx.UserContext == nil { + 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") diff --git a/internal/service/access_controls_rules_test.go b/internal/service/access_controls_rules_test.go index 12e89ed2..2fc43f51 100644 --- a/internal/service/access_controls_rules_test.go +++ b/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", - ctx: &ACLContext{ - ACLs: &model.App{ - OAuth: model.AppOAuth{Whitelist: "alice"}, - }, - UserContext: nil, - }, - expected: EffectAbstain, - }, - { - name: "abstains when user is not OAuth", + name: "allows 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", - ctx: nil, - expected: EffectAbstain, - }, - { - name: "abstains when user context is nil", + name: "denies 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, }, }