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.

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

Do not write or update sessions or their cookies for API access #14826

Merged
merged 2 commits into from
Feb 23, 2024

Conversation

oliverguenther
Copy link
Member

Accessing the API currently does not, and should not update the user's session. Still, currently we're using it to access the session for authenticating the user.

As a result, the session is loaded and we're currently outputting a Set-Cookie header as well as writing the user session on every API request.

By using session_options[:skip], we can tell rack to avoid saving the session after the request

Accessing the API currently does not, and should not update the user's session.
Still, currently we're using it to access the session for authenticating the user.

As a result, the session is loaded and we're currently outputting a Set-Cookie header as well as
writing the user session on every API request.

By using session_options[:skip], we can tell rack to avoid saving the session after the request
@toy
Copy link
Contributor

toy commented Feb 21, 2024

A call to api will still write an empty session when there is none, seems to be happening only when it is destroyed, so from logout page.

lib/api/root_api.rb Outdated Show resolved Hide resolved
Co-authored-by: Ivan Kuchin <[email protected]>
@toy
Copy link
Contributor

toy commented Feb 22, 2024

I realised that setting skip session option will also prevent session id cookie from being set, so the problem is only in creating a useless row in sessions table. Should we worry about this? Is there any automation to cleanup stale sessions?

@oliverguenther
Copy link
Member Author

I realised that setting skip session option will also prevent session id cookie from being set, so the problem is only in creating a useless row in sessions table. Should we worry about this? Is there any automation to cleanup stale sessions?

Yes, there is a cleanup job: https://github.com/opf/openproject/blob/dev/app/workers/cron/clear_old_sessions_job.rb
Ideally we would still prevent persisting it. I'll look into it

@toy
Copy link
Contributor

toy commented Feb 22, 2024

Ideally we would still prevent persisting it.

As it is happening when finding a session (ActiveRecordStore#get_session_model), I assume customising that would be required.

Copy link
Contributor

@toy toy left a comment

Choose a reason for hiding this comment

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

This should already improve situation a lot

@cbliard cbliard merged commit dd7b1f8 into dev Feb 23, 2024
9 checks passed
@cbliard cbliard deleted the performance/api-session-skip branch February 23, 2024 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants