Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: clean up session on janitor run #2567

Closed
wants to merge 1 commit into from
+28 −1
Closed

feat: clean up session on janitor run #2567

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 28 additions & 1 deletion persistence/sql/persister_consent.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -447,8 +447,9 @@ func (p *Persister) FlushInactiveLoginConsentRequests(ctx context.Context, notAf
var cr consent.ConsentRequest
var crh consent.HandledConsentRequest

return p.transaction(ctx, func(ctx context.Context, c *pop.Connection) error {
var ls consent.LoginSession

return p.transaction(ctx, func(ctx context.Context, c *pop.Connection) error {
// Delete all entries (and their FK)
// where hydra_oauth2_authentication_request were timed-out or rejected
// - hydra_oauth2_authentication_request_handled
Expand Down Expand Up @@ -517,6 +518,32 @@ func (p *Persister) FlushInactiveLoginConsentRequests(ctx context.Context, notAf
time.Now().Add(-p.config.ConsentRequestMaxAge()),
notAfter).Exec()

if err != nil {
return sqlcon.HandleError(err)
}

err = p.Connection(ctx).RawQuery(fmt.Sprintf(`
DELETE
FROM %[1]s
WHERE NOT EXISTS
(
SELECT NULL
FROM %[2]s
WHERE %[2]s.login_session_id = %[1]s.id
)
AND NOT EXISTS
(
SELECT NULL
FROM %[3]s
WHERE %[3]s.login_session_id = %[1]s.id
)
AND authenticated_at < ?
`,
(&ls).TableName(),
(&lr).TableName(),
(&cr).TableName()),
notAfter).Exec()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is problematic because the notAfter has had a different context before. Now, this could remove sessions where users are signed in and that are not expired. I think not having any references is already a good indicator if this should be removed or not but I think we probably need to move this into it's own function and then add here

hydra/cmd/cli/handler_janitor.go

Lines 131 to 136 in be8de37

case OnlyTokens:
routines = append(routines, cleanup(p.FlushInactiveAccessTokens, "access tokens"))
routines = append(routines, cleanup(p.FlushInactiveRefreshTokens, "refresh tokens"))
case OnlyRequests:
routines = append(routines, cleanup(p.FlushInactiveLoginConsentRequests, "login-consent requests"))
}

as an additional case.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aeneasr You are right! The NOT EXISTS conditions should be sufficient. The notAfter condition could be an extra peace of mind not to delete unintended records. It also could potentially help with the performance of the query since the engine may require to do fewer row scans this way (I didn't do any sort of benchmarking, I am just guessing).

I can remove it if you think that's misleading.


return sqlcon.HandleError(err)
})
}