From b8afdcf62d5922012d8f340a54c607cc572f118b Mon Sep 17 00:00:00 2001 From: kolaente Date: Thu, 4 Sep 2025 17:16:59 +0200 Subject: [PATCH] fix(user): do not reject 2fa for local users https://github.com/go-vikunja/vikunja/issues/1402 --- pkg/routes/api/v1/user_totp.go | 42 +++++++++++++-------------- pkg/webtests/user_totp_test.go | 53 ++++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 22 deletions(-) create mode 100644 pkg/webtests/user_totp_test.go diff --git a/pkg/routes/api/v1/user_totp.go b/pkg/routes/api/v1/user_totp.go index e20870730..7ab3f2de3 100644 --- a/pkg/routes/api/v1/user_totp.go +++ b/pkg/routes/api/v1/user_totp.go @@ -45,7 +45,10 @@ import ( // @Failure 500 {object} models.Message "Internal server error." // @Router /user/settings/totp/enroll [post] func UserTOTPEnroll(c echo.Context) error { - u, err := user.GetCurrentUser(c) + s := db.NewSession() + defer s.Close() + + u, err := user.GetCurrentUserFromDB(s, c) if err != nil { return handler.HandleHTTPError(err) } @@ -55,9 +58,6 @@ func UserTOTPEnroll(c echo.Context) error { return handler.HandleHTTPError(&user.ErrAccountIsNotLocal{UserID: u.ID}) } - s := db.NewSession() - defer s.Close() - t, err := user.EnrollTOTP(s, u) if err != nil { _ = s.Rollback() @@ -87,7 +87,10 @@ func UserTOTPEnroll(c echo.Context) error { // @Failure 500 {object} models.Message "Internal server error." // @Router /user/settings/totp/enable [post] func UserTOTPEnable(c echo.Context) error { - u, err := user.GetCurrentUser(c) + s := db.NewSession() + defer s.Close() + + u, err := user.GetCurrentUserFromDB(s, c) if err != nil { return handler.HandleHTTPError(err) } @@ -109,9 +112,6 @@ func UserTOTPEnable(c echo.Context) error { return echo.NewHTTPError(http.StatusBadRequest, "Invalid model provided.").SetInternal(err) } - s := db.NewSession() - defer s.Close() - err = user.EnableTOTP(s, passcode) if err != nil { _ = s.Rollback() @@ -150,7 +150,10 @@ func UserTOTPDisable(c echo.Context) error { return echo.NewHTTPError(http.StatusBadRequest, "Invalid model provided.").SetInternal(err) } - u, err := user.GetCurrentUser(c) + s := db.NewSession() + defer s.Close() + + u, err := user.GetCurrentUserFromDB(s, c) if err != nil { return handler.HandleHTTPError(err) } @@ -159,11 +162,6 @@ func UserTOTPDisable(c echo.Context) error { if !u.IsLocalUser() { return handler.HandleHTTPError(&user.ErrAccountIsNotLocal{UserID: u.ID}) } - - s := db.NewSession() - defer s.Close() - - u, err = user.GetUserByID(s, u.ID) if err != nil { _ = s.Rollback() return handler.HandleHTTPError(err) @@ -200,7 +198,10 @@ func UserTOTPDisable(c echo.Context) error { // @Failure 500 {object} models.Message "Internal server error." // @Router /user/settings/totp/qrcode [get] func UserTOTPQrCode(c echo.Context) error { - u, err := user.GetCurrentUser(c) + s := db.NewSession() + defer s.Close() + + u, err := user.GetCurrentUserFromDB(s, c) if err != nil { return handler.HandleHTTPError(err) } @@ -210,9 +211,6 @@ func UserTOTPQrCode(c echo.Context) error { return handler.HandleHTTPError(&user.ErrAccountIsNotLocal{UserID: u.ID}) } - s := db.NewSession() - defer s.Close() - qrcode, err := user.GetTOTPQrCodeForUser(s, u) if err != nil { _ = s.Rollback() @@ -245,7 +243,10 @@ func UserTOTPQrCode(c echo.Context) error { // @Failure 500 {object} models.Message "Internal server error." // @Router /user/settings/totp [get] func UserTOTP(c echo.Context) error { - u, err := user.GetCurrentUser(c) + s := db.NewSession() + defer s.Close() + + u, err := user.GetCurrentUserFromDB(s, c) if err != nil { return handler.HandleHTTPError(err) } @@ -255,9 +256,6 @@ func UserTOTP(c echo.Context) error { return handler.HandleHTTPError(&user.ErrAccountIsNotLocal{UserID: u.ID}) } - s := db.NewSession() - defer s.Close() - t, err := user.GetTOTPForUser(s, u) if err != nil { _ = s.Rollback() diff --git a/pkg/webtests/user_totp_test.go b/pkg/webtests/user_totp_test.go new file mode 100644 index 000000000..57ee154ac --- /dev/null +++ b/pkg/webtests/user_totp_test.go @@ -0,0 +1,53 @@ +// Vikunja is a to-do list application to facilitate your life. +// Copyright 2018-present Vikunja and contributors. All rights reserved. +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package webtests + +import ( + "net/http" + "testing" + + apiv1 "code.vikunja.io/api/pkg/routes/api/v1" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestUserTOTPLocalUser(t *testing.T) { + t.Run("Enroll TOTP for local user", func(t *testing.T) { + rec, err := newTestRequestWithUser(t, http.MethodPost, apiv1.UserTOTPEnroll, &testuser1, "", nil, nil) + require.NoError(t, err) + assert.Equal(t, http.StatusOK, rec.Code) + assert.Contains(t, rec.Body.String(), `"secret"`) + assert.Contains(t, rec.Body.String(), `"url"`) + assert.Contains(t, rec.Body.String(), `"enabled":false`) + }) + + t.Run("Get TOTP QR Code for enrolled local user", func(t *testing.T) { + rec, err := newTestRequestWithUser(t, http.MethodGet, apiv1.UserTOTPQrCode, &testuser1, "", nil, nil) + require.NoError(t, err) + assert.Equal(t, http.StatusOK, rec.Code) + assert.Equal(t, "image/jpeg", rec.Header().Get("Content-Type")) + }) + + t.Run("Get TOTP settings for enrolled local user", func(t *testing.T) { + rec, err := newTestRequestWithUser(t, http.MethodGet, apiv1.UserTOTP, &testuser1, "", nil, nil) + require.NoError(t, err) + assert.Equal(t, http.StatusOK, rec.Code) + assert.Contains(t, rec.Body.String(), `"secret"`) + assert.Contains(t, rec.Body.String(), `"enabled":false`) + }) +} \ No newline at end of file