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

"Remember me" feature #1316

Closed
wants to merge 5 commits into from
Closed

"Remember me" feature #1316

wants to merge 5 commits into from

Conversation

YiranDuan721
Copy link
Contributor

Motivation and Context

Related issue: #1302

Description

  1. Added a checkbox on the login page that allows users to select "Remember Me" before logging in
  2. The state whether or not "Remember Me" is checked, is saved in a temporary cookie during the login process, and stored as a jwt claim after successfully logged in
  3. In the middleware, check whether the jwt should be refreshed after validation, if so, extend the validity period in both jwt and the cookie's age
  4. A MaxTokenLifetimeWithRememberMeInDays is set, to prevent the token from being valid forever
  5. A MinUpdateIntervalInHours is set, so that (if "Remember Me" checked upon logging in) the token won't get refreshed too frequently

Steps for Testing

  1. To be able to see the effect quickly, first change MinUpdateIntervalInHours in "\tools\middlewares.go" to a smaller value, e.g. 0.01, which corresponds to 36 seconds
  2. Log in with "Remember Me" selected
  3. Refresh the website within the period of MinUpdateIntervalInHours, see the cookie remain unchanged
  4. Refresh the website after MinUpdateIntervalInHours, see both the value and the Max-Age of the cookie "jwt" is changed, and in the payload of jwt, the value of "exp" corresponds to the Max-Age of the cookie

Screenshots

Change of UI:

@YiranDuan721 YiranDuan721 linked an issue Jan 21, 2024 that may be closed by this pull request
Copy link

Your Testserver will be ready at https://1316.test.live.mm.rbg.tum.de in a few minutes.

Logins
Kurs1 Kurs2 Kurs3 Kurs4
public public loggedin enrolled
prof1 prof1 prof2 prof1
prof2
student1
student2
student3
student1
student2
student2
student3
student1
student2

@hmelder
Copy link

hmelder commented Jan 21, 2024

A 180-day session lifetime is way to long. I think one to two weeks is perfectly fine for a VoD platform.

We are already storing the cookie, so naming it "Remember Me" is a bit misleading. It is typically used when you want to use a session cookie, and a persistent cookie (with an expiration date).

A few examples:
Spotify has a "Remember Me" toggle sets the expiration date of the token to one month. When not toggled, only a session cookie is stored.

TIDAL does not have a "Remember Me" toggle and stores a sid cookie with a lifetime of 7 days.

@alexmo1997
Copy link

A 180-day session lifetime is way to long. I think one to two weeks is perfectly fine for a VoD platform.

Why do you think a 180-day session is too long?
Just now, I inspected my YouTube and Netflix cookie.
Netflix has 6 months cookie expiration dates, while YouTube seems to have 2 years.
Furthermore, I don't think I have ever been logged out of either of those before without changing anything on my end. They probably even do session renewals to extend that period.

@YiranDuan721
Copy link
Contributor Author

YiranDuan721 commented Jan 22, 2024

Thank you for the comments! As it comes to security, I do think we should be more careful.

But some thoughts on how a "remember me" feature, realised by refreshing the token for logged-in users from time to time, would actually improve security: With this feature, we can then shorten the default duration of validity of the token, or, provide an option to shorten it (lecturers and admins might want this behaviour).

And we are using jwt instead of a session cookie, which makes a great difference, since invalidating a jwt before its expiration is more tricky.

@SebiWrn SebiWrn self-requested a review March 24, 2024 09:46
Copy link
Contributor

@SebiWrn SebiWrn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this feature is no longer necessary as we are currently switching to another login system

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UX: Longer sessions
4 participants