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

Ability to extract SessionKey #306

Open
robjtede opened this issue Dec 4, 2022 Discussed in #305 · 13 comments · May be fixed by #307
Open

Ability to extract SessionKey #306

robjtede opened this issue Dec 4, 2022 Discussed in #305 · 13 comments · May be fixed by #307
Assignees
Labels
A-session Project: actix-session C-feature Category: new functionality

Comments

@robjtede
Copy link
Member

robjtede commented Dec 4, 2022

Discussed in #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 robjtede added A-session Project: actix-session C-feature Category: new functionality labels Dec 4, 2022
@robjtede
Copy link
Member Author

robjtede commented Dec 4, 2022

@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
Copy link
Contributor

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

@miketwenty1
Copy link

@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
Copy link
Contributor

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
Copy link

miketwenty1 commented Dec 10, 2022

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
Copy link
Contributor

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
Copy link

miketwenty1 commented Dec 13, 2022

@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?
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
Copy link
Contributor

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

@miketwenty1 miketwenty1 linked a pull request Dec 14, 2022 that will close this issue
4 tasks
@miketwenty1
Copy link

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

@miketwenty1
Copy link

#307

@LukeMathWalker
Copy link
Contributor

(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
Copy link

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

@robjtede robjtede linked a pull request Feb 26, 2023 that will close this issue
4 tasks
@Conni2461 Conni2461 mentioned this issue May 3, 2023
4 tasks
@behai-nguyen
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-session Project: actix-session C-feature Category: new functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants