feat: clean up session on janitor run

[mohsen3]

Related issue

Closes #2561
@aeneasr

Proposed changes

Clean up the hydra_oauth2_authentication_session table when hydra janitor $DSN --requests is run.

Checklist

• I have read the contributing 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 green light (please contact
  [email protected]) 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

hydra_oauth2_authentication_session is quite a large table and requires clean up. It is the second largest table in our database right now.

[CLAassistant]

All committers have signed the CLA.

[aeneasr]

Thank you for the PR! The checkbox I have added tests that prove my fix is effective or that my feature works. is checked but I can't seem to find any tests ;)

@Benehiko can probably assist you to find the right place to add tests!

[aeneasr]

Sorry for the long review times, kinda hectic at the moment :/

[aeneasr]

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.

[mohsen3]

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

[Benehiko]

Hi @mohsen3

This looks great! I believe since you are deleting from a new table you would have to write some additional tests to verify if the rows are being deleted or kept in hydra_oauth2_authentication_session.

You can add your tests inside TestJanitorHandler_PurgeLoginConsent, TestJanitorHandler_PurgeLoginConsentNotAfter here handler_janitor_test.go

As well as the helper tests inside janitor_test_helper.go

I believe it might be better to add a new flag to this routine since this is deleting a different aspect of the database which might not always need to be run with --requests. So in my opinion I think you would need to add a flag called --sessions and then have its own purge routines, which means adding your own test functions following the same pattern as the current janitor tests.
You would need to add a cli flag here and the function call here as well as the routine here

[mohsen3]

Hi @mohsen3

This looks great! I believe since you are deleting from a new table you would have to write some additional tests to verify if the rows are being deleted or kept in hydra_oauth2_authentication_session.

You can add your tests inside TestJanitorHandler_PurgeLoginConsent, TestJanitorHandler_PurgeLoginConsentNotAfter here handler_janitor_test.go

As well as the helper tests inside janitor_test_helper.go

I believe it might be better to add a new flag to this routine since this is deleting a different aspect of the database which might not always need to be run with --requests. So in my opinion I think you would need to add a flag called --sessions and then have its own purge routines, which means adding your own test functions following the same pattern as the current janitor tests.
You would need to add a cli flag here and the function call here as well as the routine here

Hi @Benehiko,

Please bear with me since this is my first contribution to this repo :)

1. I'll add more tests
2. I am not sure if adding an extra --sessions switch is very useful (or at least we in our service don't have a use for it). The --request switch is already deleting consent and session requests. So these sessions are just dangling around with no use. Also, deleting sessions and keeping session/consent requests cannot happen since it has foreign keys that cascades deletes.

Do you still think having a separate switch is helpful? We can start from a single switch and separate it later if we found a use for it?

[Benehiko]

Hi @mohsen3

This looks great! I believe since you are deleting from a new table you would have to write some additional tests to verify if the rows are being deleted or kept in hydra_oauth2_authentication_session.

You can add your tests inside TestJanitorHandler_PurgeLoginConsent, TestJanitorHandler_PurgeLoginConsentNotAfter here handler_janitor_test.go

As well as the helper tests inside janitor_test_helper.go

I believe it might be better to add a new flag to this routine since this is deleting a different aspect of the database which might not always need to be run with --requests. So in my opinion I think you would need to add a flag called --sessions and then have its own purge routines, which means adding your own test functions following the same pattern as the current janitor tests.
You would need to add a cli flag here and the function call here as well as the routine here

Hi @Benehiko,

Please bear with me since this is my first contribution to this repo :)

1. I'll add more tests
2. I am not sure if adding an extra --sessions switch is very useful (or at least we in our service don't have a use for it). The --request switch is already deleting consent and session requests. So these sessions are just dangling around with no use. Also, deleting sessions and keeping session/consent requests cannot happen since it has foreign keys that cascades deletes.

Do you still think having a separate switch is helpful? We can start from a single switch and separate it later if we found a use for it?

That's no problem just ping me if you need help :)

I see, yes you are right! Let's keep it then under the same switch --requests

[aeneasr]

While the PR is being worked on I will mark it as a draft. That declutters our review backlog :)

Once you're done with your changes and would like someone to review them, mark the PR as ready and request a review from one of the maintainers.

Thank you!