Add callback to decide whether a session can be saved based on the response status code

[0rzech]

IMHO, session should not be saved on client error. For instance, with always_save option it can lead to prolonging session lifetime on HTTP 401.

Maybe I'm wrong, but in general it seems weird to save session even when client made an invalid request.

But perhaps it would be better not to save on client error, but only when the session was not modified anyway, like so:

(modified || (session_config.always_save && !res.status().is_client_error())

?

Or maybe we should just stick to the Django way you showed in #220 (comment) and close this PR?

[codecov]

Codecov Report

❌ Patch coverage is 70.58824% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.68%. Comparing base (92ad026) to head (f46246e).

Files with missing lines	Patch %	Lines
src/service.rs	70.58%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #220      +/-   ##
==========================================
- Coverage   87.35%   86.68%   -0.68%     
==========================================
  Files           5        5              
  Lines         356      368      +12     
==========================================
+ Hits          311      319       +8     
- Misses         45       49       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[maxcountryman]

I think I'd like to understand better why Django doesn't do this. (Maybe there's some historical context or discussion about it.) In a similar vein, are there examples of other established frameworks also not saving on client errors?

[0rzech]

I did some archaeology and it looks like http status code exclusions were added fairly recently to, case-by-case and for different reasons - Django session middleware had been masking exceptions / status codes without these exclusions. At first no status codes were excluded, then only 500 (https://code.djangoproject.com/ticket/3881) and then status codes >= 500 (https://code.djangoproject.com/ticket/34173).

I tried to find how other frameworks do it, but I'm not sure if they implement "always save" at all. RoR doesn't seem to have it: https://api.rubyonrails.org/v7.2.2/classes/ActionDispatch/Session/CookieStore.html . Phoenix Framework doesn't seem to have such option either: https://hexdocs.pm/plug/Plug.Session.html . Same with Spring: https://docs.spring.io/spring-session/reference/configuration/common.html . It looks like developers are supposed to implement it themselves there.

[maxcountryman]

Interesting! Thanks for looking into that. Do you think we shouldn't try to implement "always save" altogether?

[0rzech]

I think it's good to have this option, either built-in or through some Fn(...).

Perhaps RFC will help us: https://datatracker.ietf.org/doc/html/rfc6265#section-3 ?

Origin servers MAY send a Set-Cookie response header with any
response. User agents MAY ignore Set-Cookie headers contained in
responses with 100-level status codes but MUST process Set-Cookie
headers contained in other responses (including responses with 400-
and 500-level status codes).

This means that setting cookies on both 4xx and 5xx is fine according to spec.

Now, the question is whether we would like to skip refreshing the session when these errors occur and how to do it?

I think there are several options:

• Remove always_save, but add a way to easily hook into the code so that developers can implement decision process themselves. I suppose the ease of doing this in other frameworks is the reason why there's no such option there.
• Add setting with status code ranges or something to be filtered out and decide whether always filter out those status codes or only in case of always_save == true.
• Remove

  tower-sessions/src/service.rs

  Line 253 in 66983f5

  && !res.status().is_server_error() =>

  because tower-sessions does not mask errors, so the primary reason for Django's workaround is not there.
• Change < 500 to < 400. This would increase hardcoded status code range, though.
• Leave the code as is. But why should server error always prevent session from being saved and its expiration time from being prolonged?

[maxcountryman]

I kind of like the first option, since that seems like the most flexible but maybe it would be annoying to implement?

[0rzech]

I like the first option too. I haven't tried to implement it yet, but SessionConfig is both Clone and Debug, so it's a no-go for Fn field there. Also no trait for us due to orphan rule.

[maxcountryman]

If we can remove Clone and Debug or otherwise rework SessionConfig, that's fine. (There's another issue where some applications would like to mutate the config after it's been set up so it may be worthwhile to rethink that altogether.)

[0rzech]

Removing Clone and Debug would break backwards compatibility. If we're OK with that, then perhaps we could wait until it's safe to update rand 0.8 to 0.9, as this is a breaking change too? Currently tower-sessions-core depends on rand 0.8.5 directly and axum-login depends on it traisitively.

[0rzech]

@maxcountryman I left the always_save setting as-is, and added a configurable callback (can_save) to decide whether the session can be saved based on the response status code. Would that work?

always_save could be also renamed to save_on_read or something, if we're ok. with breaking backwards compatibility for it.

I think I might've used a bit too much of your GH Actions minutes. Please contact me if you run out of the minutes of your current billing cycle because of this.