From 7b06fc77ae4bf5a9a52c3e750ec580dcb8e2437f Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Sun, 20 Jul 2025 16:22:52 +0200 Subject: [PATCH] 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 | 4 +- coderd/oauth2.go | 4 +- coderd/oauth2provider/authorize.go | 52 +++++++++++++---- coderd/oauth2provider/middleware.go | 83 ---------------------------- 4 files changed, 45 insertions(+), 98 deletions(-) delete mode 100644 coderd/oauth2provider/middleware.go diff --git a/coderd/coderdtest/oidctest/helper.go b/coderd/coderdtest/oidctest/helper.go index c817c8ca47..16b46ac662 100644 --- a/coderd/coderdtest/oidctest/helper.go +++ b/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) } diff --git a/coderd/oauth2.go b/coderd/oauth2.go index 9195876b9e..1e28f9b65b 100644 --- a/coderd/oauth2.go +++ b/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. diff --git a/coderd/oauth2provider/authorize.go b/coderd/oauth2provider/authorize.go index 29d0c99abc..4100b82306 100644 --- a/coderd/oauth2provider/authorize.go +++ b/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 } diff --git a/coderd/oauth2provider/middleware.go b/coderd/oauth2provider/middleware.go deleted file mode 100644 index c989d068a8..0000000000 --- a/coderd/oauth2provider/middleware.go +++ /dev/null @@ -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, - }) - }) - } -}