kyle comments

coder · deansheather · Sep 13, 2022 · Aug 26, 2022 · Aug 26, 2022 · Aug 29, 2022
commit 58653d4aa4963b848b133af47f26fc4e22a5feed
diff --git a/coderd/coderd.go b/coderd/coderd.go
@@ -165,9 +165,18 @@ func New(options *Options) *API {
 		api.handleSubdomainApplications(
 			// Middleware to impose on the served application.
 			httpmw.RateLimitPerMinute(options.APIRateLimit),
+			httpmw.UseLoginURL(func() *url.URL {
+				if options.AccessURL == nil {
+					return nil
+				}
+
+				u := *options.AccessURL
+				u.Path = "/login"
+				return &u
+			}()),
 			// This should extract the application specific API key when we
 			// implement a scoped token.
-			httpmw.ExtractAPIKey(options.Database, oauthConfigs, false),
+			httpmw.ExtractAPIKey(options.Database, oauthConfigs, true),
 			httpmw.ExtractUserParam(api.Database),
 			httpmw.ExtractWorkspaceAndAgentParam(api.Database),
 		),

diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go
@@ -64,7 +64,6 @@ import (
 )
 
 type Options struct {
-	AccessURL            *url.URL
 	AWSCertificates      awsidentity.Certificates
 	Authorizer           rbac.Authorizer
 	AzureCertificates    x509.VerifyOptions
@@ -183,8 +182,12 @@ func newWithAPI(t *testing.T, options *Options) (*codersdk.Client, io.Closer, *c
 	srv.Start()
 	t.Cleanup(srv.Close)
 
+	tcpAddr, ok := srv.Listener.Addr().(*net.TCPAddr)
+	require.True(t, ok)
+
 	serverURL, err := url.Parse(srv.URL)
 	require.NoError(t, err)
+	serverURL.Host = fmt.Sprintf("localhost:%d", tcpAddr.Port)
 
 	derpPort, err := strconv.Atoi(serverURL.Port())
 	require.NoError(t, err)
@@ -205,19 +208,13 @@ func newWithAPI(t *testing.T, options *Options) (*codersdk.Client, io.Closer, *c
 		features.Auditor = options.Auditor
 	}
 
-	accessURL := options.AccessURL
-	if accessURL == nil {
-		accessURL, err = url.Parse(srv.URL)
-		require.NoError(t, err)
-	}
-
 	// We set the handler after server creation for the access URL.
 	coderAPI := options.APIBuilder(&coderd.Options{
 		AgentConnectionUpdateFrequency: 150 * time.Millisecond,
 		// Force a long disconnection timeout to ensure
 		// agents are not marked as disconnected during slow tests.
 		AgentInactiveDisconnectTimeout: testutil.WaitShort,
-		AccessURL:                      accessURL,
+		AccessURL:                      serverURL,
 		Logger:                         slogtest.Make(t, nil).Leveled(slog.LevelDebug),
 		CacheDir:                       t.TempDir(),
 		Database:                       db,

diff --git a/coderd/httpmw/apikey.go b/coderd/httpmw/apikey.go
@@ -9,6 +9,7 @@ import (
 	"fmt"
 	"net"
 	"net/http"
+	"net/url"
 	"strings"
 	"time"
 
@@ -58,6 +59,24 @@ const (
 	internalErrorMessage  string = "An internal error occurred. Please try again or contact the system administrator."
 )
 
+type loginURLKey struct{}
+
+func getLoginURL(r *http.Request) (*url.URL, bool) {
+	val, ok := r.Context().Value(loginURLKey{}).(*url.URL)
+	return val, ok
+}
+
+// UseLoginURL sets the login URL to use for the request for handlers like
+// ExtractAPIKey.
+func UseLoginURL(loginURL *url.URL) func(http.Handler) http.Handler {
+	return func(next http.Handler) http.Handler {
+		return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
+			ctx := context.WithValue(r.Context(), loginURLKey{}, loginURL)
+			next.ServeHTTP(rw, r.WithContext(ctx))
+		})
+	}
+}
+
 // ExtractAPIKey requires authentication using a valid API key.
 // It handles extending an API key if it comes close to expiry,
 // updating the last used time in the database.
@@ -70,14 +89,37 @@ func ExtractAPIKey(db database.Store, oauth *OAuth2Configs, redirectToLogin bool
 			// pages like workspace applications.
 			write := func(code int, response codersdk.Response) {
 				if redirectToLogin {
+					var (
+						u = &url.URL{
+							Path: "/login",
+						}
+						redirectURL = func() string {
+							path := r.URL.Path
+							if r.URL.RawQuery != "" {
+								path += "?" + r.URL.RawQuery
+							}
+							return path
+						}()
+					)
+					if loginURL, ok := getLoginURL(r); ok {
+						u = loginURL
+						// Don't redirect to the current page, as it may be on
+						// a different domain and we have issues determining the
+						// scheme to redirect to.
+						redirectURL = ""
+					}
+
 					q := r.URL.Query()
 					q.Add("message", response.Message)
-					q.Add("redirect", r.URL.Path+"?"+r.URL.RawQuery)
-					r.URL.RawQuery = q.Encode()
-					r.URL.Path = "/login"
-					http.Redirect(rw, r, r.URL.String(), http.StatusTemporaryRedirect)
+					if redirectURL != "" {
+						q.Add("redirect", redirectURL)
+					}
+					u.RawQuery = q.Encode()
+
+					http.Redirect(rw, r, u.String(), http.StatusTemporaryRedirect)
 					return
 				}
+
 				httpapi.Write(rw, code, response)
 			}
 

diff --git a/coderd/users_test.go b/coderd/users_test.go
@@ -4,7 +4,6 @@ import (
 	"context"
 	"fmt"
 	"net/http"
-	"net/url"
 	"sort"
 	"strings"
 	"testing"
@@ -265,12 +264,7 @@ func TestPostLogout(t *testing.T) {
 	t.Run("Logout", func(t *testing.T) {
 		t.Parallel()
 
-		accessURL, err := url.Parse("http://somedomain.com")
-		require.NoError(t, err)
-
-		client := coderdtest.New(t, &coderdtest.Options{
-			AccessURL: accessURL,
-		})
+		client := coderdtest.New(t, nil)
 		admin := coderdtest.CreateFirstUser(t, client)
 
 		ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)

diff --git a/coderd/workspaceapps_test.go b/coderd/workspaceapps_test.go
@@ -128,7 +128,7 @@ func TestWorkspaceAppsProxyPath(t *testing.T) {
 	t.Parallel()
 	client, orgID, workspace, port := setupProxyTest(t)
 
-	t.Run("RedirectsWithoutAuth", func(t *testing.T) {
+	t.Run("LoginWithoutAuth", func(t *testing.T) {
 		t.Parallel()
 		client := codersdk.New(client.URL)
 		client.HTTPClient.CheckRedirect = func(req *http.Request, via []*http.Request) error {
@@ -141,13 +141,14 @@ func TestWorkspaceAppsProxyPath(t *testing.T) {
 		resp, err := client.Request(ctx, http.MethodGet, "/@me/"+workspace.Name+"/apps/example", nil)
 		require.NoError(t, err)
 		defer resp.Body.Close()
-		location, err := resp.Location()
+
+		loc, err := resp.Location()
 		require.NoError(t, err)
-		require.Equal(t, "/login", location.Path)
-		require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode)
+		require.True(t, loc.Query().Has("message"))
+		require.True(t, loc.Query().Has("redirect"))
 	})
 
-	t.Run("NoAccessShould401", func(t *testing.T) {
+	t.Run("NoAccessShould404", func(t *testing.T) {
 		t.Parallel()
 
 		userClient := coderdtest.CreateAnotherUser(t, client, orgID, rbac.RoleMember())
@@ -284,7 +285,7 @@ func TestWorkspaceAppsProxySubdomain(t *testing.T) {
 		}).String()
 	}
 
-	t.Run("NoAuthShould401", func(t *testing.T) {
+	t.Run("LoginWithoutAuth", func(t *testing.T) {
 		t.Parallel()
 		unauthedClient := codersdk.New(client.URL)
 		unauthedClient.HTTPClient.CheckRedirect = client.HTTPClient.CheckRedirect
@@ -296,7 +297,17 @@ func TestWorkspaceAppsProxySubdomain(t *testing.T) {
 		resp, err := unauthedClient.Request(ctx, http.MethodGet, proxyURL(t, proxyTestAppName), nil)
 		require.NoError(t, err)
 		defer resp.Body.Close()
-		require.Equal(t, http.StatusUnauthorized, resp.StatusCode)
+		require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode)
+
+		loc, err := resp.Location()
+		require.NoError(t, err)
+		require.True(t, loc.Query().Has("message"))
+		require.False(t, loc.Query().Has("redirect"))
+
+		expectedURL := *client.URL
+		expectedURL.Path = "/login"
+		loc.RawQuery = ""
+		require.Equal(t, &expectedURL, loc)
 	})
 
 	t.Run("NoAccessShould401", func(t *testing.T) {