Repository: supabase/gotrue
Pull request #1310 (2023-11-16 16:52:03)

fix: allow transactions to be committed while returning a custom error
## What kind of change does this PR introduce?
* This PR introduces a new error type `CommitWithError` that allows one to commit a transaction but also return an error. 
* This is useful in situations where `GOTRUE_MAILER_ALLOW_UNVERIFIED_EMAIL_SIGN_INS="false"` since oauth users with an unverified email will require email confirmation before being allowed to sign-in. If the transaction doesn't get committed, the new user doesn't get created and the email confirmation sent out will not be mapped to a user in the database.


Patch:
diff --git a/internal/api/external.go b/internal/api/external.go
index 2afbd89a..ea25c2af 100644
--- a/internal/api/external.go
+++ b/internal/api/external.go
@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"net/http"
 	"net/url"
+	"strconv"
 	"strings"
 	"time"
 
@@ -141,9 +142,6 @@ func (a *API) ExternalProviderCallback(w http.ResponseWriter, r *http.Request) e
 	return nil
 }
 
-var errEmailVerificationRequired = errors.New("email verification required")
-var errEmailConfirmationSent = errors.New("email confirmation sent")
-
 func (a *API) internalExternalProviderCallback(w http.ResponseWriter, r *http.Request) error {
 	ctx := r.Context()
 	db := a.db.WithContext(ctx)
@@ -187,7 +185,6 @@ func (a *API) internalExternalProviderCallback(w http.ResponseWriter, r *http.Re
 
 	var user *models.User
 	var token *AccessTokenResponse
-	var errEmailVerificationRequiredOrConfirmationSent error
 	err = db.Transaction(func(tx *storage.Connection) error {
 		var terr error
 		inviteToken := getInviteToken(ctx)
@@ -197,12 +194,6 @@ func (a *API) internalExternalProviderCallback(w http.ResponseWriter, r *http.Re
 			}
 		} else {
 			if user, terr = a.createAccountFromExternalIdentity(tx, r, userData, providerType); terr != nil {
-				if errors.Is(terr, errEmailVerificationRequired) || errors.Is(terr, errEmailConfirmationSent) {
-					// we don't want the transaction to be rolled back because the user is created
-					// but not returned as further action is necessary
-					errEmailVerificationRequiredOrConfirmationSent = terr
-					return nil
-				}
 				return terr
 			}
 		}
@@ -222,29 +213,11 @@ func (a *API) internalExternalProviderCallback(w http.ResponseWriter, r *http.Re
 		return nil
 	})
 
-	rurl := a.getExternalRedirectURL(r)
 	if err != nil {
 		return err
 	}
 
-	if errEmailVerificationRequiredOrConfirmationSent != nil {
-		flowType := models.ImplicitFlow
-		if flowState != nil {
-			flowType = models.PKCEFlow
-		}
-		msg := fmt.Sprintf("Unverified email with %v. ", providerType)
-		if errors.Is(errEmailVerificationRequiredOrConfirmationSent, errEmailVerificationRequired) {
-			msg += fmt.Sprintf("Verify the email with %v in order to sign in", providerType)
-		} else if errors.Is(errEmailVerificationRequiredOrConfirmationSent, errEmailConfirmationSent) {
-			msg += fmt.Sprintf("A confirmation email has been sent to your %v email", providerType)
-		}
-		rurl, err = a.prepErrorRedirectURL(unauthorizedError(msg), w, r, rurl, flowType)
-		if err != nil {
-			return err
-		}
-		http.Redirect(w, r, rurl, http.StatusFound)
-	}
-
+	rurl := a.getExternalRedirectURL(r)
 	if flowState != nil {
 		// This means that the callback is using PKCE
 		// Set the flowState.AuthCode to the query param here
@@ -390,9 +363,9 @@ func (a *API) createAccountFromExternalIdentity(tx *storage.Connection, r *http.
 			}
 			if !config.Mailer.AllowUnverifiedEmailSignIns {
 				if emailConfirmationSent {
-					return nil, errEmailConfirmationSent
+					return nil, storage.NewCommitWithError(unauthorizedError(fmt.Sprintf("Unverified email with %v. A confirmation email has been sent to your %v email", providerType, providerType)))
 				}
-				return nil, errEmailVerificationRequired
+				return nil, storage.NewCommitWithError(unauthorizedError(fmt.Sprintf("Unverified email with %v. Verify the email with %v in order to sign in", providerType, providerType)))
 			}
 		}
 	} else {
@@ -563,6 +536,19 @@ func (a *API) redirectErrors(handler apiHandler, w http.ResponseWriter, r *http.
 	if err != nil {
 		q := getErrorQueryString(err, errorID, log, u.Query())
 		u.RawQuery = q.Encode()
+
+		// TODO: deprecate returning error details in the query fragment
+		hq := url.Values{}
+		if q.Get("error") != "" {
+			hq.Set("error", q.Get("error"))
+		}
+		if q.Get("error_description") != "" {
+			hq.Set("error_description", q.Get("error_description"))
+		}
+		if q.Get("error_code") != "" {
+			hq.Set("error_code", q.Get("error_code"))
+		}
+		u.Fragment = hq.Encode()
 		http.Redirect(w, r, u.String(), http.StatusFound)
 	}
 }
@@ -583,6 +569,7 @@ func getErrorQueryString(err error, errorID string, log logrus.FieldLogger, q ur
 			log.WithError(e.Cause()).Info(e.Error())
 		}
 		q.Set("error_description", e.Message)
+		q.Set("error_code", strconv.Itoa(e.Code))
 	case *OAuthError:
 		q.Set("error", e.Err)
 		q.Set("error_description", e.Description)
diff --git a/internal/api/token_oidc.go b/internal/api/token_oidc.go
index 39692c6c..45214531 100644
--- a/internal/api/token_oidc.go
+++ b/internal/api/token_oidc.go
@@ -4,7 +4,6 @@ import (
 	"context"
 	"crypto/sha256"
 	"encoding/json"
-	"errors"
 	"fmt"
 	"net/http"
 
@@ -201,10 +200,6 @@ func (a *API) IdTokenGrant(ctx context.Context, w http.ResponseWriter, r *http.R
 
 		user, terr = a.createAccountFromExternalIdentity(tx, r, userData, providerType)
 		if terr != nil {
-			if errors.Is(terr, errEmailVerificationRequired) {
-				return nil
-			}
-
 			return terr
 		}
 
@@ -215,7 +210,12 @@ func (a *API) IdTokenGrant(ctx context.Context, w http.ResponseWriter, r *http.R
 
 		return nil
 	}); err != nil {
-		return oauthError("server_error", "Internal Server Error").WithInternalError(err)
+		switch err.(type) {
+		case *storage.CommitWithError:
+			return err
+		default:
+			return oauthError("server_error", "Internal Server Error").WithInternalError(err)
+		}
 	}
 
 	return sendJSON(w, http.StatusOK, token)
diff --git a/internal/storage/dial.go b/internal/storage/dial.go
index 58d5859f..ffd90af3 100644
--- a/internal/storage/dial.go
+++ b/internal/storage/dial.go
@@ -112,11 +112,41 @@ func registerOpenTelemetryDatabaseStats(db *pop.Connection) {
 	}
 }
 
+type CommitWithError struct {
+	Err error
+}
+
+func (e *CommitWithError) Error() string {
+	return e.Err.Error()
+}
+
+func (e *CommitWithError) Cause() error {
+	return e.Err
+}
+
+// NewCommitWithError creates an error that can be returned in a pop transaction
+// without rolling back the transaction. This should only be used in cases where
+// you want the transaction to commit but return an error message to the user.
+func NewCommitWithError(err error) *CommitWithError {
+	return &CommitWithError{Err: err}
+}
+
 func (c *Connection) Transaction(fn func(*Connection) error) error {
 	if c.TX == nil {
-		return c.Connection.Transaction(func(tx *pop.Connection) error {
-			return fn(&Connection{tx})
-		})
+		var returnErr error
+		if terr := c.Connection.Transaction(func(tx *pop.Connection) error {
+			err := fn(&Connection{tx})
+			switch err.(type) {
+			case *CommitWithError:
+				returnErr = err
+				return nil
+			default:
+				return err
+			}
+		}); terr != nil {
+			return terr
+		}
+		return returnErr
 	}
 	return fn(c)
 }


Test patch:
diff --git a/internal/api/external_github_test.go b/internal/api/external_github_test.go
index 55f579b0..a7a3e975 100644
--- a/internal/api/external_github_test.go
+++ b/internal/api/external_github_test.go
@@ -276,12 +276,7 @@ func (ts *ExternalTestSuite) TestSignupExternalGitHubErrorWhenVerifiedFalse() {
 
 	u := performAuthorization(ts, "github", code, "")
 
-	v, err := url.ParseQuery(u.Fragment)
-	ts.Require().NoError(err)
-	ts.Equal("unauthorized_client", v.Get("error"))
-	ts.Equal("401", v.Get("error_code"))
-	ts.Equal("Unverified email with github. A confirmation email has been sent to your github email", v.Get("error_description"))
-	assertAuthorizationFailure(ts, u, "", "", "")
+	assertAuthorizationFailure(ts, u, "Unverified email with github. A confirmation email has been sent to your github email", "unauthorized_client", "")
 }
 
 func (ts *ExternalTestSuite) TestSignupExternalGitHubErrorWhenUserBanned() {
diff --git a/internal/api/external_kakao_test.go b/internal/api/external_kakao_test.go
index 0552181c..92358a7a 100644
--- a/internal/api/external_kakao_test.go
+++ b/internal/api/external_kakao_test.go
@@ -206,12 +206,7 @@ func (ts *ExternalTestSuite) TestSignupExternalKakaoErrorWhenVerifiedFalse() {
 
 	u := performAuthorization(ts, "kakao", code, "")
 
-	v, err := url.ParseQuery(u.Fragment)
-	ts.Require().NoError(err)
-	ts.Equal("unauthorized_client", v.Get("error"))
-	ts.Equal("401", v.Get("error_code"))
-	ts.Equal("Unverified email with kakao. A confirmation email has been sent to your kakao email", v.Get("error_description"))
-	assertAuthorizationFailure(ts, u, "", "", "")
+	assertAuthorizationFailure(ts, u, "Unverified email with kakao. A confirmation email has been sent to your kakao email", "unauthorized_client", "")
 }
 
 func (ts *ExternalTestSuite) TestSignupExternalKakaoErrorWhenUserBanned() {
diff --git a/internal/storage/dial_test.go b/internal/storage/dial_test.go
index 8dbd1f49..90763e5e 100644
--- a/internal/storage/dial_test.go
+++ b/internal/storage/dial_test.go
@@ -1,10 +1,12 @@
 package storage
 
 import (
+	"errors"
 	"testing"
 
 	"github.com/gofrs/uuid"
 	"github.com/stretchr/testify/require"
+	"github.com/supabase/gotrue/internal/conf"
 )
 
 type TestUser struct {
@@ -26,3 +28,33 @@ func TestGetExcludedColumns_InvalidName(t *testing.T) {
 	_, err := getExcludedColumns(u, "adsf")
 	require.Error(t, err)
 }
+
+func TestTransaction(t *testing.T) {
+	apiTestConfig := "../../hack/test.env"
+	config, err := conf.LoadGlobal(apiTestConfig)
+	require.NoError(t, err)
+	conn, err := Dial(config)
+	require.NoError(t, err)
+	require.NotNil(t, conn)
+
+	defer func() {
+		// clean up the test table created
+		require.NoError(t, conn.RawQuery("drop table if exists test").Exec(), "Error removing table")
+	}()
+
+	commitWithError := NewCommitWithError(errors.New("commit with error"))
+	err = conn.Transaction(func(tx *Connection) error {
+		require.NoError(t, tx.RawQuery("create table if not exists test()").Exec(), "Error saving creating test table")
+		return commitWithError
+	})
+	require.Error(t, err)
+	require.ErrorIs(t, err, commitWithError)
+
+	type TestData struct{}
+
+	// check that transaction is still being committed despite returning an error above
+	data := []TestData{}
+	err = conn.RawQuery("select * from test").All(&data)
+	require.NoError(t, err)
+	require.Empty(t, data)
+}