actix / actix-extras

A collection of additional crates supporting the actix and actix-web frameworks.
https://actix.rs
Apache License 2.0
760 stars 191 forks source link

Ability to extract SessionKey #306

Open robjtede opened 1 year ago

robjtede commented 1 year ago

Discussed in https://github.com/actix/actix-extras/discussions/305

Originally posted by **miketwenty1** December 4, 2022 **Goal:** Prevent 1 user from having multiple active sessions. I'm using actix-web redis session middleware. ``` .wrap(SessionMiddleware::new( RedisActorSessionStore::new(redis_conn.clone()), secret_key.clone(), )) ``` When a user does a login, I'll go ahead and insert some data into their session. ``` session.insert("userid", id).expect("id inserted"); ``` Seemingly these key/values via the `session.insert` are inner key/values to the actual managed middleware session keys that are made automagically. **First question:** How can I extract this middleware session id key that redis uses for the inserts/sessions? My thought is I would go ahead and insert a the userid as a redis key with the value being the middleware redis session id so I could easily lookup and void any sessions that the user may have if logging in again. (seems hacky idk). **Second question:** Is there a much easier/cleaner way of doing this while still getting the nice managed sessions from the redis middleware?
robjtede commented 1 year ago

@LukeMathWalker any quick thoughts on this?

It seems like a method like the following would satisfy this use case, punting the actual purge logic to the application.

impl Session {
  fn key(&self) -> SessionKey
}
LukeMathWalker commented 1 year ago

That would work for the Redis case, yes. I'd suggest returning a secrecy::Secret<SessionKey> though.

miketwenty1 commented 1 year ago

@LukeMathWalker do you envision secrecy::Secret<SessionKey> functionality only being added for redis actor? If so, would this only be changes to redis_actor.rs?

LukeMathWalker commented 1 year ago

No, it should be added to the Session struct, as @robjtede suggested. I was just mentioning that achieving the same objective (limiting the number of concurrent sessions) with other backends (i.e. cookies) won't work in this way since the SessionKey changes based on the data attached to the session.

miketwenty1 commented 1 year ago

Is this something you would be interested in working on with me? I'm sort of a beginner but I'm interested in getting this feature and contributing with some guidance if needed.

I imagine this would need to take place in the inner struct of Session?

#[derive(Default)]
struct SessionInner {
    state: HashMap<String, String>,
    status: SessionStatus,
    session_key: secrecy::Secret<SessionKey>
}

?

Also, I'm a bit confused are you suggesting we need to also pull in the secrecy crate? for this change?

LukeMathWalker commented 1 year ago

I can definitely review the PR 😁

Yes, I'm suggesting we pull in the secrecy crate for this change. The session key is highly sensitive key material and it should be treated accordingly.

miketwenty1 commented 1 year ago

@LukeMathWalker Would you be able to help me push this commit through, I'm probably going to need hand holding as I'm feeling a bit in over my head.

Am I correct on the route I'm going? https://github.com/actix/actix-extras/compare/master...miketwenty1:actix-extras:feature/session-key If so, I need help with the "todo's"

Edit: Would the formatting changes to Cargo.toml be accepted? Is changing the InnerSession to have the field session_key necessary? I'm not understanding how to grab/reference the key in session.rs, this is sort of confusing for me.

LukeMathWalker commented 1 year ago

It's easier for me to provide feedback if you open a draft PR 👍🏻

miketwenty1 commented 1 year ago

Ok, I've opened a PR to help move the feedback discussion forward.

miketwenty1 commented 1 year ago

https://github.com/actix/actix-extras/pull/307

LukeMathWalker commented 1 year ago

(I'm a bit busy at the moment, I'll try to get to it before the end of the Christmas holidays, but no promises)

miketwenty1 commented 1 year ago

@LukeMathWalker hope you had a great holiday break. Let me know if you're interested in working on this with me. I'm looking forward to learning from you.

behai-nguyen commented 6 months ago

No, it should be added to the Session struct, as @robjtede suggested. I was just mentioning that achieving the same objective (limiting the number of concurrent sessions) with other backends (i.e. cookies) won't work in this way since the SessionKey changes based on the data attached to the session.

Hi,

I have a question regarding the session key or the id cookie, please.

I did search actix-extras but I have not found the answer.

In my middleware, I just print out the cookie id like this:

...
    fn call(&self, request: ServiceRequest) -> Self::Future {
        if let Some(value) = request.cookie("id") {
            println!("Auth -- Id {:#?}", String::from(value.to_string()));
        }
...

And every time, I have a different one. I was under the assumption that it stays fixed.

Based on the quote above, is this the expected behaviour, please?

If I need to implement a unique session Id to identify a client session, am I correct to conclude that this cookie id is not suitable for this purpose, please?

Thank you and best regards,

...behai.

Mark-Asuncion commented 3 months ago

Hi, @behai-nguyen I also have this problem, have you found a fix or another solution perhaps?

behai-nguyen commented 3 months ago

Hi, @behai-nguyen I also have this problem, have you found a fix or another solution perhaps?

Hi @Mark-Asuncion,

I did not find a solution for it, unfortunately.

I just like to use a unique SessionKey to mark the start and the end of requests in my daily log file. E.g.:

Request <SessionKey / [no session Id]> entry
...
Request <SessionKey / [no session Id]> exit

This is my work-around:

In my application, I am using JSON Web Token (JWT) for authentication. I build my own unique session identifier UUID onto the JWT.

And so everytime a request starts, I look for the token and try to extract my UUID out for logging, and when the request finishes also.

-- Of course, we need to handle the absence of the JWT as well as error arised during decoding, etc.

It is not an elegant solution... But it is okay for me for the time being.

Please update if you make any progress on this.

Thank you and best regards,

...behai.

polarkac commented 1 month ago

@behai-nguyen @Mark-Asuncion I have been dealing with the same thing. By default cookies are encrypted as per documentation https://docs.rs/actix-session/0.10.0/actix_session/config/struct.SessionMiddlewareBuilder.html#method.cookie_content_security If you insert or other way modify the content in session it will also change the cookie value regardless if it is saved as a value inside cookie or database.

If you use CookieContentSecurity::Signed cookie value will then have format signature=id so you are able to extract the ID part to identify the current session. This is not a good idea, if you save private information inside the cookie value.

behai-nguyen commented 1 month ago

@behai-nguyen @Mark-Asuncion I have been dealing with the same thing. By default cookies are encrypted as per documentation https://docs.rs/actix-session/0.10.0/actix_session/config/struct.SessionMiddlewareBuilder.html#method.cookie_content_security If you insert or other way modify the content in session it will also change the cookie value regardless if it is saved as a value inside cookie or database.

If you use CookieContentSecurity::Signed cookie value will then have format signature=id so you are able to extract the ID part to identify the current session. This is not a good idea, if you save private information inside the cookie value.

Hi @polarkac,

Thank you for very much for your update and suggestion. I appreciate it.

Best regards,

...behai.