From 6bf22fa8647f6b7c6bb34fc9daf2e42620bec903 Mon Sep 17 00:00:00 2001 From: Neeraj Gupta <254676+ua741@users.noreply.github.com> Date: Tue, 9 Apr 2024 11:07:59 +0530 Subject: [PATCH 1/3] [server] Fix unique constraint for ott for multiple apps --- server/migrations/83_fix_ott_index.down.sql | 11 +++++++++++ server/migrations/83_fix_ott_index.up.sql | 2 ++ 2 files changed, 13 insertions(+) create mode 100644 server/migrations/83_fix_ott_index.down.sql create mode 100644 server/migrations/83_fix_ott_index.up.sql diff --git a/server/migrations/83_fix_ott_index.down.sql b/server/migrations/83_fix_ott_index.down.sql new file mode 100644 index 000000000..571df81a5 --- /dev/null +++ b/server/migrations/83_fix_ott_index.down.sql @@ -0,0 +1,11 @@ + +-- Add unique index on otts for email_hash,app,ott +BEGIN; +ALTER TABLE + otts DROP CONSTRAINT unique_otts_emailhash_ott; + +ALTER TABLE + otts + ADD + CONSTRAINT unique_otts_emailhash_app_ott UNIQUE (ott,app, email_hash); +COMMIT; diff --git a/server/migrations/83_fix_ott_index.up.sql b/server/migrations/83_fix_ott_index.up.sql new file mode 100644 index 000000000..096ded0d7 --- /dev/null +++ b/server/migrations/83_fix_ott_index.up.sql @@ -0,0 +1,2 @@ +DROP TRIGGER IF EXISTS update_location_tag_updated_at ON location_tag; +DROP TABLE location_tag; From 73eacfb30dc735f484d9e986ecdc991abdcb4469 Mon Sep 17 00:00:00 2001 From: Neeraj Gupta <254676+ua741@users.noreply.github.com> Date: Tue, 9 Apr 2024 11:14:44 +0530 Subject: [PATCH 2/3] [server] Use correct app while updating ott table --- server/migrations/83_fix_ott_index.down.sql | 8 +++----- server/migrations/83_fix_ott_index.up.sql | 11 +++++++++-- server/pkg/controller/user/userauth.go | 6 +++--- server/pkg/repo/userauth.go | 16 ++++++++-------- 4 files changed, 23 insertions(+), 18 deletions(-) diff --git a/server/migrations/83_fix_ott_index.down.sql b/server/migrations/83_fix_ott_index.down.sql index 571df81a5..5eab1e6a2 100644 --- a/server/migrations/83_fix_ott_index.down.sql +++ b/server/migrations/83_fix_ott_index.down.sql @@ -1,11 +1,9 @@ - --- Add unique index on otts for email_hash,app,ott BEGIN; ALTER TABLE - otts DROP CONSTRAINT unique_otts_emailhash_ott; + otts DROP CONSTRAINT IF EXISTS unique_otts_emailhash_app_ott; ALTER TABLE otts ADD - CONSTRAINT unique_otts_emailhash_app_ott UNIQUE (ott,app, email_hash); -COMMIT; + CONSTRAINT unique_otts_emailhash_ott UNIQUE (ott, email_hash); +COMMIT; \ No newline at end of file diff --git a/server/migrations/83_fix_ott_index.up.sql b/server/migrations/83_fix_ott_index.up.sql index 096ded0d7..4c9015f85 100644 --- a/server/migrations/83_fix_ott_index.up.sql +++ b/server/migrations/83_fix_ott_index.up.sql @@ -1,2 +1,9 @@ -DROP TRIGGER IF EXISTS update_location_tag_updated_at ON location_tag; -DROP TABLE location_tag; +BEGIN; +ALTER TABLE + otts DROP CONSTRAINT IF EXISTS unique_otts_emailhash_ott; + +ALTER TABLE + otts + ADD + CONSTRAINT unique_otts_emailhash_app_ott UNIQUE (ott,app, email_hash); +COMMIT; \ No newline at end of file diff --git a/server/pkg/controller/user/userauth.go b/server/pkg/controller/user/userauth.go index bbc9942de..7247bb18f 100644 --- a/server/pkg/controller/user/userauth.go +++ b/server/pkg/controller/user/userauth.go @@ -140,7 +140,7 @@ func (c *UserController) verifyEmailOtt(context *gin.Context, email string, ott if err != nil { return stacktrace.Propagate(err, "") } - wrongAttempt, err := c.UserAuthRepo.GetMaxWrongAttempts(emailHash) + wrongAttempt, err := c.UserAuthRepo.GetMaxWrongAttempts(emailHash, auth.GetApp(context)) if err != nil { return stacktrace.Propagate(err, "") } @@ -166,12 +166,12 @@ func (c *UserController) verifyEmailOtt(context *gin.Context, email string, ott } } if !isValidOTT { - if err = c.UserAuthRepo.RecordWrongAttemptForActiveOtt(emailHash); err != nil { + if err = c.UserAuthRepo.RecordWrongAttemptForActiveOtt(emailHash, auth.GetApp(context)); err != nil { log.WithError(err).Warn("Failed to track wrong attempt") } return stacktrace.Propagate(ente.ErrIncorrectOTT, "") } - err = c.UserAuthRepo.RemoveOTT(emailHash, ott) + err = c.UserAuthRepo.RemoveOTT(emailHash, ott, auth.GetApp(context)) if err != nil { return stacktrace.Propagate(err, "") } diff --git a/server/pkg/repo/userauth.go b/server/pkg/repo/userauth.go index c5f86e8ec..c182e9e87 100644 --- a/server/pkg/repo/userauth.go +++ b/server/pkg/repo/userauth.go @@ -20,14 +20,14 @@ type UserAuthRepository struct { func (repo *UserAuthRepository) AddOTT(emailHash string, app ente.App, ott string, expirationTime int64) error { _, err := repo.DB.Exec(`INSERT INTO otts(email_hash, ott, creation_time, expiration_time, app) VALUES($1, $2, $3, $4, $5) - ON CONFLICT ON CONSTRAINT unique_otts_emailhash_ott DO UPDATE SET creation_time = $3, expiration_time = $4`, + ON CONFLICT ON CONSTRAINT unique_otts_emailhash_app_ott DO UPDATE SET creation_time = $3, expiration_time = $4`, emailHash, ott, time.Microseconds(), expirationTime, app) return stacktrace.Propagate(err, "") } // RemoveOTT removes the specified OTT (to be used when an OTT has been consumed) -func (repo *UserAuthRepository) RemoveOTT(emailHash string, ott string) error { - _, err := repo.DB.Exec(`DELETE FROM otts WHERE email_hash = $1 AND ott = $2`, emailHash, ott) +func (repo *UserAuthRepository) RemoveOTT(emailHash string, ott string, app ente.App) error { + _, err := repo.DB.Exec(`DELETE FROM otts WHERE email_hash = $1 AND ott = $2 AND app = $3`, emailHash, ott, app) return stacktrace.Propagate(err, "") } @@ -69,9 +69,9 @@ func (repo *UserAuthRepository) GetValidOTTs(emailHash string, app ente.App) ([] return otts, nil } -func (repo *UserAuthRepository) GetMaxWrongAttempts(emailHash string) (int, error) { - row := repo.DB.QueryRow(`SELECT COALESCE(MAX(wrong_attempt),0) FROM otts WHERE email_hash = $1 AND expiration_time > $2`, - emailHash, time.Microseconds()) +func (repo *UserAuthRepository) GetMaxWrongAttempts(emailHash string, app ente.App) (int, error) { + row := repo.DB.QueryRow(`SELECT COALESCE(MAX(wrong_attempt),0) FROM otts WHERE email_hash = $1 AND expiration_time > $2 AND app = $3`, + emailHash, time.Microseconds(), app) var wrongAttempt int if err := row.Scan(&wrongAttempt); err != nil { return 0, stacktrace.Propagate(err, "Failed to scan row") @@ -81,9 +81,9 @@ func (repo *UserAuthRepository) GetMaxWrongAttempts(emailHash string) (int, erro // RecordWrongAttemptForActiveOtt increases the wrong_attempt count for given emailHash and active ott. // Assuming tha we keep deleting expired OTT, max(wrong_attempt) can be used to track brute-force attack -func (repo *UserAuthRepository) RecordWrongAttemptForActiveOtt(emailHash string) error { +func (repo *UserAuthRepository) RecordWrongAttemptForActiveOtt(emailHash string, app ente.App) error { _, err := repo.DB.Exec(`UPDATE otts SET wrong_attempt = otts.wrong_attempt + 1 - WHERE email_hash = $1 AND expiration_time > $2`, emailHash, time.Microseconds()) + WHERE email_hash = $1 AND expiration_time > $2 AND app=$3`, emailHash, time.Microseconds(), app) if err != nil { return stacktrace.Propagate(err, "Failed to update wrong attempt count") } From 46188313adfe5c916c0942f6936c3495e6867c91 Mon Sep 17 00:00:00 2001 From: Neeraj Gupta <254676+ua741@users.noreply.github.com> Date: Tue, 9 Apr 2024 11:20:45 +0530 Subject: [PATCH 3/3] Minor refactor --- server/pkg/controller/user/userauth.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/server/pkg/controller/user/userauth.go b/server/pkg/controller/user/userauth.go index 7247bb18f..087e00194 100644 --- a/server/pkg/controller/user/userauth.go +++ b/server/pkg/controller/user/userauth.go @@ -136,23 +136,24 @@ func (c *UserController) SendEmailOTT(context *gin.Context, email string, client // verifyEmailOtt should be deprecated in favor of verifyEmailOttWithSession once clients are updated. func (c *UserController) verifyEmailOtt(context *gin.Context, email string, ott string) error { ott = strings.TrimSpace(ott) + app := auth.GetApp(context) emailHash, err := crypto.GetHash(email, c.HashingKey) if err != nil { return stacktrace.Propagate(err, "") } - wrongAttempt, err := c.UserAuthRepo.GetMaxWrongAttempts(emailHash, auth.GetApp(context)) + wrongAttempt, err := c.UserAuthRepo.GetMaxWrongAttempts(emailHash, app) if err != nil { return stacktrace.Propagate(err, "") } if wrongAttempt >= OTTWrongAttemptLimit { - msg := "Too many wrong attempts for ott verification" + msg := fmt.Sprintf("Too many wrong ott verification attemp for app %s", app) go c.DiscordController.NotifyPotentialAbuse(msg) return stacktrace.Propagate(ente.ErrTooManyBadRequest, "User needs to wait before active ott are expired") } - otts, err := c.UserAuthRepo.GetValidOTTs(emailHash, auth.GetApp(context)) - log.Info("Valid otts for " + emailHash + " are " + strings.Join(otts, ",")) + otts, err := c.UserAuthRepo.GetValidOTTs(emailHash, app) + log.Infof("Valid ott (app: %s) for %s are %s", app, emailHash, strings.Join(otts, ",")) if err != nil { return stacktrace.Propagate(err, "") } @@ -166,12 +167,12 @@ func (c *UserController) verifyEmailOtt(context *gin.Context, email string, ott } } if !isValidOTT { - if err = c.UserAuthRepo.RecordWrongAttemptForActiveOtt(emailHash, auth.GetApp(context)); err != nil { + if err = c.UserAuthRepo.RecordWrongAttemptForActiveOtt(emailHash, app); err != nil { log.WithError(err).Warn("Failed to track wrong attempt") } return stacktrace.Propagate(ente.ErrIncorrectOTT, "") } - err = c.UserAuthRepo.RemoveOTT(emailHash, ott, auth.GetApp(context)) + err = c.UserAuthRepo.RemoveOTT(emailHash, ott, app) if err != nil { return stacktrace.Propagate(err, "") }