refactor: simplify OAuth2 authorization flow and use 302 redirects (#18923)

# Refactor OAuth2 Provider Authorization Flow

This PR refactors the OAuth2 provider authorization flow by:

1. Removing the `authorizeMW` middleware and directly implementing its functionality in the `ShowAuthorizePage` handler
2. Simplifying function signatures by removing unnecessary parameters:
   - Removed `db` parameter from `ShowAuthorizePage`
   - Removed `accessURL` parameter from `ProcessAuthorize`
3. Changing the redirect status code in `ProcessAuthorize` from 307 (Temporary Redirect) to 302 (Found) to improve compatibility with external OAuth2 apps and browsers. (Technical explanation: we replied with a 307 to a POST request, thus the browser performs a redirect to that URL as a POST request, but we need it to be a GET request to be compatible. Thus, we use the 302 redirect so that browsers turn it into a GET request when redirecting back to the redirect_uri.)

The changes maintain the same functionality while simplifying the code and improving compatibility with external systems.

coderd/coderdtest/oidctest/helper.go

@@ -132,14 +132,14 @@ func OAuth2GetCode(rawAuthURL string, doRequest func(req *http.Request) (*http.R
 		return "", xerrors.Errorf("failed to create auth request: %w", err)
 	}
 
-	expCode := http.StatusTemporaryRedirect
 	resp, err := doRequest(r)
 	if err != nil {
 		return "", xerrors.Errorf("request: %w", err)
 	}
 	defer resp.Body.Close()
 
-	if resp.StatusCode != expCode {
+	// Accept both 302 (Found) and 307 (Temporary Redirect) as valid OAuth2 redirects
+	if resp.StatusCode != http.StatusFound && resp.StatusCode != http.StatusTemporaryRedirect {
 		return "", codersdk.ReadBodyAsError(resp)
 	}
 

coderd/oauth2.go

@@ -116,7 +116,7 @@ func (api *API) deleteOAuth2ProviderAppSecret() http.HandlerFunc {
 // @Success 200 "Returns HTML authorization page"
 // @Router /oauth2/authorize [get]
 func (api *API) getOAuth2ProviderAppAuthorize() http.HandlerFunc {
-	return oauth2provider.ShowAuthorizePage(api.Database, api.AccessURL)
+	return oauth2provider.ShowAuthorizePage(api.AccessURL)
 }
 
 // @Summary OAuth2 authorization request (POST - process authorization).
@@ -131,7 +131,7 @@ func (api *API) getOAuth2ProviderAppAuthorize() http.HandlerFunc {
 // @Success 302 "Returns redirect with authorization code"
 // @Router /oauth2/authorize [post]
 func (api *API) postOAuth2ProviderAppAuthorize() http.HandlerFunc {
-	return oauth2provider.ProcessAuthorize(api.Database, api.AccessURL)
+	return oauth2provider.ProcessAuthorize(api.Database)
 }
 
 // @Summary OAuth2 token exchange.

coderd/oauth2provider/authorize.go

@@ -16,6 +16,7 @@ import (
 	"github.com/coder/coder/v2/coderd/httpapi"
 	"github.com/coder/coder/v2/coderd/httpmw"
 	"github.com/coder/coder/v2/codersdk"
+	"github.com/coder/coder/v2/site"
 )
 
 type authorizeParams struct {
@@ -67,16 +68,46 @@ func extractAuthorizeParams(r *http.Request, callbackURL *url.URL) (authorizePar
 }
 
 // ShowAuthorizePage handles GET /oauth2/authorize requests to display the HTML authorization page.
-// It uses authorizeMW which intercepts GET requests to show the authorization form.
-func ShowAuthorizePage(db database.Store, accessURL *url.URL) http.HandlerFunc {
-	handler := authorizeMW(accessURL)(ProcessAuthorize(db, accessURL))
-	return handler.ServeHTTP
+func ShowAuthorizePage(accessURL *url.URL) http.HandlerFunc {
+	return func(rw http.ResponseWriter, r *http.Request) {
+		app := httpmw.OAuth2ProviderApp(r)
+		ua := httpmw.UserAuthorization(r.Context())
+
+		callbackURL, err := url.Parse(app.CallbackURL)
+		if err != nil {
+			site.RenderStaticErrorPage(rw, r, site.ErrorPageData{Status: http.StatusInternalServerError, HideStatus: false, Title: "Internal Server Error", Description: err.Error(), RetryEnabled: false, DashboardURL: accessURL.String(), Warnings: nil})
+			return
+		}
+
+		params, validationErrs, err := extractAuthorizeParams(r, callbackURL)
+		if err != nil {
+			errStr := make([]string, len(validationErrs))
+			for i, err := range validationErrs {
+				errStr[i] = err.Detail
+			}
+			site.RenderStaticErrorPage(rw, r, site.ErrorPageData{Status: http.StatusBadRequest, HideStatus: false, Title: "Invalid Query Parameters", Description: "One or more query parameters are missing or invalid.", RetryEnabled: false, DashboardURL: accessURL.String(), Warnings: errStr})
+			return
+		}
+
+		cancel := params.redirectURL
+		cancelQuery := params.redirectURL.Query()
+		cancelQuery.Add("error", "access_denied")
+		cancel.RawQuery = cancelQuery.Encode()
+
+		site.RenderOAuthAllowPage(rw, r, site.RenderOAuthAllowData{
+			AppIcon:     app.Icon,
+			AppName:     app.Name,
+			CancelURI:   cancel.String(),
+			RedirectURI: r.URL.String(),
+			Username:    ua.FriendlyName,
+		})
+	}
 }
 
 // ProcessAuthorize handles POST /oauth2/authorize requests to process the user's authorization decision
-// and generate an authorization code. GET requests are handled by authorizeMW.
-func ProcessAuthorize(db database.Store, accessURL *url.URL) http.HandlerFunc {
-	handler := func(rw http.ResponseWriter, r *http.Request) {
+// and generate an authorization code.
+func ProcessAuthorize(db database.Store) http.HandlerFunc {
+	return func(rw http.ResponseWriter, r *http.Request) {
 		ctx := r.Context()
 		apiKey := httpmw.APIKey(r)
 		app := httpmw.OAuth2ProviderApp(r)
@@ -159,9 +190,8 @@ func ProcessAuthorize(db database.Store, accessURL *url.URL) http.HandlerFunc {
 		}
 		params.redirectURL.RawQuery = newQuery.Encode()
 
-		http.Redirect(rw, r, params.redirectURL.String(), http.StatusTemporaryRedirect)
+		// (ThomasK33): Use a 302 redirect as some (external) OAuth 2 apps and browsers
+		// do not work with the 307.
+		http.Redirect(rw, r, params.redirectURL.String(), http.StatusFound)
 	}
-
-	// Always wrap with its custom mw.
-	return authorizeMW(accessURL)(http.HandlerFunc(handler)).ServeHTTP
 }

coderd/oauth2provider/middleware.go

@@ -1,83 +0,0 @@
-package oauth2provider
-
-import (
-	"net/http"
-	"net/url"
-
-	"github.com/coder/coder/v2/coderd/httpmw"
-	"github.com/coder/coder/v2/site"
-)
-
-// authorizeMW serves to remove some code from the primary authorize handler.
-// It decides when to show the html allow page, and when to just continue.
-func authorizeMW(accessURL *url.URL) func(next http.Handler) http.Handler {
-	return func(next http.Handler) http.Handler {
-		return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
-			app := httpmw.OAuth2ProviderApp(r)
-			ua := httpmw.UserAuthorization(r.Context())
-
-			// If this is a POST request, it means the user clicked the "Allow" button
-			// on the consent form. Process the authorization.
-			if r.Method == http.MethodPost {
-				next.ServeHTTP(rw, r)
-				return
-			}
-
-			// For GET requests, show the authorization consent page
-			// TODO: For now only browser-based auth flow is officially supported but
-			// in a future PR we should support a cURL-based flow where we output text
-			// instead of HTML.
-
-			callbackURL, err := url.Parse(app.CallbackURL)
-			if err != nil {
-				site.RenderStaticErrorPage(rw, r, site.ErrorPageData{
-					Status:       http.StatusInternalServerError,
-					HideStatus:   false,
-					Title:        "Internal Server Error",
-					Description:  err.Error(),
-					RetryEnabled: false,
-					DashboardURL: accessURL.String(),
-					Warnings:     nil,
-				})
-				return
-			}
-
-			// Extract the form parameters for two reasons:
-			// 1. We need the redirect URI to build the cancel URI.
-			// 2. Since validation will run once the user clicks "allow", it is
-			//    better to validate now to avoid wasting the user's time clicking a
-			//    button that will just error anyway.
-			params, validationErrs, err := extractAuthorizeParams(r, callbackURL)
-			if err != nil {
-				errStr := make([]string, len(validationErrs))
-				for i, err := range validationErrs {
-					errStr[i] = err.Detail
-				}
-				site.RenderStaticErrorPage(rw, r, site.ErrorPageData{
-					Status:       http.StatusBadRequest,
-					HideStatus:   false,
-					Title:        "Invalid Query Parameters",
-					Description:  "One or more query parameters are missing or invalid.",
-					RetryEnabled: false,
-					DashboardURL: accessURL.String(),
-					Warnings:     errStr,
-				})
-				return
-			}
-
-			cancel := params.redirectURL
-			cancelQuery := params.redirectURL.Query()
-			cancelQuery.Add("error", "access_denied")
-			cancel.RawQuery = cancelQuery.Encode()
-
-			// Render the consent page with the current URL (no need to add redirected parameter)
-			site.RenderOAuthAllowPage(rw, r, site.RenderOAuthAllowData{
-				AppIcon:     app.Icon,
-				AppName:     app.Name,
-				CancelURI:   cancel.String(),
-				RedirectURI: r.URL.String(),
-				Username:    ua.FriendlyName,
-			})
-		})
-	}
-}