From 65e15f24c1bf275919e0961b29ca1ac973a63780 Mon Sep 17 00:00:00 2001 From: Nick Meves Date: Fri, 27 Nov 2020 09:07:21 -0800 Subject: [PATCH] Support only allowed_groups querystring --- CHANGELOG.md | 8 ++++---- oauthproxy.go | 22 ++++++++-------------- oauthproxy_test.go | 20 ++++++++++---------- 3 files changed, 22 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 504222d9..10b6aa88 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,8 +7,7 @@ - [#936](https://github.com/oauth2-proxy/oauth2-proxy/pull/936) `--user-id-claim` option is deprecated and replaced by `--oidc-email-claim` - [#630](https://github.com/oauth2-proxy/oauth2-proxy/pull/630) Gitlab projects needs a Gitlab application with the extra `read_api` enabled - [#849](https://github.com/oauth2-proxy/oauth2-proxy/pull/849) `/oauth2/auth` `allowed_groups` querystring parameter can be paired with the `allowed-groups` configuration option. - - In this scenario, the user's group must be in both lists to not get a 401 response code. - - The `allowed_group` querystring parameter can be specified multiple times to support multiple groups. + - In this scenario, the user's group must be in both lists to not get a 401 or 403 response code. - The `allowed_groups` querystring parameter can specify multiple comma delimited groups. - [#905](https://github.com/oauth2-proxy/oauth2-proxy/pull/905) Existing sessions from v6.0.0 or earlier are no longer valid. They will trigger a reauthentication. - [#826](https://github.com/oauth2-proxy/oauth2-proxy/pull/826) `skip-auth-strip-headers` now applies to all requests, not just those where authentication would be skipped. @@ -23,7 +22,8 @@ - [#575](https://github.com/oauth2-proxy/oauth2-proxy/pull/575) Sessions from v5.1.1 or earlier will no longer validate since they were not signed with SHA1. - Sessions from v6.0.0 or later had a graceful conversion to SHA256 that resulted in no reauthentication - Upgrading from v5.1.1 or earlier will result in a reauthentication -- [#616](https://github.com/oauth2-proxy/oauth2-proxy/pull/616) Ensure you have configured oauth2-proxy to use the `groups` scope. The user may be logged out initially as they may not currently have the `groups` claim however after going back through login process wil be authenticated. +- [#616](https://github.com/oauth2-proxy/oauth2-proxy/pull/616) Ensure you have configured oauth2-proxy to use the `groups` scope. + - The user may be logged out initially as they may not currently have the `groups` claim however after going back through login process wil be authenticated. - [#839](https://github.com/oauth2-proxy/oauth2-proxy/pull/839) Enables complex data structures for group claim entries, which are output as Json by default. ## Breaking Changes @@ -55,7 +55,7 @@ - [#816](https://github.com/oauth2-proxy/oauth2-proxy/pull/816) (via [#936](https://github.com/oauth2-proxy/oauth2-proxy/pull/936)) Support non-list group claims (@loafoe) - [#936](https://github.com/oauth2-proxy/oauth2-proxy/pull/936) Refactor OIDC Provider and support groups from Profile URL (@NickMeves) - [#869](https://github.com/oauth2-proxy/oauth2-proxy/pull/869) Streamline provider interface method names and signatures (@NickMeves) -- [#849](https://github.com/oauth2-proxy/oauth2-proxy/pull/849) Support group authorization on `oauth2/auth` endpoint via `allowed_group` & `allowed_groups` querystring parameters (@NickMeves) +- [#849](https://github.com/oauth2-proxy/oauth2-proxy/pull/849) Support group authorization on `oauth2/auth` endpoint via `allowed_groups` querystring (@NickMeves) - [#925](https://github.com/oauth2-proxy/oauth2-proxy/pull/925) Fix basic auth legacy header conversion (@JoelSpeed) - [#916](https://github.com/oauth2-proxy/oauth2-proxy/pull/916) Add AlphaOptions struct to prepare for alpha config loading (@JoelSpeed) - [#923](https://github.com/oauth2-proxy/oauth2-proxy/pull/923) Support TLS 1.3 (@aajisaka) diff --git a/oauthproxy.go b/oauthproxy.go index c11b83b1..999e1fbb 100644 --- a/oauthproxy.go +++ b/oauthproxy.go @@ -1027,8 +1027,8 @@ func (p *OAuthProxy) getAuthenticatedSession(rw http.ResponseWriter, req *http.R // authOnlyAuthorize handles special authorization logic that is only done // on the AuthOnly endpoint for use with Nginx subrequest architectures. func authOnlyAuthorize(req *http.Request, s *sessionsapi.SessionState) bool { - // Allow secondary group restrictions based on the `allowed_group` or - // `allowed_groups` querystring parameter + // Allow secondary group restrictions based on the `allowed_groups` + // querystring parameter if !checkAllowedGroups(req, s) { return false } @@ -1053,19 +1053,13 @@ func checkAllowedGroups(req *http.Request, s *sessionsapi.SessionState) bool { func extractAllowedGroups(req *http.Request) map[string]struct{} { groups := map[string]struct{}{} + query := req.URL.Query() - - // multi-key singular support - if multiGroups, ok := query["allowed_group"]; ok { - for _, group := range multiGroups { - groups[group] = struct{}{} - } - } - - // single key plural comma delimited support - for _, group := range strings.Split(query.Get("allowed_groups"), ",") { - if group != "" { - groups[group] = struct{}{} + for _, allowedGroups := range query["allowed_groups"] { + for _, group := range strings.Split(allowedGroups, ",") { + if group != "" { + groups[group] = struct{}{} + } } } diff --git a/oauthproxy_test.go b/oauthproxy_test.go index 3cc19447..56e046c2 100644 --- a/oauthproxy_test.go +++ b/oauthproxy_test.go @@ -2732,7 +2732,14 @@ func TestAuthOnlyAllowedGroups(t *testing.T) { name: "UserInQuerystringGroup", allowedGroups: []string{"a", "b"}, groups: []string{"a", "c"}, - querystring: "?allowed_group=a", + querystring: "?allowed_groups=a", + expectedStatusCode: http.StatusAccepted, + }, + { + name: "UserInMultiParamQuerystringGroup", + allowedGroups: []string{"a", "b"}, + groups: []string{"b"}, + querystring: "?allowed_groups=a&allowed_groups=b,d", expectedStatusCode: http.StatusAccepted, }, { @@ -2742,13 +2749,6 @@ func TestAuthOnlyAllowedGroups(t *testing.T) { querystring: "?allowed_groups=a,b", expectedStatusCode: http.StatusAccepted, }, - { - name: "UserInMultiParamQuerystringGroup", - allowedGroups: []string{"a", "b"}, - groups: []string{"b"}, - querystring: "?allowed_group=a&allowed_group=b", - expectedStatusCode: http.StatusAccepted, - }, { name: "UserInDelimitedQuerystringGroup", allowedGroups: []string{"a", "b", "c"}, @@ -2760,14 +2760,14 @@ func TestAuthOnlyAllowedGroups(t *testing.T) { name: "UserNotInQuerystringGroup", allowedGroups: []string{}, groups: []string{"c"}, - querystring: "?allowed_group=a&allowed_group=b", + querystring: "?allowed_groups=a,b", expectedStatusCode: http.StatusForbidden, }, { name: "UserInConfigGroupNotInQuerystringGroup", allowedGroups: []string{"a", "b", "c"}, groups: []string{"c"}, - querystring: "?allowed_group=a&allowed_group=b", + querystring: "?allowed_groups=a,b", expectedStatusCode: http.StatusForbidden, }, {