Pr feedback, add authorized redirect urls

Emyrk · Emyrk · commit 1c7e8b47d2f4 · 2023-08-25T10:02:43.000-05:00
diff --git a/coderd/coderdtest/oidctest/helper.go b/coderd/coderdtest/oidctest/helper.go
@@ -19,20 +19,20 @@ import (
 // It is mainly because refreshing oauth tokens is a bit tricky and requires
 // some database manipulation.
 type LoginHelper struct {
-	fake  *FakeIDP
-	owner *codersdk.Client
+	fake   *FakeIDP
+	client *codersdk.Client
 }
 
-func NewLoginHelper(owner *codersdk.Client, fake *FakeIDP) *LoginHelper {
-	if owner == nil {
-		panic("owner must not be nil")
+func NewLoginHelper(client *codersdk.Client, fake *FakeIDP) *LoginHelper {
+	if client == nil {
+		panic("client must not be nil")
 	}
 	if fake == nil {
 		panic("fake must not be nil")
 	}
 	return &LoginHelper{
-		fake:  fake,
-		owner: owner,
+		fake:   fake,
+		client: client,
 	}
 }
 
@@ -41,13 +41,13 @@ func NewLoginHelper(owner *codersdk.Client, fake *FakeIDP) *LoginHelper {
 // convenience method.
 func (h *LoginHelper) Login(t *testing.T, idTokenClaims jwt.MapClaims) (*codersdk.Client, *http.Response) {
 	t.Helper()
-	unauthenticatedClient := codersdk.New(h.owner.URL)
+	unauthenticatedClient := codersdk.New(h.client.URL)
 
 	return h.fake.Login(t, unauthenticatedClient, idTokenClaims)
 }
 
 // ExpireOauthToken expires the oauth token for the given user.
-func (*LoginHelper) ExpireOauthToken(t *testing.T, db database.Store, user *codersdk.Client) (refreshToken string) {
+func (*LoginHelper) ExpireOauthToken(t *testing.T, db database.Store, user *codersdk.Client) database.UserLink {
 	t.Helper()
 
 	//nolint:gocritic // Testing
@@ -68,7 +68,7 @@ func (*LoginHelper) ExpireOauthToken(t *testing.T, db database.Store, user *code
 	require.NoError(t, err, "get user link")
 
 	// Expire the oauth link for the given user.
-	_, err = db.UpdateUserLink(ctx, database.UpdateUserLinkParams{
+	updated, err := db.UpdateUserLink(ctx, database.UpdateUserLinkParams{
 		OAuthAccessToken:  link.OAuthAccessToken,
 		OAuthRefreshToken: link.OAuthRefreshToken,
 		OAuthExpiry:       time.Now().Add(time.Hour * -1),
@@ -77,7 +77,7 @@ func (*LoginHelper) ExpireOauthToken(t *testing.T, db database.Store, user *code
 	})
 	require.NoError(t, err, "expire user link")
 
-	return link.OAuthRefreshToken
+	return updated
 }
 
 // ForceRefresh forces the client to refresh its oauth token. It does this by
@@ -88,13 +88,13 @@ func (*LoginHelper) ExpireOauthToken(t *testing.T, db database.Store, user *code
 func (h *LoginHelper) ForceRefresh(t *testing.T, db database.Store, user *codersdk.Client, idToken jwt.MapClaims) {
 	t.Helper()
 
-	refreshToken := h.ExpireOauthToken(t, db, user)
+	link := h.ExpireOauthToken(t, db, user)
 	// Updates the claims that the IDP will return. By default, it always
 	// uses the original claims for the original oauth token.
-	h.fake.UpdateRefreshClaims(refreshToken, idToken)
+	h.fake.UpdateRefreshClaims(link.OAuthRefreshToken, idToken)
 
 	t.Cleanup(func() {
-		require.True(t, h.fake.RefreshUsed(refreshToken), "refresh token must be used, but has not. Did you forget to call the returned function from this call?")
+		require.True(t, h.fake.RefreshUsed(link.OAuthRefreshToken), "refresh token must be used, but has not. Did you forget to call the returned function from this call?")
 	})
 
 	// Do any authenticated call to force the refresh
diff --git a/coderd/coderdtest/oidctest/idp.go b/coderd/coderdtest/oidctest/idp.go
@@ -60,9 +60,13 @@ type FakeIDP struct {
 	refreshIDTokenClaims *SyncMap[string, jwt.MapClaims]
 
 	// hooks
-	hookUserInfo  func(email string) jwt.MapClaims
-	fakeCoderd    func(req *http.Request) (*http.Response, error)
-	hookOnRefresh func(email string) error
+	// hookValidRedirectURL can be used to reject a redirect url from the
+	// IDP -> Application. Almost all IDPs have the concept of
+	// "Authorized Redirect URLs". This can be used to emulate that.
+	hookValidRedirectURL func(redirectURL string) error
+	hookUserInfo         func(email string) jwt.MapClaims
+	fakeCoderd           func(req *http.Request) (*http.Response, error)
+	hookOnRefresh        func(email string) error
 	// Custom authentication for the client. This is useful if you want
 	// to test something like PKI auth vs a client_secret.
 	hookAuthenticateClient func(t testing.TB, req *http.Request) (url.Values, error)
@@ -74,6 +78,12 @@ type FakeIDP struct {
 
 type FakeIDPOpt func(idp *FakeIDP)
 
+func WithAuthorizedRedirectURL(hook func(redirectURL string) error) func(*FakeIDP) {
+	return func(f *FakeIDP) {
+		f.hookValidRedirectURL = hook
+	}
+}
+
 // WithRefreshHook is called when a refresh token is used. The email is
 // the email of the user that is being refreshed assuming the claims are correct.
 func WithRefreshHook(hook func(email string) error) func(*FakeIDP) {
@@ -421,6 +431,8 @@ func (f *FakeIDP) httpHandler(t testing.TB) http.Handler {
 	// This endpoint is required to initialize the OIDC provider.
 	// It is used to get the OIDC configuration.
 	mux.Get("/.well-known/openid-configuration", func(rw http.ResponseWriter, r *http.Request) {
+		f.logger.Info(r.Context(), "HTTP OIDC Config", slog.F("url", r.URL.String()))
+
 		_ = json.NewEncoder(rw).Encode(f.provider)
 	})
 
@@ -429,19 +441,19 @@ func (f *FakeIDP) httpHandler(t testing.TB) http.Handler {
 	// w/e and clicking "Allow". They will be redirected back to the redirect
 	// when this is done.
 	mux.Handle(authorizePath, http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
-		f.logger.Info(r.Context(), "HTTP Call Authorize", slog.F("url", string(r.URL.String())))
+		f.logger.Info(r.Context(), "HTTP Call Authorize", slog.F("url", r.URL.String()))
 
 		clientID := r.URL.Query().Get("client_id")
-		if clientID != f.clientID {
-			t.Errorf("unexpected client_id %q", clientID)
+		if !assert.Equal(t, f.clientID, clientID, "unexpected client_id") {
 			http.Error(rw, "invalid client_id", http.StatusBadRequest)
+			return
 		}
 
 		redirectURI := r.URL.Query().Get("redirect_uri")
 		state := r.URL.Query().Get("state")
 
 		scope := r.URL.Query().Get("scope")
-		_ = scope
+		assert.NotEmpty(t, scope, "scope is empty")
 
 		responseType := r.URL.Query().Get("response_type")
 		switch responseType {
@@ -456,10 +468,17 @@ func (f *FakeIDP) httpHandler(t testing.TB) http.Handler {
 			return
 		}
 
+		err := f.hookValidRedirectURL(redirectURI)
+		if err != nil {
+			t.Errorf("not authorized redirect_uri by custom hook %q: %s", redirectURI, err.Error())
+			http.Error(rw, fmt.Sprintf("invalid redirect_uri: %s", err.Error()), http.StatusBadRequest)
+			return
+		}
+
 		ru, err := url.Parse(redirectURI)
 		if err != nil {
-			t.Errorf("invalid redirect_uri %q", redirectURI)
-			http.Error(rw, "invalid redirect_uri", http.StatusBadRequest)
+			t.Errorf("invalid redirect_uri %q: %s", redirectURI, err.Error())
+			http.Error(rw, fmt.Sprintf("invalid redirect_uri: %s", err.Error()), http.StatusBadRequest)
 			return
 		}
 
@@ -573,6 +592,7 @@ func (f *FakeIDP) httpHandler(t testing.TB) http.Handler {
 		token, err := f.authenticateBearerTokenRequest(t, r)
 		f.logger.Info(r.Context(), "HTTP Call UserInfo",
 			slog.Error(err),
+			slog.F("url", r.URL.String()),
 		)
 		if err != nil {
 			http.Error(rw, fmt.Sprintf("invalid user info request: %s", err.Error()), http.StatusBadRequest)
