mirror of
https://codeberg.org/forgejo/forgejo.git
synced 2025-08-01 17:38:33 +00:00
fix(sec): consider webauthn for external login
- Currently during external login (such as OAuth2), if the user is
enrolled into Webauthn and not enrolled into TOTP then no 2FA is being
done during external login and when account linking is set to `auto` then
also during automatic linking. This results in bypassing the 2FA of the
user.
- Create a new unified function that checks if the user is enrolled into
2FA and use this when necessary. Rename the old `HasTwoFactorByUID`
function to `HasTOTPByUID` which is a more appropiate naming.
(cherry picked from commit df5d656827
)
Conflicts:
the original commit was trimmed down to be fit for backport
This commit is contained in:
parent
c5601e9399
commit
5d7953def4
10 changed files with 71 additions and 18 deletions
21
models/auth/two_factor.go
Normal file
21
models/auth/two_factor.go
Normal file
|
@ -0,0 +1,21 @@
|
|||
// Copyright 2025 The Forgejo Authors. All rights reserved.
|
||||
// SPDX-License-Identifier: GPL-3.0-or-later
|
||||
package auth
|
||||
|
||||
import (
|
||||
"context"
|
||||
)
|
||||
|
||||
// HasTwoFactorByUID returns true if the user has TOTP or WebAuthn enabled for
|
||||
// their account.
|
||||
func HasTwoFactorByUID(ctx context.Context, userID int64) (bool, error) {
|
||||
hasTOTP, err := HasTOTPByUID(ctx, userID)
|
||||
if err != nil {
|
||||
return false, err
|
||||
}
|
||||
if hasTOTP {
|
||||
return true, nil
|
||||
}
|
||||
|
||||
return HasWebAuthnRegistrationsByUID(ctx, userID)
|
||||
}
|
34
models/auth/two_factor_test.go
Normal file
34
models/auth/two_factor_test.go
Normal file
|
@ -0,0 +1,34 @@
|
|||
// Copyright 2025 The Forgejo Authors. All rights reserved.
|
||||
// SPDX-License-Identifier: GPL-3.0-or-later
|
||||
package auth
|
||||
|
||||
import (
|
||||
"testing"
|
||||
|
||||
"forgejo.org/models/unittest"
|
||||
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
)
|
||||
|
||||
func TestHasTwoFactorByUID(t *testing.T) {
|
||||
require.NoError(t, unittest.PrepareTestDatabase())
|
||||
|
||||
t.Run("No twofactor", func(t *testing.T) {
|
||||
ok, err := HasTwoFactorByUID(t.Context(), 2)
|
||||
require.NoError(t, err)
|
||||
assert.False(t, ok)
|
||||
})
|
||||
|
||||
t.Run("WebAuthn credential", func(t *testing.T) {
|
||||
ok, err := HasTwoFactorByUID(t.Context(), 32)
|
||||
require.NoError(t, err)
|
||||
assert.True(t, ok)
|
||||
})
|
||||
|
||||
t.Run("TOTP", func(t *testing.T) {
|
||||
ok, err := HasTwoFactorByUID(t.Context(), 24)
|
||||
require.NoError(t, err)
|
||||
assert.True(t, ok)
|
||||
})
|
||||
}
|
|
@ -139,9 +139,9 @@ func GetTwoFactorByUID(ctx context.Context, uid int64) (*TwoFactor, error) {
|
|||
return twofa, nil
|
||||
}
|
||||
|
||||
// HasTwoFactorByUID returns the two-factor authentication token associated with
|
||||
// the user, if any.
|
||||
func HasTwoFactorByUID(ctx context.Context, uid int64) (bool, error) {
|
||||
// HasTOTPByUID returns the TOTP authentication token associated with
|
||||
// the user, if the user has TOTP enabled for their account.
|
||||
func HasTOTPByUID(ctx context.Context, uid int64) (bool, error) {
|
||||
return db.GetEngine(ctx).Where("uid=?", uid).Exist(&TwoFactor{})
|
||||
}
|
||||
|
||||
|
|
|
@ -249,7 +249,7 @@ func prepareUserInfo(ctx *context.Context) *user_model.User {
|
|||
}
|
||||
ctx.Data["Sources"] = sources
|
||||
|
||||
hasTOTP, err := auth.HasTwoFactorByUID(ctx, u.ID)
|
||||
hasTOTP, err := auth.HasTOTPByUID(ctx, u.ID)
|
||||
if err != nil {
|
||||
ctx.ServerError("auth.HasTwoFactorByUID", err)
|
||||
return nil
|
||||
|
|
|
@ -242,7 +242,7 @@ 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)
|
||||
hasTOTPtwofa, err := auth.HasTOTPByUID(ctx, u.ID)
|
||||
if err != nil {
|
||||
ctx.ServerError("UserSignIn", err)
|
||||
return
|
||||
|
|
|
@ -155,15 +155,14 @@ func linkAccount(ctx *context.Context, u *user_model.User, gothUser goth.User, r
|
|||
// 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.HasTwoFactorByUID(ctx, u.ID)
|
||||
if err != nil {
|
||||
if !auth.IsErrTwoFactorNotEnrolled(err) {
|
||||
ctx.ServerError("UserLinkAccount", err)
|
||||
return
|
||||
}
|
||||
ctx.ServerError("UserLinkAccount", err)
|
||||
return
|
||||
}
|
||||
|
||||
err = externalaccount.LinkAccountToUser(ctx, u, gothUser)
|
||||
if err != nil {
|
||||
if !hasTwoFactor {
|
||||
if err := externalaccount.LinkAccountToUser(ctx, u, gothUser); err != nil {
|
||||
ctx.ServerError("UserLinkAccount", err)
|
||||
return
|
||||
}
|
||||
|
|
|
@ -1243,12 +1243,11 @@ func handleOAuth2SignIn(ctx *context.Context, source *auth.Source, u *user_model
|
|||
|
||||
needs2FA := false
|
||||
if !source.Cfg.(*oauth2.Source).SkipLocalTwoFA {
|
||||
_, err := auth.GetTwoFactorByUID(ctx, u.ID)
|
||||
if err != nil && !auth.IsErrTwoFactorNotEnrolled(err) {
|
||||
needs2FA, err = auth.HasTwoFactorByUID(ctx, u.ID)
|
||||
if err != nil {
|
||||
ctx.ServerError("UserSignIn", err)
|
||||
return
|
||||
}
|
||||
needs2FA = err == nil
|
||||
}
|
||||
|
||||
oauth2Source := source.Cfg.(*oauth2.Source)
|
||||
|
|
|
@ -36,7 +36,7 @@ func WebAuthn(ctx *context.Context) {
|
|||
return
|
||||
}
|
||||
|
||||
hasTwoFactor, err := auth.HasTwoFactorByUID(ctx, ctx.Session.Get("twofaUid").(int64))
|
||||
hasTwoFactor, err := auth.HasTOTPByUID(ctx, ctx.Session.Get("twofaUid").(int64))
|
||||
if err != nil {
|
||||
ctx.ServerError("HasTwoFactorByUID", err)
|
||||
return
|
||||
|
|
|
@ -55,7 +55,7 @@ func DeleteAccountLink(ctx *context.Context) {
|
|||
}
|
||||
|
||||
func loadSecurityData(ctx *context.Context) {
|
||||
enrolled, err := auth_model.HasTwoFactorByUID(ctx, ctx.Doer.ID)
|
||||
enrolled, err := auth_model.HasTOTPByUID(ctx, ctx.Doer.ID)
|
||||
if err != nil {
|
||||
ctx.ServerError("SettingsTwoFactor", err)
|
||||
return
|
||||
|
|
|
@ -689,7 +689,7 @@ func SendRemovedSecurityKey(ctx context.Context, u *user_model.User, securityKey
|
|||
if err != nil {
|
||||
return err
|
||||
}
|
||||
hasTOTP, err := auth_model.HasTwoFactorByUID(ctx, u.ID)
|
||||
hasTOTP, err := auth_model.HasTOTPByUID(ctx, u.ID)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
|
Loading…
Add table
Add a link
Reference in a new issue