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

Conversation

mathcantin
Copy link
Contributor

Allow to override the global configuration domains and whitelist in the rules.

If whitelist or domains are specified in the rule this override the global configurations (not merged).

The new priority for auth is whitelist in rule > domains in rule > config whitelist > global domains.

Note, it's my first attempt of go.

Copy link
Owner

@thomseddon thomseddon left a comment

Choose a reason for hiding this comment

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

This is brilliant, if you could address the points in this review then LGTM

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

}
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

}
}

_, 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]


_, ruleExists := config.Rules[rule]
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

@sylvaindd
Copy link

Would this permit something like this :

  • Allow mail1,mail2 with google on domain2
  • Allow mail1,mail3 with google on domain3
  • whitelist mail1

So mail1 could access every domains, mail2 domain2 and mail3 domain3 ?

@tdorsey
Copy link

tdorsey commented Jun 5, 2020

@thomseddon, @mathcantin: Has this change been obsoleted by work elsewhere? #94 mentions it, but it doesn't look like a replacement. I'm happy to make the changes needed to get this merged in otherwise

tdorsey added a commit to tdorsey/traefik-forward-auth that referenced this pull request Jun 6, 2020
tdorsey added a commit to tdorsey/traefik-forward-auth that referenced this pull request Jun 6, 2020
tdorsey added a commit to tdorsey/traefik-forward-auth that referenced this pull request Jun 6, 2020
Builds on thomseddon#63, adding support for the matchWhitelistOrDomain flag
tdorsey added a commit to tdorsey/traefik-forward-auth that referenced this pull request Jun 6, 2020
@thomseddon
Copy link
Owner

Hoping to merge this in #169

@thomseddon thomseddon closed this Aug 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants