lowered the default mfa duration and reorganized internal record pre/post handling

apis/record_auth_with_otp_test.go

@@ -406,10 +406,10 @@ func TestRecordAuthWithOTP(t *testing.T) {
 				"OnModelCreate":             1,
 				"OnModelCreateExecute":      1,
 				"OnModelAfterCreateSuccess": 1,
-				// OTP delete + 2 ExternalAuth delete
+				// 2 record OTPs + 2 ExternalAuths delete
-				"OnModelDelete":             3,
+				"OnModelDelete":             4,
-				"OnModelDeleteExecute":      3,
+				"OnModelDeleteExecute":      4,
-				"OnModelAfterDeleteSuccess": 3,
+				"OnModelAfterDeleteSuccess": 4,
 				// user verified update
 				"OnModelUpdate":             1,
 				"OnModelUpdateExecute":      1,
@@ -419,9 +419,9 @@ func TestRecordAuthWithOTP(t *testing.T) {
 				"OnRecordCreate":             1,
 				"OnRecordCreateExecute":      1,
 				"OnRecordAfterCreateSuccess": 1,
-				"OnRecordDelete":             3,
+				"OnRecordDelete":             4,
-				"OnRecordDeleteExecute":      3,
+				"OnRecordDeleteExecute":      4,
-				"OnRecordAfterDeleteSuccess": 3,
+				"OnRecordAfterDeleteSuccess": 4,
 				"OnRecordUpdate":             1,
 				"OnRecordUpdateExecute":      1,
 				"OnRecordAfterUpdateSuccess": 1,
@@ -436,6 +436,15 @@ func TestRecordAuthWithOTP(t *testing.T) {
 					t.Fatal("Expected the user to be marked as verified")
 				}
 
+				// ensure that all pre-existing OTPs are cleared
+				otps, err := app.FindAllOTPsByRecord(user)
+				if err != nil {
+					t.Fatal(err)
+				}
+				if len(otps) > 0 {
+					t.Fatalf("Expected all OTPs to be cleared, found %d", len(otps))
+				}
+
 				// ensure that all pre-existing OAuth2 links are cleared
 				externalAuths, err := app.FindAllExternalAuthsByRecord(user)
 				if err != nil {

core/collection_model_auth_options.go

@@ -60,7 +60,7 @@ func (m *Collection) setDefaultAuthOptions() {
 		},
 		MFA: MFAConfig{
 			Enabled:  false,
-			Duration: 1800, // 30min
+			Duration: 600, // 10min
 		},
 		OTP: OTPConfig{
 			Enabled:       false,

core/external_auth_model.go

@@ -137,4 +137,42 @@ func (app *BaseApp) registerExternalAuthHooks() {
 		},
 		Priority: 99,
 	})
+
+	// delete all pre-existing external auths on verified upgrade
+	app.OnRecordUpdateExecute().Bind(&hook.Handler[*RecordEvent]{
+		Func: func(e *RecordEvent) error {
+			if !e.Record.Collection().IsAuth() {
+				return e.Next()
+			}
+
+			hasUpgradedVerified := !e.Record.Original().IsNew() && !e.Record.Original().Verified() && e.Record.Verified()
+
+			if !hasUpgradedVerified {
+				return e.Next()
+			}
+
+			originalApp := e.App
+			return e.App.RunInTransaction(func(txApp App) error {
+				e.App = txApp
+				defer func() { e.App = originalApp }()
+
+				externalAuths, err := txApp.FindAllExternalAuthsByRecord(e.Record)
+				if err != nil {
+					return err
+				}
+				if len(externalAuths) > 0 {
+					// delete all pre-existing external auths
+					if err := txApp.DeleteAllExternalAuthsByRecord(e.Record); err != nil {
+						return err
+					}
+
+					// force refresh tokens reset (if not already)
+					e.Record.RefreshTokenKey()
+				}
+
+				return e.Next()
+			})
+		},
+		Priority: 99,
+	})
 }

core/mfa_model.go

@@ -3,6 +3,7 @@ package core
 import (
 	"context"
 	"errors"
+	"fmt"
 	"time"
 
 	"github.com/pocketbase/pocketbase/tools/hook"
@@ -141,11 +142,11 @@ func (app *BaseApp) registerMFAHooks() {
 			if old != new {
 				err = e.App.DeleteAllMFAsByRecord(e.Record)
 				if err != nil {
-					e.App.Logger().Warn(
+					return fmt.Errorf(
-						"Failed to delete all previous mfas",
+						"[%s] failed to delete all previos MFAs for record %q: %w",
-						"error", err,
+						e.Record.Collection().Name,
-						"recordId", e.Record.Id,
+						e.Record.Id,
-						"collectionId", e.Record.Collection().Id,
+						err,
 					)
 				}
 			}

core/otp_model.go

@@ -3,8 +3,10 @@ package core
 import (
 	"context"
 	"errors"
+	"fmt"
 	"time"
 
+	"github.com/pocketbase/pocketbase/tools/hook"
 	"github.com/pocketbase/pocketbase/tools/types"
 )
 
@@ -124,4 +126,29 @@ func (app *BaseApp) registerOTPHooks() {
 			app.Logger().Warn("Failed to delete expired OTP sessions", "error", err)
 		}
 	})
+
+	// delete all record OTPs on tokenKey change to minimize the risk of hijacking attacks
+	app.OnRecordUpdateExecute().Bind(&hook.Handler[*RecordEvent]{
+		Func: func(e *RecordEvent) error {
+			err := e.Next()
+			if err != nil || !e.Record.Collection().IsAuth() {
+				return err
+			}
+
+			if e.Record.Original().TokenKey() != e.Record.TokenKey() {
+				err := e.App.DeleteAllOTPsByRecord(e.Record)
+				if err != nil {
+					return fmt.Errorf(
+						"[%s] failed to delete all previos OTPs for record %q: %w",
+						e.Record.Collection().Name,
+						e.Record.Id,
+						err,
+					)
+				}
+			}
+
+			return nil
+		},
+		Priority: 99,
+	})
 }

core/record_model.go

@@ -1427,10 +1427,7 @@ func onRecordValidate(e *RecordEvent) error {
 }
 
 func onRecordSaveExecute(e *RecordEvent) error {
-	var needToDeleteExternalAuths bool
-
 	if e.Record.Collection().IsAuth() {
-		// auth resets to prevent (pre)hijacking vulnerabilities
 		if !e.Record.IsNew() {
 			lastSavedRecord, err := e.App.FindRecordById(e.Record.Collection(), e.Record.Id)
 			if err != nil {
@@ -1443,15 +1440,10 @@ func onRecordSaveExecute(e *RecordEvent) error {
 					lastSavedRecord.Email() != e.Record.Email()) {
 				e.Record.RefreshTokenKey()
 			}
-
-			// in case upgrading from "unverified" -> "verified" mark all pre-existing OAuth2 links
-			// for deletion since there is no reliable way to verify that they weren't created by an attacker
-			if !lastSavedRecord.Verified() && e.Record.Verified() {
-				needToDeleteExternalAuths = true
-			}
 		}
 
-		// cross-check that the auth record id is unique across all auth collections
+		// loosely cross-check that the auth record id is unique across all auth collections
+		// to minimize impact of mistakes in API rules when multiple auth collections are used
 		authCollections, err := e.App.FindAllCollections(CollectionTypeAuth)
 		if err != nil {
 			return fmt.Errorf("unable to fetch the auth collections for cross-id unique check: %w", err)
@@ -1469,45 +1461,16 @@ func onRecordSaveExecute(e *RecordEvent) error {
 		}
 	}
 
-	finalizer := func() error {
+	err := e.Next()
-		err := e.Next()
+	if err == nil {
-		if err == nil {
+		return nil
-			return nil
-		}
-
-		return validators.NormalizeUniqueIndexError(
-			err,
-			e.Record.Collection().Name,
-			e.Record.Collection().Fields.FieldNames(),
-		)
 	}
 
-	if needToDeleteExternalAuths {
+	return validators.NormalizeUniqueIndexError(
-		originalApp := e.App
+		err,
-
+		e.Record.Collection().Name,
-		return e.App.RunInTransaction(func(txApp App) error {
+		e.Record.Collection().Fields.FieldNames(),
-			e.App = txApp
+	)
-			defer func() { e.App = originalApp }()
-
-			externalAuths, err := txApp.FindAllExternalAuthsByRecord(e.Record)
-			if err != nil {
-				return err
-			}
-			if len(externalAuths) > 0 {
-				// delete all pre-existing external auths
-				if err := txApp.DeleteAllExternalAuthsByRecord(e.Record); err != nil {
-					return err
-				}
-
-				// force refresh tokens reset (if not already)
-				e.Record.RefreshTokenKey()
-			}
-
-			return finalizer()
-		})
-	}
-
-	return finalizer()
 }
 
 func onRecordDeleteExecute(e *RecordEvent) error {

ui/dist/index.html

@@ -13,7 +13,7 @@
 
     <!-- prism -->
     <script src="./libs/prism/prism.js" data-manual></script>
-  <script type="module" crossorigin src="./assets/index-CP6VPsEr.js"></script>
+  <script type="module" crossorigin src="./assets/index-DbuYk4bQ.js"></script>
   <link rel="modulepreload" crossorigin href="./assets/pocketbase.es-B_4DUNUU.js">
   <link rel="stylesheet" crossorigin href="./assets/index-ouas71Vg.css">
 </head>

ui/package-lock.json

@@ -224,9 +224,9 @@
             }
         },
         "node_modules/@napi-rs/wasm-runtime": {
-            "version": "1.1.3",
+            "version": "1.1.4",
-            "resolved": "https://registry.npmjs.org/@napi-rs/wasm-runtime/-/wasm-runtime-1.1.3.tgz",
+            "resolved": "https://registry.npmjs.org/@napi-rs/wasm-runtime/-/wasm-runtime-1.1.4.tgz",
-            "integrity": "sha512-xK9sGVbJWYb08+mTJt3/YV24WxvxpXcXtP6B172paPZ+Ts69Re9dAr7lKwJoeIx8OoeuimEiRZ7umkiUVClmmQ==",
+            "integrity": "sha512-3NQNNgA1YSlJb/kMH1ildASP9HW7/7kYnRI2szWJaofaS1hWmbGI4H+d3+22aGzXXN9IJ+n+GiFVcGipJP18ow==",
             "dev": true,
             "license": "MIT",
             "optional": true,
@@ -937,9 +937,9 @@
             "license": "MIT"
         },
         "node_modules/postcss": {
-            "version": "8.5.9",
+            "version": "8.5.10",
-            "resolved": "https://registry.npmjs.org/postcss/-/postcss-8.5.9.tgz",
+            "resolved": "https://registry.npmjs.org/postcss/-/postcss-8.5.10.tgz",
-            "integrity": "sha512-7a70Nsot+EMX9fFU3064K/kdHWZqGVY+BADLyXc8Dfv+mTLLVl6JzJpPaCZ2kQL9gIJvKXSLMHhqdRRjwQeFtw==",
+            "integrity": "sha512-pMMHxBOZKFU6HgAZ4eyGnwXF/EvPGGqUr0MnZ5+99485wwW41kW91A4LOGxSHhgugZmSChL5AlElNdwlNgcnLQ==",
             "dev": true,
             "funding": [
                 {

ui/src/collections/mfaAccordion.js

@@ -6,7 +6,7 @@ export function mfaAccordion(collection) {
             if (!collection.mfa) {
                 collection.mfa = {
                     enabled: false,
-                    duration: 900,
+                    duration: 600,
                     rule: "",
                 };
             }