fastn_session with expires_at and Login with Custom Expiration
#49
Conversation
| conn: &mut ft_sdk::Connection, | ||
| user_id: &ft_sdk::UserId, | ||
| session_id: Option<ft_sdk::auth::SessionID>, | ||
| session_expiration_duration: Option<chrono::Duration>, |
There was a problem hiding this comment.
This is a global configuration concern. A site will have a single global expiration. Use environment variable to get its value.
There was a problem hiding this comment.
ft_sdk::env::var doesn't work on hostn.
There was a problem hiding this comment.
Why would it not work? If it does not work we will make it work. Write the code with the assumtion that the functions we have provided in sdk work.
| } | ||
| Some(session_id) => Ok(ft_sdk::auth::session::set_user_id( | ||
| conn, session_id, *user_id, | ||
| Some(session_id) if session_id.0 == "hello" => Ok(ft_sdk::auth::session::create_with_user( |
There was a problem hiding this comment.
Lets remove this hello hack now. I think we did it because we had some wrong cookie set with this value which we were not able to delete.
| data -> Text, | ||
| updated_at -> Timestamptz, | ||
| created_at -> Timestamptz, | ||
| expires_at -> Nullable<Timestamptz>, |
There was a problem hiding this comment.
Not sure it should be nullable. Lets make every session expire after certain timeout. And we can then add a function which will extend the expires_at by 30 days every time we access it. Also shouldnt tracking expiry by cookie expiry be enough, why do we have to store this in DB?
There was a problem hiding this comment.
Cookie will surely delete it but how are we going to delete this from db?
There was a problem hiding this comment.
So a background worker should read from db when the session should expire and then delete it. If my understanding is correct then we need expires_at column in the fastn_session table.
There was a problem hiding this comment.
As discussed, yes. But since there are multiple possible behaviours for session expiry, let's not tackle session expiry related stuff now. First let's have a discussion and full design on session expiry then we write code.
| match session { | ||
| Some((session_id, true)) => { | ||
| // Session is expired, delete it and create a new one | ||
| diesel::delete(fastn_session::table.filter(fastn_session::id.eq(session_id.as_str()))) |
There was a problem hiding this comment.
Not fond of this, we should clear up session from session table via some periodic task, and not during logout, so things can be instigated in case of problem. Deleting cookie should be enough.
| pub fn set_session_cookie( | ||
| conn: &mut ft_sdk::Connection, | ||
| ft_sdk::auth::SessionID(session_id): ft_sdk::auth::SessionID, | ||
| host: ft_sdk::Host, | ||
| ) -> Result<http::HeaderValue, ft_sdk::Error> { |
There was a problem hiding this comment.
Dont think this function should exist. Also if it existed this function must generate the session id, why would you call one function to get session id and then call another function to set session cookie, when one function will do.
There was a problem hiding this comment.
The session id is passed as access_token in url parameter. So we need to set it after careful check of its validity.
| /// | ||
| /// This method queries the database to find a session associated with the | ||
| /// given user ID. | ||
| pub fn from_user_id( |
There was a problem hiding this comment.
This is not from user_id, this if for user id. You create a new session for a user, not create a session from user. How is this different from login()?
There was a problem hiding this comment.
It fetches the session_id for the given user_id from the fastn_session table.
There was a problem hiding this comment.
A user can have many sessions. This is wrong, it should return a list of session ids. Or it should be called get_all_sessions_of_user or get_first/last_session_of_user.
There was a problem hiding this comment.
Then why do we have SetUserIDError::MultipleSessionsFound error?
There was a problem hiding this comment.
That is doing the right thing, it is updating session table using session id. Since session id is unique this error should never be thrown, but we have added it instead of panicking to handle that case where diesel returns modified count > 1.
| /// This function checks the validity of the session associated with the session ID. | ||
| /// If the session is found and is not expired, it returns `Ok(())`. | ||
| /// If the session is expired or not found, it returns an appropriate error. | ||
| pub fn validate_session(&self, conn: &mut ft_sdk::Connection) -> Result<(), SessionIDError> { |
There was a problem hiding this comment.
Nopes. If session id cookie exists, and session is present in DB it is valid. We dont need this.
There was a problem hiding this comment.
We have added expires_at column in the fastn_session table. So if the session expires it becomes invalid.
There was a problem hiding this comment.
Let's remove session expiry related stuff from here, this will need more thought. We need multiple behaviours, and let developer opt in to their desired behaviour related to session expiry.
| pub fn ud_from_session_key( | ||
| conn: &mut ft_sdk::Connection, | ||
| session_id: &ft_sdk::auth::SessionID, | ||
| ) -> Result<Option<ft_sys::UserData>, UserDataError> { |
There was a problem hiding this comment.
I do not think we should have two method. Merge the two ud related methods please.
There was a problem hiding this comment.
Since we are not session_id passing in cookie but in Authorization header. We need to have other function to fetch ud from Authorization header. This we can do by having something similar to ft_sdk::Cookie with FromRequest implemented which I guess I am going to do.
There was a problem hiding this comment.
Change the original ud() method to accept SessionID instead of Cookie<"session-id">. Then you won't need this duplication.
|
@Arpita-Jaiswal delete the ft-stripe stuff from here, so we can review the PR more easily. |
This PR introduces better to the session-handling functionality. Key changes include:
expires_atcolumn in thefastn_sessiontable to manage session expiration.login_with_custom_session_expirationto handle login with customizable session expiration.set_user_idto check session expiration and update session data accordingly.set_session_cookieandexpire_session_cookieto set and expire session cookie respectively.SessionID::from_user_idmethod to retrieve a session ID based on a given user ID.