Repository: supabase/gotrue
Pull request #1335 (2023-12-06 11:46:23)

feat: use `DO` blocks around SQL statements in migrations
Migrations that contain `SELECT`, `INSERT`, `UPDATE`, `DELETE` that are not already wrapped in a `DO` block, are wrapped. This is because anything that executes in a `DO` block _does not get counted as activity_ on the database.

kangmingtay: @hf let's make sure that this helps to solve the issue of counting auth migrations as activity before we merge it in or it's just an additional maintenance overhead 

Patch:
diff --git a/internal/api/hooks.go b/internal/api/hooks.go
index 5c73981d..2fd31983 100644
--- a/internal/api/hooks.go
+++ b/internal/api/hooks.go
@@ -6,6 +6,7 @@ import (
 	"crypto/sha256"
 	"encoding/hex"
 	"encoding/json"
+	"fmt"
 	"io"
 	"net"
 	"net/http"
@@ -17,6 +18,7 @@ import (
 	jwt "github.com/golang-jwt/jwt"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
+	"github.com/supabase/auth/internal/hooks"
 
 	"github.com/supabase/auth/internal/conf"
 	"github.com/supabase/auth/internal/models"
@@ -287,3 +289,139 @@ type connectionWatcher struct {
 func (c *connectionWatcher) GotConn(_ httptrace.GotConnInfo) {
 	c.gotConn = true
 }
+
+func (a *API) runHook(ctx context.Context, name string, input, output any) ([]byte, error) {
+	db := a.db.WithContext(ctx)
+
+	request, err := json.Marshal(input)
+	if err != nil {
+		panic(err)
+	}
+
+	var response []byte
+	if err := db.Transaction(func(tx *storage.Connection) error {
+		// We rely on Postgres timeouts to ensure the function doesn't overrun
+		if terr := tx.RawQuery(fmt.Sprintf("set local statement_timeout TO '%d';", hooks.DefaultTimeout)).Exec(); terr != nil {
+			return terr
+		}
+
+		if terr := tx.RawQuery(fmt.Sprintf("select %s(?);", name), request).First(&response); terr != nil {
+			return terr
+		}
+
+		// reset the timeout
+		if terr := tx.RawQuery("set local statement_timeout TO default;").Exec(); terr != nil {
+			return terr
+		}
+
+		return nil
+	}); err != nil {
+		return nil, err
+	}
+
+	if err := json.Unmarshal(response, output); err != nil {
+		return response, err
+	}
+
+	return response, nil
+}
+
+func (a *API) invokeHook(ctx context.Context, input, output any) error {
+	config := a.config
+	switch input.(type) {
+	case *hooks.MFAVerificationAttemptInput:
+		hookOutput, ok := output.(*hooks.MFAVerificationAttemptOutput)
+		if !ok {
+			panic("output should be *hooks.MFAVerificationAttemptOutput")
+		}
+
+		if _, err := a.runHook(ctx, config.Hook.MFAVerificationAttempt.HookName, input, output); err != nil {
+			return internalServerError("Error invoking MFA verification hook.").WithInternalError(err)
+		}
+
+		if hookOutput.IsError() {
+			httpCode := hookOutput.HookError.HTTPCode
+
+			if httpCode == 0 {
+				httpCode = http.StatusInternalServerError
+			}
+
+			httpError := &HTTPError{
+				Code:    httpCode,
+				Message: hookOutput.HookError.Message,
+			}
+
+			return httpError.WithInternalError(&hookOutput.HookError)
+		}
+
+		return nil
+	case *hooks.PasswordVerificationAttemptInput:
+		hookOutput, ok := output.(*hooks.PasswordVerificationAttemptOutput)
+		if !ok {
+			panic("output should be *hooks.PasswordVerificationAttemptOutput")
+		}
+
+		if _, err := a.runHook(ctx, config.Hook.PasswordVerificationAttempt.HookName, input, output); err != nil {
+			return internalServerError("Error invoking password verification hook.").WithInternalError(err)
+		}
+
+		if hookOutput.IsError() {
+			httpCode := hookOutput.HookError.HTTPCode
+
+			if httpCode == 0 {
+				httpCode = http.StatusInternalServerError
+			}
+
+			httpError := &HTTPError{
+				Code:    httpCode,
+				Message: hookOutput.HookError.Message,
+			}
+
+			return httpError.WithInternalError(&hookOutput.HookError)
+		}
+
+		return nil
+	case *hooks.CustomAccessTokenInput:
+		hookOutput, ok := output.(*hooks.CustomAccessTokenOutput)
+		if !ok {
+			panic("output should be *hooks.CustomAccessTokenOutput")
+		}
+
+		if _, err := a.runHook(ctx, config.Hook.CustomAccessToken.HookName, input, output); err != nil {
+			return internalServerError("Error invoking access token hook.").WithInternalError(err)
+		}
+
+		if hookOutput.IsError() {
+			httpCode := hookOutput.HookError.HTTPCode
+
+			if httpCode == 0 {
+				httpCode = http.StatusInternalServerError
+			}
+
+			httpError := &HTTPError{
+				Code:    httpCode,
+				Message: hookOutput.HookError.Message,
+			}
+
+			return httpError.WithInternalError(&hookOutput.HookError)
+		}
+		if err := validateTokenClaims(hookOutput.Claims); err != nil {
+			httpCode := hookOutput.HookError.HTTPCode
+
+			if httpCode == 0 {
+				httpCode = http.StatusInternalServerError
+			}
+
+			httpError := &HTTPError{
+				Code:    httpCode,
+				Message: err.Error(),
+			}
+
+			return httpError
+		}
+		return nil
+
+	default:
+		panic("unknown hook input type")
+	}
+}
diff --git a/internal/api/mfa.go b/internal/api/mfa.go
index dc6aa44f..03fb00e6 100644
--- a/internal/api/mfa.go
+++ b/internal/api/mfa.go
@@ -2,7 +2,6 @@ package api
 
 import (
 	"bytes"
-	"context"
 	"encoding/json"
 	"fmt"
 	"net/http"
@@ -132,13 +131,16 @@ func (a *API) EnrollFactor(w http.ResponseWriter, r *http.Request) error {
 	}
 	svgData.End()
 
-	factor, err := models.NewFactor(user, params.FriendlyName, params.FactorType, models.FactorStateUnverified, key.Secret())
-	if err != nil {
-		return internalServerError("database error creating factor").WithInternalError(err)
-	}
+	factor := models.NewFactor(user, params.FriendlyName, params.FactorType, models.FactorStateUnverified, key.Secret())
+
 	err = a.db.Transaction(func(tx *storage.Connection) error {
 		if terr := tx.Create(factor); terr != nil {
+			pgErr := utilities.NewPostgresError(terr)
+			if pgErr.IsUniqueConstraintViolated() {
+				return badRequestError(fmt.Sprintf("a factor with the friendly name %q for this user likely already exists", factor.FriendlyName))
+			}
 			return terr
+
 		}
 		if terr := models.NewAuditLogEntry(r, tx, user, models.EnrollFactorAction, r.RemoteAddr, map[string]interface{}{
 			"factor_id": factor.ID,
@@ -171,12 +173,9 @@ func (a *API) ChallengeFactor(w http.ResponseWriter, r *http.Request) error {
 	user := getUser(ctx)
 	factor := getFactor(ctx)
 	ipAddress := utilities.GetIPAddress(r)
-	challenge, err := models.NewChallenge(factor, ipAddress)
-	if err != nil {
-		return internalServerError("Database error creating challenge").WithInternalError(err)
-	}
+	challenge := models.NewChallenge(factor, ipAddress)
 
-	err = a.db.Transaction(func(tx *storage.Connection) error {
+	err := a.db.Transaction(func(tx *storage.Connection) error {
 		if terr := tx.Create(challenge); terr != nil {
 			return terr
 		}
@@ -198,142 +197,6 @@ func (a *API) ChallengeFactor(w http.ResponseWriter, r *http.Request) error {
 	})
 }
 
-func (a *API) runHook(ctx context.Context, name string, input, output any) ([]byte, error) {
-	db := a.db.WithContext(ctx)
-
-	request, err := json.Marshal(input)
-	if err != nil {
-		panic(err)
-	}
-
-	var response []byte
-	if err := db.Transaction(func(tx *storage.Connection) error {
-		// We rely on Postgres timeouts to ensure the function doesn't overrun
-		if terr := tx.RawQuery(fmt.Sprintf("set local statement_timeout TO '%d';", hooks.DefaultTimeout)).Exec(); terr != nil {
-			return terr
-		}
-
-		if terr := tx.RawQuery(fmt.Sprintf("select %s(?);", name), request).First(&response); terr != nil {
-			return terr
-		}
-
-		// reset the timeout
-		if terr := tx.RawQuery("set local statement_timeout TO default;").Exec(); terr != nil {
-			return terr
-		}
-
-		return nil
-	}); err != nil {
-		return nil, err
-	}
-
-	if err := json.Unmarshal(response, output); err != nil {
-		return response, err
-	}
-
-	return response, nil
-}
-
-func (a *API) invokeHook(ctx context.Context, input, output any) error {
-	config := a.config
-	switch input.(type) {
-	case *hooks.MFAVerificationAttemptInput:
-		hookOutput, ok := output.(*hooks.MFAVerificationAttemptOutput)
-		if !ok {
-			panic("output should be *hooks.MFAVerificationAttemptOutput")
-		}
-
-		if _, err := a.runHook(ctx, config.Hook.MFAVerificationAttempt.HookName, input, output); err != nil {
-			return internalServerError("Error invoking MFA verification hook.").WithInternalError(err)
-		}
-
-		if hookOutput.IsError() {
-			httpCode := hookOutput.HookError.HTTPCode
-
-			if httpCode == 0 {
-				httpCode = http.StatusInternalServerError
-			}
-
-			httpError := &HTTPError{
-				Code:    httpCode,
-				Message: hookOutput.HookError.Message,
-			}
-
-			return httpError.WithInternalError(&hookOutput.HookError)
-		}
-
-		return nil
-	case *hooks.PasswordVerificationAttemptInput:
-		hookOutput, ok := output.(*hooks.PasswordVerificationAttemptOutput)
-		if !ok {
-			panic("output should be *hooks.PasswordVerificationAttemptOutput")
-		}
-
-		if _, err := a.runHook(ctx, config.Hook.PasswordVerificationAttempt.HookName, input, output); err != nil {
-			return internalServerError("Error invoking password verification hook.").WithInternalError(err)
-		}
-
-		if hookOutput.IsError() {
-			httpCode := hookOutput.HookError.HTTPCode
-
-			if httpCode == 0 {
-				httpCode = http.StatusInternalServerError
-			}
-
-			httpError := &HTTPError{
-				Code:    httpCode,
-				Message: hookOutput.HookError.Message,
-			}
-
-			return httpError.WithInternalError(&hookOutput.HookError)
-		}
-
-		return nil
-	case *hooks.CustomAccessTokenInput:
-		hookOutput, ok := output.(*hooks.CustomAccessTokenOutput)
-		if !ok {
-			panic("output should be *hooks.CustomAccessTokenOutput")
-		}
-
-		if _, err := a.runHook(ctx, config.Hook.CustomAccessToken.HookName, input, output); err != nil {
-			return internalServerError("Error invoking access token hook.").WithInternalError(err)
-		}
-
-		if hookOutput.IsError() {
-			httpCode := hookOutput.HookError.HTTPCode
-
-			if httpCode == 0 {
-				httpCode = http.StatusInternalServerError
-			}
-
-			httpError := &HTTPError{
-				Code:    httpCode,
-				Message: hookOutput.HookError.Message,
-			}
-
-			return httpError.WithInternalError(&hookOutput.HookError)
-		}
-		if err := validateTokenClaims(hookOutput.Claims); err != nil {
-			httpCode := hookOutput.HookError.HTTPCode
-
-			if httpCode == 0 {
-				httpCode = http.StatusInternalServerError
-			}
-
-			httpError := &HTTPError{
-				Code:    httpCode,
-				Message: err.Error(),
-			}
-
-			return httpError
-		}
-		return nil
-
-	default:
-		panic("unknown hook input type")
-	}
-}
-
 func (a *API) VerifyFactor(w http.ResponseWriter, r *http.Request) error {
 	var err error
 	ctx := r.Context()
diff --git a/internal/models/challenge.go b/internal/models/challenge.go
index c69a1cb5..99758e63 100644
--- a/internal/models/challenge.go
+++ b/internal/models/challenge.go
@@ -22,7 +22,7 @@ func (Challenge) TableName() string {
 	return tableName
 }
 
-func NewChallenge(factor *Factor, ipAddress string) (*Challenge, error) {
+func NewChallenge(factor *Factor, ipAddress string) *Challenge {
 	id := uuid.Must(uuid.NewV4())
 
 	challenge := &Challenge{
@@ -30,7 +30,7 @@ func NewChallenge(factor *Factor, ipAddress string) (*Challenge, error) {
 		FactorID:  factor.ID,
 		IPAddress: ipAddress,
 	}
-	return challenge, nil
+	return challenge
 }
 
 func FindChallengeByChallengeID(tx *storage.Connection, challengeID uuid.UUID) (*Challenge, error) {
diff --git a/internal/models/factor.go b/internal/models/factor.go
index e4162a0b..5af45772 100644
--- a/internal/models/factor.go
+++ b/internal/models/factor.go
@@ -124,7 +124,7 @@ func (Factor) TableName() string {
 	return tableName
 }
 
-func NewFactor(user *User, friendlyName string, factorType string, state FactorState, secret string) (*Factor, error) {
+func NewFactor(user *User, friendlyName string, factorType string, state FactorState, secret string) *Factor {
 	id := uuid.Must(uuid.NewV4())
 
 	factor := &Factor{
@@ -135,7 +135,7 @@ func NewFactor(user *User, friendlyName string, factorType string, state FactorS
 		Secret:       secret,
 		FactorType:   factorType,
 	}
-	return factor, nil
+	return factor
 }
 
 // FindFactorsByUser returns all factors belonging to a user ordered by timestamp
diff --git a/internal/utilities/postgres.go b/internal/utilities/postgres.go
index 08a57b6a..4d7fde8d 100644
--- a/internal/utilities/postgres.go
+++ b/internal/utilities/postgres.go
@@ -34,6 +34,10 @@ func NewPostgresError(err error) *PostgresError {
 
 	return nil
 }
+func (pg *PostgresError) IsUniqueConstraintViolated() bool {
+	// See https://www.postgresql.org/docs/current/errcodes-appendix.html for list of error codes
+	return pg.Code == "23505"
+}
 
 // isPubliclyAccessiblePostgresError checks if the Postgres error should be
 // made accessible.
diff --git a/migrations/20221125140132_backfill_email_identity.up.sql b/migrations/20221125140132_backfill_email_identity.up.sql
index 497acdd7..cd06425c 100644
--- a/migrations/20221125140132_backfill_email_identity.up.sql
+++ b/migrations/20221125140132_backfill_email_identity.up.sql
@@ -1,7 +1,11 @@
 -- backfill the auth.identities column by adding an email identity 
 -- for all auth.users with an email and password 
 
-insert into {{ index .Options "Namespace" }}.identities (id, user_id, identity_data, provider, last_sign_in_at, created_at, updated_at)
-select id, id as user_id, jsonb_build_object('sub', id, 'email', email) as identity_data, 'email' as provider, null as last_sign_in_at, '2022-11-25' as created_at, '2022-11-25' as updated_at
-from {{ index .Options "Namespace" }}.users as users
-where encrypted_password != '' and email is not null and not exists(select user_id from {{ index .Options "Namespace" }}.identities where user_id = users.id);
+do $$
+begin
+	insert into {{ index .Options "Namespace" }}.identities (id, user_id, identity_data, provider, last_sign_in_at, created_at, updated_at)
+	select id, id as user_id, jsonb_build_object('sub', id, 'email', email) as identity_data, 'email' as provider, null as last_sign_in_at, '2022-11-25' as created_at, '2022-11-25' as updated_at
+	from {{ index .Options "Namespace" }}.users as users
+	where encrypted_password != '' and email is not null and not exists(select user_id from {{ index .Options "Namespace" }}.identities where user_id = users.id);
+end;
+$$;
diff --git a/migrations/20221208132122_backfill_email_last_sign_in_at.up.sql b/migrations/20221208132122_backfill_email_last_sign_in_at.up.sql
index ee54bfc5..19ec79e9 100644
--- a/migrations/20221208132122_backfill_email_last_sign_in_at.up.sql
+++ b/migrations/20221208132122_backfill_email_last_sign_in_at.up.sql
@@ -1,5 +1,7 @@
 -- previous backfill migration left last_sign_in_at to be null, which broke some projects
 
+do $$
+begin
 update {{ index .Options "Namespace" }}.identities
   set last_sign_in_at = '2022-11-25'
   where
@@ -8,3 +10,4 @@ update {{ index .Options "Namespace" }}.identities
     updated_at = '2022-11-25' and
     provider = 'email' and
     id = user_id::text;
+end $$;
diff --git a/migrations/20221215195800_add_identities_email_column.up.sql b/migrations/20221215195800_add_identities_email_column.up.sql
index b66a9a2f..eb60334c 100644
--- a/migrations/20221215195800_add_identities_email_column.up.sql
+++ b/migrations/20221215195800_add_identities_email_column.up.sql
@@ -1,9 +1,12 @@
-update
-  {{ index .Options "Namespace" }}.identities as identities
-set
-  identity_data = identity_data || jsonb_build_object('email', (select email from {{ index .Options "Namespace" }}.users where id = identities.user_id)),
-  updated_at = '2022-11-25'
-where identities.provider = 'email' and identity_data->>'email' is null;
+do $$
+begin
+  update
+    {{ index .Options "Namespace" }}.identities as identities
+  set
+    identity_data = identity_data || jsonb_build_object('email', (select email from {{ index .Options "Namespace" }}.users where id = identities.user_id)),
+    updated_at = '2022-11-25'
+  where identities.provider = 'email' and identity_data->>'email' is null;
+end $$;
 
 alter table only {{ index .Options "Namespace" }}.identities
   add column if not exists email text generated always as (lower(identity_data->>'email')) stored;
diff --git a/migrations/20230131181311_backfill_invite_identities.up.sql b/migrations/20230131181311_backfill_invite_identities.up.sql
index e7bfb044..2fcb3582 100644
--- a/migrations/20230131181311_backfill_invite_identities.up.sql
+++ b/migrations/20230131181311_backfill_invite_identities.up.sql
@@ -1,6 +1,9 @@
 -- backfills the missing email identity for invited users
 
-insert into {{ index .Options "Namespace" }}.identities (id, user_id, identity_data, provider, last_sign_in_at, created_at, updated_at)
-select id, id as user_id, jsonb_build_object('sub', id, 'email', email) as identity_data, 'email' as provider, null as last_sign_in_at, '2023-01-25' as created_at, '2023-01-25' as updated_at
-from {{ index .Options "Namespace" }}.users as users
-where invited_at is not null and not exists (select user_id from {{ index .Options "Namespace" }}.identities where user_id = users.id and provider = 'email');
+do $$
+begin
+	insert into {{ index .Options "Namespace" }}.identities (id, user_id, identity_data, provider, last_sign_in_at, created_at, updated_at)
+	select id, id as user_id, jsonb_build_object('sub', id, 'email', email) as identity_data, 'email' as provider, null as last_sign_in_at, '2023-01-25' as created_at, '2023-01-25' as updated_at
+	from {{ index .Options "Namespace" }}.users as users
+	where invited_at is not null and not exists (select user_id from {{ index .Options "Namespace" }}.identities where user_id = users.id and provider = 'email');
+end $$;


Test patch:
diff --git a/internal/api/admin_test.go b/internal/api/admin_test.go
index b7a361dd..35447bcf 100644
--- a/internal/api/admin_test.go
+++ b/internal/api/admin_test.go
@@ -721,9 +721,7 @@ func (ts *AdminTestSuite) TestAdminUserDeleteFactor() {
 	require.NoError(ts.T(), err, "Error making new user")
 	require.NoError(ts.T(), ts.API.db.Create(u), "Error creating user")
 
-	f, err := models.NewFactor(u, "testSimpleName", models.TOTP, models.FactorStateVerified, "secretkey")
-
-	require.NoError(ts.T(), err, "Error creating test factor model")
+	f := models.NewFactor(u, "testSimpleName", models.TOTP, models.FactorStateVerified, "secretkey")
 	require.NoError(ts.T(), ts.API.db.Create(f), "Error saving new test factor")
 
 	// Setup request
@@ -746,8 +744,7 @@ func (ts *AdminTestSuite) TestAdminUserGetFactors() {
 	require.NoError(ts.T(), err, "Error making new user")
 	require.NoError(ts.T(), ts.API.db.Create(u), "Error creating user")
 
-	f, err := models.NewFactor(u, "testSimpleName", models.TOTP, models.FactorStateUnverified, "secretkey")
-	require.NoError(ts.T(), err, "Error creating test factor model")
+	f := models.NewFactor(u, "testSimpleName", models.TOTP, models.FactorStateUnverified, "secretkey")
 	require.NoError(ts.T(), ts.API.db.Create(f), "Error saving new test factor")
 
 	// Setup request
@@ -768,8 +765,7 @@ func (ts *AdminTestSuite) TestAdminUserUpdateFactor() {
 	require.NoError(ts.T(), err, "Error making new user")
 	require.NoError(ts.T(), ts.API.db.Create(u), "Error creating user")
 
-	f, err := models.NewFactor(u, "testSimpleName", models.TOTP, models.FactorStateUnverified, "secretkey")
-	require.NoError(ts.T(), err, "Error creating test factor model")
+	f := models.NewFactor(u, "testSimpleName", models.TOTP, models.FactorStateUnverified, "secretkey")
 	require.NoError(ts.T(), ts.API.db.Create(f), "Error saving new test factor")
 
 	var cases = []struct {
diff --git a/internal/api/mfa_test.go b/internal/api/mfa_test.go
index 9aa57605..5fe7f0bc 100644
--- a/internal/api/mfa_test.go
+++ b/internal/api/mfa_test.go
@@ -57,8 +57,7 @@ func (ts *MFATestSuite) SetupTest() {
 	require.NoError(ts.T(), err, "Error creating test user model")
 	require.NoError(ts.T(), ts.API.db.Create(u), "Error saving new test user")
 	// Create Factor
-	f, err := models.NewFactor(u, "test_factor", models.TOTP, models.FactorStateUnverified, "secretkey")
-	require.NoError(ts.T(), err, "Error creating test factor model")
+	f := models.NewFactor(u, "test_factor", models.TOTP, models.FactorStateUnverified, "secretkey")
 	require.NoError(ts.T(), ts.API.db.Create(f), "Error saving new test factor")
 	// Create corresponding session
 	s, err := models.NewSession()
@@ -120,7 +119,6 @@ func (ts *MFATestSuite) TestEnrollFactor() {
 			issuer:       ts.TestDomain,
 			expectedCode: http.StatusBadRequest,
 		},
-
 		{
 			desc:         "TOTP: Factor has friendly name",
 			friendlyName: testFriendlyName,
@@ -158,6 +156,22 @@ func (ts *MFATestSuite) TestEnrollFactor() {
 			}
 		})
 	}
+}
+
+func (ts *MFATestSuite) TestDuplicateEnrollsReturnExpectedMessage() {
+	friendlyName := "mary"
+	token, _, err := ts.API.generateAccessToken(context.Background(), ts.API.db, ts.TestUser, nil, models.TOTPSignIn)
+	require.NoError(ts.T(), err)
+	_ = performEnrollFlow(ts, token, friendlyName, models.TOTP, "https://issuer.com", http.StatusOK)
+	response := performEnrollFlow(ts, token, friendlyName, models.TOTP, "https://issuer.com", http.StatusBadRequest)
+
+	var errorResponse HTTPError
+	err = json.NewDecoder(response.Body).Decode(&errorResponse)
+	require.NoError(ts.T(), err)
+
+	// Convert the response body to a string and check for the expected error message
+	expectedErrorMessage := fmt.Sprintf("a factor with the friendly name %q for this user likely already exists", friendlyName)
+	require.Contains(ts.T(), errorResponse.Message, expectedErrorMessage)
 
 }
 
@@ -221,8 +235,7 @@ func (ts *MFATestSuite) TestMFAVerifyFactor() {
 			req := httptest.NewRequest(http.MethodPost, fmt.Sprintf("/factors/%s/verify", f.ID), &buffer)
 			testIPAddress := utilities.GetIPAddress(req)
 			req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token))
-			c, err := models.NewChallenge(f, testIPAddress)
-			require.NoError(ts.T(), err, "Error creating test Challenge model")
+			c := models.NewChallenge(f, testIPAddress)
 			require.NoError(ts.T(), ts.API.db.Create(c), "Error saving new test challenge")
 			if !v.validChallenge {
 				// Set challenge creation so that it has expired in present time.
@@ -650,5 +663,4 @@ func cleanupHook(ts *MFATestSuite, hookName string) {
 	cleanupHookSQL := fmt.Sprintf("drop function if exists %s", hookName)
 	err := ts.API.db.RawQuery(cleanupHookSQL).Exec()
 	require.NoError(ts.T(), err)
-
 }
diff --git a/internal/models/factor_test.go b/internal/models/factor_test.go
index 63f04e0a..eb5dccb9 100644
--- a/internal/models/factor_test.go
+++ b/internal/models/factor_test.go
@@ -50,8 +50,7 @@ func (ts *FactorTestSuite) createFactor() *Factor {
 	err = ts.db.Create(user)
 	require.NoError(ts.T(), err)
 
-	factor, err := NewFactor(user, "asimplename", TOTP, FactorStateUnverified, "topsecret")
-	require.NoError(ts.T(), err)
+	factor := NewFactor(user, "asimplename", TOTP, FactorStateUnverified, "topsecret")
 
 	err = ts.db.Create(factor)
 	require.NoError(ts.T(), err)
@@ -63,8 +62,7 @@ func (ts *FactorTestSuite) TestUpdateStatus() {
 	u, err := NewUser("", "", "", "", nil)
 	require.NoError(ts.T(), err)
 
-	f, err := NewFactor(u, "", TOTP, FactorStateUnverified, "some-secret")
-	require.NoError(ts.T(), err)
+	f := NewFactor(u, "", TOTP, FactorStateUnverified, "some-secret")
 	require.NoError(ts.T(), f.UpdateStatus(ts.db, newFactorStatus))
 	require.Equal(ts.T(), newFactorStatus.String(), f.Status)
 }
@@ -74,8 +72,7 @@ func (ts *FactorTestSuite) TestUpdateFriendlyName() {
 	u, err := NewUser("", "", "", "", nil)
 	require.NoError(ts.T(), err)
 
-	f, err := NewFactor(u, "A1B2C3", TOTP, FactorStateUnverified, "some-secret")
-	require.NoError(ts.T(), err)
+	f := NewFactor(u, "A1B2C3", TOTP, FactorStateUnverified, "some-secret")
 	require.NoError(ts.T(), f.UpdateFriendlyName(ts.db, newSimpleName))
 	require.Equal(ts.T(), newSimpleName, f.FriendlyName)
 }
@@ -84,8 +81,7 @@ func (ts *FactorTestSuite) TestEncodedFactorDoesNotLeakSecret() {
 	u, err := NewUser("", "", "", "", nil)
 	require.NoError(ts.T(), err)
 
-	f, err := NewFactor(u, "A1B2C3", TOTP, FactorStateUnverified, "some-secret")
-	require.NoError(ts.T(), err)
+	f := NewFactor(u, "A1B2C3", TOTP, FactorStateUnverified, "some-secret")
 	encodedFactor, err := json.Marshal(f)
 	require.NoError(ts.T(), err)
 	decodedFactor := Factor{}