Repository: supabase/gotrue
Pull request #1096 (2023-04-28 19:06:16)

feat: allow updating saml providers `metadata_xml`
## What kind of change does this PR introduce?

Fix the `PUT` `/admin/providers/<id>` endpoint not committing the SAMLProvider changes to the database when updating the metadata XML (resulting in a no-op).

## What is the current behavior?

Updates to the metadata XML via the `/admin/providers/<id>` should be reflected on the `saml_provider` database object.

## What is the new behavior?

The provider metadata XML can now be correctly updated.



Patch:
diff --git a/README.md b/README.md
index c6e711a9..63432aa6 100644
--- a/README.md
+++ b/README.md
@@ -707,7 +707,7 @@ Or Messagebird credentials, which can be obtained in the [Dashboard](https://das
 
 ### CAPTCHA
 
-- If enabled, CAPTCHA will check the request body for the `hcaptcha_token` field and make a verification request to the CAPTCHA provider.
+- If enabled, CAPTCHA will check the request body for the `captcha_token` field and make a verification request to the CAPTCHA provider.
 
 `SECURITY_CAPTCHA_ENABLED` - `string`
 
@@ -715,12 +715,12 @@ Whether captcha middleware is enabled
 
 `SECURITY_CAPTCHA_PROVIDER` - `string`
 
-for now the only option supported is: `hcaptcha`
+for now the only options supported are: `hcaptcha` and `turnstile`
 
 - `SECURITY_CAPTCHA_SECRET` - `string`
 - `SECURITY_CAPTCHA_TIMEOUT` - `string`
 
-Retrieve from hcaptcha account
+Retrieve from hcaptcha or turnstile account
 
 ### Reauthentication
 
diff --git a/internal/api/external.go b/internal/api/external.go
index ceb15353..4351d994 100644
--- a/internal/api/external.go
+++ b/internal/api/external.go
@@ -225,7 +225,10 @@ func (a *API) internalExternalProviderCallback(w http.ResponseWriter, r *http.Re
 	if flowState != nil {
 		// This means that the callback is using PKCE
 		// Set the flowState.AuthCode to the query param here
-		rurl = a.prepPKCERedirectURL(rurl, flowState.AuthCode)
+		rurl, err = a.prepPKCERedirectURL(rurl, flowState.AuthCode)
+		if err != nil {
+			return err
+		}
 	} else if token != nil {
 		q := url.Values{}
 		q.Set("provider_token", providerAccessToken)
diff --git a/internal/api/middleware.go b/internal/api/middleware.go
index 9f9d5c5e..8300a19f 100644
--- a/internal/api/middleware.go
+++ b/internal/api/middleware.go
@@ -158,13 +158,13 @@ func (a *API) verifyCaptcha(w http.ResponseWriter, req *http.Request) (context.C
 		return ctx, nil
 	}
 
-	verificationResult, err := security.VerifyRequest(req, strings.TrimSpace(config.Security.Captcha.Secret))
+	verificationResult, err := security.VerifyRequest(req, strings.TrimSpace(config.Security.Captcha.Secret), config.Security.Captcha.Provider)
 	if err != nil {
-		return nil, internalServerError("hCaptcha verification process failed").WithInternalError(err)
+		return nil, internalServerError("captcha verification process failed").WithInternalError(err)
 	}
 
 	if !verificationResult.Success {
-		return nil, badRequestError("hCaptcha protection: request disallowed (%s)", strings.Join(verificationResult.ErrorCodes, ", "))
+		return nil, badRequestError("captcha protection: request disallowed (%s)", strings.Join(verificationResult.ErrorCodes, ", "))
 
 	}
 
diff --git a/internal/api/ssoadmin.go b/internal/api/ssoadmin.go
index e720d03e..19f4211e 100644
--- a/internal/api/ssoadmin.go
+++ b/internal/api/ssoadmin.go
@@ -279,6 +279,7 @@ func (a *API) adminSSOProvidersUpdate(w http.ResponseWriter, r *http.Request) er
 	}
 
 	modified := false
+	updateSAMLProvider := false
 
 	provider := getSSOProvider(ctx)
 
@@ -298,6 +299,7 @@ func (a *API) adminSSOProvidersUpdate(w http.ResponseWriter, r *http.Request) er
 		}
 
 		provider.SAMLProvider.MetadataXML = string(rawMetadata)
+		updateSAMLProvider = true
 		modified = true
 	}
 
@@ -360,7 +362,7 @@ func (a *API) adminSSOProvidersUpdate(w http.ResponseWriter, r *http.Request) er
 				}
 			}
 
-			if updateAttributeMapping {
+			if updateAttributeMapping || updateSAMLProvider {
 				if terr := tx.Eager().Update(&provider.SAMLProvider); terr != nil {
 					return terr
 				}
diff --git a/internal/api/verify.go b/internal/api/verify.go
index 0df7b24b..a508c96f 100644
--- a/internal/api/verify.go
+++ b/internal/api/verify.go
@@ -167,7 +167,10 @@ func (a *API) verifyGet(w http.ResponseWriter, r *http.Request) error {
 		q.Set("type", params.Type)
 		rurl = token.AsRedirectURL(rurl, q)
 	} else if isPKCEFlow(flowType) {
-		rurl = a.prepPKCERedirectURL(rurl, authCode)
+		rurl, err = a.prepPKCERedirectURL(rurl, authCode)
+		if err != nil {
+			return err
+		}
 	}
 	http.Redirect(w, r, rurl, http.StatusSeeOther)
 	return nil
@@ -370,10 +373,15 @@ func (a *API) prepRedirectURL(message string, rurl string) string {
 	return rurl + "#" + q.Encode()
 }
 
-func (a *API) prepPKCERedirectURL(rurl, code string) string {
-	q := url.Values{}
+func (a *API) prepPKCERedirectURL(rurl, code string) (string, error) {
+	u, err := url.Parse(rurl)
+	if err != nil {
+		return "", err
+	}
+	q := u.Query()
 	q.Set("code", code)
-	return rurl + "?" + q.Encode()
+	u.RawQuery = q.Encode()
+	return u.String(), nil
 }
 
 func (a *API) emailChangeVerify(r *http.Request, ctx context.Context, conn *storage.Connection, params *VerifyParams, user *models.User) (*models.User, error) {
diff --git a/internal/conf/configuration.go b/internal/conf/configuration.go
index 8579932c..b104e6eb 100644
--- a/internal/conf/configuration.go
+++ b/internal/conf/configuration.go
@@ -235,14 +235,14 @@ func (c *CaptchaConfiguration) Validate() error {
 		return nil
 	}
 
-	if c.Provider != "hcaptcha" {
+	if c.Provider != "hcaptcha" && c.Provider != "turnstile" {
 		return fmt.Errorf("unsupported captcha provider: %s", c.Provider)
 	}
 
 	c.Secret = strings.TrimSpace(c.Secret)
 
 	if c.Secret == "" {
-		return errors.New("hcaptcha provider secret is empty")
+		return errors.New("captcha provider secret is empty")
 	}
 
 	return nil
diff --git a/internal/models/errors.go b/internal/models/errors.go
index d51a7170..47513719 100644
--- a/internal/models/errors.go
+++ b/internal/models/errors.go
@@ -11,10 +11,6 @@ func IsNotFoundError(err error) bool {
 		return true
 	case RefreshTokenNotFoundError, *RefreshTokenNotFoundError:
 		return true
-	case InstanceNotFoundError, *InstanceNotFoundError:
-		return true
-	case TotpSecretNotFoundError, *TotpSecretNotFoundError:
-		return true
 	case IdentityNotFoundError, *IdentityNotFoundError:
 		return true
 	case ChallengeNotFoundError, *ChallengeNotFoundError:
@@ -65,13 +61,6 @@ func (e RefreshTokenNotFoundError) Error() string {
 	return "Refresh Token not found"
 }
 
-// InstanceNotFoundError represents when an instance is not found.
-type InstanceNotFoundError struct{}
-
-func (e InstanceNotFoundError) Error() string {
-	return "Instance not found"
-}
-
 // FactorNotFoundError represents when a user is not found.
 type FactorNotFoundError struct{}
 
@@ -86,12 +75,6 @@ func (e ChallengeNotFoundError) Error() string {
 	return "Challenge not found"
 }
 
-type TotpSecretNotFoundError struct{}
-
-func (e TotpSecretNotFoundError) Error() string {
-	return "Totp Secret not found"
-}
-
 // SSOProviderNotFoundError represents an error when a SSO Provider can't be
 // found.
 type SSOProviderNotFoundError struct{}
diff --git a/internal/security/hcaptcha.go b/internal/security/captcha.go
similarity index 67%
rename from internal/security/hcaptcha.go
rename to internal/security/captcha.go
index 91d4cac1..9525eb58 100644
--- a/internal/security/hcaptcha.go
+++ b/internal/security/captcha.go
@@ -10,6 +10,7 @@ import (
 	"strings"
 	"time"
 
+	"fmt"
 	"github.com/pkg/errors"
 	"github.com/supabase/gotrue/internal/utilities"
 )
@@ -44,7 +45,7 @@ func init() {
 	Client = &http.Client{Timeout: defaultTimeout}
 }
 
-func VerifyRequest(r *http.Request, secretKey string) (VerificationResponse, error) {
+func VerifyRequest(r *http.Request, secretKey, captchaProvider string) (VerificationResponse, error) {
 	bodyBytes, err := utilities.GetBodyBytes(r)
 	if err != nil {
 		return VerificationResponse{}, err
@@ -59,38 +60,53 @@ func VerifyRequest(r *http.Request, secretKey string) (VerificationResponse, err
 	captchaResponse := strings.TrimSpace(requestBody.Security.Token)
 
 	if captchaResponse == "" {
-		return VerificationResponse{}, errors.New("no hCaptcha response (captcha_token) found in request")
+		return VerificationResponse{}, errors.New("no captcha response (captcha_token) found in request")
 	}
 
 	clientIP := utilities.GetIPAddress(r)
+	captchaURL, err := GetCaptchaURL(captchaProvider)
+	if err != nil {
+		return VerificationResponse{}, err
+	}
 
-	return verifyCaptchaCode(captchaResponse, secretKey, clientIP)
+	return verifyCaptchaCode(captchaResponse, secretKey, clientIP, captchaURL)
 }
 
-func verifyCaptchaCode(token string, secretKey string, clientIP string) (VerificationResponse, error) {
+func verifyCaptchaCode(token, secretKey, clientIP, captchaURL string) (VerificationResponse, error) {
 	data := url.Values{}
 	data.Set("secret", secretKey)
 	data.Set("response", token)
 	data.Set("remoteip", clientIP)
 	// TODO (darora): pipe through sitekey
 
-	r, err := http.NewRequest("POST", "https://hcaptcha.com/siteverify", strings.NewReader(data.Encode()))
+	r, err := http.NewRequest("POST", captchaURL, strings.NewReader(data.Encode()))
 	if err != nil {
-		return VerificationResponse{}, errors.Wrap(err, "couldn't initialize request object for hcaptcha check")
+		return VerificationResponse{}, errors.Wrap(err, "couldn't initialize request object for captcha check")
 	}
 	r.Header.Add("Content-Type", "application/x-www-form-urlencoded")
 	r.Header.Add("Content-Length", strconv.Itoa(len(data.Encode())))
 	res, err := Client.Do(r)
 	if err != nil {
-		return VerificationResponse{}, errors.Wrap(err, "failed to verify hcaptcha response")
+		return VerificationResponse{}, errors.Wrap(err, "failed to verify captcha response")
 	}
 	defer utilities.SafeClose(res.Body)
 
 	var verificationResponse VerificationResponse
 
 	if err := json.NewDecoder(res.Body).Decode(&verificationResponse); err != nil {
-		return VerificationResponse{}, errors.Wrap(err, "failed to decode hcaptcha response: not JSON")
+		return VerificationResponse{}, errors.Wrap(err, "failed to decode captcha response: not JSON")
 	}
 
 	return verificationResponse, nil
 }
+
+func GetCaptchaURL(captchaProvider string) (string, error) {
+	switch captchaProvider {
+	case "hcaptcha":
+		return "https://hcaptcha.com/siteverify", nil
+	case "turnstile":
+		return "https://challenges.cloudflare.com/turnstile/v0/siteverify", nil
+	default:
+		return "", fmt.Errorf("captcha Provider %q could not be found", captchaProvider)
+	}
+}


Test patch:
diff --git a/internal/api/middleware_test.go b/internal/api/middleware_test.go
index ada47cc2..0d1ad20e 100644
--- a/internal/api/middleware_test.go
+++ b/internal/api/middleware_test.go
@@ -17,8 +17,9 @@ import (
 )
 
 const (
-	HCaptchaSecret   string = "0x0000000000000000000000000000000000000000"
-	HCaptchaResponse string = "10000000-aaaa-bbbb-cccc-000000000001"
+	HCaptchaSecret         string = "0x0000000000000000000000000000000000000000"
+	CaptchaResponse        string = "10000000-aaaa-bbbb-cccc-000000000001"
+	TurnstileCaptchaSecret string = "1x0000000000000000000000000000000AA"
 )
 
 type MiddlewareTestSuite struct {
@@ -42,8 +43,6 @@ func TestMiddlewareFunctions(t *testing.T) {
 
 func (ts *MiddlewareTestSuite) TestVerifyCaptchaValid() {
 	ts.Config.Security.Captcha.Enabled = true
-	ts.Config.Security.Captcha.Provider = "hcaptcha"
-	ts.Config.Security.Captcha.Secret = HCaptchaSecret
 
 	adminClaims := &GoTrueClaims{
 		Role: "supabase_admin",
@@ -51,22 +50,44 @@ func (ts *MiddlewareTestSuite) TestVerifyCaptchaValid() {
 	adminJwt, err := jwt.NewWithClaims(jwt.SigningMethodHS256, adminClaims).SignedString([]byte(ts.Config.JWT.Secret))
 	require.NoError(ts.T(), err)
 	cases := []struct {
-		desc          string
-		adminJwt      string
-		captcha_token string
+		desc             string
+		adminJwt         string
+		captcha_token    string
+		captcha_provider string
 	}{
 		{
 			"Valid captcha response",
 			"",
-			HCaptchaResponse,
+			CaptchaResponse,
+			"hcaptcha",
+		},
+		{
+			"Valid captcha response",
+			"",
+			CaptchaResponse,
+			"turnstile",
+		},
+		{
+			"Ignore captcha if admin role is present",
+			adminJwt,
+			"",
+			"hcaptcha",
 		},
 		{
 			"Ignore captcha if admin role is present",
 			adminJwt,
 			"",
+			"turnstile",
 		},
 	}
 	for _, c := range cases {
+		ts.Config.Security.Captcha.Provider = c.captcha_provider
+		if c.captcha_provider == "turnstile" {
+			ts.Config.Security.Captcha.Secret = TurnstileCaptchaSecret
+		} else if c.captcha_provider == "hcaptcha" {
+			ts.Config.Security.Captcha.Secret = HCaptchaSecret
+		}
+
 		var buffer bytes.Buffer
 		require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]interface{}{
 			"email":    "test@example.com",
@@ -122,7 +143,17 @@ func (ts *MiddlewareTestSuite) TestVerifyCaptchaInvalid() {
 				Secret:   "test",
 			},
 			http.StatusBadRequest,
-			"hCaptcha protection: request disallowed (not-using-dummy-secret)",
+			"captcha protection: request disallowed (not-using-dummy-secret)",
+		},
+		{
+			"Captcha validation failed",
+			&conf.CaptchaConfiguration{
+				Enabled:  true,
+				Provider: "turnstile",
+				Secret:   "anothertest",
+			},
+			http.StatusBadRequest,
+			"captcha protection: request disallowed (invalid-input-secret)",
 		},
 	}
 	for _, c := range cases {
@@ -133,7 +164,7 @@ func (ts *MiddlewareTestSuite) TestVerifyCaptchaInvalid() {
 				"email":    "test@example.com",
 				"password": "secret",
 				"gotrue_meta_security": map[string]interface{}{
-					"captcha_token": HCaptchaResponse,
+					"captcha_token": CaptchaResponse,
 				},
 			}))
 			req := httptest.NewRequest(http.MethodPost, "http://localhost", &buffer)