-
-
Notifications
You must be signed in to change notification settings - Fork 409
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 custom key to be used for whitelist and X-Forwarded-User instead of the hardcoded email #159
base: master
Are you sure you want to change the base?
Changes from 19 commits
1b7d054
bcadb3a
b290b21
f041759
0e2bb23
b0f7b17
c792263
189e4a1
399f3da
40bd110
2a2d542
22aa772
d9c2ec2
cb02259
c77e649
4b554e7
c7f5f0a
fb70085
42b3750
a33a869
9ea7d98
49439c7
4906a18
4091bb1
58d555c
dc20081
a98e568
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -156,12 +156,12 @@ Application Options: | |
--csrf-cookie-name= CSRF Cookie Name (default: _forward_auth_csrf) [$CSRF_COOKIE_NAME] | ||
--default-action=[auth|allow] Default action (default: auth) [$DEFAULT_ACTION] | ||
--default-provider=[google|oidc|generic-oauth] Default provider (default: google) [$DEFAULT_PROVIDER] | ||
--domain= Only allow given email domains, can be set multiple times [$DOMAIN] | ||
--domain= Only allow given email domains, comma separated, can be set multiple times [$DOMAIN] | ||
--lifetime= Lifetime in seconds (default: 43200) [$LIFETIME] | ||
--logout-redirect= URL to redirect to following logout [$LOGOUT_REDIRECT] | ||
--url-path= Callback URL Path (default: /_oauth) [$URL_PATH] | ||
--secret= Secret used for signing (required) [$SECRET] | ||
--whitelist= Only allow given email addresses, can be set multiple times [$WHITELIST] | ||
--whitelist= Only allow given email addresses, comma separated, can be set multiple times [$WHITELIST] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe this should now also be -Only allow given email addresses
+Only allow given user id There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice, good catch! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
--rule.<name>.<param>= Rule definitions, param can be: "action", "rule" or "provider" | ||
|
||
Google Provider: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -67,56 +67,61 @@ func TestAuthValidateEmail(t *testing.T) { | |
config, _ = NewConfig([]string{}) | ||
|
||
// Should allow any | ||
v := ValidateEmail("[email protected]") | ||
v := ValidateUser("[email protected]") | ||
assert.True(v, "should allow any domain if email domain is not defined") | ||
v = ValidateEmail("[email protected]") | ||
v = ValidateUser("[email protected]") | ||
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("[email protected]") | ||
v = ValidateUser("[email protected]") | ||
assert.False(v, "should not allow user from another domain") | ||
|
||
// Should allow matching domain | ||
config.Domains = []string{"test.com"} | ||
v = ValidateEmail("[email protected]") | ||
v = ValidateUser("[email protected]") | ||
assert.True(v, "should allow user from allowed domain") | ||
|
||
// Should block non whitelisted email address | ||
config.Domains = []string{} | ||
config.Whitelist = []string{"[email protected]"} | ||
v = ValidateEmail("[email protected]") | ||
v = ValidateUser("[email protected]") | ||
assert.False(v, "should not allow user not in whitelist") | ||
|
||
// Should allow matching whitelisted email address | ||
config.Domains = []string{} | ||
config.Whitelist = []string{"[email protected]"} | ||
v = ValidateEmail("[email protected]") | ||
v = ValidateUser("[email protected]") | ||
assert.True(v, "should allow user in whitelist") | ||
|
||
// Should allow only matching email address when | ||
// MatchWhitelistOrDomain is disabled | ||
config.Domains = []string{"example.com"} | ||
config.Whitelist = []string{"[email protected]"} | ||
config.MatchWhitelistOrDomain = false | ||
v = ValidateEmail("[email protected]") | ||
v = ValidateUser("[email protected]") | ||
assert.True(v, "should allow user in whitelist") | ||
v = ValidateEmail("[email protected]") | ||
v = ValidateUser("[email protected]") | ||
assert.False(v, "should not allow user from valid domain") | ||
v = ValidateEmail("[email protected]") | ||
v = ValidateUser("[email protected]") | ||
assert.False(v, "should not allow user not in either") | ||
|
||
// Should allow either matching domain or email address when | ||
// MatchWhitelistOrDomain is enabled | ||
config.Domains = []string{"example.com"} | ||
config.Whitelist = []string{"[email protected]"} | ||
config.MatchWhitelistOrDomain = true | ||
v = ValidateEmail("[email protected]") | ||
v = ValidateUser("[email protected]") | ||
assert.True(v, "should allow user in whitelist") | ||
v = ValidateEmail("[email protected]") | ||
v = ValidateUser("[email protected]") | ||
assert.True(v, "should allow user from valid domain") | ||
v = ValidateEmail("[email protected]") | ||
v = ValidateUser("[email protected]") | ||
assert.False(v, "should not allow user not in either") | ||
|
||
// Should allow comma separated email | ||
config.Whitelist = []string{"[email protected]", "[email protected]"} | ||
v = ValidateUser("[email protected]") | ||
assert.True(v, "should allow user in whitelist") | ||
} | ||
|
||
func TestRedirectUri(t *testing.T) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -105,6 +105,17 @@ func TestConfigParseRuleError(t *testing.T) { | |
assert.Equal(map[string]*Rule{}, c.Rules) | ||
} | ||
|
||
func TestConfigCommaSeperated(t *testing.T) { | ||
assert := assert.New(t) | ||
c, err := NewConfig([]string{ | ||
"[email protected],[email protected]", | ||
}) | ||
require.Nil(t, err) | ||
|
||
expected1 := CommaSeparatedList{"[email protected]", "[email protected]"} | ||
assert.Equal(expected1, c.Whitelist, "should read legacy comma separated list whitelist") | ||
} | ||
|
||
func TestConfigFlagBackwardsCompatability(t *testing.T) { | ||
assert := assert.New(t) | ||
c, err := NewConfig([]string{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -133,8 +133,8 @@ func TestGenericOAuthGetUser(t *testing.T) { | |
// AuthStyleInHeader is attempted | ||
p.Config.Endpoint.AuthStyle = oauth2.AuthStyleInParams | ||
|
||
user, err := p.GetUser("123456789") | ||
user, err := p.GetUser("123456789", "email") | ||
assert.Nil(err) | ||
|
||
assert.Equal("[email protected]", user.Email) | ||
assert.Equal("[email protected]", user) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -141,8 +141,8 @@ func TestGoogleGetUser(t *testing.T) { | |
}, | ||
} | ||
|
||
user, err := p.GetUser("123456789") | ||
user, err := p.GetUser("123456789", "email") | ||
assert.Nil(err) | ||
|
||
assert.Equal("[email protected]", user.Email) | ||
assert.Equal("[email protected]", user) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,7 +13,7 @@ import ( | |
"time" | ||
|
||
"github.com/stretchr/testify/assert" | ||
jose "gopkg.in/square/go-jose.v2" | ||
"gopkg.in/square/go-jose.v2" | ||
) | ||
|
||
// Tests | ||
|
@@ -122,9 +122,9 @@ func TestOIDCGetUser(t *testing.T) { | |
}`)) | ||
|
||
// Get user | ||
user, err := provider.GetUser(token) | ||
user, err := provider.GetUser(token, "") | ||
assert.Nil(err) | ||
assert.Equal("[email protected]", user.Email) | ||
assert.Equal("[email protected]", user) | ||
} | ||
|
||
// Utils | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strictly speaking, these version updates would normally not be part of a pull request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you mean? Only @thomseddon should make these changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's completely up to the maintainer, but in many projects I contributed to, a PR was really just about one specific feature or bugfix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverted