From 04f5499f0bb38c7dffbda8e244cb125e9ac09e36 Mon Sep 17 00:00:00 2001 From: Thom Seddon Date: Wed, 23 Sep 2020 14:50:15 +0100 Subject: [PATCH] Allow override of domains and whitelist in rules (#169) Co-authored-by: Mathieu Cantin Co-authored-by: Pete Shaw --- README.md | 9 ++- internal/auth.go | 58 ++++++++++++++------ internal/auth_test.go | 125 +++++++++++++++++++++++++++++++++--------- internal/config.go | 16 +++++- internal/server.go | 2 +- 5 files changed, 164 insertions(+), 46 deletions(-) diff --git a/README.md b/README.md index 54cb5798..b0c5444b 100644 --- a/README.md +++ b/README.md @@ -321,6 +321,7 @@ All options can be supplied in any of the following ways, in the following prece - `action` - same usage as [`default-action`](#default-action), supported values: - `auth` (default) - `allow` + - `domains` - optional, same usage as [`domain`](#domain) - `provider` - same usage as [`default-provider`](#default-provider), supported values: - `google` - `oidc` @@ -333,6 +334,7 @@ All options can be supplied in any of the following ways, in the following prece - ``Path(`path`, `/articles/{category}/{id:[0-9]+}`, ...)`` - ``PathPrefix(`/products/`, `/articles/{category}/{id:[0-9]+}`)`` - ``Query(`foo=bar`, `bar=baz`)`` + - `whitelist` - optional, same usage as whitelist`](#whitelist) For example: ``` @@ -348,6 +350,11 @@ All options can be supplied in any of the following ways, in the following prece rule.oidc.action = auth rule.oidc.provider = oidc rule.oidc.rule = PathPrefix(`/github`) + + # Allow jane@example.com to `/janes-eyes-only` + rule.two.action = allow + rule.two.rule = Path(`/janes-eyes-only`) + rule.two.whitelist = jane@example.com ``` Note: It is possible to break your redirect flow with rules, please be careful not to create an `allow` rule that matches your redirect_uri unless you know what you're doing. This limitation is being tracked in in #101 and the behaviour will change in future releases. @@ -361,7 +368,7 @@ You can restrict who can login with the following parameters: * `domain` - Use this to limit logins to a specific domain, e.g. test.com only * `whitelist` - Use this to only allow specific users to login e.g. thom@test.com only -Note, if you pass both `whitelist` and `domain`, then the default behaviour is for only `whitelist` to be used and `domain` will be effectively ignored. You can allow users matching *either* `whitelist` or `domain` by passing the `match-whitelist-or-domain` parameter (this will be the default behaviour in v3). +Note, if you pass both `whitelist` and `domain`, then the default behaviour is for only `whitelist` to be used and `domain` will be effectively ignored. You can allow users matching *either* `whitelist` or `domain` by passing the `match-whitelist-or-domain` parameter (this will be the default behaviour in v3). If you set `domains` or `whitelist` on a rule, the global configuration is ignored. ### Forwarded Headers diff --git a/internal/auth.go b/internal/auth.go index 64a2dc10..0b0a9676 100644 --- a/internal/auth.go +++ b/internal/auth.go @@ -59,18 +59,28 @@ func ValidateCookie(r *http.Request, c *http.Cookie) (string, error) { // ValidateEmail checks if the given email address matches either a whitelisted // email address, as defined by the "whitelist" config parameter. Or is part of // a permitted domain, as defined by the "domains" config parameter -func ValidateEmail(email string) bool { +func ValidateEmail(email, ruleName string) bool { + // Use global config by default + whitelist := config.Whitelist + domains := config.Domains + + if rule, ok := config.Rules[ruleName]; ok { + // Override with rule config if found + if len(rule.Whitelist) > 0 || len(rule.Domains) > 0 { + whitelist = rule.Whitelist + domains = rule.Domains + } + } + // Do we have any validation to perform? - if len(config.Whitelist) == 0 && len(config.Domains) == 0 { + if len(whitelist) == 0 && len(domains) == 0 { return true } // Email whitelist validation - if len(config.Whitelist) > 0 { - for _, whitelist := range config.Whitelist { - if email == whitelist { - return true - } + if len(whitelist) > 0 { + if ValidateWhitelist(email, whitelist) { + return true } // If we're not matching *either*, stop here @@ -80,18 +90,34 @@ func ValidateEmail(email string) bool { } // Domain validation - if len(config.Domains) > 0 { - parts := strings.Split(email, "@") - if len(parts) < 2 { - return false - } - for _, domain := range config.Domains { - if domain == parts[1] { - return true - } + if len(domains) > 0 && ValidateDomains(email, domains) { + return true + } + + return false +} + +// ValidateWhitelist checks if the email is in whitelist +func ValidateWhitelist(email string, whitelist CommaSeparatedList) bool { + for _, whitelist := range whitelist { + if email == whitelist { + return true } } + return false +} +// ValidateDomains checks if the email matches a whitelisted domain +func ValidateDomains(email string, domains CommaSeparatedList) bool { + parts := strings.Split(email, "@") + if len(parts) < 2 { + return false + } + for _, domain := range domains { + if domain == parts[1] { + return true + } + } return false } diff --git a/internal/auth_test.go b/internal/auth_test.go index d013dfc5..5b0bedaf 100644 --- a/internal/auth_test.go +++ b/internal/auth_test.go @@ -65,32 +65,25 @@ func TestAuthValidateEmail(t *testing.T) { assert := assert.New(t) config, _ = NewConfig([]string{}) - // Should allow any - v := ValidateEmail("test@test.com") + // Should allow any with no whitelist/domain is specified + v := ValidateEmail("test@test.com", "default") assert.True(v, "should allow any domain if email domain is not defined") - v = ValidateEmail("one@two.com") + v = ValidateEmail("one@two.com", "default") assert.True(v, "should allow any domain if email domain is not defined") - // Should block non matching domain - config.Domains = []string{"test.com"} - v = ValidateEmail("one@two.com") - assert.False(v, "should not allow user from another domain") - // Should allow matching domain config.Domains = []string{"test.com"} - v = ValidateEmail("test@test.com") + v = ValidateEmail("one@two.com", "default") + assert.False(v, "should not allow user from another domain") + v = ValidateEmail("test@test.com", "default") assert.True(v, "should allow user from allowed domain") - // Should block non whitelisted email address - config.Domains = []string{} - config.Whitelist = []string{"test@test.com"} - v = ValidateEmail("one@two.com") - assert.False(v, "should not allow user not in whitelist") - // Should allow matching whitelisted email address config.Domains = []string{} config.Whitelist = []string{"test@test.com"} - v = ValidateEmail("test@test.com") + v = ValidateEmail("one@two.com", "default") + assert.False(v, "should not allow user not in whitelist") + v = ValidateEmail("test@test.com", "default") assert.True(v, "should allow user in whitelist") // Should allow only matching email address when @@ -98,24 +91,106 @@ func TestAuthValidateEmail(t *testing.T) { config.Domains = []string{"example.com"} config.Whitelist = []string{"test@test.com"} config.MatchWhitelistOrDomain = false - v = ValidateEmail("test@test.com") - assert.True(v, "should allow user in whitelist") - v = ValidateEmail("test@example.com") - assert.False(v, "should not allow user from valid domain") - v = ValidateEmail("one@two.com") + v = ValidateEmail("one@two.com", "default") assert.False(v, "should not allow user not in either") + v = ValidateEmail("test@example.com", "default") + assert.False(v, "should not allow user from allowed domain") + v = ValidateEmail("test@test.com", "default") + assert.True(v, "should allow user in whitelist") // Should allow either matching domain or email address when // MatchWhitelistOrDomain is enabled config.Domains = []string{"example.com"} config.Whitelist = []string{"test@test.com"} config.MatchWhitelistOrDomain = true - v = ValidateEmail("test@test.com") + v = ValidateEmail("one@two.com", "default") + assert.False(v, "should not allow user not in either") + v = ValidateEmail("test@example.com", "default") + assert.True(v, "should allow user from allowed domain") + v = ValidateEmail("test@test.com", "default") assert.True(v, "should allow user in whitelist") - v = ValidateEmail("test@example.com") - assert.True(v, "should allow user from valid domain") - v = ValidateEmail("one@two.com") + + // Rule testing + + // Should use global whitelist/domain when not specified on rule + config.Domains = []string{"example.com"} + config.Whitelist = []string{"test@test.com"} + config.Rules = map[string]*Rule{"test": NewRule()} + config.MatchWhitelistOrDomain = true + v = ValidateEmail("one@two.com", "test") assert.False(v, "should not allow user not in either") + v = ValidateEmail("test@example.com", "test") + assert.True(v, "should allow user from allowed global domain") + v = ValidateEmail("test@test.com", "test") + assert.True(v, "should allow user in global whitelist") + + // Should allow matching domain in rule + config.Domains = []string{"testglobal.com"} + config.Whitelist = []string{} + rule := NewRule() + config.Rules = map[string]*Rule{"test": rule} + rule.Domains = []string{"testrule.com"} + config.MatchWhitelistOrDomain = false + v = ValidateEmail("one@two.com", "test") + assert.False(v, "should not allow user from another domain") + v = ValidateEmail("one@testglobal.com", "test") + assert.False(v, "should not allow user from global domain") + v = ValidateEmail("test@testrule.com", "test") + assert.True(v, "should allow user from allowed domain") + + // Should allow matching whitelist in rule + config.Domains = []string{} + config.Whitelist = []string{"test@testglobal.com"} + rule = NewRule() + config.Rules = map[string]*Rule{"test": rule} + rule.Whitelist = []string{"test@testrule.com"} + config.MatchWhitelistOrDomain = false + v = ValidateEmail("one@two.com", "test") + assert.False(v, "should not allow user from another domain") + v = ValidateEmail("test@testglobal.com", "test") + assert.False(v, "should not allow user from global domain") + v = ValidateEmail("test@testrule.com", "test") + assert.True(v, "should allow user from allowed domain") + + // Should allow only matching email address when + // MatchWhitelistOrDomain is disabled + config.Domains = []string{"exampleglobal.com"} + config.Whitelist = []string{"test@testglobal.com"} + rule = NewRule() + config.Rules = map[string]*Rule{"test": rule} + rule.Domains = []string{"examplerule.com"} + rule.Whitelist = []string{"test@testrule.com"} + config.MatchWhitelistOrDomain = false + v = ValidateEmail("one@two.com", "test") + assert.False(v, "should not allow user not in either") + v = ValidateEmail("test@testglobal.com", "test") + assert.False(v, "should not allow user in global whitelist") + v = ValidateEmail("test@exampleglobal.com", "test") + assert.False(v, "should not allow user from global domain") + v = ValidateEmail("test@examplerule.com", "test") + assert.False(v, "should not allow user from allowed domain") + v = ValidateEmail("test@testrule.com", "test") + assert.True(v, "should allow user in whitelist") + + // Should allow either matching domain or email address when + // MatchWhitelistOrDomain is enabled + config.Domains = []string{"exampleglobal.com"} + config.Whitelist = []string{"test@testglobal.com"} + rule = NewRule() + config.Rules = map[string]*Rule{"test": rule} + rule.Domains = []string{"examplerule.com"} + rule.Whitelist = []string{"test@testrule.com"} + config.MatchWhitelistOrDomain = true + v = ValidateEmail("one@two.com", "test") + assert.False(v, "should not allow user not in either") + v = ValidateEmail("test@testglobal.com", "test") + assert.False(v, "should not allow user in global whitelist") + v = ValidateEmail("test@exampleglobal.com", "test") + assert.False(v, "should not allow user from global domain") + v = ValidateEmail("test@examplerule.com", "test") + assert.True(v, "should allow user from allowed domain") + v = ValidateEmail("test@testrule.com", "test") + assert.True(v, "should allow user in whitelist") } func TestRedirectUri(t *testing.T) { diff --git a/internal/config.go b/internal/config.go index e35a732c..40546121 100644 --- a/internal/config.go +++ b/internal/config.go @@ -210,6 +210,14 @@ func (c *Config) parseUnknownFlag(option string, arg flags.SplitArgument, args [ rule.Rule = val case "provider": rule.Provider = val + case "whitelist": + list := CommaSeparatedList{} + list.UnmarshalFlag(val) + rule.Whitelist = list + case "domains": + list := CommaSeparatedList{} + list.UnmarshalFlag(val) + rule.Domains = list default: return args, fmt.Errorf("invalid route param: %v", option) } @@ -327,9 +335,11 @@ func (c *Config) setupProvider(name string) error { // Rule holds defined rules type Rule struct { - Action string - Rule string - Provider string + Action string + Rule string + Provider string + Whitelist CommaSeparatedList + Domains CommaSeparatedList } // NewRule creates a new rule object diff --git a/internal/server.go b/internal/server.go index ad7e8301..4bce9635 100644 --- a/internal/server.go +++ b/internal/server.go @@ -101,7 +101,7 @@ func (s *Server) AuthHandler(providerName, rule string) http.HandlerFunc { } // Validate user - valid := ValidateEmail(email) + valid := ValidateEmail(email, rule) if !valid { logger.WithField("email", email).Warn("Invalid email") http.Error(w, "Not authorized", 401)