From 2e6a748e98d4af968e8d4fdfe054158bafad1d7d Mon Sep 17 00:00:00 2001 From: patrickhuie19 Date: Tue, 29 Aug 2023 09:34:00 -0400 Subject: [PATCH 1/4] fix/BCF-2497-session-error-log: refactoring sessions orm to avoid a confusing rollback tx error log --- core/sessions/orm.go | 33 ++++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/core/sessions/orm.go b/core/sessions/orm.go index a4060ce487c..521baa8b443 100644 --- a/core/sessions/orm.go +++ b/core/sessions/orm.go @@ -97,23 +97,26 @@ func (o *orm) AuthorizedUserWithSession(sessionID string) (User, error) { } 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 - } + var foundSession struct { + Email string + Valid bool + } - 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") - } + if err := o.q.Get(&foundSession, "SELECT email, last_used + $2 >= now() as valid FROM sessions WHERE id = $1 FOR UPDATE", sessionID, o.sessionDuration); err != nil { + o.lggr.Infof("query result: %v", foundSession) + return User{}, errors.Wrap(err, "no matching user for provided session token") + } + + if !foundSession.Valid { + return User{}, ErrUserSessionExpired + } + + if err := o.q.Get(&user, "SELECT * FROM users WHERE lower(email) = lower($1)", foundSession.Email); err != nil { + return User{}, errors.Wrap(err, "no matching user for provided session email") + } + + err := o.q.Transaction(func(tx pg.Queryer) error { // 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 { From 505f3679b3e8df2ddfd3cf59393ab83f7a969a78 Mon Sep 17 00:00:00 2001 From: patrickhuie19 Date: Tue, 29 Aug 2023 09:43:20 -0400 Subject: [PATCH 2/4] fix/BCF-2497: updating changelog --- docs/CHANGELOG.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) 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 ``` From 00d005d3538ba9b628b70dc962ef9bc5c653eaad Mon Sep 17 00:00:00 2001 From: patrickhuie19 Date: Tue, 5 Sep 2023 08:23:51 -0400 Subject: [PATCH 3/4] fix/BCF-2497: refactoring out redundant query implementations --- core/sessions/orm.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/core/sessions/orm.go b/core/sessions/orm.go index 521baa8b443..9cd45680d34 100644 --- a/core/sessions/orm.go +++ b/core/sessions/orm.go @@ -112,11 +112,12 @@ func (o *orm) AuthorizedUserWithSession(sessionID string) (User, error) { return User{}, ErrUserSessionExpired } - if err := o.q.Get(&user, "SELECT * FROM users WHERE lower(email) = lower($1)", foundSession.Email); err != nil { + user, err := o.findUser(foundSession.Email) + if err != nil { return User{}, errors.Wrap(err, "no matching user for provided session email") } - err := o.q.Transaction(func(tx pg.Queryer) error { + err = o.q.Transaction(func(tx pg.Queryer) error { // 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 { From 825df1a162d2a18450389483d288861d733624e9 Mon Sep 17 00:00:00 2001 From: patrickhuie19 Date: Tue, 5 Sep 2023 10:38:03 -0400 Subject: [PATCH 4/4] fix/BCF-2497: decomposition and redundancy refactoring --- core/sessions/orm.go | 53 ++++++++++++++++++--------------------- core/sessions/orm_test.go | 6 ++--- 2 files changed, 28 insertions(+), 31 deletions(-) diff --git a/core/sessions/orm.go b/core/sessions/orm.go index 9cd45680d34..eaac211f242 100644 --- a/core/sessions/orm.go +++ b/core/sessions/orm.go @@ -86,47 +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 - - var foundSession struct { - Email string - Valid bool - } - - if err := o.q.Get(&foundSession, "SELECT email, last_used + $2 >= now() as valid FROM sessions WHERE id = $1 FOR UPDATE", sessionID, o.sessionDuration); err != nil { - o.lggr.Infof("query result: %v", foundSession) - return User{}, errors.Wrap(err, "no matching user for provided session token") - } - - if !foundSession.Valid { + email, err := o.findValidSession(sessionID) + if err != nil { return User{}, ErrUserSessionExpired } - user, err := o.findUser(foundSession.Email) + user, err = o.findUser(email) if err != nil { - return User{}, errors.Wrap(err, "no matching user for provided session email") + return User{}, ErrUserSessionExpired } - err = o.q.Transaction(func(tx pg.Queryer) error { - // 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 - }) - - if err != nil { + 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 {