fix: listing sessions query #2958
Conversation
persistence/sql/persister_session.go
Outdated
| @@ -78,6 +79,12 @@ func (p *Persister) ListSessions(ctx context.Context, active *bool, paginatorOpt | |||
| q := c.Where("nid = ?", nid) | |||
| if active != nil { | |||
| q = q.Where("active = ?", *active) | |||
|
|
|||
| if *active { | |||
| q.Where("expires_at >= ?", time.Now()) | |||
There was a problem hiding this comment.
| q.Where("expires_at >= ?", time.Now()) | |
| q.Where("expires_at >= ?", time.Now().UTC()) |
persistence/sql/persister_session.go
Outdated
| if *active { | ||
| q.Where("expires_at >= ?", time.Now()) | ||
| } else { | ||
| q.Where("expires_at < ?", time.Now()) |
There was a problem hiding this comment.
| q.Where("expires_at < ?", time.Now()) | |
| q.Where("expires_at < ?", time.Now().UTC()) |
persistence/sql/persister_session.go
Outdated
| @@ -128,12 +135,23 @@ func (p *Persister) ListSessionsByIdentity(ctx context.Context, iID uuid.UUID, a | |||
| nid := p.NetworkID(ctx) | |||
|
|
|||
| if err := p.Transaction(ctx, func(ctx context.Context, c *pop.Connection) error { | |||
| i, err := p.GetIdentity(ctx, iID) | |||
There was a problem hiding this comment.
We're now no longer respecting the expand property and instead fetch the identity always. I don't think that's intended? Please move it back to where it was, but of course outside of the loop to prevent unneccessary queries
if expandables.Has(session.ExpandSessionIdentity) {
i, err := p.GetIdentity(ctx, iID)
// ...
for _, s := range s {
persistence/sql/persister_session.go
Outdated
| q.Where("expires_at >= ?", time.Now()) | ||
| } else { | ||
| q.Where("expires_at < ?", time.Now()) |
There was a problem hiding this comment.
At some point we realized that all DB timestamps need to be UTC
| q.Where("expires_at >= ?", time.Now()) | |
| } else { | |
| q.Where("expires_at < ?", time.Now()) | |
| q.Where("expires_at >= ?", time.Now().UTC()) | |
| } else { | |
| q.Where("expires_at < ?", time.Now().UTC()) |
| @@ -153,17 +153,6 @@ func (s Session) TableName(ctx context.Context) string { | |||
| return "sessions" | |||
| } | |||
|
|
|||
| func (s Session) MarshalJSON() ([]byte, error) { | |||
There was a problem hiding this comment.
I think this is a cleaner solution than setting the field upon loading it from the database, because expiry is time based, and we always want the "freshest" value for Active. Also a bit confused as to why no tests are failing if this is removed
There was a problem hiding this comment.
This was removed because it alters the value of list items based on Identity state after the listing query has run.
As in, a request for ?active=true yields active session records but then this code block alters the value. This may result in result set with active: false due to s.Active && s.ExpiresAt.After(time.Now()) && (s.Identity == nil || s.Identity.IsActive()), which will confuse the API consumer. To try and solve this problem, I've made corrections to the query in eddece5
| result, err := json.Marshal(sl(s)) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return result, nil |
There was a problem hiding this comment.
Could just be:
return json.Marshal(sl(s))
Codecov Report
@@ Coverage Diff @@
## master #2958 +/- ##
==========================================
- Coverage 76.16% 76.16% -0.01%
==========================================
Files 310 309 -1
Lines 19110 19054 -56
==========================================
- Hits 14555 14512 -43
+ Misses 3425 3417 -8
+ Partials 1130 1125 -5
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
7270461
to
eddece5
Compare
9b520b4
to
9148410
Compare
|
Re-organized listing tests to do seed data setup once and run against that |
Changes
Whereclause added to also check session expiry timestamp to return correct listRelated issue(s)
#2930
Checklist
introduces a new feature.
contributing code guidelines.
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got the approval (please contact
security@ory.sh) from the maintainers to push
the changes.
works.
Further Comments