-
Notifications
You must be signed in to change notification settings - Fork 8
Remember me by default #416
base: develop
Are you sure you want to change the base?
Conversation
@@ -129,7 +129,7 @@ export default function LoginForm() { | |||
<div className={classes.loginOptions}> | |||
<FormControlLabel | |||
className={classes.rememberSwitch} | |||
control={<Switch size="small" />} | |||
control={<Switch size="small" checked={true} />} |
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'm not sure this actually ever worked 🤔 did a bit of code digging and it looks like the frontend "forgot" to pass the value of this properly to the backend on the API call...
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.
what if the 'remember me' switch creates a cookie when enabled, and at the start of the page check if that cookie exists?
that way we don't need to send it to the server (unless it's something that we want...)
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.
if the pseudo code it's clear I can implement it :)
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.
If you look at the proto definition of the auth req, I believe it is important we send this back to the server/backend so the session remains valid for longer.
@aapeliv may have more details of what the session is valid from creation until expiry, with no need to be used in between
means (e.g. I'm not sure how expiry is determined)
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.
High level outline of approach:
- Make sure the switch has a
name
property as that's important for React hook Form to update the form state correctly - Update
authActions.passwordLogin
to accept therememberMe
boolean - Check that the underlying call in that function to the gRPC API also takes this value in - you may have to update some code in one of the functions in the
service
folder
Test that it works? Actually this is where I'm a bit confused 😅 , as even the state of it right now, you don't have to log in again anyway (since cookie stays valid for 7 days after last user action according to the comment in the proto definition)? So does "remember me" mean you don't have to type in your email/password again? If that is the case, that sounds more like a frontend concern to me than a backend one, but somehow the same terminologies are being used here which makes things confusing...
The backend will set a session in the browser. That session has an expiry (by default 90 days) that matches what's stored in the database.
I will increase the cookie expiry from 90 days to much longer soon (Couchers-org/couchers#4059). |
I see, thanks for the clarification! Isn't 90 days already quite a generous timeframe (since you mention you're planning to increase it)? Do people actually complain of needing to log in again once around every 3 months? 😅 |
The issue is that without remember me, they have to log in again if they are inactive longer than a week. That is very common.On Apr 15, 2024, at 21:08, Darren Vong ***@***.***> wrote:
I see, thanks for the clarification! Isn't 90 days already quite a generous timeframe (since you mention you're planning to increase it)? Do people actually complain of needing to log in again once around every 3 months? 😅
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Web frontend checklist
make format
make lint