Move AuthOnly authorize logic to a dedicated method

jjungnickel · Dec 24, 2020 · 025056c · 025056c
1 parent 44d83e5
commit 025056c
Showing 2 changed files with 20 additions and 8 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -54,13 +54,13 @@
 - [#938](https://github.com/oauth2-proxy/oauth2-proxy/pull/938) Cleanup missed provider renaming refactor methods (@NickMeves)
 - [#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)
 - [#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)
 - [#918](https://github.com/oauth2-proxy/oauth2-proxy/pull/918) Fix log header output (@JoelSpeed)
 - [#911](https://github.com/oauth2-proxy/oauth2-proxy/pull/911) Validate provider type on startup.
-- [#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)
 - [#906](https://github.com/oauth2-proxy/oauth2-proxy/pull/906) Set up v6.1.x versioned documentation as default documentation (@JoelSpeed)
 - [#905](https://github.com/oauth2-proxy/oauth2-proxy/pull/905) Remove v5 legacy sessions support (@NickMeves)
 - [#904](https://github.com/oauth2-proxy/oauth2-proxy/pull/904) Set `skip-auth-strip-headers` to `true` by default (@NickMeves)

diff --git a/oauthproxy.go b/oauthproxy.go
@@ -926,17 +926,17 @@ func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) {
 }
 
 // AuthOnly checks whether the user is currently logged in (both authentication
-// and optional authorization via `allowed_groups` querystring).
+// and optional authorization).
 func (p *OAuthProxy) AuthOnly(rw http.ResponseWriter, req *http.Request) {
 	session, err := p.getAuthenticatedSession(rw, req)
 	if err != nil {
 		http.Error(rw, http.StatusText(http.StatusUnauthorized), http.StatusUnauthorized)
 		return
 	}
 
-	// Allow secondary group restrictions based on the `allowed_group` or
-	// `allowed_groups` querystring parameter
-	if !checkAllowedGroups(req, session) {
+	// Unauthorized cases need to return 403 to prevent infinite redirects with
+	// subrequest architectures
+	if !authOnlyAuthorize(req, session) {
 		http.Error(rw, http.StatusText(http.StatusForbidden), http.StatusForbidden)
 		return
 	}
@@ -1024,13 +1024,25 @@ func (p *OAuthProxy) getAuthenticatedSession(rw http.ResponseWriter, req *http.R
 	return session, nil
 }
 
-func checkAllowedGroups(req *http.Request, session *sessionsapi.SessionState) bool {
+// 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
+	if !checkAllowedGroups(req, s) {
+		return false
+	}
+
+	return true
+}
+
+func checkAllowedGroups(req *http.Request, s *sessionsapi.SessionState) bool {
 	allowedGroups := extractAllowedGroups(req)
 	if len(allowedGroups) == 0 {
 		return true
 	}
 
-	for _, group := range session.Groups {
+	for _, group := range s.Groups {
 		if _, ok := allowedGroups[group]; ok {
 			return true
 		}