Repository: supabase/gotrue
Pull request #1030 (2023-04-10 09:42:10)

refactor: explain automatic reuse detection
## What kind of change does this PR introduce?
* Add comments to explain how the reuse detection works


Patch:
diff --git a/internal/api/external.go b/internal/api/external.go
index 70b591a9..1700c609 100644
--- a/internal/api/external.go
+++ b/internal/api/external.go
@@ -45,9 +45,8 @@ func (a *API) ExternalProviderRedirect(w http.ResponseWriter, r *http.Request) e
 	query := r.URL.Query()
 	providerType := query.Get("provider")
 	scopes := query.Get("scopes")
-	flowType := query.Get("flow_type")
 	codeChallenge := query.Get("code_challenge")
-	codeChallengeMethodParam := query.Get("code_challenge_method")
+	codeChallengeMethod := query.Get("code_challenge_method")
 
 	p, err := a.Provider(ctx, providerType, scopes)
 	if err != nil {
@@ -68,25 +67,18 @@ func (a *API) ExternalProviderRedirect(w http.ResponseWriter, r *http.Request) e
 	redirectURL := a.getRedirectURLOrReferrer(r, query.Get("redirect_to"))
 	log := observability.GetLogEntry(r)
 	log.WithField("provider", providerType).Info("Redirecting to external provider")
+	if err := validatePKCEParams(codeChallengeMethod, codeChallenge); err != nil {
+		return err
+	}
+	flowType := getFlowFromChallenge(codeChallenge)
 
 	flowStateID := ""
-	switch true {
-	case flowType == PKCE && (codeChallenge == "" || codeChallengeMethodParam == ""):
-		return badRequestError("code challenge and code challenge method are required to perform PKCE")
-	case flowType == PKCE && codeChallenge != "" && codeChallengeMethodParam != "":
-		var codeChallengeMethod models.CodeChallengeMethod
-		switch strings.ToLower(codeChallengeMethodParam) {
-		case "plain":
-			codeChallengeMethod = models.Plain
-		case "s256":
-			codeChallengeMethod = models.SHA256
-		default:
-			return badRequestError("code challenge method is unsupported")
-		}
-		if valid, err := isValidCodeChallenge(codeChallenge); !valid {
+	var codeChallengeMethodType models.CodeChallengeMethod
+	if flowType == models.PKCEFlow {
+		if codeChallengeMethodType, err = models.ParseCodeChallengeMethod(codeChallengeMethod); err != nil {
 			return err
 		}
-		flowState, err := models.NewFlowState(providerType, codeChallenge, codeChallengeMethod, models.OAuth)
+		flowState, err := models.NewFlowState(providerType, codeChallenge, codeChallengeMethodType, models.OAuth)
 		if err != nil {
 			return err
 		}
@@ -94,12 +86,6 @@ func (a *API) ExternalProviderRedirect(w http.ResponseWriter, r *http.Request) e
 			return err
 		}
 		flowStateID = flowState.ID.String()
-	// Implicit Flow
-	case flowType == "":
-		break
-	default:
-		// Should not reach here
-		return badRequestError("invalid request parameters")
 	}
 
 	token := jwt.NewWithClaims(jwt.SigningMethodHS256, ExternalProviderClaims{
diff --git a/internal/api/pkce.go b/internal/api/pkce.go
index 6025af87..762d42e4 100644
--- a/internal/api/pkce.go
+++ b/internal/api/pkce.go
@@ -20,7 +20,6 @@ var codeChallengePattern = regexp.MustCompile("^[a-zA-Z._~0-9-]+$")
 
 func isValidCodeChallenge(codeChallenge string) (bool, error) {
 	// See RFC 7636 Section 4.2: https://www.rfc-editor.org/rfc/rfc7636#section-4.2
-
 	switch codeChallengeLength := len(codeChallenge); {
 	case codeChallengeLength < MinCodeChallengeLength, codeChallengeLength > MaxCodeChallengeLength:
 		return false, badRequestError("code challenge has to be between %v and %v characters", MinCodeChallengeLength, MaxCodeChallengeLength)
diff --git a/internal/api/sms_provider/twilio.go b/internal/api/sms_provider/twilio.go
index d8be83a5..673b23a8 100644
--- a/internal/api/sms_provider/twilio.go
+++ b/internal/api/sms_provider/twilio.go
@@ -56,25 +56,27 @@ func NewTwilioProvider(config conf.TwilioProviderConfiguration) (SmsProvider, er
 
 func (t *TwilioProvider) SendMessage(phone string, message string, channel string) error {
 	switch channel {
-	case SMSProvider:
-		return t.SendSms(phone, message)
-	case WhatsappProvider:
-		return t.SendWhatsappMessage(phone, message)
+	case SMSProvider, WhatsappProvider:
+		return t.SendSms(phone, message, channel)
 	default:
 		return fmt.Errorf("channel type %q is not supported for Twilio", channel)
 	}
 }
 
-// Send a Whatsapp message containing the OTP with Twilio's API
-// TODO (J0) Merge with SendSms once stable
-func (t *TwilioProvider) SendWhatsappMessage(phone string, message string) error {
+// Send an SMS containing the OTP with Twilio's API
+func (t *TwilioProvider) SendSms(phone, message, channel string) error {
+	sender := t.Config.MessageServiceSid
+	receiver := "+" + phone
+	if channel == WhatsappProvider {
+		receiver = channel + ":" + receiver
+		sender = channel + ":" + sender
+	}
 	body := url.Values{
-		"To":      {"whatsapp:" + "+" + phone}, // twilio api requires "+" extension to be included
-		"Channel": {"whatsapp"},
-		"From":    {"whatsapp:" + t.Config.MessageServiceSid},
+		"To":      {receiver}, // twilio api requires "+" extension to be included
+		"Channel": {channel},
+		"From":    {sender},
 		"Body":    {message},
 	}
-
 	client := &http.Client{Timeout: defaultTimeout}
 	r, err := http.NewRequest("POST", t.APIPath, strings.NewReader(body.Encode()))
 	if err != nil {
@@ -94,49 +96,6 @@ func (t *TwilioProvider) SendWhatsappMessage(phone string, message string) error
 		}
 		return resp
 	}
-
-	// validate sms status
-	resp := &SmsStatus{}
-	derr := json.NewDecoder(res.Body).Decode(resp)
-	if derr != nil {
-		return derr
-	}
-
-	if resp.Status == "failed" || resp.Status == "undelivered" {
-		return fmt.Errorf("twilio error: %v %v", resp.ErrorMessage, resp.ErrorCode)
-	}
-
-	return nil
-}
-
-// Send an SMS containing the OTP with Twilio's API
-func (t *TwilioProvider) SendSms(phone string, message string) error {
-	body := url.Values{
-		"To":      {"+" + phone}, // twilio api requires "+" extension to be included
-		"Channel": {"sms"},
-		"From":    {t.Config.MessageServiceSid},
-		"Body":    {message},
-	}
-	client := &http.Client{Timeout: defaultTimeout}
-	r, err := http.NewRequest("POST", t.APIPath, strings.NewReader(body.Encode()))
-	if err != nil {
-		return err
-	}
-	r.Header.Add("Content-Type", "application/x-www-form-urlencoded")
-	r.SetBasicAuth(t.Config.AccountSid, t.Config.AuthToken)
-	res, err := client.Do(r)
-	if err != nil {
-		return err
-	}
-	if res.StatusCode/100 != 2 {
-		resp := &twilioErrResponse{}
-		if err := json.NewDecoder(res.Body).Decode(resp); err != nil {
-			return err
-		}
-		return resp
-	}
-	defer utilities.SafeClose(res.Body)
-
 	// validate sms status
 	resp := &SmsStatus{}
 	derr := json.NewDecoder(res.Body).Decode(resp)
diff --git a/internal/api/token.go b/internal/api/token.go
index 2391f304..f214bfcc 100644
--- a/internal/api/token.go
+++ b/internal/api/token.go
@@ -311,23 +311,30 @@ func (a *API) RefreshTokenGrant(ctx context.Context, w http.ResponseWriter, r *h
 		}
 	}
 
-	var newToken *models.RefreshToken
+	var latestValidToken *models.RefreshToken
 	if token.Revoked {
 		a.clearCookieTokens(config, w)
+		/**
+		For a revoked refresh token to be reused, it has to fulfil 2 conditions:
+			1. The revoked refresh token has to be the parent of the most recently issued valid refresh token.
+			2. The revoked refresh token has to fall within the reuse interval.
+		**/
 		err = db.Transaction(func(tx *storage.Connection) error {
-			validToken, terr := models.GetValidChildToken(tx, token)
+			validChildToken, terr := models.GetValidChildToken(tx, token)
 			if terr != nil {
 				if errors.Is(terr, models.RefreshTokenNotFoundError{}) {
-					// revoked token has no descendants
+					// revoked token has no valid child token
 					return nil
 				}
 				return terr
 			}
-			// check if token is the last previous revoked token
-			if validToken.Parent == storage.NullString(token.Token) {
+			// check if the parent of the child token is the revoked token being used
+			// validChildToken.Parent and token.Token will never be null here
+			if validChildToken.Parent == storage.NullString(token.Token) {
 				refreshTokenReuseWindow := token.UpdatedAt.Add(time.Second * time.Duration(config.Security.RefreshTokenReuseInterval))
+				// check if the revoked token falls within the reuse interval
 				if time.Now().Before(refreshTokenReuseWindow) {
-					newToken = validToken
+					latestValidToken = validChildToken
 				}
 			}
 			return nil
@@ -336,7 +343,7 @@ func (a *API) RefreshTokenGrant(ctx context.Context, w http.ResponseWriter, r *h
 			return internalServerError("Error validating reuse interval").WithInternalError(err)
 		}
 
-		if newToken == nil {
+		if latestValidToken == nil {
 			if config.Security.RefreshTokenRotationEnabled {
 				// Revoke all tokens in token family
 				err = db.Transaction(func(tx *storage.Connection) error {
@@ -363,13 +370,13 @@ func (a *API) RefreshTokenGrant(ctx context.Context, w http.ResponseWriter, r *h
 			return terr
 		}
 
-		if newToken == nil {
-			newToken, terr = models.GrantRefreshTokenSwap(r, tx, user, token)
+		if latestValidToken == nil {
+			latestValidToken, terr = models.GrantRefreshTokenSwap(r, tx, user, token)
 			if terr != nil {
 				return internalServerError(terr.Error())
 			}
 		}
-		tokenString, terr = generateAccessToken(tx, user, newToken.SessionId, time.Second*time.Duration(config.JWT.Exp), config.JWT.Secret)
+		tokenString, terr = generateAccessToken(tx, user, latestValidToken.SessionId, time.Second*time.Duration(config.JWT.Exp), config.JWT.Secret)
 
 		if terr != nil {
 			return internalServerError("error generating jwt token").WithInternalError(terr)
@@ -379,7 +386,7 @@ func (a *API) RefreshTokenGrant(ctx context.Context, w http.ResponseWriter, r *h
 			Token:        tokenString,
 			TokenType:    "bearer",
 			ExpiresIn:    config.JWT.Exp,
-			RefreshToken: newToken.Token,
+			RefreshToken: latestValidToken.Token,
 			User:         user,
 		}
 		if terr = a.setCookieTokens(config, newTokenResponse, false, w); terr != nil {
diff --git a/internal/models/flow_state.go b/internal/models/flow_state.go
index d9eb4df7..d0049640 100644
--- a/internal/models/flow_state.go
+++ b/internal/models/flow_state.go
@@ -38,8 +38,8 @@ const (
 	Plain
 )
 
-func (authMethod CodeChallengeMethod) String() string {
-	switch authMethod {
+func (codeChallengeMethod CodeChallengeMethod) String() string {
+	switch codeChallengeMethod {
 	case SHA256:
 		return "s256"
 	case Plain:
@@ -48,14 +48,14 @@ func (authMethod CodeChallengeMethod) String() string {
 	return ""
 }
 
-func ParseCodeChallengeMethod(authMethod string) (CodeChallengeMethod, error) {
-	switch strings.ToLower(authMethod) {
+func ParseCodeChallengeMethod(codeChallengeMethod string) (CodeChallengeMethod, error) {
+	switch strings.ToLower(codeChallengeMethod) {
 	case "s256":
 		return SHA256, nil
 	case "plain":
 		return Plain, nil
 	}
-	return 0, fmt.Errorf("unsupported code_challenge method %q", authMethod)
+	return 0, fmt.Errorf("unsupported code_challenge method %q", codeChallengeMethod)
 }
 
 type FlowType int
diff --git a/openapi.yaml b/openapi.yaml
index fe187279..d0360d05 100644
--- a/openapi.yaml
+++ b/openapi.yaml
@@ -280,14 +280,6 @@ paths:
           schema:
             type: string
             format: uri
-        - name: flow_type
-          in: query
-          description: (Optional) OAuth flow used for authorization. When not specified, `implicit` is used. Given that `pkce` is a more secure option, it is recommended that users of this API use `pkce`.
-          schema:
-            type: string
-            enum:
-              - pkce
-              - implicit
         - name: code_challenge_method
           in: query
           description: (Optional) Method used to encrypt the verifier. Can be `plain` (no transformation) or `s256` (where SHA-256 is used). It is always recommended that `s256` is used.
@@ -330,7 +322,6 @@ paths:
                 value:
                   email: user@example.com
                   password: password1
-                  flow_type: pkce
                   code_challenge_method: s256
                   code_challenge: elU6u5zyqQT2f92GRQUq6PautAeNDf4DQPayyR0ek_c&
             schema:


Test patch:
diff --git a/internal/api/external_test.go b/internal/api/external_test.go
index e57ec277..524e6df7 100644
--- a/internal/api/external_test.go
+++ b/internal/api/external_test.go
@@ -77,7 +77,7 @@ func performAuthorizationRequest(ts *ExternalTestSuite, provider string, inviteT
 }
 
 func performPKCEAuthorizationRequest(ts *ExternalTestSuite, provider, codeChallenge, codeChallengeMethod string) *httptest.ResponseRecorder {
-	authorizeURL := "http://localhost/authorize?flow_type=pkce&provider=" + provider
+	authorizeURL := "http://localhost/authorize?provider=" + provider
 	if codeChallenge != "" {
 		authorizeURL = authorizeURL + "&code_challenge=" + codeChallenge + "&code_challenge_method=" + codeChallengeMethod
 	}
diff --git a/internal/api/sms_provider/sms_provider_test.go b/internal/api/sms_provider/sms_provider_test.go
index 08ba0895..4192d6bb 100644
--- a/internal/api/sms_provider/sms_provider_test.go
+++ b/internal/api/sms_provider/sms_provider_test.go
@@ -127,7 +127,7 @@ func (ts *SmsProviderTestSuite) TestTwilioSendSms() {
 
 	for _, c := range cases {
 		ts.Run(c.Desc, func() {
-			err = twilioProvider.SendSms(phone, message)
+			err = twilioProvider.SendSms(phone, message, SMSProvider)
 			require.Equal(ts.T(), c.ExpectedError, err)
 		})
 	}
diff --git a/internal/api/token_test.go b/internal/api/token_test.go
index f6acc131..6ac92449 100644
--- a/internal/api/token_test.go
+++ b/internal/api/token_test.go
@@ -380,7 +380,7 @@ func (ts *TokenTestSuite) TestMagicLinkPKCESignIn() {
 	require.NoError(ts.T(), err)
 
 	// Verify OTP
-	requestUrl := fmt.Sprintf("http://localhost/verify?type=%v&token=%v&flow_type=%v", "magiclink", u.RecoveryToken, models.PKCEFlow.String())
+	requestUrl := fmt.Sprintf("http://localhost/verify?type=%v&token=%v", "magiclink", u.RecoveryToken)
 	req = httptest.NewRequest(http.MethodGet, requestUrl, &buffer)
 	req.Header.Set("Content-Type", "application/json")
 
diff --git a/internal/api/verify_test.go b/internal/api/verify_test.go
index c5085469..ec4c9ead 100644
--- a/internal/api/verify_test.go
+++ b/internal/api/verify_test.go
@@ -71,7 +71,8 @@ func (ts *VerifyTestSuite) TestVerifyPasswordRecovery() {
 		{
 			desc: "PKCE Flow",
 			body: map[string]interface{}{
-				"email":                 testEmail,
+				"email": testEmail,
+				// Code Challenge needs to be at least 43 characters long
 				"code_challenge":        "6b151854-cc15-4e29-8db7-3d3a9f15b3066b151854-cc15-4e29-8db7-3d3a9f15b306",
 				"code_challenge_method": models.SHA256.String(),
 			},