-
Notifications
You must be signed in to change notification settings - Fork 0
fastn_session with expires_at and Login with Custom Expiration
#49
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
base: main
Are you sure you want to change the base?
Changes from all commits
80a361f
6960337
6d1f1af
5b266d4
5468d79
bc002d7
fbecd93
d9687fa
4e10bd4
dd1abe3
2e515e5
9521c85
9e84717
0fd8f3f
63674da
514616d
8d8e6cf
d9eb55f
29bb8a4
b2c3233
144fb21
a4164c6
863eded
23ae0ae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -290,23 +290,57 @@ pub fn create_user( | |
| Ok(ft_sdk::auth::UserId(user_id)) | ||
| } | ||
|
|
||
| /// persist the user in session and redirect to `next` | ||
| /// Logs in a user and manages session creation or update. | ||
| /// | ||
| /// `identity`: Eg for GitHub, it could be the username. This is stored in the cookie so can be | ||
| /// retrieved without a db call to show a user identifiable information. | ||
| /// # Arguments | ||
| /// | ||
| /// * `conn` - Mutable reference to a `ft_sdk::Connection` to interact with the database. | ||
| /// * `user_id` - Reference to a `ft_sdk::UserId` representing the user's ID. | ||
| /// * `session_id` - Optional `ft_sdk::auth::SessionID` representing an existing session ID. | ||
| /// | ||
| /// # Returns | ||
| /// | ||
| /// A `Result` containing a `ft_sdk::auth::SessionID` if the login operation is successful, | ||
| /// or a `LoginError` if there's an issue with the login process. | ||
| pub fn login( | ||
| conn: &mut ft_sdk::Connection, | ||
| ft_sdk::UserId(user_id): &ft_sdk::UserId, | ||
| user_id: &ft_sdk::UserId, | ||
| session_id: Option<ft_sdk::auth::SessionID>, | ||
amitu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ) -> Result<ft_sdk::auth::SessionID, LoginError> { | ||
| login_with_custom_session_expiration(conn, user_id, session_id, None) | ||
| } | ||
|
|
||
| /// Logs in a user with customizable session expiration and manages session creation or update. | ||
| /// | ||
| /// # Arguments | ||
| /// | ||
| /// * `conn` - Mutable reference to a `ft_sdk::Connection` to interact with the database. | ||
| /// * `user_id` - Reference to a `ft_sdk::UserId` representing the user's ID. | ||
| /// * `session_id` - Optional `ft_sdk::auth::SessionID` representing an existing session ID. | ||
| /// * `session_expiration_duration` - Optional `chrono::Duration` for custom session expiration. | ||
| /// | ||
| /// # Returns | ||
| /// | ||
| /// A `Result` containing a `ft_sdk::auth::SessionID` if the login operation is successful, | ||
| /// or a `LoginError` if there's an issue with the login process. | ||
| pub fn login_with_custom_session_expiration( | ||
| conn: &mut ft_sdk::Connection, | ||
| user_id: &ft_sdk::UserId, | ||
| session_id: Option<ft_sdk::auth::SessionID>, | ||
| session_expiration_duration: Option<chrono::Duration>, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a global configuration concern. A site will have a single global expiration. Use environment variable to get its value.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| ) -> Result<ft_sdk::auth::SessionID, LoginError> { | ||
| match session_id { | ||
| Some(session_id) if session_id.0 == "hello" => { | ||
| Ok(ft_sdk::auth::session::create_with_user(conn, *user_id)?) | ||
| } | ||
| 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( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lets remove this |
||
| conn, | ||
| user_id, | ||
| session_expiration_duration, | ||
| )?), | ||
| _ => Ok(ft_sdk::auth::session::set_user_id( | ||
| conn, | ||
| session_id, | ||
| user_id, | ||
| session_expiration_duration, | ||
| )?), | ||
| None => Ok(ft_sdk::auth::session::create_with_user(conn, *user_id)?), | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -316,7 +350,6 @@ pub enum LoginError { | |
| DatabaseError(#[from] diesel::result::Error), | ||
| #[error("set user id for session {0}")] | ||
| SetUserIDError(#[from] ft_sdk::auth::session::SetUserIDError), | ||
|
|
||
| #[error("json error: {0}")] | ||
| JsonError(#[from] serde_json::Error), | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ diesel::table! { | |
| data -> Text, | ||
| updated_at -> Timestamptz, | ||
| created_at -> Timestamptz, | ||
| expires_at -> Nullable<Timestamptz>, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cookie will surely delete it but how are we going to delete this from db?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A background worker.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| } | ||
| } | ||
|
|
||
|
|
||
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.
I do not think we should have two method. Merge the two ud related methods please.
Uh oh!
There was an error while loading. Please reload this page.
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.
Since we are not
session_idpassing in cookie but inAuthorizationheader. We need to have other function to fetch ud fromAuthorizationheader. This we can do by having something similar toft_sdk::CookiewithFromRequestimplemented which I guess I am going to do.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.
Change the original
ud()method to acceptSessionIDinstead ofCookie<"session-id">. Then you won't need this duplication.