fix: listing sessions query #2958

kelkarajay · 2022-12-15T15:28:50Z

Changes

Where clause added to also check session expiry timestamp to return correct list
Fixed to query identity only once
Remove marshalling mutation

Related issue(s)

#2930

Checklist

I have read the contributing guidelines.
I have referenced an issue containing the design document if my change
introduces a new feature.
I am following the
contributing code guidelines.
I have read the security policy.
I confirm that this pull request does not address a security
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.
I have added tests that prove my fix is effective or that my feature
works.
I have added or changed the documentation.

Further Comments

persistence/sql/persister_session.go

aeneasr · 2022-12-17T09:39:49Z

persistence/sql/persister_session.go

@@ -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())


Suggested change

q.Where("expires_at >= ?", time.Now())

q.Where("expires_at >= ?", time.Now().UTC())

aeneasr · 2022-12-17T09:39:55Z

persistence/sql/persister_session.go

+			if *active {
+				q.Where("expires_at >= ?", time.Now())
+			} else {
+				q.Where("expires_at < ?", time.Now())


Suggested change

q.Where("expires_at < ?", time.Now())

q.Where("expires_at < ?", time.Now().UTC())

aeneasr · 2022-12-17T09:42:12Z

persistence/sql/persister_session.go

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


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 {

aeneasr · 2022-12-17T09:42:36Z

persistence/sql/persister_session.go

+				q.Where("expires_at >= ?", time.Now())
+			} else {
+				q.Where("expires_at < ?", time.Now())


At some point we realized that all DB timestamps need to be UTC

Suggested change

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())

aeneasr · 2022-12-17T09:46:12Z

session/session.go

@@ -153,17 +153,6 @@ func (s Session) TableName(ctx context.Context) string {
 	return "sessions"
 }

-func (s Session) MarshalJSON() ([]byte, error) {


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

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

aeneasr · 2022-12-17T09:46:29Z

session/session.go

-	result, err := json.Marshal(sl(s))
-	if err != nil {
-		return nil, err
-	}
-	return result, nil


Could just be:

return json.Marshal(sl(s))

codecov · 2022-12-17T10:05:45Z

Codecov Report

Merging #2958 (52ee63c) into master (e11ba52) will decrease coverage by 0.00%.
The diff coverage is 98.91%.

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

Impacted Files	Coverage Δ
session/session.go	`77.68% <ø> (+0.34%)`	⬆️
persistence/sql/persister_session.go	`83.82% <86.66%> (+0.64%)`	⬆️
session/test/persistence.go	`99.05% <100.00%> (-0.01%)`	⬇️
credentialmigrate/migrate.go	`75.00% <0.00%> (ø)`
selfservice/strategy/webauthn/nodes.go	`100.00% <0.00%> (ø)`
identity/credentials_webauthn.go
x/keys.go
identity/credentials_lookup.go
selfservice/strategy/lookup/credentials.go	`100.00% <0.00%> (ø)`
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

kelkarajay · 2022-12-19T17:09:32Z

Re-organized listing tests to do seed data setup once and run against that

Closes ory#2930

kelkarajay self-assigned this Dec 15, 2022

kelkarajay marked this pull request as ready for review December 15, 2022 18:06

kelkarajay requested review from aeneasr and zepatrik as code owners December 15, 2022 18:06

kelkarajay changed the title ~~feat: add persister clause based on active check param~~ fix: listing sessions query Dec 15, 2022

aeneasr requested changes Dec 17, 2022

View reviewed changes

Ajay Kelkar added 4 commits December 19, 2022 10:16

feat: add persister clause based on active check param

6d948c5

fix: error handling

e839bef

fix: remove marshal mutation

9fc2243

fix: listing active state query

eddece5

kelkarajay force-pushed the fix/identity-listing branch from 7270461 to eddece5 Compare December 19, 2022 09:16

Ajay Kelkar added 2 commits December 19, 2022 11:30

test: assertions corrected

1537990

test: consolidate list api tests and reorganize state tests

9148410

kelkarajay force-pushed the fix/identity-listing branch from 9b520b4 to 9148410 Compare December 19, 2022 16:27

fix: goddamned missing sql brackets

52ee63c

kelkarajay requested a review from aeneasr December 19, 2022 17:07

aeneasr approved these changes Dec 20, 2022

View reviewed changes

aeneasr merged commit 3e06c99 into master Dec 20, 2022

aeneasr deleted the fix/identity-listing branch December 20, 2022 07:48

peturgeorgievv pushed a commit to senteca/kratos-fork that referenced this pull request Jun 30, 2023

fix: listing sessions query (ory#2958)

67346e6

Closes ory#2930

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: listing sessions query #2958

fix: listing sessions query #2958

kelkarajay commented Dec 15, 2022 •

edited

aeneasr Dec 17, 2022

aeneasr Dec 17, 2022

aeneasr Dec 17, 2022

aeneasr Dec 17, 2022

aeneasr Dec 17, 2022

kelkarajay Dec 19, 2022

aeneasr Dec 17, 2022

codecov bot commented Dec 17, 2022 •

edited

kelkarajay commented Dec 19, 2022

	q.Where("expires_at >= ?", time.Now())
	q.Where("expires_at >= ?", time.Now().UTC())

	q.Where("expires_at < ?", time.Now())
	q.Where("expires_at < ?", time.Now().UTC())

fix: listing sessions query #2958

fix: listing sessions query #2958

Conversation

kelkarajay commented Dec 15, 2022 • edited

Changes

Related issue(s)

Checklist

Further Comments

aeneasr Dec 17, 2022

Choose a reason for hiding this comment

aeneasr Dec 17, 2022

Choose a reason for hiding this comment

aeneasr Dec 17, 2022

Choose a reason for hiding this comment

aeneasr Dec 17, 2022

Choose a reason for hiding this comment

aeneasr Dec 17, 2022

Choose a reason for hiding this comment

kelkarajay Dec 19, 2022

Choose a reason for hiding this comment

aeneasr Dec 17, 2022

Choose a reason for hiding this comment

codecov bot commented Dec 17, 2022 • edited

Codecov Report

kelkarajay commented Dec 19, 2022

kelkarajay commented Dec 15, 2022 •

edited

codecov bot commented Dec 17, 2022 •

edited