Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to override domains and whitelist in rules #63

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,8 @@ 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).
- `domains` - optional, same usage as [`domain`](#domain).

For example:
```
Expand All @@ -283,7 +285,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 `whitelist` then only this is checked and `domain` is effectively ignored.
Note, if you pass `whitelist` then only this is checked and `domain` is effectively ignored. If you set `domains` or `whitelist` on a rules, the global configuration is ignored.

### Forwarded Headers

Expand Down
52 changes: 36 additions & 16 deletions internal/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,31 +56,51 @@ func ValidateCookie(r *http.Request, c *http.Cookie) (string, error) {
}

// Validate email
func ValidateEmail(email string) bool {
func ValidateEmail(email string, rule string) bool {
found := false
if len(config.Whitelist) > 0 {
for _, whitelist := range config.Whitelist {
if email == whitelist {
found = true
}
}

_, ruleExists := config.Rules[rule]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make the following code shorter, could you rename the function parameter to ruleName and then assign the first return argument of this function into a variable rule:

rule, ruleExists := config.Rules[ruleName]

if ruleExists && len(config.Rules[rule].Whitelist) > 0 {
found = ValidateWhitelist(email, config.Rules[rule].Whitelist)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On reflection, we don't actually need the found variable, so every branch in this function can just directly return

} else if ruleExists && len(config.Rules[rule].Domains) > 0 {
found = ValidateDomains(email, config.Rules[rule].Domains)
} else if len(config.Whitelist) > 0 {
found = ValidateWhitelist(email, config.Whitelist)
} else if len(config.Domains) > 0 {
parts := strings.Split(email, "@")
if len(parts) < 2 {
return false
}
for _, domain := range config.Domains {
if domain == parts[1] {
found = true
}
}
found = ValidateDomains(email, config.Domains)
} else {
return true
}

return found
}

// Validate email is in whitelist
func ValidateWhitelist(email string, whitelist CommaSeparatedList) bool {
found := false
for _, whitelist := range whitelist {
if email == whitelist {
found = true
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this is now a separate function we don't need the temporary found variable and this can just return true here

}
}
return found
}

// Validate email match a domains
func ValidateDomains(email string, domains CommaSeparatedList) bool {
found := false
parts := strings.Split(email, "@")
if len(parts) < 2 {
return false
}
for _, domain := range domains {
if domain == parts[1] {
found = true
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, this can just return here

}
}
return found
}

// OAuth Methods

// Get login url
Expand Down
45 changes: 39 additions & 6 deletions internal/auth_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,32 +67,65 @@ func TestAuthValidateEmail(t *testing.T) {
config, _ = NewConfig([]string{})

// Should allow any
v := ValidateEmail("test@test.com")
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")
v = ValidateEmail("one@two.com", "default")
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("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")
v = ValidateEmail("one@two.com", "default")
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("test@test.com", "default")
assert.True(v, "should allow user in whitelist")

// Should allow matching whitelisted in rules email address
config.Domains = []string{"globaltestdomain.com"}
config.Whitelist = []string{}
config.Rules = map[string]*Rule{"test": NewRule()}
config.Rules["test"].Whitelist = []string{"test@test.com"}
// Validation for user in the rule whitelist
v = ValidateEmail("test@test.com", "test")
assert.True(v, "should allow user in rule whitelist")
// Validation for user not in the rule whitelist
v = ValidateEmail("test2@test.com", "test")
assert.False(v, "should not allow user not in rule whitelist")
// Validation for user in global domain but not in rule
v = ValidateEmail("test@globaltestdomain.com", "test")
assert.False(v, "should not allow user in global but not in rule")
// Validation for user in the whitelist, but that not this rule
v = ValidateEmail("test@test.com", "default")
assert.False(v, "should not allow user not in the rule whitelisted")

// Should allow matching domains
config.Domains = []string{"globaltestdomain.com"}
config.Whitelist = []string{}
config.Rules = map[string]*Rule{"test": NewRule()}
config.Rules["test"].Domains = []string{"test.com"}
// Validation for user in the rule domains
v = ValidateEmail("test@test.com", "test")
assert.True(v, "should allow user in rule domains")
// Validation for user not in the rule whitelist
v = ValidateEmail("test@test2.com", "test")
assert.False(v, "should not allow user not in rule domains")
// Validation for user in the whitelist, but that not this rule
v = ValidateEmail("test@test.com", "default")
assert.False(v, "should not allow user not in the rule")
}

// TODO: Split google tests out
Expand Down
16 changes: 13 additions & 3 deletions internal/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,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("inavlid route param: %v", option)
}
Expand Down Expand Up @@ -266,9 +274,11 @@ func (c Config) String() string {
}

type Rule struct {
Action string
Rule string
Provider string
Action string
Rule string
Provider string
Whitelist CommaSeparatedList
Domains CommaSeparatedList
}

func NewRule() *Rule {
Expand Down
2 changes: 1 addition & 1 deletion internal/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ func (s *Server) AuthHandler(rule string) http.HandlerFunc {
}

// Validate user
valid := ValidateEmail(email)
valid := ValidateEmail(email, rule)
if !valid {
logger.WithFields(logrus.Fields{
"email": email,
Expand Down