fix(auth): set WebAuthn user verification per request (#38805)

Registration omitted `userVerification`, so Chromium raised the
credential to credProtect level 3 and the authenticator then hid it from
the second-factor login, which asked for `discouraged`. Registration and
each login now set their own value, with `preferred` on the second
factor so credentials already registered at level 3 keep working without
re-enrollment.

Also add relevant e2e test coverage for webauthn, one test chromium only
because Firefox lacks the APIs needed.

Fixes https://github.com/go-gitea/gitea/issues/33531
Fixes https://github.com/go-gitea/gitea/issues/36019
Fixes https://github.com/go-gitea/gitea/issues/38139

routers/web/auth/2fa.go

@@ -73,12 +73,9 @@ func TwoFactorPost(ctx *context.Context) {
 			return
 		}
 
-		if ctx.Session.Get("linkAccount") != nil {
-			err = linkAccountFromContext(ctx, u)
-			if err != nil {
-				ctx.ServerError("UserSignIn", err)
-				return
-			}
+		if err = completePendingLinks(ctx, u); err != nil {
+			ctx.ServerError("completePendingLinks", err)
+			return
 		}
 
 		_ = ctx.Session.Set(session.KeyUserHasTwoFactorAuth, true)
@@ -145,6 +142,11 @@ func TwoFactorScratchPost(ctx *context.Context) {
 			return
 		}
 
+		if err = completePendingLinks(ctx, u); err != nil {
+			ctx.ServerError("completePendingLinks", err)
+			return
+		}
+
 		handleSignInFull(ctx, u, remember)
 		if ctx.Written() {
 			return

routers/web/auth/auth.go

@@ -8,6 +8,7 @@ import (
 	"errors"
 	"fmt"
 	"html/template"
+	"maps"
 	"net/http"
 	"net/url"
 	"strings"
@@ -328,46 +329,35 @@ func SignInPost(ctx *context.Context) {
 
 	// If this user is enrolled in 2FA TOTP, we can't sign the user in just yet.
 	// Instead, redirect them to the 2FA authentication page.
-	hasTOTPtwofa, err := auth.HasTwoFactorByUID(ctx, u.ID)
+	hasTwoFactor, err := auth.HasTwoFactorOrWebAuthn(ctx, u.ID)
 	if err != nil {
-		ctx.ServerError("UserSignIn", err)
+		ctx.ServerError("HasTwoFactorOrWebAuthn", err)
 		return
 	}
-
-	// Check if the user has webauthn registration
-	hasWebAuthnTwofa, err := auth.HasWebAuthnRegistrationsByUID(ctx, u.ID)
-	if err != nil {
-		ctx.ServerError("UserSignIn", err)
-		return
-	}
-
-	if !hasTOTPtwofa && !hasWebAuthnTwofa {
-		// No two-factor auth configured we can sign in the user
+	if !hasTwoFactor {
 		handleSignIn(ctx, u, form.Remember)
 		return
 	}
 
-	updates := map[string]any{
-		// User will need to use 2FA TOTP or WebAuthn, save data
-		"twofaUid":      u.ID,
-		"twofaRemember": form.Remember,
-	}
-	if hasTOTPtwofa {
-		// User will need to use WebAuthn, save data
-		updates["totpEnrolled"] = u.ID
-	}
+	handleTwoFactorRequired(ctx, u, form.Remember, nil)
+}
+
+func handleTwoFactorRequired(ctx *context.Context, u *user_model.User, remember bool, extra map[string]any) {
+	updates := map[string]any{"twofaUid": u.ID, "twofaRemember": remember}
+	maps.Copy(updates, extra)
 	if err := regenerateSession(ctx, updates); err != nil {
-		ctx.ServerError("UserSignIn: Unable to update session", err)
+		ctx.ServerError("RegenerateSession", err)
 		return
 	}
-
-	// If we have WebAuthn redirect there first
-	if hasWebAuthnTwofa {
+	hasWebAuthn, err := auth.HasWebAuthnRegistrationsByUID(ctx, u.ID)
+	if err != nil {
+		ctx.ServerError("HasWebAuthnRegistrationsByUID", err)
+		return
+	}
+	if hasWebAuthn {
 		ctx.Redirect(setting.AppSubURL + "/user/webauthn")
 		return
 	}
-
-	// Fallback to 2FA
 	ctx.Redirect(setting.AppSubURL + "/user/two_factor")
 }
 

routers/web/auth/auth_test.go

@@ -11,6 +11,7 @@ import (
 	"testing"
 
 	auth_model "gitea.dev/models/auth"
+	"gitea.dev/models/unittest"
 	user_model "gitea.dev/models/user"
 	"gitea.dev/modules/session"
 	"gitea.dev/modules/setting"
@@ -182,3 +183,19 @@ func TestWebAuthOAuth2(t *testing.T) {
 		})
 	})
 }
+
+func TestOpenIDRequireTwoFactor(t *testing.T) {
+	require.NoError(t, unittest.PrepareTestDatabase())
+	mockOpt := contexttest.MockContextOption{SessionStore: session.NewMockMemStore("dummy-sid-openid")}
+
+	user32 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 32}) // has a webauthn credential
+	ctx, resp := contexttest.MockContext(t, "/user/openid/connect", mockOpt)
+	openIDRequireTwoFactor(ctx, user32, false, "https://example.com/id")
+	assert.Equal(t, "/user/webauthn", test.RedirectURL(resp))
+	unittest.AssertNotExistsBean(t, &user_model.UserOpenID{UID: user32.ID}) // not attached before the key answered
+
+	user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
+	ctx, _ = contexttest.MockContext(t, "/user/openid/connect", mockOpt)
+	openIDRequireTwoFactor(ctx, user2, false, "https://example.com/id")
+	assert.False(t, ctx.Written())
+}

routers/web/auth/linkaccount.go

@@ -148,15 +148,13 @@ func oauth2LinkAccount(ctx *context.Context, u *user_model.User, linkAccountData
 	// If this user is enrolled in 2FA, we can't sign the user in just yet.
 	// Instead, redirect them to the 2FA authentication page.
 	// We deliberately ignore the skip local 2fa setting here because we are linking to a previous user here
-	_, err := auth.GetTwoFactorByUID(ctx, u.ID)
+	hasTwoFactor, err := auth.HasTwoFactorOrWebAuthn(ctx, u.ID)
 	if err != nil {
-		if !auth.IsErrTwoFactorNotEnrolled(err) {
-			ctx.ServerError("UserLinkAccount", err)
-			return
-		}
-
-		err = externalaccount.LinkAccountToUser(ctx, linkAccountData.AuthSourceID, u, linkAccountData.GothUser)
-		if err != nil {
+		ctx.ServerError("UserLinkAccount", err)
+		return
+	}
+	if !hasTwoFactor {
+		if err := externalaccount.LinkAccountToUser(ctx, linkAccountData.AuthSourceID, u, linkAccountData.GothUser); err != nil {
 			ctx.ServerError("UserLinkAccount", err)
 			return
 		}
@@ -170,25 +168,10 @@ func oauth2LinkAccount(ctx *context.Context, u *user_model.User, linkAccountData
 		return
 	}
 
-	if err := regenerateSession(ctx, map[string]any{
-		// User needs to use 2FA, save data and redirect to 2FA page.
-		"twofaUid":              u.ID,
-		"twofaRemember":         remember,
+	handleTwoFactorRequired(ctx, u, remember, map[string]any{
 		"linkAccount":           true,
 		session.KeySignInMethod: session.SignInMethodOAuth2,
-	}); err != nil {
-		ctx.ServerError("RegenerateSession", err)
-		return
-	}
-
-	// If WebAuthn is enrolled -> Redirect to WebAuthn instead
-	regs, err := auth.GetWebAuthnCredentialsByUID(ctx, u.ID)
-	if err == nil && len(regs) > 0 {
-		ctx.Redirect(setting.AppSubURL + "/user/webauthn")
-		return
-	}
-
-	ctx.Redirect(setting.AppSubURL + "/user/two_factor")
+	})
 }
 
 // LinkAccountPostRegister handle the creation of a new account for an external account using signUp
@@ -279,6 +262,15 @@ func LinkAccountPostRegister(ctx *context.Context) {
 	handleSignIn(ctx, u, false)
 }
 
+func completePendingLinks(ctx *context.Context, user *user_model.User) error {
+	if ctx.Session.Get("linkAccount") != nil {
+		if err := linkAccountFromContext(ctx, user); err != nil {
+			return err
+		}
+	}
+	return openIDConnectFromContext(ctx, user)
+}
+
 func linkAccountFromContext(ctx *context.Context, user *user_model.User) error {
 	linkAccountData := oauth2GetLinkAccountData(ctx)
 	if linkAccountData == nil {

routers/web/auth/oauth.go

@@ -361,12 +361,11 @@ func handleOAuth2SignIn(ctx *context.Context, authSource *auth.Source, u *user_m
 
 	needs2FA := false
 	if !authSource.TwoFactorShouldSkip() {
-		_, err := auth.GetTwoFactorByUID(ctx, u.ID)
-		if err != nil && !auth.IsErrTwoFactorNotEnrolled(err) {
+		var err error
+		if needs2FA, err = auth.HasTwoFactorOrWebAuthn(ctx, u.ID); err != nil {
 			ctx.ServerError("UserSignIn", err)
 			return
 		}
-		needs2FA = err == nil
 	}
 
 	oauth2Source := authSource.Cfg.(*oauth2.Source)
@@ -453,24 +452,7 @@ func handleOAuth2SignIn(ctx *context.Context, authSource *auth.Source, u *user_m
 		}
 	}
 
-	if err := regenerateSession(ctx, map[string]any{
-		// User needs to use 2FA, save data and redirect to 2FA page.
-		"twofaUid":              u.ID,
-		"twofaRemember":         false,
-		session.KeySignInMethod: session.SignInMethodOAuth2,
-	}); err != nil {
-		ctx.ServerError("updateSession", err)
-		return
-	}
-
-	// If WebAuthn is enrolled -> Redirect to WebAuthn instead
-	regs, err := auth.GetWebAuthnCredentialsByUID(ctx, u.ID)
-	if err == nil && len(regs) > 0 {
-		ctx.Redirect(setting.AppSubURL + "/user/webauthn")
-		return
-	}
-
-	ctx.Redirect(setting.AppSubURL + "/user/two_factor")
+	handleTwoFactorRequired(ctx, u, false, map[string]any{session.KeySignInMethod: session.SignInMethodOAuth2})
 }
 
 // OAuth2UserLoginCallback attempts to handle the callback from the OAuth2 provider and if successful

routers/web/auth/openid.go

@@ -8,6 +8,7 @@ import (
 	"net/http"
 	"net/url"
 
+	auth_model "gitea.dev/models/auth"
 	user_model "gitea.dev/models/user"
 	"gitea.dev/modules/auth/openid"
 	"gitea.dev/modules/log"
@@ -26,6 +27,36 @@ const (
 	tplSignUpOID    templates.TplName = "user/auth/signup_openid_register"
 )
 
+// the OpenID is attached only after the second factor passed, so a stolen password cannot leave one behind
+func openIDRequireTwoFactor(ctx *context.Context, u *user_model.User, remember bool, pendingURI string) {
+	hasTwoFactor, err := auth_model.HasTwoFactorOrWebAuthn(ctx, u.ID)
+	if err != nil {
+		ctx.ServerError("HasTwoFactorOrWebAuthn", err)
+		return
+	}
+	if !hasTwoFactor {
+		return
+	}
+	handleTwoFactorRequired(ctx, u, remember, map[string]any{"openidPendingURI": pendingURI})
+}
+
+func openIDConnectFromContext(ctx *context.Context, u *user_model.User) error {
+	uri, _ := ctx.Session.Get("openidPendingURI").(string)
+	if uri == "" {
+		return nil
+	}
+	if err := ctx.Session.Delete("openidPendingURI"); err != nil {
+		return err
+	}
+	if err := user_model.AddUserOpenID(ctx, &user_model.UserOpenID{UID: u.ID, URI: uri}); err != nil {
+		if !user_model.IsErrOpenIDAlreadyUsed(err) {
+			return err
+		}
+		ctx.Flash.Error(ctx.Tr("form.openid_been_used", uri))
+	}
+	return nil
+}
+
 // SignInOpenID render sign in page
 func SignInOpenID(ctx *context.Context) {
 	ctx.Data["Title"] = ctx.Tr("sign_in")
@@ -154,6 +185,10 @@ func signInOpenIDVerify(ctx *context.Context) {
 		log.Trace("User exists, logging in")
 		remember, _ := ctx.Session.Get("openid_signin_remember").(bool)
 		log.Trace("Session stored openid-remember: %t", remember)
+		openIDRequireTwoFactor(ctx, u, remember, "")
+		if ctx.Written() {
+			return
+		}
 		handleSignIn(ctx, u, remember)
 		return
 	}
@@ -270,7 +305,12 @@ func ConnectOpenIDPost(ctx *context.Context) {
 		return
 	}
 
-	// add OpenID for the user
+	remember, _ := ctx.Session.Get("openid_signin_remember").(bool)
+	openIDRequireTwoFactor(ctx, u, remember, oid)
+	if ctx.Written() {
+		return
+	}
+
 	userOID := &user_model.UserOpenID{UID: u.ID, URI: oid}
 	if err := user_model.AddUserOpenID(ctx, userOID); err != nil {
 		if user_model.IsErrOpenIDAlreadyUsed(err) {
@@ -282,9 +322,6 @@ func ConnectOpenIDPost(ctx *context.Context) {
 	}
 
 	ctx.Flash.Success(ctx.Tr("settings.add_openid_success"))
-
-	remember, _ := ctx.Session.Get("openid_signin_remember").(bool)
-	log.Trace("Session stored openid-remember: %t", remember)
 	handleSignIn(ctx, u, remember)
 }
 

routers/web/auth/password.go

@@ -238,6 +238,19 @@ func ResetPasswdPost(ctx *context.Context) {
 		return
 	}
 
+	// the reset form only carries a TOTP field, so a WebAuthn-only user finishes on its own page
+	if twofa == nil {
+		hasWebAuthn, err := auth.HasWebAuthnRegistrationsByUID(ctx, u.ID)
+		if err != nil {
+			ctx.ServerError("HasWebAuthnRegistrationsByUID", err)
+			return
+		}
+		if hasWebAuthn {
+			handleTwoFactorRequired(ctx, u, remember, nil)
+			return
+		}
+	}
+
 	handleSignIn(ctx, u, remember)
 }
 

routers/web/auth/webauthn.go

@@ -54,7 +54,8 @@ func WebAuthnPasskeyAssertion(ctx *context.Context) {
 		return
 	}
 
-	assertion, sessionData, err := wa.WebAuthn.BeginDiscoverableLogin()
+	// a passkey is the only factor here
+	assertion, sessionData, err := wa.WebAuthn.BeginDiscoverableLogin(webauthn.WithUserVerification(protocol.VerificationRequired))
 	if err != nil {
 		ctx.ServerError("webauthn.BeginDiscoverableLogin", err)
 		return
@@ -91,7 +92,7 @@ func WebAuthnPasskeyLogin(ctx *context.Context) {
 	parsedResponse, err := protocol.ParseCredentialRequestResponse(ctx.Req)
 	if err != nil {
 		// Failed authentication attempt.
-		log.Info("Failed authentication attempt for %s from %s: %v", user.Name, ctx.RemoteAddr(), err)
+		log.Info("Failed authentication attempt from %s: %v", ctx.RemoteAddr(), err)
 		ctx.Status(http.StatusForbidden)
 		return
 	}
@@ -147,12 +148,9 @@ func WebAuthnPasskeyLogin(ctx *context.Context) {
 		return
 	}
 
-	// Now handle account linking if that's requested
-	if ctx.Session.Get("linkAccount") != nil {
-		if err := linkAccountFromContext(ctx, user); err != nil {
-			ctx.ServerError("LinkAccountFromStore", err)
-			return
-		}
+	if err := completePendingLinks(ctx, user); err != nil {
+		ctx.ServerError("completePendingLinks", err)
+		return
 	}
 
 	remember := false // TODO: implement remember me
@@ -186,7 +184,8 @@ func WebAuthnLoginAssertion(ctx *context.Context) {
 	}
 
 	webAuthnUser := wa.NewWebAuthnUser(ctx, user)
-	assertion, sessionData, err := wa.WebAuthn.BeginLogin(webAuthnUser)
+	// "discouraged" would hide credProtect protected credentials
+	assertion, sessionData, err := wa.WebAuthn.BeginLogin(webAuthnUser, webauthn.WithUserVerification(protocol.VerificationPreferred))
 	if err != nil {
 		ctx.ServerError("webauthn.BeginLogin", err)
 		return
@@ -261,12 +260,9 @@ func WebAuthnLoginAssertionPost(ctx *context.Context) {
 		return
 	}
 
-	// Now handle account linking if that's requested
-	if ctx.Session.Get("linkAccount") != nil {
-		if err := linkAccountFromContext(ctx, user); err != nil {
-			ctx.ServerError("LinkAccountFromStore", err)
-			return
-		}
+	if err := completePendingLinks(ctx, user); err != nil {
+		ctx.ServerError("completePendingLinks", err)
+		return
 	}
 
 	remember := ctx.Session.Get("twofaRemember").(bool)

routers/web/user/setting/security/webauthn.go

@@ -53,8 +53,17 @@ func WebAuthnRegister(ctx *context.Context) {
 	}
 
 	webAuthnUser := wa.NewWebAuthnUser(ctx, ctx.Doer)
-	credentialOptions, sessionData, err := wa.WebAuthn.BeginRegistration(webAuthnUser, webauthn.WithAuthenticatorSelection(protocol.AuthenticatorSelection{
+	// the exclusions stop enrolling the same authenticator twice
+	credentials, err := auth.GetWebAuthnCredentialsByUID(ctx, ctx.Doer.ID)
+	if err != nil {
+		ctx.ServerError("GetWebAuthnCredentialsByUID", err)
+		return
+	}
+	exclusions := webauthn.Credentials(credentials.ToCredentials()).CredentialDescriptors()
+	credentialOptions, sessionData, err := wa.WebAuthn.BeginRegistration(webAuthnUser, webauthn.WithExclusions(exclusions), webauthn.WithAuthenticatorSelection(protocol.AuthenticatorSelection{
 		ResidentKey: protocol.ResidentKeyRequirementRequired,
+		// anything else makes Chromium raise it to credProtect level 3, hiding it from the second factor
+		UserVerification: protocol.VerificationRequired,
 	}))
 	if err != nil {
 		ctx.ServerError("Unable to BeginRegistration", err)