-
Notifications
You must be signed in to change notification settings - Fork 209
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
Feature/session key #307
base: master
Are you sure you want to change the base?
Feature/session key #307
Conversation
modified session_key.rs to impl secrecy::Zeroize for SessionKey modified session.rs in attempts of adding secrecy::Secret<SessionKey> to InnerSession struct.
actix-session/src/session.rs
Outdated
@@ -77,6 +79,7 @@ impl Default for SessionStatus { | |||
struct SessionInner { | |||
state: HashMap<String, String>, | |||
status: SessionStatus, | |||
session_key: SessionKey, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This must be an Option
, since the key will only exist if the user has an active session.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be populated by
actix-extras/actix-session/src/middleware.rs
Line 221 in 1774b8a
Session::set_session(&mut req, session_state); |
actix-extras/actix-session/src/middleware.rs
Line 218 in 1774b8a
let (session_key, session_state) = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and this logic needs to be replicated in session.rs? This is tricky for me to understand. This is populated within get_session_key
fn I'm proposing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can add a Session::set_session_key
method which is then invoked by middleware.rs
, next to the passage I highlighted.
#[derive(Debug, PartialEq, Eq)] | ||
pub struct SessionKey(String); | ||
#[derive(Debug, PartialEq, Eq, Default, Clone)] | ||
pub struct SessionKey(pub String); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't want to expose the inner field here.
We can change SessionKey
to be SessionKey(secrecy::Secret<String>)
and expose into_inner
/inner
methods to access the inner value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the code we are impl a AsRef
impl AsRef<str> for SessionKey { |
method `as_ref` has an incompatible type for trait
expected fn pointer `fn(&storage::session_key::SessionKey) -> &str`
found fn pointer `fn(&storage::session_key::SessionKey) -> &secrecy::Secret<std::string::String>`
However it's not clear to me how to do with with secrecy data
https://docs.rs/secrecy/0.8.0/secrecy/struct.Secret.html#trait-implementations
Should I be looking for a trait AsRef here?
If I update to
impl AsRef<secrecy::Secret<String>> for SessionKey {
fn as_ref(&self) -> &secrecy::Secret<String> {
&self.0
}
}
Does this mean I need to update redis_rs.rs / redis_actor.rs / cookie.rs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
follow to @LukeMathWalker on his feedback for session key exposure. Jan 3, 2023.
actix-session/src/session.rs
Outdated
@@ -77,6 +79,7 @@ impl Default for SessionStatus { | |||
struct SessionInner { | |||
state: HashMap<String, String>, | |||
status: SessionStatus, | |||
session_key: SessionKey, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and this logic needs to be replicated in session.rs? This is tricky for me to understand. This is populated within get_session_key
fn I'm proposing?
#[derive(Debug, PartialEq, Eq)] | ||
pub struct SessionKey(String); | ||
#[derive(Debug, PartialEq, Eq, Default, Clone)] | ||
pub struct SessionKey(pub String); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the code we are impl a AsRef
impl AsRef<str> for SessionKey { |
method `as_ref` has an incompatible type for trait
expected fn pointer `fn(&storage::session_key::SessionKey) -> &str`
found fn pointer `fn(&storage::session_key::SessionKey) -> &secrecy::Secret<std::string::String>`
However it's not clear to me how to do with with secrecy data
https://docs.rs/secrecy/0.8.0/secrecy/struct.Secret.html#trait-implementations
Should I be looking for a trait AsRef here?
If I update to
impl AsRef<secrecy::Secret<String>> for SessionKey {
fn as_ref(&self) -> &secrecy::Secret<String> {
&self.0
}
}
Does this mean I need to update redis_rs.rs / redis_actor.rs / cookie.rs
@LukeMathWalker did you need me to do more work on this before getting next bits of feedback? I have some outstanding questions. |
PR Type
Feature
PR Checklist
cargo +nightly fmt
).Overview
The goal is add this feature to be utilized by the redis session middleware without breaking any other session middleware.
Notes from discussion:
#306
Dev Notes:
This is not a working PR, changes will come after feedback.