fix: bad interaction between session state changes and renewal

[hulxv]

PR Type

Bug Fix

PR Checklist

• Tests for the changes have been added / updated.
• Documentation comments have been added / updated.
• A changelog entry has been made for the appropriate packages.
• Format code with the nightly rustfmt (cargo +nightly fmt).

Overview

Closes #244

[LukeMathWalker]

Thanks for working on this!

[robjtede]

For the sake of readers like me who haven't really looked into the problem, is there a comment that can be left on these conditionals explaining the behavior? Not obvious at first glance (to me) what this solves.

[hulxv]

That's a simple explanation for the issue from poem-web/poem#196 and that issue happens in actix-session too.

Expected Behavior

When using Session, calling session.renew() always renews the session, indepentent of it's location in the handler (unless there is a session.purge()).

Actual Behavior

When calling session.set("x", "y") after session.renew(), the session does not get renewed.

From my understanding of the code, this is caused by this line in session.set() (and session.remove()):
https://github.com/poem-web/poem/blob/ad20fe7ebaf9660a295d1af194ee82b6d2d80037/poem/src/session/session.rs#L85

This unconditionally overwrites the status and deletes the information that a session renew was requested. What I think should happen instead is that it only overwrites the status if it is currently set to Unchanged.

[robjtede]

Ok, got it. So, if I understand correctly, this won't break the other way because a renewed session will reassign the cookie with the changed state as well.

If that's the case, it almost feels like the enum states don't quite capture the model. I.e., "changed" and "renewed" not being mutually exclusive is should make the state more like setting independent boolean flags than an enum? But that's a later problem. We'll get this PR shipped.

Excuse if I have any of this wrong, I'm not actually a user of this crate myself (yet).

[hulxv]

is should make the state more like setting independent boolean flags than an enum?

Nope, Enums is working well and I think I resolved the issue that is happening.

@LukeMathWalker Can you give us your opinion?

[LukeMathWalker]

I think @robjtede's intuition is correct - renewing the session id and changes to the session state are independent and should be tracked separately.

[hulxv]

So is there anything I should do more or is everything just fine?

[robjtede]

My concern is that this might break it the other way; storage implementations handle independently a change in session content and session lifespan?

It would not affect cookie storage but in Redis storage it would be the different... eg:

CHANGED           => SET key new_contents KEEPTTL
RENEWED           => SETEX key ttl
CHANGED + RENEWED => SET key new_contents EX ttl

^ untested but hopefully you see the point

Unless SET also updates the TTL, which might be the case... lemme check.

I'm less sure about things as I'm making edits.

[robjtede]

Here's what I want to know:

1. Does this fix the issue Bad interaction between session state changes and renewal #244 without introducing new bugs?
2. Is this the ideal representation of session state and TTL metadata?

Answering "yes" to 1. will allow this PR to be merged.

Understanding 2. will be helpful for opening an issue to move us toward that ideal representation that I suspect will reduce the number of non-obvious nested ifs necessary in this logic.

[hulxv]

1. Does this fix the issue Bad interaction between session state changes and renewal #244 without introducing new bugs?

I think yes because that is the way how poem developers solved this issue and there's no one reported another issue about that more and they never change session.rs after 20532aa.

2. Is this the ideal representation of session state and TTL metadata?

Honestly, I'm don't familiar with Redis, So, I don't understand what you mean here

CHANGED           => SET key new_contents >KEEPTTL
RENEWED           => SETEX key ttl
CHANGED + RENEWED => SET key >new_contents EX ttl

and I don't know what the ideal representation.

[LukeMathWalker]

This should not introduce new bugs, as far as my understanding goes.

The middleware instructs the backend to perform a delete on the data associated with the old session key and a save of the current session state against the new key (see

actix-extras/actix-session/src/middleware.rs

Line 275 in cfd16c5

SessionStatus::Renewed => {

), so this should behave correctly regardless of the session storage implementation details.