From 1ce33aa38d1d258d14523ff2c7c2dbf339f22b74 Mon Sep 17 00:00:00 2001 From: Gusted Date: Sun, 11 Aug 2024 05:13:01 +0200 Subject: [PATCH] fix: extend `forgejo_auth_token` table - Add a `purpose` column, this allows the `forgejo_auth_token` table to be used by other parts of Forgejo, while still enjoying the no-compromise architecture. - Remove the 'roll your own crypto' time limited code functions and migrate them to the `forgejo_auth_token` table. This migration ensures generated codes can only be used for their purpose and ensure they are invalidated after their usage by deleting it from the database, this also should help making auditing of the security code easier, as we're no longer trying to stuff a lot of data into a HMAC construction. -Helper functions are rewritten to ensure a safe-by-design approach to these tokens. - Add the `forgejo_auth_token` to dbconsistency doctor and add it to the `deleteUser` function. - TODO: Add cron job to delete expired authorization tokens. - Unit and integration tests added. --- models/auth/auth_token.go | 26 +++- models/forgejo_migrations/migrate.go | 2 + models/forgejo_migrations/v24.go | 19 +++ models/user/email_address.go | 19 --- models/user/user.go | 76 ++++++---- models/user/user_test.go | 66 +++++++++ modules/base/tool.go | 66 --------- modules/base/tool_test.go | 57 ------- routers/web/auth/auth.go | 100 ++++++------- routers/web/auth/password.go | 18 ++- routers/web/user/setting/account.go | 15 +- services/context/context_cookie.go | 2 +- services/doctor/dbconsistency.go | 3 + services/mailer/mail.go | 47 ++++-- services/user/delete.go | 1 + tests/integration/auth_token_test.go | 8 +- tests/integration/org_team_invite_test.go | 7 +- tests/integration/user_test.go | 172 ++++++++++++++++++++++ 18 files changed, 448 insertions(+), 256 deletions(-) create mode 100644 models/forgejo_migrations/v24.go diff --git a/models/auth/auth_token.go b/models/auth/auth_token.go index 2c3ca90734..8955b57a4f 100644 --- a/models/auth/auth_token.go +++ b/models/auth/auth_token.go @@ -15,12 +15,31 @@ import ( "code.gitea.io/gitea/modules/util" ) +type AuthorizationPurpose string + +var ( + // Used to store long term authorization tokens. + LongTermAuthorization AuthorizationPurpose = "long_term_authorization" + + // Used to activate a user account. + UserActivation AuthorizationPurpose = "user_activation" + + // Used to reset the password. + PasswordReset AuthorizationPurpose = "password_reset" +) + +// Used to activate the specified email address for a user. +func EmailActivation(email string) AuthorizationPurpose { + return AuthorizationPurpose("email_activation:" + email) +} + // AuthorizationToken represents a authorization token to a user. type AuthorizationToken struct { ID int64 `xorm:"pk autoincr"` UID int64 `xorm:"INDEX"` LookupKey string `xorm:"INDEX UNIQUE"` HashedValidator string + Purpose AuthorizationPurpose `xorm:"NOT NULL"` Expiry timeutil.TimeStamp } @@ -41,7 +60,7 @@ func (authToken *AuthorizationToken) IsExpired() bool { // GenerateAuthToken generates a new authentication token for the given user. // It returns the lookup key and validator values that should be passed to the // user via a long-term cookie. -func GenerateAuthToken(ctx context.Context, userID int64, expiry timeutil.TimeStamp) (lookupKey, validator string, err error) { +func GenerateAuthToken(ctx context.Context, userID int64, expiry timeutil.TimeStamp, purpose AuthorizationPurpose) (lookupKey, validator string, err error) { // Request 64 random bytes. The first 32 bytes will be used for the lookupKey // and the other 32 bytes will be used for the validator. rBytes, err := util.CryptoRandomBytes(64) @@ -56,14 +75,15 @@ func GenerateAuthToken(ctx context.Context, userID int64, expiry timeutil.TimeSt Expiry: expiry, LookupKey: lookupKey, HashedValidator: HashValidator(rBytes[32:]), + Purpose: purpose, }) return lookupKey, validator, err } // FindAuthToken will find a authorization token via the lookup key. -func FindAuthToken(ctx context.Context, lookupKey string) (*AuthorizationToken, error) { +func FindAuthToken(ctx context.Context, lookupKey string, purpose AuthorizationPurpose) (*AuthorizationToken, error) { var authToken AuthorizationToken - has, err := db.GetEngine(ctx).Where("lookup_key = ?", lookupKey).Get(&authToken) + has, err := db.GetEngine(ctx).Where("lookup_key = ? AND purpose = ?", lookupKey, purpose).Get(&authToken) if err != nil { return nil, err } else if !has { diff --git a/models/forgejo_migrations/migrate.go b/models/forgejo_migrations/migrate.go index 116579eeff..53159e31f2 100644 --- a/models/forgejo_migrations/migrate.go +++ b/models/forgejo_migrations/migrate.go @@ -84,6 +84,8 @@ var migrations = []*Migration{ NewMigration("Add `legacy` to `web_authn_credential` table", AddLegacyToWebAuthnCredential), // v23 -> v24 NewMigration("Add `delete_branch_after_merge` to `auto_merge` table", AddDeleteBranchAfterMergeToAutoMerge), + // v24 -> v25 + NewMigration("Add `purpose` column to `forgejo_auth_token` table", AddPurposeToForgejoAuthToken), } // GetCurrentDBVersion returns the current Forgejo database version. diff --git a/models/forgejo_migrations/v24.go b/models/forgejo_migrations/v24.go new file mode 100644 index 0000000000..8c751e3f5f --- /dev/null +++ b/models/forgejo_migrations/v24.go @@ -0,0 +1,19 @@ +// Copyright 2024 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package forgejo_migrations //nolint:revive + +import "xorm.io/xorm" + +func AddPurposeToForgejoAuthToken(x *xorm.Engine) error { + type ForgejoAuthToken struct { + ID int64 `xorm:"pk autoincr"` + Purpose string `xorm:"NOT NULL"` + } + if err := x.Sync(new(ForgejoAuthToken)); err != nil { + return err + } + + _, err := x.Exec("UPDATE `forgejo_auth_token` SET purpose = 'long_term_authorization' WHERE purpose = ''") + return err +} diff --git a/models/user/email_address.go b/models/user/email_address.go index b14ff7886c..54667986ac 100644 --- a/models/user/email_address.go +++ b/models/user/email_address.go @@ -8,10 +8,8 @@ import ( "context" "fmt" "strings" - "time" "code.gitea.io/gitea/models/db" - "code.gitea.io/gitea/modules/base" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/optional" "code.gitea.io/gitea/modules/setting" @@ -246,23 +244,6 @@ func updateActivation(ctx context.Context, email *EmailAddress, activate bool) e return UpdateUserCols(ctx, user, "rands") } -// VerifyActiveEmailCode verifies active email code when active account -func VerifyActiveEmailCode(ctx context.Context, code, email string) *EmailAddress { - if user := GetVerifyUser(ctx, code); user != nil { - // time limit code - prefix := code[:base.TimeLimitCodeLength] - data := fmt.Sprintf("%d%s%s%s%s", user.ID, email, user.LowerName, user.Passwd, user.Rands) - - if base.VerifyTimeLimitCode(time.Now(), data, setting.Service.ActiveCodeLives, prefix) { - emailAddress := &EmailAddress{UID: user.ID, Email: email} - if has, _ := db.GetEngine(ctx).Get(emailAddress); has { - return emailAddress - } - } - } - return nil -} - // SearchEmailOrderBy is used to sort the results from SearchEmails() type SearchEmailOrderBy string diff --git a/models/user/user.go b/models/user/user.go index 43bab4f3e9..0fa8bb0ca9 100644 --- a/models/user/user.go +++ b/models/user/user.go @@ -7,7 +7,9 @@ package user import ( "context" + "crypto/subtle" "encoding/hex" + "errors" "fmt" "net/mail" "net/url" @@ -318,15 +320,14 @@ func (u *User) OrganisationLink() string { return setting.AppSubURL + "/org/" + url.PathEscape(u.Name) } -// GenerateEmailActivateCode generates an activate code based on user information and given e-mail. -func (u *User) GenerateEmailActivateCode(email string) string { - code := base.CreateTimeLimitCode( - fmt.Sprintf("%d%s%s%s%s", u.ID, email, u.LowerName, u.Passwd, u.Rands), - setting.Service.ActiveCodeLives, time.Now(), nil) - - // Add tail hex username - code += hex.EncodeToString([]byte(u.LowerName)) - return code +// GenerateEmailAuthorizationCode generates an activation code based for the user for the specified purpose. +// The standard expiry is ActiveCodeLives minutes. +func (u *User) GenerateEmailAuthorizationCode(ctx context.Context, purpose auth.AuthorizationPurpose) (string, error) { + lookup, validator, err := auth.GenerateAuthToken(ctx, u.ID, timeutil.TimeStampNow().Add(int64(setting.Service.ActiveCodeLives)*60), purpose) + if err != nil { + return "", err + } + return lookup + ":" + validator, nil } // GetUserFollowers returns range of user's followers. @@ -838,35 +839,50 @@ func countUsers(ctx context.Context, opts *CountUserFilter) int64 { return count } -// GetVerifyUser get user by verify code -func GetVerifyUser(ctx context.Context, code string) (user *User) { - if len(code) <= base.TimeLimitCodeLength { - return nil +// VerifyUserActiveCode verifies that the code is valid for the given purpose for this user. +// If delete is specified, the token will be deleted. +func VerifyUserAuthorizationToken(ctx context.Context, code string, purpose auth.AuthorizationPurpose, delete bool) (*User, error) { + lookupKey, validator, found := strings.Cut(code, ":") + if !found { + return nil, nil } - // use tail hex username query user - hexStr := code[base.TimeLimitCodeLength:] - if b, err := hex.DecodeString(hexStr); err == nil { - if user, err = GetUserByName(ctx, string(b)); user != nil { - return user + authToken, err := auth.FindAuthToken(ctx, lookupKey, purpose) + if err != nil { + if errors.Is(err, util.ErrNotExist) { + return nil, nil } - log.Error("user.getVerifyUser: %v", err) + return nil, err } - return nil -} + if authToken.IsExpired() { + return nil, auth.DeleteAuthToken(ctx, authToken) + } -// VerifyUserActiveCode verifies active code when active account -func VerifyUserActiveCode(ctx context.Context, code string) (user *User) { - if user = GetVerifyUser(ctx, code); user != nil { - // time limit code - prefix := code[:base.TimeLimitCodeLength] - data := fmt.Sprintf("%d%s%s%s%s", user.ID, user.Email, user.LowerName, user.Passwd, user.Rands) - if base.VerifyTimeLimitCode(time.Now(), data, setting.Service.ActiveCodeLives, prefix) { - return user + rawValidator, err := hex.DecodeString(validator) + if err != nil { + return nil, err + } + + if subtle.ConstantTimeCompare([]byte(authToken.HashedValidator), []byte(auth.HashValidator(rawValidator))) == 0 { + return nil, errors.New("validator doesn't match") + } + + u, err := GetUserByID(ctx, authToken.UID) + if err != nil { + if IsErrUserNotExist(err) { + return nil, nil + } + return nil, err + } + + if delete { + if err := auth.DeleteAuthToken(ctx, authToken); err != nil { + return nil, err } } - return nil + + return u, nil } // ValidateUser check if user is valid to insert / update into database diff --git a/models/user/user_test.go b/models/user/user_test.go index 082c21063c..6f20a577f3 100644 --- a/models/user/user_test.go +++ b/models/user/user_test.go @@ -7,6 +7,7 @@ package user_test import ( "context" "crypto/rand" + "encoding/hex" "fmt" "strings" "testing" @@ -21,7 +22,9 @@ import ( "code.gitea.io/gitea/modules/optional" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/structs" + "code.gitea.io/gitea/modules/test" "code.gitea.io/gitea/modules/timeutil" + "code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/modules/validation" "code.gitea.io/gitea/tests" @@ -700,3 +703,66 @@ func TestDisabledUserFeatures(t *testing.T) { assert.True(t, user_model.IsFeatureDisabledWithLoginType(user, f)) } } + +func TestGenerateEmailAuthorizationCode(t *testing.T) { + defer test.MockVariableValue(&setting.Service.ActiveCodeLives, 2)() + require.NoError(t, unittest.PrepareTestDatabase()) + + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + + code, err := user.GenerateEmailAuthorizationCode(db.DefaultContext, auth.UserActivation) + require.NoError(t, err) + + lookupKey, validator, ok := strings.Cut(code, ":") + assert.True(t, ok) + + rawValidator, err := hex.DecodeString(validator) + require.NoError(t, err) + + authToken, err := auth.FindAuthToken(db.DefaultContext, lookupKey, auth.UserActivation) + require.NoError(t, err) + assert.False(t, authToken.IsExpired()) + assert.EqualValues(t, authToken.HashedValidator, auth.HashValidator(rawValidator)) + + authToken.Expiry = authToken.Expiry.Add(-int64(setting.Service.ActiveCodeLives) * 60) + assert.True(t, authToken.IsExpired()) +} + +func TestVerifyUserAuthorizationToken(t *testing.T) { + defer test.MockVariableValue(&setting.Service.ActiveCodeLives, 2)() + require.NoError(t, unittest.PrepareTestDatabase()) + + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + + code, err := user.GenerateEmailAuthorizationCode(db.DefaultContext, auth.UserActivation) + require.NoError(t, err) + + lookupKey, _, ok := strings.Cut(code, ":") + assert.True(t, ok) + + t.Run("Wrong purpose", func(t *testing.T) { + u, err := user_model.VerifyUserAuthorizationToken(db.DefaultContext, code, auth.PasswordReset, false) + require.NoError(t, err) + assert.Nil(t, u) + }) + + t.Run("No delete", func(t *testing.T) { + u, err := user_model.VerifyUserAuthorizationToken(db.DefaultContext, code, auth.UserActivation, false) + require.NoError(t, err) + assert.EqualValues(t, user.ID, u.ID) + + authToken, err := auth.FindAuthToken(db.DefaultContext, lookupKey, auth.UserActivation) + require.NoError(t, err) + assert.NotNil(t, authToken) + }) + + t.Run("Delete", func(t *testing.T) { + u, err := user_model.VerifyUserAuthorizationToken(db.DefaultContext, code, auth.UserActivation, true) + require.NoError(t, err) + assert.EqualValues(t, user.ID, u.ID) + + authToken, err := auth.FindAuthToken(db.DefaultContext, lookupKey, auth.UserActivation) + require.ErrorIs(t, err, util.ErrNotExist) + assert.Nil(t, authToken) + }) +} diff --git a/modules/base/tool.go b/modules/base/tool.go index 7612fff73a..02f1db59d3 100644 --- a/modules/base/tool.go +++ b/modules/base/tool.go @@ -4,26 +4,20 @@ package base import ( - "crypto/hmac" - "crypto/sha1" "crypto/sha256" - "crypto/subtle" "encoding/base64" "encoding/hex" "errors" "fmt" - "hash" "os" "path/filepath" "runtime" "strconv" "strings" - "time" "unicode/utf8" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/log" - "code.gitea.io/gitea/modules/setting" "github.com/dustin/go-humanize" ) @@ -54,66 +48,6 @@ func BasicAuthDecode(encoded string) (string, string, error) { return "", "", errors.New("invalid basic authentication") } -// VerifyTimeLimitCode verify time limit code -func VerifyTimeLimitCode(now time.Time, data string, minutes int, code string) bool { - if len(code) <= 18 { - return false - } - - startTimeStr := code[:12] - aliveTimeStr := code[12:18] - aliveTime, _ := strconv.Atoi(aliveTimeStr) // no need to check err, if anything wrong, the following code check will fail soon - - // check code - retCode := CreateTimeLimitCode(data, aliveTime, startTimeStr, nil) - if subtle.ConstantTimeCompare([]byte(retCode), []byte(code)) != 1 { - retCode = CreateTimeLimitCode(data, aliveTime, startTimeStr, sha1.New()) // TODO: this is only for the support of legacy codes, remove this in/after 1.23 - if subtle.ConstantTimeCompare([]byte(retCode), []byte(code)) != 1 { - return false - } - } - - // check time is expired or not: startTime <= now && now < startTime + minutes - startTime, _ := time.ParseInLocation("200601021504", startTimeStr, time.Local) - return (startTime.Before(now) || startTime.Equal(now)) && now.Before(startTime.Add(time.Minute*time.Duration(minutes))) -} - -// TimeLimitCodeLength default value for time limit code -const TimeLimitCodeLength = 12 + 6 + 40 - -// CreateTimeLimitCode create a time-limited code. -// Format: 12 length date time string + 6 minutes string (not used) + 40 hash string, some other code depends on this fixed length -// If h is nil, then use the default hmac hash. -func CreateTimeLimitCode[T time.Time | string](data string, minutes int, startTimeGeneric T, h hash.Hash) string { - const format = "200601021504" - - var start time.Time - var startTimeAny any = startTimeGeneric - if t, ok := startTimeAny.(time.Time); ok { - start = t - } else { - var err error - start, err = time.ParseInLocation(format, startTimeAny.(string), time.Local) - if err != nil { - return "" // return an invalid code because the "parse" failed - } - } - startStr := start.Format(format) - end := start.Add(time.Minute * time.Duration(minutes)) - - if h == nil { - h = hmac.New(sha1.New, setting.GetGeneralTokenSigningSecret()) - } - _, _ = fmt.Fprintf(h, "%s%s%s%s%d", data, hex.EncodeToString(setting.GetGeneralTokenSigningSecret()), startStr, end.Format(format), minutes) - encoded := hex.EncodeToString(h.Sum(nil)) - - code := fmt.Sprintf("%s%06d%s", startStr, minutes, encoded) - if len(code) != TimeLimitCodeLength { - panic("there is a hard requirement for the length of time-limited code") // it shouldn't happen - } - return code -} - // FileSize calculates the file size and generate user-friendly string. func FileSize(s int64) string { return humanize.IBytes(uint64(s)) diff --git a/modules/base/tool_test.go b/modules/base/tool_test.go index 81fd4b6a9e..ed1b469161 100644 --- a/modules/base/tool_test.go +++ b/modules/base/tool_test.go @@ -4,13 +4,7 @@ package base import ( - "crypto/sha1" - "fmt" "testing" - "time" - - "code.gitea.io/gitea/modules/setting" - "code.gitea.io/gitea/modules/test" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -46,57 +40,6 @@ func TestBasicAuthDecode(t *testing.T) { require.Error(t, err) } -func TestVerifyTimeLimitCode(t *testing.T) { - defer test.MockVariableValue(&setting.InstallLock, true)() - initGeneralSecret := func(secret string) { - setting.InstallLock = true - setting.CfgProvider, _ = setting.NewConfigProviderFromData(fmt.Sprintf(` -[oauth2] -JWT_SECRET = %s -`, secret)) - setting.LoadCommonSettings() - } - - initGeneralSecret("KZb_QLUd4fYVyxetjxC4eZkrBgWM2SndOOWDNtgUUko") - now := time.Now() - - t.Run("TestGenericParameter", func(t *testing.T) { - time2000 := time.Date(2000, 1, 2, 3, 4, 5, 0, time.Local) - assert.Equal(t, "2000010203040000026fa5221b2731b7cf80b1b506f5e39e38c115fee5", CreateTimeLimitCode("test-sha1", 2, time2000, sha1.New())) - assert.Equal(t, "2000010203040000026fa5221b2731b7cf80b1b506f5e39e38c115fee5", CreateTimeLimitCode("test-sha1", 2, "200001020304", sha1.New())) - assert.Equal(t, "2000010203040000024842227a2f87041ff82025199c0187410a9297bf", CreateTimeLimitCode("test-hmac", 2, time2000, nil)) - assert.Equal(t, "2000010203040000024842227a2f87041ff82025199c0187410a9297bf", CreateTimeLimitCode("test-hmac", 2, "200001020304", nil)) - }) - - t.Run("TestInvalidCode", func(t *testing.T) { - assert.False(t, VerifyTimeLimitCode(now, "data", 2, "")) - assert.False(t, VerifyTimeLimitCode(now, "data", 2, "invalid code")) - }) - - t.Run("TestCreateAndVerify", func(t *testing.T) { - code := CreateTimeLimitCode("data", 2, now, nil) - assert.False(t, VerifyTimeLimitCode(now.Add(-time.Minute), "data", 2, code)) // not started yet - assert.True(t, VerifyTimeLimitCode(now, "data", 2, code)) - assert.True(t, VerifyTimeLimitCode(now.Add(time.Minute), "data", 2, code)) - assert.False(t, VerifyTimeLimitCode(now.Add(time.Minute), "DATA", 2, code)) // invalid data - assert.False(t, VerifyTimeLimitCode(now.Add(2*time.Minute), "data", 2, code)) // expired - }) - - t.Run("TestDifferentSecret", func(t *testing.T) { - // use another secret to ensure the code is invalid for different secret - verifyDataCode := func(c string) bool { - return VerifyTimeLimitCode(now, "data", 2, c) - } - code1 := CreateTimeLimitCode("data", 2, now, sha1.New()) - code2 := CreateTimeLimitCode("data", 2, now, nil) - assert.True(t, verifyDataCode(code1)) - assert.True(t, verifyDataCode(code2)) - initGeneralSecret("000_QLUd4fYVyxetjxC4eZkrBgWM2SndOOWDNtgUUko") - assert.False(t, verifyDataCode(code1)) - assert.False(t, verifyDataCode(code2)) - }) -} - func TestFileSize(t *testing.T) { var size int64 = 512 assert.Equal(t, "512 B", FileSize(size)) diff --git a/routers/web/auth/auth.go b/routers/web/auth/auth.go index 1f5ebef7d7..941586db72 100644 --- a/routers/web/auth/auth.go +++ b/routers/web/auth/auth.go @@ -5,8 +5,6 @@ package auth import ( - "crypto/subtle" - "encoding/hex" "errors" "fmt" "net/http" @@ -63,38 +61,11 @@ func autoSignIn(ctx *context.Context) (bool, error) { return false, nil } - lookupKey, validator, found := strings.Cut(authCookie, ":") - if !found { - return false, nil - } - - authToken, err := auth.FindAuthToken(ctx, lookupKey) + u, err := user_model.VerifyUserAuthorizationToken(ctx, authCookie, auth.LongTermAuthorization, false) if err != nil { - if errors.Is(err, util.ErrNotExist) { - return false, nil - } - return false, err + return false, fmt.Errorf("VerifyUserAuthorizationToken: %w", err) } - - if authToken.IsExpired() { - err = auth.DeleteAuthToken(ctx, authToken) - return false, err - } - - rawValidator, err := hex.DecodeString(validator) - if err != nil { - return false, err - } - - if subtle.ConstantTimeCompare([]byte(authToken.HashedValidator), []byte(auth.HashValidator(rawValidator))) == 0 { - return false, nil - } - - u, err := user_model.GetUserByID(ctx, authToken.UID) - if err != nil { - if !user_model.IsErrUserNotExist(err) { - return false, fmt.Errorf("GetUserByID: %w", err) - } + if u == nil { return false, nil } @@ -633,7 +604,10 @@ func handleUserCreated(ctx *context.Context, u *user_model.User, gothUser *goth. return false } - mailer.SendActivateAccountMail(ctx.Locale, u) + if err := mailer.SendActivateAccountMail(ctx, u); err != nil { + ctx.ServerError("SendActivateAccountMail", err) + return false + } ctx.Data["IsSendRegisterMail"] = true ctx.Data["Email"] = u.Email @@ -674,7 +648,10 @@ func Activate(ctx *context.Context) { ctx.Data["ResendLimited"] = true } else { ctx.Data["ActiveCodeLives"] = timeutil.MinutesToFriendly(setting.Service.ActiveCodeLives, ctx.Locale) - mailer.SendActivateAccountMail(ctx.Locale, ctx.Doer) + if err := mailer.SendActivateAccountMail(ctx, ctx.Doer); err != nil { + ctx.ServerError("SendActivateAccountMail", err) + return + } if err := ctx.Cache.Put(cacheKey+ctx.Doer.LowerName, ctx.Doer.LowerName, 180); err != nil { log.Error("Set cache(MailResendLimit) fail: %v", err) @@ -687,7 +664,12 @@ func Activate(ctx *context.Context) { return } - user := user_model.VerifyUserActiveCode(ctx, code) + user, err := user_model.VerifyUserAuthorizationToken(ctx, code, auth.UserActivation, false) + if err != nil { + ctx.ServerError("VerifyUserAuthorizationToken", err) + return + } + // if code is wrong if user == nil { ctx.Data["IsCodeInvalid"] = true @@ -751,7 +733,12 @@ func ActivatePost(ctx *context.Context) { return } - user := user_model.VerifyUserActiveCode(ctx, code) + user, err := user_model.VerifyUserAuthorizationToken(ctx, code, auth.UserActivation, true) + if err != nil { + ctx.ServerError("VerifyUserAuthorizationToken", err) + return + } + // if code is wrong if user == nil { ctx.Data["IsCodeInvalid"] = true @@ -835,23 +822,32 @@ func ActivateEmail(ctx *context.Context) { code := ctx.FormString("code") emailStr := ctx.FormString("email") - // Verify code. - if email := user_model.VerifyActiveEmailCode(ctx, code, emailStr); email != nil { - if err := user_model.ActivateEmail(ctx, email); err != nil { - ctx.ServerError("ActivateEmail", err) - return - } - - log.Trace("Email activated: %s", email.Email) - ctx.Flash.Success(ctx.Tr("settings.add_email_success")) - - if u, err := user_model.GetUserByID(ctx, email.UID); err != nil { - log.Warn("GetUserByID: %d", email.UID) - } else { - // Allow user to validate more emails - _ = ctx.Cache.Delete("MailResendLimit_" + u.LowerName) - } + u, err := user_model.VerifyUserAuthorizationToken(ctx, code, auth.EmailActivation(emailStr), true) + if err != nil { + ctx.ServerError("VerifyUserAuthorizationToken", err) + return } + if u == nil { + ctx.Redirect(setting.AppSubURL + "/user/settings/account") + return + } + + email, err := user_model.GetEmailAddressOfUser(ctx, emailStr, u.ID) + if err != nil { + ctx.ServerError("GetEmailAddressOfUser", err) + return + } + + if err := user_model.ActivateEmail(ctx, email); err != nil { + ctx.ServerError("ActivateEmail", err) + return + } + + log.Trace("Email activated: %s", email.Email) + ctx.Flash.Success(ctx.Tr("settings.add_email_success")) + + // Allow user to validate more emails + _ = ctx.Cache.Delete("MailResendLimit_" + u.LowerName) // FIXME: e-mail verification does not require the user to be logged in, // so this could be redirecting to the login page. diff --git a/routers/web/auth/password.go b/routers/web/auth/password.go index d25bd682e2..363c01c6a8 100644 --- a/routers/web/auth/password.go +++ b/routers/web/auth/password.go @@ -86,7 +86,10 @@ func ForgotPasswdPost(ctx *context.Context) { return } - mailer.SendResetPasswordMail(u) + if err := mailer.SendResetPasswordMail(ctx, u); err != nil { + ctx.ServerError("SendResetPasswordMail", err) + return + } if err = ctx.Cache.Put("MailResendLimit_"+u.LowerName, u.LowerName, 180); err != nil { log.Error("Set cache(MailResendLimit) fail: %v", err) @@ -97,7 +100,7 @@ func ForgotPasswdPost(ctx *context.Context) { ctx.HTML(http.StatusOK, tplForgotPassword) } -func commonResetPassword(ctx *context.Context) (*user_model.User, *auth.TwoFactor) { +func commonResetPassword(ctx *context.Context, shouldDeleteToken bool) (*user_model.User, *auth.TwoFactor) { code := ctx.FormString("code") ctx.Data["Title"] = ctx.Tr("auth.reset_password") @@ -113,7 +116,12 @@ func commonResetPassword(ctx *context.Context) (*user_model.User, *auth.TwoFacto } // Fail early, don't frustrate the user - u := user_model.VerifyUserActiveCode(ctx, code) + u, err := user_model.VerifyUserAuthorizationToken(ctx, code, auth.PasswordReset, shouldDeleteToken) + if err != nil { + ctx.ServerError("VerifyUserAuthorizationToken", err) + return nil, nil + } + if u == nil { ctx.Flash.Error(ctx.Tr("auth.invalid_code_forgot_password", fmt.Sprintf("%s/user/forgot_password", setting.AppSubURL)), true) return nil, nil @@ -145,7 +153,7 @@ func commonResetPassword(ctx *context.Context) (*user_model.User, *auth.TwoFacto func ResetPasswd(ctx *context.Context) { ctx.Data["IsResetForm"] = true - commonResetPassword(ctx) + commonResetPassword(ctx, false) if ctx.Written() { return } @@ -155,7 +163,7 @@ func ResetPasswd(ctx *context.Context) { // ResetPasswdPost response from account recovery request func ResetPasswdPost(ctx *context.Context) { - u, twofa := commonResetPassword(ctx) + u, twofa := commonResetPassword(ctx, true) if ctx.Written() { return } diff --git a/routers/web/user/setting/account.go b/routers/web/user/setting/account.go index 7e4d45e991..6f40e39c8d 100644 --- a/routers/web/user/setting/account.go +++ b/routers/web/user/setting/account.go @@ -155,9 +155,15 @@ func EmailPost(ctx *context.Context) { return } // Only fired when the primary email is inactive (Wrong state) - mailer.SendActivateAccountMail(ctx.Locale, ctx.Doer) + if err := mailer.SendActivateAccountMail(ctx, ctx.Doer); err != nil { + ctx.ServerError("SendActivateAccountMail", err) + return + } } else { - mailer.SendActivateEmailMail(ctx.Doer, email.Email) + if err := mailer.SendActivateEmailMail(ctx, ctx.Doer, email.Email); err != nil { + ctx.ServerError("SendActivateEmailMail", err) + return + } } address = email.Email @@ -218,7 +224,10 @@ func EmailPost(ctx *context.Context) { // Send confirmation email if setting.Service.RegisterEmailConfirm { - mailer.SendActivateEmailMail(ctx.Doer, form.Email) + if err := mailer.SendActivateEmailMail(ctx, ctx.Doer, form.Email); err != nil { + ctx.ServerError("SendActivateEmailMail", err) + return + } if err := ctx.Cache.Put("MailResendLimit_"+ctx.Doer.LowerName, ctx.Doer.LowerName, 180); err != nil { log.Error("Set cache(MailResendLimit) fail: %v", err) } diff --git a/services/context/context_cookie.go b/services/context/context_cookie.go index 39e3218d1b..3699f81071 100644 --- a/services/context/context_cookie.go +++ b/services/context/context_cookie.go @@ -47,7 +47,7 @@ func (ctx *Context) GetSiteCookie(name string) string { // SetLTACookie will generate a LTA token and add it as an cookie. func (ctx *Context) SetLTACookie(u *user_model.User) error { days := 86400 * setting.LogInRememberDays - lookup, validator, err := auth_model.GenerateAuthToken(ctx, u.ID, timeutil.TimeStampNow().Add(int64(days))) + lookup, validator, err := auth_model.GenerateAuthToken(ctx, u.ID, timeutil.TimeStampNow().Add(int64(days)), auth_model.LongTermAuthorization) if err != nil { return err } diff --git a/services/doctor/dbconsistency.go b/services/doctor/dbconsistency.go index 7fce505d52..7207c3ff50 100644 --- a/services/doctor/dbconsistency.go +++ b/services/doctor/dbconsistency.go @@ -243,6 +243,9 @@ func checkDBConsistency(ctx context.Context, logger log.Logger, autofix bool) er // find archive download count without existing release genericOrphanCheck("Archive download count without existing Release", "repo_archive_download_count", "release", "repo_archive_download_count.release_id=release.id"), + // find authorization tokens without existing user + genericOrphanCheck("Authorization token without existing User", + "forgejo_auth_token", "user", "forgejo_auth_token.uid=user.id"), ) for _, c := range consistencyChecks { diff --git a/services/mailer/mail.go b/services/mailer/mail.go index 01ab84bcf5..bfede28bbe 100644 --- a/services/mailer/mail.go +++ b/services/mailer/mail.go @@ -70,7 +70,7 @@ func SendTestMail(email string) error { } // sendUserMail sends a mail to the user -func sendUserMail(language string, u *user_model.User, tpl base.TplName, code, subject, info string) { +func sendUserMail(language string, u *user_model.User, tpl base.TplName, code, subject, info string) error { locale := translation.NewLocale(language) data := map[string]any{ "locale": locale, @@ -84,47 +84,66 @@ func sendUserMail(language string, u *user_model.User, tpl base.TplName, code, s var content bytes.Buffer if err := bodyTemplates.ExecuteTemplate(&content, string(tpl), data); err != nil { - log.Error("Template: %v", err) - return + return err } msg := NewMessage(u.EmailTo(), subject, content.String()) msg.Info = fmt.Sprintf("UID: %d, %s", u.ID, info) SendAsync(msg) + return nil } // SendActivateAccountMail sends an activation mail to the user (new user registration) -func SendActivateAccountMail(locale translation.Locale, u *user_model.User) { +func SendActivateAccountMail(ctx context.Context, u *user_model.User) error { if setting.MailService == nil { // No mail service configured - return + return nil } - sendUserMail(locale.Language(), u, mailAuthActivate, u.GenerateEmailActivateCode(u.Email), locale.TrString("mail.activate_account"), "activate account") + + locale := translation.NewLocale(u.Language) + code, err := u.GenerateEmailAuthorizationCode(ctx, auth_model.UserActivation) + if err != nil { + return err + } + + return sendUserMail(locale.Language(), u, mailAuthActivate, code, locale.TrString("mail.activate_account"), "activate account") } // SendResetPasswordMail sends a password reset mail to the user -func SendResetPasswordMail(u *user_model.User) { +func SendResetPasswordMail(ctx context.Context, u *user_model.User) error { if setting.MailService == nil { // No mail service configured - return + return nil } + locale := translation.NewLocale(u.Language) - sendUserMail(u.Language, u, mailAuthResetPassword, u.GenerateEmailActivateCode(u.Email), locale.TrString("mail.reset_password"), "recover account") + code, err := u.GenerateEmailAuthorizationCode(ctx, auth_model.PasswordReset) + if err != nil { + return err + } + + return sendUserMail(u.Language, u, mailAuthResetPassword, code, locale.TrString("mail.reset_password"), "recover account") } // SendActivateEmailMail sends confirmation email to confirm new email address -func SendActivateEmailMail(u *user_model.User, email string) { +func SendActivateEmailMail(ctx context.Context, u *user_model.User, email string) error { if setting.MailService == nil { // No mail service configured - return + return nil } + locale := translation.NewLocale(u.Language) + code, err := u.GenerateEmailAuthorizationCode(ctx, auth_model.EmailActivation(email)) + if err != nil { + return err + } + data := map[string]any{ "locale": locale, "DisplayName": u.DisplayName(), "ActiveCodeLives": timeutil.MinutesToFriendly(setting.Service.ActiveCodeLives, locale), - "Code": u.GenerateEmailActivateCode(email), + "Code": code, "Email": email, "Language": locale.Language(), } @@ -132,14 +151,14 @@ func SendActivateEmailMail(u *user_model.User, email string) { var content bytes.Buffer if err := bodyTemplates.ExecuteTemplate(&content, string(mailAuthActivateEmail), data); err != nil { - log.Error("Template: %v", err) - return + return err } msg := NewMessage(email, locale.TrString("mail.activate_email"), content.String()) msg.Info = fmt.Sprintf("UID: %d, activate email", u.ID) SendAsync(msg) + return nil } // SendRegisterNotifyMail triggers a notify e-mail by admin created a account. diff --git a/services/user/delete.go b/services/user/delete.go index 74dbc09b82..587e3c2a8f 100644 --- a/services/user/delete.go +++ b/services/user/delete.go @@ -96,6 +96,7 @@ func deleteUser(ctx context.Context, u *user_model.User, purge bool) (err error) &user_model.BlockedUser{BlockID: u.ID}, &user_model.BlockedUser{UserID: u.ID}, &actions_model.ActionRunnerToken{OwnerID: u.ID}, + &auth_model.AuthorizationToken{UID: u.ID}, ); err != nil { return fmt.Errorf("deleteBeans: %w", err) } diff --git a/tests/integration/auth_token_test.go b/tests/integration/auth_token_test.go index 2c39c87da2..d1fd5dda83 100644 --- a/tests/integration/auth_token_test.go +++ b/tests/integration/auth_token_test.go @@ -84,7 +84,7 @@ func TestLTACookie(t *testing.T) { assert.True(t, found) rawValidator, err := hex.DecodeString(validator) require.NoError(t, err) - unittest.AssertExistsAndLoadBean(t, &auth.AuthorizationToken{LookupKey: lookupKey, HashedValidator: auth.HashValidator(rawValidator), UID: user.ID}) + unittest.AssertExistsAndLoadBean(t, &auth.AuthorizationToken{LookupKey: lookupKey, HashedValidator: auth.HashValidator(rawValidator), UID: user.ID, Purpose: auth.LongTermAuthorization}) // Check if the LTA cookie it provides authentication. // If LTA cookie provides authentication /user/login shouldn't return status 200. @@ -143,7 +143,7 @@ func TestLTAExpiry(t *testing.T) { assert.True(t, found) // Ensure it's not expired. - lta := unittest.AssertExistsAndLoadBean(t, &auth.AuthorizationToken{UID: user.ID, LookupKey: lookupKey}) + lta := unittest.AssertExistsAndLoadBean(t, &auth.AuthorizationToken{UID: user.ID, LookupKey: lookupKey, Purpose: auth.LongTermAuthorization}) assert.False(t, lta.IsExpired()) // Manually stub LTA's expiry. @@ -151,7 +151,7 @@ func TestLTAExpiry(t *testing.T) { require.NoError(t, err) // Ensure it's expired. - lta = unittest.AssertExistsAndLoadBean(t, &auth.AuthorizationToken{UID: user.ID, LookupKey: lookupKey}) + lta = unittest.AssertExistsAndLoadBean(t, &auth.AuthorizationToken{UID: user.ID, LookupKey: lookupKey, Purpose: auth.LongTermAuthorization}) assert.True(t, lta.IsExpired()) // Should return 200 OK, because LTA doesn't provide authorization anymore. @@ -160,5 +160,5 @@ func TestLTAExpiry(t *testing.T) { session.MakeRequest(t, req, http.StatusOK) // Ensure it's deleted. - unittest.AssertNotExistsBean(t, &auth.AuthorizationToken{UID: user.ID, LookupKey: lookupKey}) + unittest.AssertNotExistsBean(t, &auth.AuthorizationToken{UID: user.ID, LookupKey: lookupKey, Purpose: auth.LongTermAuthorization}) } diff --git a/tests/integration/org_team_invite_test.go b/tests/integration/org_team_invite_test.go index d04199a2c1..2fe296e8c3 100644 --- a/tests/integration/org_team_invite_test.go +++ b/tests/integration/org_team_invite_test.go @@ -10,6 +10,7 @@ import ( "strings" "testing" + "code.gitea.io/gitea/models/auth" "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/models/organization" "code.gitea.io/gitea/models/unittest" @@ -293,8 +294,10 @@ func TestOrgTeamEmailInviteRedirectsNewUserWithActivation(t *testing.T) { require.NoError(t, err) session.jar.SetCookies(baseURL, cr.Cookies()) - activateURL := fmt.Sprintf("/user/activate?code=%s", user.GenerateEmailActivateCode("doesnotexist@example.com")) - req = NewRequestWithValues(t, "POST", activateURL, map[string]string{ + code, err := user.GenerateEmailAuthorizationCode(db.DefaultContext, auth.UserActivation) + require.NoError(t, err) + + req = NewRequestWithValues(t, "POST", "/user/activate?code="+url.QueryEscape(code), map[string]string{ "password": "examplePassword!1", }) diff --git a/tests/integration/user_test.go b/tests/integration/user_test.go index f706f7e755..d2b5f112a3 100644 --- a/tests/integration/user_test.go +++ b/tests/integration/user_test.go @@ -5,14 +5,18 @@ package integration import ( + "bytes" + "encoding/hex" "fmt" "net/http" + "net/url" "strconv" "strings" "testing" "time" auth_model "code.gitea.io/gitea/models/auth" + "code.gitea.io/gitea/models/db" issues_model "code.gitea.io/gitea/models/issues" repo_model "code.gitea.io/gitea/models/repo" unit_model "code.gitea.io/gitea/models/unit" @@ -836,3 +840,171 @@ func TestUserRepos(t *testing.T) { } } } + +func TestUserActivate(t *testing.T) { + defer tests.PrepareTestEnv(t)() + defer test.MockVariableValue(&setting.Service.RegisterEmailConfirm, true)() + + called := false + code := "" + defer test.MockVariableValue(&mailer.SendAsync, func(msgs ...*mailer.Message) { + called = true + assert.Len(t, msgs, 1) + assert.Equal(t, `"doesnotexist" `, msgs[0].To) + assert.EqualValues(t, translation.NewLocale("en-US").Tr("mail.activate_account"), msgs[0].Subject) + + messageDoc := NewHTMLParser(t, bytes.NewBuffer([]byte(msgs[0].Body))) + link, ok := messageDoc.Find("a").Attr("href") + assert.True(t, ok) + u, err := url.Parse(link) + require.NoError(t, err) + code = u.Query()["code"][0] + })() + + session := emptyTestSession(t) + req := NewRequestWithValues(t, "POST", "/user/sign_up", map[string]string{ + "_csrf": GetCSRF(t, session, "/user/sign_up"), + "user_name": "doesnotexist", + "email": "doesnotexist@example.com", + "password": "examplePassword!1", + "retype": "examplePassword!1", + }) + session.MakeRequest(t, req, http.StatusOK) + assert.True(t, called) + + queryCode, err := url.QueryUnescape(code) + require.NoError(t, err) + + lookupKey, validator, ok := strings.Cut(queryCode, ":") + assert.True(t, ok) + + rawValidator, err := hex.DecodeString(validator) + require.NoError(t, err) + + authToken, err := auth_model.FindAuthToken(db.DefaultContext, lookupKey, auth_model.UserActivation) + require.NoError(t, err) + assert.False(t, authToken.IsExpired()) + assert.EqualValues(t, authToken.HashedValidator, auth_model.HashValidator(rawValidator)) + + req = NewRequest(t, "POST", "/user/activate?code="+code) + session.MakeRequest(t, req, http.StatusOK) + + unittest.AssertNotExistsBean(t, &auth_model.AuthorizationToken{ID: authToken.ID}) + unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "doesnotexist", IsActive: true}) +} + +func TestUserPasswordReset(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + + called := false + code := "" + defer test.MockVariableValue(&mailer.SendAsync, func(msgs ...*mailer.Message) { + if called { + return + } + called = true + + assert.Len(t, msgs, 1) + assert.Equal(t, user2.EmailTo(), msgs[0].To) + assert.EqualValues(t, translation.NewLocale("en-US").Tr("mail.reset_password"), msgs[0].Subject) + + messageDoc := NewHTMLParser(t, bytes.NewBuffer([]byte(msgs[0].Body))) + link, ok := messageDoc.Find("a").Attr("href") + assert.True(t, ok) + u, err := url.Parse(link) + require.NoError(t, err) + code = u.Query()["code"][0] + })() + + session := emptyTestSession(t) + req := NewRequestWithValues(t, "POST", "/user/forgot_password", map[string]string{ + "_csrf": GetCSRF(t, session, "/user/forgot_password"), + "email": user2.Email, + }) + session.MakeRequest(t, req, http.StatusOK) + assert.True(t, called) + + queryCode, err := url.QueryUnescape(code) + require.NoError(t, err) + + lookupKey, validator, ok := strings.Cut(queryCode, ":") + assert.True(t, ok) + + rawValidator, err := hex.DecodeString(validator) + require.NoError(t, err) + + authToken, err := auth_model.FindAuthToken(db.DefaultContext, lookupKey, auth_model.PasswordReset) + require.NoError(t, err) + assert.False(t, authToken.IsExpired()) + assert.EqualValues(t, authToken.HashedValidator, auth_model.HashValidator(rawValidator)) + + req = NewRequestWithValues(t, "POST", "/user/recover_account", map[string]string{ + "_csrf": GetCSRF(t, session, "/user/recover_account"), + "code": code, + "password": "new_password", + }) + session.MakeRequest(t, req, http.StatusSeeOther) + + unittest.AssertNotExistsBean(t, &auth_model.AuthorizationToken{ID: authToken.ID}) + assert.True(t, unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}).ValidatePassword("new_password")) +} + +func TestActivateEmailAddress(t *testing.T) { + defer tests.PrepareTestEnv(t)() + defer test.MockVariableValue(&setting.Service.RegisterEmailConfirm, true)() + + user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + + called := false + code := "" + defer test.MockVariableValue(&mailer.SendAsync, func(msgs ...*mailer.Message) { + if called { + return + } + called = true + + assert.Len(t, msgs, 1) + assert.Equal(t, "newemail@example.org", msgs[0].To) + assert.EqualValues(t, translation.NewLocale("en-US").Tr("mail.activate_email"), msgs[0].Subject) + + messageDoc := NewHTMLParser(t, bytes.NewBuffer([]byte(msgs[0].Body))) + link, ok := messageDoc.Find("a").Attr("href") + assert.True(t, ok) + u, err := url.Parse(link) + require.NoError(t, err) + code = u.Query()["code"][0] + })() + + session := loginUser(t, user2.Name) + req := NewRequestWithValues(t, "POST", "/user/settings/account/email", map[string]string{ + "_csrf": GetCSRF(t, session, "/user/settings"), + "email": "newemail@example.org", + }) + session.MakeRequest(t, req, http.StatusSeeOther) + assert.True(t, called) + + queryCode, err := url.QueryUnescape(code) + require.NoError(t, err) + + lookupKey, validator, ok := strings.Cut(queryCode, ":") + assert.True(t, ok) + + rawValidator, err := hex.DecodeString(validator) + require.NoError(t, err) + + authToken, err := auth_model.FindAuthToken(db.DefaultContext, lookupKey, auth_model.EmailActivation("newemail@example.org")) + require.NoError(t, err) + assert.False(t, authToken.IsExpired()) + assert.EqualValues(t, authToken.HashedValidator, auth_model.HashValidator(rawValidator)) + + req = NewRequestWithValues(t, "POST", "/user/activate_email", map[string]string{ + "code": code, + "email": "newemail@example.org", + }) + session.MakeRequest(t, req, http.StatusSeeOther) + + unittest.AssertNotExistsBean(t, &auth_model.AuthorizationToken{ID: authToken.ID}) + unittest.AssertExistsAndLoadBean(t, &user_model.EmailAddress{UID: user2.ID, IsActivated: true, Email: "newemail@example.org"}) +}