refactor: moved hashing logic into application layer for security;

fix: error handling in auth service for database; refactor: removed redundant taken email check; chore: removed todos that were completed/not needed; fix: leaking transactions in complete registration and login on error; refactor: got rid of txless requests during transactions;
2025-07-06 13:01:00 +03:00
parent 5e32c3cbd3
commit 333817c9e1
9 changed files with 177 additions and 121 deletions
--- a/backend/go.mod
+++ b/backend/go.mod
@@ -31,6 +31,7 @@ require (
 	github.com/go-playground/universal-translator v0.18.1 // indirect
 	github.com/go-viper/mapstructure/v2 v2.2.1 // indirect
 	github.com/goccy/go-json v0.10.5 // indirect
+	github.com/jackc/pgerrcode v0.0.0-20240316143900-6e2875d9b438 // indirect
 	github.com/jackc/pgpassfile v1.0.0 // indirect
 	github.com/jackc/pgservicefile v0.0.0-20240606120523-5a60cdf6a761 // indirect
 	github.com/jackc/puddle/v2 v2.2.2 // indirect
--- a/backend/go.sum
+++ b/backend/go.sum
@@ -48,6 +48,8 @@ github.com/golang-jwt/jwt/v5 v5.2.2/go.mod h1:pqrtFR0X4osieyHYxtmOUWsAWrfe1Q5UVI
 github.com/google/go-cmp v0.7.0 h1:wk8382ETsv4JYUZwIsn6YpYiWiBsYLSJiTsyBybVuN8=
 github.com/google/go-cmp v0.7.0/go.mod h1:pXiqmnSA92OHEEa9HXL2W4E7lf9JzCmGVUdgjX3N/iU=
 github.com/google/gofuzz v1.0.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg=
+github.com/jackc/pgerrcode v0.0.0-20240316143900-6e2875d9b438 h1:Dj0L5fhJ9F82ZJyVOmBx6msDp/kfd1t9GRfny/mfJA0=
+github.com/jackc/pgerrcode v0.0.0-20240316143900-6e2875d9b438/go.mod h1:a/s9Lp5W7n/DD0VrVoyJ00FbP2ytTPDVOivvn2bMlds=
 github.com/jackc/pgpassfile v1.0.0 h1:/6Hmqy13Ss2zCq62VdNG8tM1wchn8zjSGOBJ6icpsIM=
 github.com/jackc/pgpassfile v1.0.0/go.mod h1:CEx0iS5ambNFdcRtxPj5JhEz+xB6uRky5eyVu/W2HEg=
 github.com/jackc/pgservicefile v0.0.0-20240606120523-5a60cdf6a761 h1:iCEnooe7UlwOQYpKFhBabPMi4aNAfoODPEFNiAnClxo=
--- a/backend/internal/database/query.sql.go
+++ b/backend/internal/database/query.sql.go
@@ -46,17 +46,17 @@ func (q *Queries) CreateBannedUser(ctx context.Context, arg CreateBannedUserPara

 const createConfirmationCode = `-- name: CreateConfirmationCode :one
 INSERT INTO confirmation_codes(user_id, code_type, code_hash)
-VALUES ($1, $2, crypt($3::text, gen_salt('bf'))) RETURNING id, user_id, code_type, code_hash, expires_at, used, deleted
+VALUES ($1, $2, $3) RETURNING id, user_id, code_type, code_hash, expires_at, used, deleted
 `

 type CreateConfirmationCodeParams struct {
 	UserID   int64
 	CodeType int32
-	Code     string
+	CodeHash string
 }

 func (q *Queries) CreateConfirmationCode(ctx context.Context, arg CreateConfirmationCodeParams) (ConfirmationCode, error) {
-	row := q.db.QueryRow(ctx, createConfirmationCode, arg.UserID, arg.CodeType, arg.Code)
+	row := q.db.QueryRow(ctx, createConfirmationCode, arg.UserID, arg.CodeType, arg.CodeHash)
 	var i ConfirmationCode
 	err := row.Scan(
 		&i.ID,
@@ -72,17 +72,17 @@ func (q *Queries) CreateConfirmationCode(ctx context.Context, arg CreateConfirma

 const createLoginInformation = `-- name: CreateLoginInformation :one
 INSERT INTO login_informations(user_id, email, password_hash)
-VALUES ( $1, $2, crypt($3::text, gen_salt('bf')) ) RETURNING id, user_id, email, password_hash, totp_encrypted, email_2fa_enabled, password_change_date
+VALUES ( $1, $2, $3::text ) RETURNING id, user_id, email, password_hash, totp_encrypted, email_2fa_enabled, password_change_date
 `

 type CreateLoginInformationParams struct {
-	UserID   int64
-	Email    *string
-	Password string
+	UserID       int64
+	Email        *string
+	PasswordHash string
 }

 func (q *Queries) CreateLoginInformation(ctx context.Context, arg CreateLoginInformationParams) (LoginInformation, error) {
-	row := q.db.QueryRow(ctx, createLoginInformation, arg.UserID, arg.Email, arg.Password)
+	row := q.db.QueryRow(ctx, createLoginInformation, arg.UserID, arg.Email, arg.PasswordHash)
 	var i LoginInformation
 	err := row.Scan(
 		&i.ID,
@@ -229,37 +229,6 @@ func (q *Queries) DeleteUserByUsername(ctx context.Context, username string) err
 	return err
 }

-const getConfirmationCodeByCode = `-- name: GetConfirmationCodeByCode :one
-SELECT id, user_id, code_type, code_hash, expires_at, used, deleted FROM confirmation_codes
-WHERE 
-  user_id = $1 AND
-  code_type = $2 AND
-  expires_at > CURRENT_TIMESTAMP AND
-  used IS FALSE AND
-  code_hash = crypt($3::text, code_hash)
-`
-
-type GetConfirmationCodeByCodeParams struct {
-	UserID   int64
-	CodeType int32
-	Code     string
-}
-
-func (q *Queries) GetConfirmationCodeByCode(ctx context.Context, arg GetConfirmationCodeByCodeParams) (ConfirmationCode, error) {
-	row := q.db.QueryRow(ctx, getConfirmationCodeByCode, arg.UserID, arg.CodeType, arg.Code)
-	var i ConfirmationCode
-	err := row.Scan(
-		&i.ID,
-		&i.UserID,
-		&i.CodeType,
-		&i.CodeHash,
-		&i.ExpiresAt,
-		&i.Used,
-		&i.Deleted,
-	)
-	return i, err
-}
-
 const getLoginInformationByUsername = `-- name: GetLoginInformationByUsername :one
 SELECT login_informations.id, login_informations.user_id, login_informations.email, login_informations.password_hash, login_informations.totp_encrypted, login_informations.email_2fa_enabled, login_informations.password_change_date FROM login_informations
 JOIN users ON users.id = login_informations.user_id
@@ -550,47 +519,6 @@ func (q *Queries) GetUserByEmail(ctx context.Context, email string) (User, error
 	return i, err
 }

-const getUserByLoginCredentials = `-- name: GetUserByLoginCredentials :one
-SELECT
-  users.id,
-  users.username,
-  linfo.password_hash,
-  linfo.totp_encrypted
-FROM users
-JOIN login_informations AS linfo ON users.id = linfo.user_id
-LEFT JOIN banned_users AS banned ON users.id = banned.user_id
-WHERE 
-  users.username = $1 AND
-  users.verified IS TRUE AND -- Verified
-  users.deleted IS FALSE AND -- Not deleted
-  banned.user_id IS NULL AND -- Not banned
-  linfo.password_hash = crypt($2::text, linfo.password_hash)
-`
-
-type GetUserByLoginCredentialsParams struct {
-	Username string
-	Password string
-}
-
-type GetUserByLoginCredentialsRow struct {
-	ID            int64
-	Username      string
-	PasswordHash  string
-	TotpEncrypted *string
-}
-
-func (q *Queries) GetUserByLoginCredentials(ctx context.Context, arg GetUserByLoginCredentialsParams) (GetUserByLoginCredentialsRow, error) {
-	row := q.db.QueryRow(ctx, getUserByLoginCredentials, arg.Username, arg.Password)
-	var i GetUserByLoginCredentialsRow
-	err := row.Scan(
-		&i.ID,
-		&i.Username,
-		&i.PasswordHash,
-		&i.TotpEncrypted,
-	)
-	return i, err
-}
-
 const getUserByUsername = `-- name: GetUserByUsername :one
 SELECT id, username, verified, registration_date, deleted FROM users 
 WHERE username = $1
@@ -644,6 +572,78 @@ func (q *Queries) GetUserSessions(ctx context.Context, userID int64) ([]Session,
 	return items, nil
 }

+const getValidConfirmationCodeByCode = `-- name: GetValidConfirmationCodeByCode :one
+SELECT id, user_id, code_type, code_hash, expires_at, used, deleted FROM confirmation_codes
+WHERE 
+  user_id = $1 AND
+  code_type = $2 AND
+  expires_at > CURRENT_TIMESTAMP AND
+  used IS FALSE AND
+  code_hash = crypt($3::text, code_hash)
+`
+
+type GetValidConfirmationCodeByCodeParams struct {
+	UserID   int64
+	CodeType int32
+	Code     string
+}
+
+func (q *Queries) GetValidConfirmationCodeByCode(ctx context.Context, arg GetValidConfirmationCodeByCodeParams) (ConfirmationCode, error) {
+	row := q.db.QueryRow(ctx, getValidConfirmationCodeByCode, arg.UserID, arg.CodeType, arg.Code)
+	var i ConfirmationCode
+	err := row.Scan(
+		&i.ID,
+		&i.UserID,
+		&i.CodeType,
+		&i.CodeHash,
+		&i.ExpiresAt,
+		&i.Used,
+		&i.Deleted,
+	)
+	return i, err
+}
+
+const getValidUserByLoginCredentials = `-- name: GetValidUserByLoginCredentials :one
+SELECT
+  users.id,
+  users.username,
+  linfo.password_hash,
+  linfo.totp_encrypted
+FROM users
+JOIN login_informations AS linfo ON users.id = linfo.user_id
+LEFT JOIN banned_users AS banned ON users.id = banned.user_id
+WHERE 
+  users.username = $1 AND
+  users.verified IS TRUE AND -- Verified
+  users.deleted IS FALSE AND -- Not deleted
+  banned.user_id IS NULL AND -- Not banned
+  linfo.password_hash = crypt($2::text, linfo.password_hash)
+`
+
+type GetValidUserByLoginCredentialsParams struct {
+	Username string
+	Password string
+}
+
+type GetValidUserByLoginCredentialsRow struct {
+	ID            int64
+	Username      string
+	PasswordHash  string
+	TotpEncrypted *string
+}
+
+func (q *Queries) GetValidUserByLoginCredentials(ctx context.Context, arg GetValidUserByLoginCredentialsParams) (GetValidUserByLoginCredentialsRow, error) {
+	row := q.db.QueryRow(ctx, getValidUserByLoginCredentials, arg.Username, arg.Password)
+	var i GetValidUserByLoginCredentialsRow
+	err := row.Scan(
+		&i.ID,
+		&i.Username,
+		&i.PasswordHash,
+		&i.TotpEncrypted,
+	)
+	return i, err
+}
+
 const pruneExpiredConfirmationCodes = `-- name: PruneExpiredConfirmationCodes :exec
 DELETE FROM confirmation_codes
 WHERE expires_at < CURRENT_TIMESTAMP
--- a/backend/internal/errors/postgres.go
+++ b/backend/internal/errors/postgres.go
@@ -0,0 +1,17 @@
+package errors
+
+import (
+	"errors"
+
+	"github.com/jackc/pgx/v5/pgconn"
+)
+
+func GetPgError(err error) string {
+	var pgErr *pgconn.PgError
+	errors.As(err, &pgErr)
+	return pgErr.Code
+}
+
+func MatchPgError(err error, code string) bool {
+	return GetPgError(err) == code
+}
--- a/backend/internal/models/auth.go
+++ b/backend/internal/models/auth.go
@@ -33,7 +33,6 @@ type RegistrationCompleteRequest struct {
 	VerificationCode	string  `json:"verification_code" binding:"required"`
 	Name							string  `json:"name" binding:"required" validate:"name"`
 	Birthday					*string `json:"birthday"`
-	AvatarUrl					*string `json:"avatar_url" binding:"http_url,max=255"`
 }

 type RegistrationCompleteResponse struct {
--- a/backend/internal/services/auth.go
+++ b/backend/internal/services/auth.go
@@ -25,6 +25,7 @@ import (
 	"easywish/internal/utils/enums"
 	"errors"

+	"github.com/jackc/pgerrcode"
 	"github.com/jackc/pgx/v5"
 	"go.uber.org/zap"
 )
@@ -49,6 +50,8 @@ func (a *authServiceImpl) RegistrationBegin(request models.RegistrationBeginRequ

 	var user database.User
 	var generatedCode string
+	var generatedCodeHash string
+	var passwordHash string

 	helper, db, _ := database.NewDbHelperTransaction(a.dbctx)
  defer helper.Rollback()
@@ -56,46 +59,37 @@ func (a *authServiceImpl) RegistrationBegin(request models.RegistrationBeginRequ
 	var err error

 	if user, err = db.TXQueries.CreateUser(db.CTX, request.Username); err != nil {
+		
+			if errs.MatchPgError(err, pgerrcode.UniqueViolation) {
+				a.log.Warn(
+					"Attempted registration for a taken username",
+					zap.String("username", request.Username),
+					zap.Error(err))

-		if errors.Is(err, pgx.ErrNoRows) {
-			a.log.Warn(
-				"Attempted registration for a taken username",
-				zap.String("username", request.Username),
-				zap.Error(err))
-			return false, errs.ErrUsernameTaken
-		}
+				return false, errs.ErrUsernameTaken
+			}

 		a.log.Error("Failed to add user to database", zap.Error(err))
 		return false, errs.ErrServerError
 	}

-	if _, emailerr := db.TXQueries.GetUserByEmail(db.CTX, *request.Email); emailerr == nil {
-		a.log.Warn(
-			"Attempted registration for a taken email",
-			zap.String("email", *request.Email))
-		return false, errs.ErrEmailTaken
+	if passwordHash, err = utils.HashPassword(request.Password); err != nil {
+		a.log.Error("Error on hashing password", zap.Error(err))

-	} else if !errors.Is(emailerr, pgx.ErrNoRows) {
-		a.log.Error(
-			"Failed to check if email is not taken",
-			zap.String("email", *request.Email),
-			zap.Error(err))
 		return false, errs.ErrServerError
-
-	} else {
-		a.log.Debug("Verified that email is not taken", zap.String("email", *request.Email))
 	}

-	a.log.Info(
-		"Registraion of a new user",
-		zap.String("username", user.Username),
-		zap.Int64("id", user.ID))
-
 	if _, err = db.TXQueries.CreateLoginInformation(db.CTX, database.CreateLoginInformationParams{
 		UserID: user.ID,
 		Email: request.Email,
-		Password: request.Password, // Hashed in database
+		PasswordHash: passwordHash, // Hashed in database
 	}); err != nil {
+
+		if errs.MatchPgError(err, pgerrcode.UniqueViolation) {
+			// Since we've already checked for username previously, only email is left
+			return false, errs.ErrEmailTaken
+		}
+
 		a.log.Error("Failed to add login information for user to database", zap.Error(err))
 		return false, errs.ErrServerError
 	}
@@ -104,21 +98,32 @@ func (a *authServiceImpl) RegistrationBegin(request models.RegistrationBeginRequ
 		a.log.Error("Failed to generate a registration code", zap.Error(err))
 		return false, errs.ErrServerError
 	}
+	if generatedCodeHash, err = utils.HashPassword(generatedCode); err != nil {
+		a.log.Error("Failed to hash generated verification code", zap.Error(err))
+		return false, errs.ErrServerError
+	}

 	if _, err = db.TXQueries.CreateConfirmationCode(db.CTX, database.CreateConfirmationCodeParams{
     UserID: user.ID,
 		 CodeType: int32(enums.RegistrationCodeType),
-		 Code: generatedCode, // Hashed in database
+		 CodeHash: generatedCodeHash, // Hashed in database
 	}); err != nil {
 		a.log.Error("Failed to add registration code to database", zap.Error(err))
 		return false, errs.ErrServerError
 	}

-	a.log.Info("Registered a new user", zap.String("username", user.Username))
+	a.log.Info(
+		"Registered a new user",
+		zap.String("username", user.Username),
+		zap.Int64("id", user.ID))

 	helper.Commit()

-	a.log.Debug("Declated registration code for a new user", zap.String("username", user.Username), zap.String("code", generatedCode))
+	// TODO: get rid of this when email verification will start working
+	a.log.Debug(
+		"Declated registration code for a new user",
+		zap.String("username", user.Username),
+		zap.String("code", generatedCode))

 	// TODO: Send verification email

@@ -135,6 +140,7 @@ func (a *authServiceImpl) RegistrationComplete(request models.RegistrationComple
 	var err error

 	helper, db, _ := database.NewDbHelperTransaction(a.dbctx)
+	defer helper.Rollback()

 	user, err = db.TXQueries.GetUserByUsername(db.CTX, request.Username)

@@ -154,7 +160,7 @@ func (a *authServiceImpl) RegistrationComplete(request models.RegistrationComple
 		return nil, errs.ErrServerError
 	}

-	confirmationCode, err = db.TXQueries.GetConfirmationCodeByCode(db.CTX, database.GetConfirmationCodeByCodeParams{
+	confirmationCode, err = db.TXQueries.GetValidConfirmationCodeByCode(db.CTX, database.GetValidConfirmationCodeByCodeParams{
    UserID: user.ID,
 		CodeType: int32(enums.RegistrationCodeType),
 		Code: request.VerificationCode,
@@ -192,6 +198,7 @@ func (a *authServiceImpl) RegistrationComplete(request models.RegistrationComple
 	}

  err = db.TXQueries.UpdateUser(db.CTX, database.UpdateUserParams{
+		ID: user.ID,
 		Verified: utils.NewPointer(true),
 	})

@@ -205,7 +212,6 @@ func (a *authServiceImpl) RegistrationComplete(request models.RegistrationComple
  profile, err = db.TXQueries.CreateProfile(db.CTX, database.CreateProfileParams{
 		UserID: user.ID,
 		Name: request.Name,
-		AvatarUrl: request.AvatarUrl,
 	})

 	if err != nil {
@@ -262,13 +268,12 @@ func (a *authServiceImpl) RegistrationComplete(request models.RegistrationComple
 		RefreshToken: refreshToken,
 	}}

-	return &response, errs.ErrNotImplemented
+	return &response, nil
 }

 // TODO: totp
-// TODO: banned user check
 func (a *authServiceImpl) Login(request models.LoginRequest) (*models.LoginResponse, error) {
-	var userRow database.GetUserByLoginCredentialsRow
+	var userRow database.GetValidUserByLoginCredentialsRow
 	var session database.Session

 	helper, db, _ := database.NewDbHelperTransaction(a.dbctx)
@@ -276,7 +281,7 @@ func (a *authServiceImpl) Login(request models.LoginRequest) (*models.LoginRespo

 	var err error

-	userRow, err = db.TXQueries.GetUserByLoginCredentials(db.CTX, database.GetUserByLoginCredentialsParams{
+	userRow, err = db.TXQueries.GetValidUserByLoginCredentials(db.CTX, database.GetValidUserByLoginCredentialsParams{
 		Username: request.Username,
 		Password: request.Password,
 	})
@@ -298,7 +303,8 @@ func (a *authServiceImpl) Login(request models.LoginRequest) (*models.LoginRespo
 		return nil, returnedError
 	}

-	session, err = db.TXlessQueries.CreateSession(db.CTX, database.CreateSessionParams{
+	session, err = db.TXQueries.CreateSession(db.CTX, database.CreateSessionParams{
+		// TODO: use actual values for session metadata
 		UserID: userRow.ID,
 		Name: utils.NewPointer("New device"),
 		Platform: utils.NewPointer("Unknown"),
--- a/backend/internal/utils/security.go
+++ b/backend/internal/utils/security.go
--- a/backend/internal/utils/hash.go
+++ b/backend/internal/utils/hash.go
@@ -0,0 +1,31 @@
+// Copyright (c) 2025 Nikolai Papin
+// 
+// This file is part of Easywish
+// 
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU 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 General Public License for more details.
+// 
+// You should have received a copy of the GNU General Public License
+// along with this program.  If not, see <https://www.gnu.org/licenses/>.
+
+package utils
+
+import "golang.org/x/crypto/bcrypt"
+
+
+func HashPassword(password string) (string, error) {
+    bytes, err := bcrypt.GenerateFromPassword([]byte(password), 14)
+    return string(bytes), err
+}
+
+func CheckPasswordHash(password, hash string) bool {
+    err := bcrypt.CompareHashAndPassword([]byte(hash), []byte(password))
+    return err == nil
+}