Skip to content
This repository has been archived by the owner on Apr 20, 2024. It is now read-only.

Remember me by default #416

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Remember me by default #416

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion features/auth/login/LoginForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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} />}
Copy link
Member

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...

Copy link
Contributor

@bakeiro bakeiro Apr 14, 2024 •
edited
Loading

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...)

Copy link
Contributor

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 :)

Copy link
Member

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)

Copy link
Member

@darrenvong darrenvong Apr 14, 2024 •
edited
Loading

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 the rememberMe 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...

label={t("auth:login_page.form.remember_me")}
/>
{!loginWithLink && (
Expand Down