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

@@ -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
 	}
 

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 {

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
 ```