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

Feature/session key #307

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

miketwenty1
Copy link

@miketwenty1 miketwenty1 commented Dec 14, 2022

PR Type

Feature

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

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.

modified session_key.rs to impl secrecy::Zeroize for SessionKey
modified session.rs in attempts of adding secrecy::Secret<SessionKey> to InnerSession struct.
@robjtede robjtede marked this pull request as draft December 14, 2022 15:02
@@ -77,6 +79,7 @@ impl Default for SessionStatus {
struct SessionInner {
state: HashMap<String, String>,
status: SessionStatus,
session_key: SessionKey,
Copy link
Contributor

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.

Copy link
Contributor

@LukeMathWalker LukeMathWalker Jan 3, 2023

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

Session::set_session(&mut req, session_state);
using the value from
let (session_key, session_state) =

Copy link
Author

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?

Copy link
Contributor

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

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.

Copy link
Author

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

Copy link
Author

@miketwenty1 miketwenty1 left a 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 Show resolved Hide resolved
@@ -77,6 +79,7 @@ impl Default for SessionStatus {
struct SessionInner {
state: HashMap<String, String>,
status: SessionStatus,
session_key: SessionKey,
Copy link
Author

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);
Copy link
Author

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

@miketwenty1
Copy link
Author

@LukeMathWalker did you need me to do more work on this before getting next bits of feedback? I have some outstanding questions.

@robjtede robjtede added A-session Project: actix-session B-semver-minor labels Feb 26, 2023
@robjtede robjtede linked an issue Feb 26, 2023 that may be closed by this pull request
@Conni2461 Conni2461 mentioned this pull request May 3, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-session Project: actix-session B-semver-minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to extract SessionKey
3 participants