From d043a6b66a1850ca5f54d2b82204aa202442849b Mon Sep 17 00:00:00 2001 From: George MacRorie Date: Wed, 15 Apr 2020 12:28:48 +0100 Subject: [PATCH 1/3] chore(kv): add failing test for session renewal extension --- testing/session.go | 68 ++++++++++++++++++++++++++++++++++++---------- 1 file changed, 53 insertions(+), 15 deletions(-) diff --git a/testing/session.go b/testing/session.go index 809bd69dbff..4f47eecbc7b 100644 --- a/testing/session.go +++ b/testing/session.go @@ -18,19 +18,23 @@ const ( sessionTwoID = "020f755c3c082001" ) -var sessionCmpOptions = cmp.Options{ - cmp.Comparer(func(x, y []byte) bool { - return bytes.Equal(x, y) - }), - cmp.Transformer("Sort", func(in []*platform.Session) []*platform.Session { - out := append([]*platform.Session(nil), in...) // Copy input to avoid mutating it - sort.Slice(out, func(i, j int) bool { - return out[i].ID.String() > out[j].ID.String() - }) - return out - }), - cmpopts.IgnoreFields(platform.Session{}, "CreatedAt", "ExpiresAt", "Permissions"), - cmpopts.EquateEmpty(), +var sessionCmpOptions = sessionCompareOptions("CreatedAt", "ExpiresAt", "Permissions") + +func sessionCompareOptions(ignore ...string) cmp.Options { + return cmp.Options{ + cmp.Comparer(func(x, y []byte) bool { + return bytes.Equal(x, y) + }), + cmp.Transformer("Sort", func(in []*platform.Session) []*platform.Session { + out := append([]*platform.Session(nil), in...) // Copy input to avoid mutating it + sort.Slice(out, func(i, j int) bool { + return out[i].ID.String() > out[j].ID.String() + }) + return out + }), + cmpopts.IgnoreFields(platform.Session{}, ignore...), + cmpopts.EquateEmpty(), + } } // SessionFields will include the IDGenerator, TokenGenerator, Sessions, and Users @@ -336,6 +340,39 @@ func RenewSession( }, }, }, + { + name: "renew session with an earlier time than existing expiration", + fields: SessionFields{ + IDGenerator: mock.NewIDGenerator(sessionTwoID, t), + TokenGenerator: mock.NewTokenGenerator("abc123xyz", nil), + Sessions: []*platform.Session{ + { + ID: MustIDBase16(sessionOneID), + UserID: MustIDBase16(sessionTwoID), + Key: "abc123xyz", + ExpiresAt: time.Date(2031, 9, 26, 0, 0, 0, 0, time.UTC), + }, + }, + }, + args: args{ + session: &platform.Session{ + ID: MustIDBase16(sessionOneID), + UserID: MustIDBase16(sessionTwoID), + Key: "abc123xyz", + ExpiresAt: time.Date(2031, 9, 26, 0, 0, 0, 0, time.UTC), + }, + key: "abc123xyz", + expireAt: time.Date(2030, 9, 26, 0, 0, 10, 0, time.UTC), + }, + wants: wants{ + session: &platform.Session{ + ID: MustIDBase16(sessionOneID), + UserID: MustIDBase16(sessionTwoID), + Key: "abc123xyz", + ExpiresAt: time.Date(2031, 9, 26, 0, 0, 10, 0, time.UTC), + }, + }, + }, { name: "renew nil session", fields: SessionFields{ @@ -364,7 +401,7 @@ func RenewSession( ID: MustIDBase16(sessionOneID), UserID: MustIDBase16(sessionTwoID), Key: "abc123xyz", - ExpiresAt: time.Date(2031, 9, 26, 0, 0, 10, 0, time.UTC), + ExpiresAt: time.Date(2030, 9, 26, 0, 0, 0, 0, time.UTC), }, }, }, @@ -384,7 +421,8 @@ func RenewSession( t.Errorf("err in find session %v", err) } - if diff := cmp.Diff(session, tt.wants.session, sessionCmpOptions...); diff != "" { + cmpOptions := sessionCompareOptions("CreatedAt", "Permissions") + if diff := cmp.Diff(session, tt.wants.session, cmpOptions...); diff != "" { t.Errorf("session is different -got/+want\ndiff %s", diff) } }) From 62928d773ab1361cb986581e7046834c36f2b2b4 Mon Sep 17 00:00:00 2001 From: George MacRorie Date: Wed, 15 Apr 2020 12:36:44 +0100 Subject: [PATCH 2/3] fix(kv): ensure renew session only updates expiration if it is newer than existing --- kv/session.go | 24 ++++++++++++++++++++++-- testing/session.go | 4 ++-- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/kv/session.go b/kv/session.go index 8bfe9624c66..3f4d3e49c73 100644 --- a/kv/session.go +++ b/kv/session.go @@ -28,13 +28,33 @@ func (s *Service) RenewSession(ctx context.Context, session *influxdb.Session, n Msg: "session is nil", } } + + // session already has longer expiration + if newExpiration.Before(session.ExpiresAt) { + return nil + } + return s.kv.Update(ctx, func(tx Tx) error { - session.ExpiresAt = newExpiration - if err := s.putSession(ctx, tx, session); err != nil { + sess, err := s.findSession(ctx, tx, session.Key) + if err != nil { + return err + } + + // session already has longer expiration + if newExpiration.Before(session.ExpiresAt) { + return nil + } + + sess.ExpiresAt = newExpiration + + if err := s.putSession(ctx, tx, sess); err != nil { return &influxdb.Error{ Err: err, } } + + *session = *sess + return nil }) } diff --git a/testing/session.go b/testing/session.go index 4f47eecbc7b..e926caecc94 100644 --- a/testing/session.go +++ b/testing/session.go @@ -362,14 +362,14 @@ func RenewSession( ExpiresAt: time.Date(2031, 9, 26, 0, 0, 0, 0, time.UTC), }, key: "abc123xyz", - expireAt: time.Date(2030, 9, 26, 0, 0, 10, 0, time.UTC), + expireAt: time.Date(2030, 9, 26, 0, 0, 0, 0, time.UTC), }, wants: wants{ session: &platform.Session{ ID: MustIDBase16(sessionOneID), UserID: MustIDBase16(sessionTwoID), Key: "abc123xyz", - ExpiresAt: time.Date(2031, 9, 26, 0, 0, 10, 0, time.UTC), + ExpiresAt: time.Date(2031, 9, 26, 0, 0, 0, 0, time.UTC), }, }, }, From 545ace283dca88dca5f422d13449ea08e32edd8c Mon Sep 17 00:00:00 2001 From: George MacRorie Date: Wed, 15 Apr 2020 12:44:02 +0100 Subject: [PATCH 3/3] chore: update changelog to reflect renew session expiration fix --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6c2fb4d100a..0246f79b4ae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### Bug Fixes 1. [17618](https://github.com/influxdata/influxdb/pull/17618): Add index for URM by user ID to improve lookup performance +1. [17751](https://github.com/influxdata/influxdb/pull/17751): Existing session expiration time is respected on session renewal ### UI Improvements