Support only allowed_groups querystring

jjungnickel · Dec 24, 2020 · 65e15f2 · 65e15f2
1 parent 025056c
commit 65e15f2
Showing 3 changed files with 22 additions and 28 deletions.
diff --git 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
@@ -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{}{}
+	query := req.URL.Query()
+	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
@@ -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,
 		},
 		{