refactor: Use db.Get[] instead of db.GetEngine(ctx).Get(bean) to avoid zero value fetching wrong database record (#37977)
This PR replaces a set of struct-based `Get` lookups with explicit `db.Get` / `db.Exist` conditions in places where zero-value fields can lead to ambiguous matches or incorrect records being returned. The main goal is to make read paths deterministic and avoid accidentally matching the wrong row when only part of a struct is populated. ### What changed - replace many `db.GetEngine(ctx).Get(bean)` calls with explicit `builder.Eq` conditions across models such as actions, admin tasks, issues, pull requests, repositories, users, packages, redirects, watches, stars, and follows - use quoted column names where needed for reserved fields like `index`, `type`, and `name` - add dedicated user lookup helpers for: - primary email - OAuth login source / login name - update sign-in and OAuth-related flows to use explicit individual-user lookups instead of partially populated `User` structs - tighten package property and Terraform lock lookups to avoid ambiguous reads and updates - keep existing fallback behavior where needed, while removing reliance on zero-value struct matching ### User-facing impact These changes primarily affect authentication and account lookup paths: - email/username sign-in now re-fetches users through explicit keys - OAuth2 auto-linking now resolves users by name or primary email explicitly - OAuth2 login/sync now looks up users by login source, login type, and login name explicitly - non-individual accounts are no longer implicitly matched through partial user lookups in these flows This should reduce the risk of incorrect account matches and make query behavior more predictable across the codebase. --------- Co-authored-by: bircni <bircni@icloud.com>
This commit is contained in:
@@ -348,8 +348,8 @@ func VerifyActiveEmailCode(ctx context.Context, code, email string) *EmailAddres
|
||||
opts := &TimeLimitCodeOptions{Purpose: TimeLimitCodeActivateEmail, NewEmail: email}
|
||||
data := makeTimeLimitCodeHashData(opts, user)
|
||||
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 {
|
||||
emailAddress, has, _ := db.Get[EmailAddress](ctx, builder.Eq{"uid": user.ID, "email": email})
|
||||
if has {
|
||||
return emailAddress
|
||||
}
|
||||
}
|
||||
|
||||
@@ -8,6 +8,8 @@ import (
|
||||
|
||||
"gitea.dev/models/db"
|
||||
"gitea.dev/modules/timeutil"
|
||||
|
||||
"xorm.io/builder"
|
||||
)
|
||||
|
||||
// Follow represents relations of user and their followers.
|
||||
@@ -24,7 +26,7 @@ func init() {
|
||||
|
||||
// IsFollowing returns true if user is following followID.
|
||||
func IsFollowing(ctx context.Context, userID, followID int64) bool {
|
||||
has, _ := db.GetEngine(ctx).Get(&Follow{UserID: userID, FollowID: followID})
|
||||
has, _ := db.Exist[Follow](ctx, builder.Eq{"user_id": userID, "follow_id": followID})
|
||||
return has
|
||||
}
|
||||
|
||||
|
||||
@@ -9,6 +9,8 @@ import (
|
||||
|
||||
"gitea.dev/models/db"
|
||||
"gitea.dev/modules/util"
|
||||
|
||||
"xorm.io/builder"
|
||||
)
|
||||
|
||||
// UserOpenID is the list of all OpenID identities of a user.
|
||||
@@ -43,7 +45,7 @@ func isOpenIDUsed(ctx context.Context, uri string) (bool, error) {
|
||||
return true, nil
|
||||
}
|
||||
|
||||
return db.GetEngine(ctx).Get(&UserOpenID{URI: uri})
|
||||
return db.Exist[UserOpenID](ctx, builder.Eq{"uri": uri})
|
||||
}
|
||||
|
||||
// ErrOpenIDAlreadyUsed represents a "OpenIDAlreadyUsed" kind of error.
|
||||
|
||||
@@ -10,6 +10,8 @@ import (
|
||||
|
||||
"gitea.dev/models/db"
|
||||
"gitea.dev/modules/util"
|
||||
|
||||
"xorm.io/builder"
|
||||
)
|
||||
|
||||
// ErrUserRedirectNotExist represents a "UserRedirectNotExist" kind of error.
|
||||
@@ -50,8 +52,8 @@ func init() {
|
||||
// LookupUserRedirect look up userID if a user has a redirect name
|
||||
func LookupUserRedirect(ctx context.Context, userName string) (int64, error) {
|
||||
userName = strings.ToLower(userName)
|
||||
redirect := &Redirect{LowerName: userName}
|
||||
if has, err := db.GetEngine(ctx).Get(redirect); err != nil {
|
||||
redirect, has, err := db.Get[Redirect](ctx, builder.Eq{"lower_name": userName})
|
||||
if err != nil {
|
||||
return 0, err
|
||||
} else if !has {
|
||||
return 0, ErrUserRedirectNotExist{Name: userName}
|
||||
|
||||
@@ -125,8 +125,7 @@ func GetUserSetting(ctx context.Context, userID int64, key string, def ...string
|
||||
return "", err
|
||||
}
|
||||
|
||||
setting := &Setting{UserID: userID, SettingKey: key}
|
||||
has, err := db.GetEngine(ctx).Get(setting)
|
||||
setting, has, err := db.Get[Setting](ctx, builder.Eq{"user_id": userID, "setting_key": key})
|
||||
if err != nil {
|
||||
return "", err
|
||||
}
|
||||
|
||||
@@ -1276,8 +1276,7 @@ func GetUserByEmail(ctx context.Context, email string) (*User, error) {
|
||||
|
||||
email = strings.ToLower(email)
|
||||
// Otherwise, check in alternative list for activated email addresses
|
||||
emailAddress := &EmailAddress{LowerEmail: email, IsActivated: true}
|
||||
has, err := db.GetEngine(ctx).Get(emailAddress)
|
||||
emailAddress, has, err := db.Get[EmailAddress](ctx, builder.Eq{"lower_email": email, "is_activated": true})
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
@@ -1297,13 +1296,29 @@ func GetUserByEmail(ctx context.Context, email string) (*User, error) {
|
||||
return nil, ErrUserNotExist{Name: email}
|
||||
}
|
||||
|
||||
func GetIndividualUser(ctx context.Context, user *User) (bool, error) {
|
||||
// FIXME: the design is wrong, empty User fields won't apply, this function should be removed in the future
|
||||
has, err := db.GetEngine(ctx).Get(user)
|
||||
func GetIndividualUserByPrimaryEmail(ctx context.Context, email string) (*User, error) {
|
||||
email = strings.ToLower(strings.TrimSpace(email))
|
||||
if len(email) == 0 {
|
||||
return nil, ErrUserNotExist{Name: email}
|
||||
}
|
||||
|
||||
user, has, err := db.Get[User](ctx, builder.Eq{"email": email, "type": UserTypeIndividual})
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
if !has {
|
||||
return nil, ErrUserNotExist{Name: email}
|
||||
}
|
||||
return user, nil
|
||||
}
|
||||
|
||||
func GetIndividualUserByLoginSource(ctx context.Context, loginType auth.Type, loginSource int64, loginName string) (*User, bool, error) {
|
||||
user, has, err := db.Get[User](ctx, builder.Eq{"login_type": loginType, "login_source": loginSource, "login_name": loginName})
|
||||
if has && user.Type != UserTypeIndividual {
|
||||
has = false
|
||||
user = nil
|
||||
}
|
||||
return has, err
|
||||
return user, has, err
|
||||
}
|
||||
|
||||
// GetUserByOpenID returns the user object by given OpenID if exists.
|
||||
|
||||
Reference in New Issue
Block a user