From 6f8a302fe11f5409af6aaeca5d8f7110e6bd6ae4 Mon Sep 17 00:00:00 2001 From: Patrick Date: Wed, 6 Sep 2023 14:25:45 -0400 Subject: [PATCH] BCF-2497: Refactoring sessions orm query pattern to avoid a confusing rollback tx error log (#10378) * fix/BCF-2497-session-error-log: refactoring sessions orm to avoid a confusing rollback tx error log * fix/BCF-2497: updating changelog * fix/BCF-2497: refactoring out redundant query implementations * fix/BCF-2497: decomposition and redundancy refactoring --- core/sessions/orm.go | 57 ++++++++++++++++++++------------------- core/sessions/orm_test.go | 6 ++--- docs/CHANGELOG.md | 5 ++-- 3 files changed, 35 insertions(+), 33 deletions(-) diff --git a/core/sessions/orm.go b/core/sessions/orm.go index a4060ce487c..eaac211f242 100644 --- a/core/sessions/orm.go +++ b/core/sessions/orm.go @@ -86,43 +86,44 @@ func (o *orm) ListUsers() (users []User, err error) { return } +// findValidSession finds an unexpired session by its ID and returns the associated email. +func (o *orm) findValidSession(sessionID string) (email string, err error) { + if err := o.q.Get(&email, "SELECT email FROM sessions WHERE id = $1 AND last_used + $2 >= now() FOR UPDATE", sessionID, o.sessionDuration); err != nil { + o.lggr.Infof("query result: %v", email) + return email, errors.Wrap(err, "no matching user for provided session token") + } + return email, nil +} + +// updateSessionLastUsed updates a session by its ID and sets the LastUsed field to now(). +func (o *orm) updateSessionLastUsed(sessionID string) error { + return o.q.ExecQ("UPDATE sessions SET last_used = now() WHERE id = $1", sessionID) +} + // ErrUserSessionExpired defines the error triggered when the user session has expired -var ErrUserSessionExpired = errors.New("session missing or expired, please login again") +var ( + ErrUserSessionExpired = errors.New("user session missing or expired, please login again") + ErrEmptySessionID = errors.New("session ID cannot be empty") +) // AuthorizedUserWithSession will return the API user associated with the Session ID if it // exists and hasn't expired, and update session's LastUsed field. -func (o *orm) AuthorizedUserWithSession(sessionID string) (User, error) { +func (o *orm) AuthorizedUserWithSession(sessionID string) (user User, err error) { if len(sessionID) == 0 { - return User{}, errors.New("Session ID cannot be empty") + return User{}, ErrEmptySessionID } - var user User - err := o.q.Transaction(func(tx pg.Queryer) error { - // First find user based on session token - var foundSession struct { - Email string - Valid bool - } - if err := tx.Get(&foundSession, "SELECT email, last_used + $2 >= now() as valid FROM sessions WHERE id = $1 FOR UPDATE", sessionID, o.sessionDuration); err != nil { - return errors.Wrap(err, "no matching user for provided session token") - } - - if !foundSession.Valid { - return ErrUserSessionExpired - } - - if err := tx.Get(&user, "SELECT * FROM users WHERE lower(email) = lower($1)", foundSession.Email); err != nil { - return errors.Wrap(err, "no matching user for provided session email") - } - // Session valid and tied to user, update last_used - _, err := tx.Exec("UPDATE sessions SET last_used = now() WHERE id = $1 AND last_used + $2 >= now()", sessionID, o.sessionDuration) - if err != nil { - return errors.Wrap(err, "unable to update sessions table") - } - return nil - }) + email, err := o.findValidSession(sessionID) + if err != nil { + return User{}, ErrUserSessionExpired + } + user, err = o.findUser(email) if err != nil { + return User{}, ErrUserSessionExpired + } + + if err := o.updateSessionLastUsed(sessionID); err != nil { return User{}, err } diff --git a/core/sessions/orm_test.go b/core/sessions/orm_test.go index 681b50987bf..804ea2dbb87 100644 --- a/core/sessions/orm_test.go +++ b/core/sessions/orm_test.go @@ -59,9 +59,9 @@ func TestORM_AuthorizedUserWithSession(t *testing.T) { wantEmail string }{ {"authorized", "correctID", cltest.MustParseDuration(t, "3m"), "", "have@email"}, - {"expired", "correctID", cltest.MustParseDuration(t, "0m"), "session missing or expired, please login again", ""}, - {"incorrect", "wrong", cltest.MustParseDuration(t, "3m"), "no matching user for provided session token: sql: no rows in result set", ""}, - {"empty", "", cltest.MustParseDuration(t, "3m"), "Session ID cannot be empty", ""}, + {"expired", "correctID", cltest.MustParseDuration(t, "0m"), sessions.ErrUserSessionExpired.Error(), ""}, + {"incorrect", "wrong", cltest.MustParseDuration(t, "3m"), sessions.ErrUserSessionExpired.Error(), ""}, + {"empty", "", cltest.MustParseDuration(t, "3m"), sessions.ErrEmptySessionID.Error(), ""}, } for _, test := range tests { diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 93ae8b611f5..f75628dcfb5 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -10,9 +10,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [dev] ### Fixed + - Unauthenticated users executing CLI commands previously generated a confusing error log, which is now removed: +```[ERROR] Error in transaction, rolling back: session missing or expired, please login again pg/transaction.go:118 ``` - Fixed a bug that was preventing job runs to be displayed when the job `chainID` was disabled. - +... ## 2.5.0 - UNRELEASED ### Fixed @@ -21,7 +23,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Upcoming Required Configuration Change - Starting in 2.6.0, chainlink nodes will no longer allow insecure configuration for production builds. Any TOML configuration that sets the following line will fail validation checks in `node start` or `node validate`: - ``` AllowSimplePasswords=true ```