parse-community / parse-dashboard

A dashboard for managing Parse Server
https://parseplatform.org
Other
3.74k stars 1.39k forks source link

Login session ends when browser session ends, cookie maxAge ignored #2365

Closed zivchen closed 1 year ago

zivchen commented 1 year ago

New Issue Checklist

Issue Description

cookie-session - maxAge property has no effect, at the moment its in an object

Steps to reproduce

Just open chrome inspect elements and see that the cookie expires on session

Actual Outcome

cookie expires on session

Expected Outcome

cookie will expire in 2 weeks

Environment

Dashboard

The problem is at Authentication.js row 59, maxAge should be a prop of the configuration object and not nested in cookie :{}

See actual API https://github.com/expressjs/cookie-session

A feature request for this bug will be to have maxAge property via config

Would love to make a PR...

parse-github-assistant[bot] commented 1 year ago

Thanks for opening this issue!

mtrezza commented 1 year ago

Could you provide more details about what the change is you're suggesting?

zivchen commented 1 year ago

@mtrezza Sure The fix it self is easy (put maxAge in the root object of cookie-session initialization) but it will change the current behavior (session only cookie) that i assume people are use to. I created a PR that adds a cookieMaxAge configuration option and chain it all the way similar to cookieSessionSecret so if not provided it will remain the way it is today. PR: https://github.com/parse-community/parse-dashboard/pull/2366

mtrezza commented 1 year ago

the cookie expires on session

What do you mean by that?

it will change the current behavior (session only cookie)

Could you give more details what the current behavior is vs. the new behavior?

zivchen commented 1 year ago

@mtrezza

What do you mean by that?

The current bug in maxAge definition makes the package cookie-session to add a login cookie with no maxAge / expires which means the browser will save it for the current session only (it doesn't do "remember me")

Could you give more details what the current behavior is vs. the new behavior?

Well the bug is easily fixed, but its very old which means i assume that the dashboard users are use to the "buggy" behavior which is session only cookie and not 2 weeks cookie as the code tried to make, so my PR keeps session only cookie by default but gives the ability to change by config to a longer cookie ( 2weeks or even more)

To be honest i just need my dashboard users not to do a login every time and that it will remember them

mtrezza commented 1 year ago

the browser will save it for the current session only

You are referring to the browser session, not to the dashboard login session, correct?

In other words, the current behavior is:

  1. user logs in to dashboard
  2. user closes browser and reopens browser
  3. user needs to re-log in to dashboard

The behavior you expect is:

  1. user logs in to dashboard
  2. user closes browser and reopens browser
  3. user only needs to re-log in to dashboard if the cookie has expired

This would be a breaking change that relates to security. It requires users to behave differently and manually log out to end the dashboard session.

We could:

zivchen commented 1 year ago

@mtrezza Yes and yes, my PR handle these steps the same way you wrote it. The reason i opened this issue as a bug is because the code currently has this maxAge definition but just not in the correct place.