Skip to content

Commit 7b06fc7

Browse files
ThomasK33ThomasK33
authored
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.
1 parent 071383b commit 7b06fc7

File tree

4 files changed

+45
-98
lines changed

4 files changed

+45
-98
lines changed

‎coderd/coderdtest/oidctest/helper.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -132,14 +132,14 @@ func OAuth2GetCode(rawAuthURL string, doRequest func(req *http.Request) (*http.R
132132
return "", xerrors.Errorf("failed to create auth request: %w", err)
133133
}
134134

135-
expCode := http.StatusTemporaryRedirect
136135
resp, err := doRequest(r)
137136
if err != nil {
138137
return "", xerrors.Errorf("request: %w", err)
139138
}
140139
defer resp.Body.Close()
141140

142-
if resp.StatusCode != expCode {
141+
// Accept both 302 (Found) and 307 (Temporary Redirect) as valid OAuth2 redirects
142+
if resp.StatusCode != http.StatusFound && resp.StatusCode != http.StatusTemporaryRedirect {
143143
return "", codersdk.ReadBodyAsError(resp)
144144
}
145145

‎coderd/oauth2.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,7 @@ func (api *API) deleteOAuth2ProviderAppSecret() http.HandlerFunc {
116116
// @Success 200 "Returns HTML authorization page"
117117
// @Router /oauth2/authorize [get]
118118
func (api *API) getOAuth2ProviderAppAuthorize() http.HandlerFunc {
119-
return oauth2provider.ShowAuthorizePage(api.Database, api.AccessURL)
119+
return oauth2provider.ShowAuthorizePage(api.AccessURL)
120120
}
121121

122122
// @Summary OAuth2 authorization request (POST - process authorization).
@@ -131,7 +131,7 @@ func (api *API) getOAuth2ProviderAppAuthorize() http.HandlerFunc {
131131
// @Success 302 "Returns redirect with authorization code"
132132
// @Router /oauth2/authorize [post]
133133
func (api *API) postOAuth2ProviderAppAuthorize() http.HandlerFunc {
134-
return oauth2provider.ProcessAuthorize(api.Database, api.AccessURL)
134+
return oauth2provider.ProcessAuthorize(api.Database)
135135
}
136136

137137
// @Summary OAuth2 token exchange.

‎coderd/oauth2provider/authorize.go

Lines changed: 41 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import (
1616
"github.com/coder/coder/v2/coderd/httpapi"
1717
"github.com/coder/coder/v2/coderd/httpmw"
1818
"github.com/coder/coder/v2/codersdk"
19+
"github.com/coder/coder/v2/site"
1920
)
2021

2122
type authorizeParams struct {
@@ -67,16 +68,46 @@ func extractAuthorizeParams(r *http.Request, callbackURL *url.URL) (authorizePar
6768
}
6869

6970
// ShowAuthorizePage handles GET /oauth2/authorize requests to display the HTML authorization page.
70-
// It uses authorizeMW which intercepts GET requests to show the authorization form.
71-
func ShowAuthorizePage(db database.Store, accessURL *url.URL) http.HandlerFunc {
72-
handler := authorizeMW(accessURL)(ProcessAuthorize(db, accessURL))
73-
return handler.ServeHTTP
71+
func ShowAuthorizePage(accessURL *url.URL) http.HandlerFunc {
72+
return func(rw http.ResponseWriter, r *http.Request) {
73+
app := httpmw.OAuth2ProviderApp(r)
74+
ua := httpmw.UserAuthorization(r.Context())
75+
76+
callbackURL, err := url.Parse(app.CallbackURL)
77+
if err != nil {
78+
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})
79+
return
80+
}
81+
82+
params, validationErrs, err := extractAuthorizeParams(r, callbackURL)
83+
if err != nil {
84+
errStr := make([]string, len(validationErrs))
85+
for i, err := range validationErrs {
86+
errStr[i] = err.Detail
87+
}
88+
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})
89+
return
90+
}
91+
92+
cancel := params.redirectURL
93+
cancelQuery := params.redirectURL.Query()
94+
cancelQuery.Add("error", "access_denied")
95+
cancel.RawQuery = cancelQuery.Encode()
96+
97+
site.RenderOAuthAllowPage(rw, r, site.RenderOAuthAllowData{
98+
AppIcon: app.Icon,
99+
AppName: app.Name,
100+
CancelURI: cancel.String(),
101+
RedirectURI: r.URL.String(),
102+
Username: ua.FriendlyName,
103+
})
104+
}
74105
}
75106

76107
// ProcessAuthorize handles POST /oauth2/authorize requests to process the user's authorization decision
77-
// and generate an authorization code. GET requests are handled by authorizeMW.
78-
func ProcessAuthorize(db database.Store, accessURL *url.URL) http.HandlerFunc {
79-
handler := func(rw http.ResponseWriter, r *http.Request) {
108+
// and generate an authorization code.
109+
func ProcessAuthorize(db database.Store) http.HandlerFunc {
110+
return func(rw http.ResponseWriter, r *http.Request) {
80111
ctx := r.Context()
81112
apiKey := httpmw.APIKey(r)
82113
app := httpmw.OAuth2ProviderApp(r)
@@ -159,9 +190,8 @@ func ProcessAuthorize(db database.Store, accessURL *url.URL) http.HandlerFunc {
159190
}
160191
params.redirectURL.RawQuery = newQuery.Encode()
161192

162-
http.Redirect(rw, r, params.redirectURL.String(), http.StatusTemporaryRedirect)
193+
// (ThomasK33): Use a 302 redirect as some (external) OAuth 2 apps and browsers
194+
// do not work with the 307.
195+
http.Redirect(rw, r, params.redirectURL.String(), http.StatusFound)
163196
}
164-
165-
// Always wrap with its custom mw.
166-
return authorizeMW(accessURL)(http.HandlerFunc(handler)).ServeHTTP
167197
}

‎coderd/oauth2provider/middleware.go

Lines changed: 0 additions & 83 deletions
This file was deleted.

0 commit comments

Comments
 (0)