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

feat: extendSessionOnUse to automatically renew Parse Sessions #8505

Merged
merged 6 commits into from
May 17, 2023

Conversation

dblythy
Copy link
Member

@dblythy dblythy commented Apr 12, 2023

Pull Request

Issue

Currently, there is no mechanism to renew sessions. Expires at is immutable.

Closes: #7248

Approach

Introduces a server parameter renewSessions. This feature will extend the session expiry date to the sessionLength when the session is used by a client.

With this feature, sessionLength is more or less the "inactive period", rather than a flat expiry date. This could allow for a lower default sessionLength instead of 1 year.

Tasks

  • Add tests
  • Add changes to documentation (guides, repository pages, code comments)

@parse-github-assistant
Copy link

Thanks for opening this pull request!

@codecov
Copy link

codecov bot commented Apr 12, 2023

Codecov Report

Patch coverage: 93.10% and project coverage change: -0.01 ⚠️

Comparison is base (afd0515) 94.31% compared to head (e933040) 94.31%.

❗ Current head e933040 differs from pull request most recent head 373ce33. Consider uploading reports for the commit 373ce33 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##            alpha    #8505      +/-   ##
==========================================
- Coverage   94.31%   94.31%   -0.01%     
==========================================
  Files         183      183              
  Lines       14520    14547      +27     
==========================================
+ Hits        13695    13720      +25     
- Misses        825      827       +2     
Impacted Files Coverage Δ
src/Options/Definitions.js 100.00% <ø> (ø)
src/Options/index.js 100.00% <ø> (ø)
src/Auth.js 98.85% <92.59%> (-0.73%) ⬇️
src/Config.js 91.07% <100.00%> (+0.05%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mman
Copy link
Contributor

mman commented Apr 12, 2023

Thanks @dblythy for looking into this, Parse Server has been really missing this functionality. Looking at the code it is pretty clear, however from the test case I am not sure what is expected to happen when you use expired sessionToken? The test is written in a way that your session expires now, and yet immediate session.fetch() will (probably IIUC) succeed? Should this be made clearer at least in the test what you mean?

@dblythy
Copy link
Member Author

dblythy commented Apr 12, 2023

No client side changes happen here, and if expiresAt is in the past, invalid session token will still return. The change is that using a valid token changes its expiry.

The test is just a way to edit the raw session object to emulate a valid token that hasn't expired but hasn't been used in a while (updatedAt).

The session expiring now was just to check that the new expiry was updated. The test didn't fail with "invalid session token" because the session tokens are cached for a short period.

@mman
Copy link
Contributor

mman commented Apr 12, 2023

Thanks @dblythy for addressing my question. The test code now makes it clear that expired token should 209, while valid token will extend. I had very similar implementation in my mind as well.

src/Config.js Outdated Show resolved Hide resolved
@mtrezza
Copy link
Member

mtrezza commented May 16, 2023

It seems that all Postgres jobs fail with:

  1. Auth can use renewSessions
  • error: invalid input syntax for type timestamp with time zone: "{"__type":"Date","iso":"2010-01-01T00:00:00.000Z"}" (line 3782)

@dblythy dblythy changed the title feat: renewSessions to automatically renew Parse Sessions feat: extendSessionOnUse to automatically renew Parse Sessions May 16, 2023
@dblythy dblythy requested a review from a team May 16, 2023 06:34
Copy link
Member

@mtrezza mtrezza left a comment

Choose a reason for hiding this comment

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

Looks good!

@mtrezza mtrezza merged commit 6f885d3 into parse-community:alpha May 17, 2023
parseplatformorg pushed a commit that referenced this pull request May 17, 2023
# [6.1.0-alpha.11](6.1.0-alpha.10...6.1.0-alpha.11) (2023-05-17)

### Features

* `extendSessionOnUse` to automatically renew Parse Sessions ([#8505](#8505)) ([6f885d3](6f885d3))
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 6.1.0-alpha.11

@parseplatformorg parseplatformorg added the state:released-alpha Released as alpha version label May 17, 2023
@dblythy dblythy deleted the token-expiry branch May 18, 2023 13:15
parseplatformorg pushed a commit that referenced this pull request Jun 10, 2023
# [6.3.0-beta.1](6.2.0...6.3.0-beta.1) (2023-06-10)

### Bug Fixes

* Cloud Code Trigger `afterSave` executes even if not set ([#8520](#8520)) ([afd0515](afd0515))
* GridFS file storage doesn't work with certain `enableSchemaHooks` settings ([#8467](#8467)) ([d4cda4b](d4cda4b))
* Inaccurate table total row count for PostgreSQL ([#8511](#8511)) ([0823a02](0823a02))
* LiveQuery server is not shut down properly when `handleShutdown` is called ([#8491](#8491)) ([967700b](967700b))
* Rate limit feature is incompatible with Node 14 ([#8578](#8578)) ([f911f2c](f911f2c))
* Unnecessary log entries by `extendSessionOnUse` ([#8562](#8562)) ([fd6a007](fd6a007))

### Features

* `extendSessionOnUse` to automatically renew Parse Sessions ([#8505](#8505)) ([6f885d3](6f885d3))
* Add new Parse Server option `preventSignupWithUnverifiedEmail` to prevent returning a user without session token on sign-up with unverified email address ([#8451](#8451)) ([82da308](82da308))
* Add option to change the log level of logs emitted by Cloud Functions ([#8530](#8530)) ([2caea31](2caea31))
* Add support for `$eq` query constraint in LiveQuery ([#8614](#8614)) ([656d673](656d673))
* Add zones for rate limiting by `ip`, `user`, `session`, `global` ([#8508](#8508)) ([03fba97](03fba97))
* Allow `Parse.Object` pointers in Cloud Code arguments ([#8490](#8490)) ([28aeda3](28aeda3))

### Reverts

* fix: Inaccurate table total row count for PostgreSQL ([6722110](6722110))
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 6.3.0-beta.1

@parseplatformorg parseplatformorg added the state:released-beta Released as beta version label Jun 10, 2023
parseplatformorg pushed a commit that referenced this pull request Jun 18, 2023
# [6.3.0-alpha.1](6.2.0...6.3.0-alpha.1) (2023-06-18)

### Bug Fixes

* Cloud Code Trigger `afterSave` executes even if not set ([#8520](#8520)) ([afd0515](afd0515))
* GridFS file storage doesn't work with certain `enableSchemaHooks` settings ([#8467](#8467)) ([d4cda4b](d4cda4b))
* Inaccurate table total row count for PostgreSQL ([#8511](#8511)) ([0823a02](0823a02))
* LiveQuery server is not shut down properly when `handleShutdown` is called ([#8491](#8491)) ([967700b](967700b))
* Rate limit feature is incompatible with Node 14 ([#8578](#8578)) ([f911f2c](f911f2c))
* Unnecessary log entries by `extendSessionOnUse` ([#8562](#8562)) ([fd6a007](fd6a007))

### Features

* `extendSessionOnUse` to automatically renew Parse Sessions ([#8505](#8505)) ([6f885d3](6f885d3))
* Add new Parse Server option `preventSignupWithUnverifiedEmail` to prevent returning a user without session token on sign-up with unverified email address ([#8451](#8451)) ([82da308](82da308))
* Add option to change the log level of logs emitted by Cloud Functions ([#8530](#8530)) ([2caea31](2caea31))
* Add support for `$eq` query constraint in LiveQuery ([#8614](#8614)) ([656d673](656d673))
* Add zones for rate limiting by `ip`, `user`, `session`, `global` ([#8508](#8508)) ([03fba97](03fba97))
* Allow `Parse.Object` pointers in Cloud Code arguments ([#8490](#8490)) ([28aeda3](28aeda3))

### Reverts

* fix: Inaccurate table total row count for PostgreSQL ([6722110](6722110))
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 6.3.0-alpha.1

{ sessionToken },
{ limit: 1 }
).execute();
console.log({ results });
Copy link
Member

Choose a reason for hiding this comment

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

console.log() here

return;
}
clearTimeout(throttle[sessionToken]);
throttle[sessionToken] = setTimeout(async () => {
Copy link
Member

Choose a reason for hiding this comment

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

throttle seems to be a simple object never cleared, and it's not an LRU, memory is exposed to DDOS issues (since it seems that every token will be stored in memory), and the memory will grow infinitely

Copy link
Member

Choose a reason for hiding this comment

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

I checked also clearTimeout function do not unset in the record on the throttle object. So the timeout instance is cancelled but still there in memory

Copy link
Member Author

@dblythy dblythy Jul 6, 2023

Choose a reason for hiding this comment

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

Good point, I’ll open a new issue and fix

Copy link
Member

Choose a reason for hiding this comment

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

I checked also clearTimeout function do not unset in the record on the throttle object. So the timeout instance is cancelled but still there in memory

@Moumouls
Copy link
Member

Moumouls commented Jul 6, 2023

@dblythy @mtrezza i'm late here but there is some issues with this implementation :)

@Moumouls
Copy link
Member

Moumouls commented Jul 6, 2023

Hi there, i was just reviewing this feature from a merge on my fork.

I just want to add a comment to help may be other developers landing here, if i understand correctly the system here:

Context:

  • set sessionLength to 3 days
  • use extendSessionOnUse

User:

  • Login and receive token that will expire in 3 days
  • If user perform API calls with the provided session token on the second day the token expiry is rescheduled in 3 days.

Limitations:

  • extend feature should not be considered as a improved security system for session token, since it's not a session token change but a rescheduled expiry
  • extend feature can't be combined with a session length of less than a day ( ex short lived token of 15min)

Security concerns:

  • A "one time" stolen token could be renew infinitely
  • Since the token do not change through time, this feature should not be considered as a strong mechanism
    Behavior of this feature:
  • Be able to automatically logout inactive users ( out of sessionLength range), and force to login again after some time

Suggested futur improvements:

  • Follow the common refresh_token and access_token strategy to strongly secure authentication and token renewal.

Some doc from Internet Task Force: https://mailarchive.ietf.org/arch/msg/oauth/vSmJ0zjQzZFjeFbRz_qpvjfpAeU/
Here the RFC of a modern refresh, session and token management : https://www.rfc-editor.org/rfc/rfc6749#section-1.4

@Moumouls
Copy link
Member

Moumouls commented Jul 6, 2023

Btw @dblythy , this feature is okay, and it will help many developers to have shorter session length, but we should not advertise this feature as a true renewal mechanism. I think the issue should be reopened waiting a proper PR with a full Oauth2 mechanism using refresh tokens.

What do you think ?

@dblythy
Copy link
Member Author

dblythy commented Jul 6, 2023

Hmmm, perhaps we should try to have full JWT support for Parse Server 7

@Moumouls
Copy link
Member

Moumouls commented Jul 6, 2023

i think a new Oauth 2 standard system with token renewal, access token and refresh token seems a good idea :)

But it need also a important amount of work i think @dblythy

@mtrezza
Copy link
Member

mtrezza commented Jul 6, 2023

I agree that JWT support would be nice. Should we open a new issue and put a bounty on it?

I'd like to throw in that, a self-extending token comes with similar risks as a long-living token. So this feature bears a similar risk as setting the option sessionLength to a high value.

extend feature should not be considered as a improved security system for session token

I don't think that has been the premise or that it's being promoted as such (it's off by default). It's a feature that may have its use cases in certain applications, just like a long-living session.

There may be clients who do not support a refresh token mechanism (IoT). For them it may be more secure to have a session token that expires after not using it for a while than issuing a session token that never expires. I can also imagine that this feature be extended in the future to conditionally extend the session, allowing for even more use cases.

In any case, I wouldn't swap a refresh token mechanism for the current extend session feature, just like we don't limit the max session length a developer can set.

We have the API docs and a Best Practice section in the docs to add any supplementary warning note about the implications of setting Parse Server options, so these can always be amended if we feel something is underdocumented.

@formatCvt
Copy link

btw don't forget about refresh token rotation :)

@mtrezza
Copy link
Member

mtrezza commented Jul 7, 2023

@formatCvt Could you open a new issue for adding JWT with refresh token and add your comment? This PR is closed and related to a different solution and won't be tracked.

parseplatformorg pushed a commit that referenced this pull request Sep 16, 2023
# [6.3.0](6.2.2...6.3.0) (2023-09-16)

### Bug Fixes

* Cloud Code Trigger `afterSave` executes even if not set ([#8520](#8520)) ([afd0515](afd0515))
* GridFS file storage doesn't work with certain `enableSchemaHooks` settings ([#8467](#8467)) ([d4cda4b](d4cda4b))
* Inaccurate table total row count for PostgreSQL ([#8511](#8511)) ([0823a02](0823a02))
* LiveQuery server is not shut down properly when `handleShutdown` is called ([#8491](#8491)) ([967700b](967700b))
* Rate limit feature is incompatible with Node 14 ([#8578](#8578)) ([f911f2c](f911f2c))
* Unnecessary log entries by `extendSessionOnUse` ([#8562](#8562)) ([fd6a007](fd6a007))

### Features

* `extendSessionOnUse` to automatically renew Parse Sessions ([#8505](#8505)) ([6f885d3](6f885d3))
* Add new Parse Server option `preventSignupWithUnverifiedEmail` to prevent returning a user without session token on sign-up with unverified email address ([#8451](#8451)) ([82da308](82da308))
* Add option to change the log level of logs emitted by Cloud Functions ([#8530](#8530)) ([2caea31](2caea31))
* Add support for `$eq` query constraint in LiveQuery ([#8614](#8614)) ([656d673](656d673))
* Add zones for rate limiting by `ip`, `user`, `session`, `global` ([#8508](#8508)) ([03fba97](03fba97))
* Allow `Parse.Object` pointers in Cloud Code arguments ([#8490](#8490)) ([28aeda3](28aeda3))

### Reverts

* fix: Inaccurate table total row count for PostgreSQL ([6722110](6722110))
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 6.3.0

@parseplatformorg parseplatformorg added the state:released Released as stable version label Sep 16, 2023
@magnacartatron
Copy link

@mtrezza I don't understand how this feature works.
So this is where the magic happens and the session gets extended.

      const lastUpdated = new Date((_session = session) === null || _session === void 0 ? void 0 : _session.updatedAt);
      const yesterday = new Date();
      yesterday.setDate(yesterday.getDate() - 1);
      if (lastUpdated > yesterday || !session) {
        return;
      }
      const expiresAt = config.generateSessionExpiresAt();
      await new _RestWrite.default(config, master(config), '_Session', {
        objectId: session.objectId
      }, {
        expiresAt: Parse._encode(expiresAt)
      }).execute();

But looking at this piece of code it takes lastUpdated and yesterday and if lastUpdated is greater than yesterday it returns.

So if I've doing 60 minute sessions, well this will never actually extend the session. So for this to work a session needs to be at least 24 hours.

This isn't documented anywhere and it's counterintuitive.

Am I missing something.

@mtrezza
Copy link
Member

mtrezza commented Feb 27, 2024

Not sure why this is not simply comparing the date timestamp. Looks odd. Would you open an issue? It seems, the fix could be quite simple as well by comparing the dates instead of introducing a yesterday var.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state:released Released as stable version state:released-alpha Released as alpha version state:released-beta Released as beta version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sessiontoken renewal
7 participants