SessionToken cache for invalid sessions tokens #4926

flavionegrao · 2018-08-03T15:07:33Z

When a subscription in the live query server has a sessionToken attached and an updated from the parse servers comes in triggered by a afterSave or afterDelete the live query server has to match the permissions against the User related to the subscription/sessionToken.

When the sessionToken for some reason gets invalidated, the subscription in the live query remains quering the backend every time.
This pull request created a entry in the SessionTokenCache as undefined for this situation then avoiding redundant queries.

flovilmart

Thanks for the Pr.

Can you address the different questions as well as revert the changes in the tests?

We like to avoid breaking changes and all the changes in the tests are kinda scary.

Also, I’m not sure those changes about the cache won’t have any bad effect, as the new code interprets an undefined value as an invalid session, which may not be always true.

flovilmart · 2018-08-03T19:14:49Z

spec/SessionTokenCache.spec.js

      done();
    });
-  });
+    fit('can get inexisting userId', function(done) {


Oops, this blocks the tests from executing

flovilmart · 2018-08-03T19:15:07Z

src/LiveQuery/SessionTokenCache.js

@@ -2,14 +2,16 @@ import Parse from 'parse/node';
 import LRU from 'lru-cache';
 import logger from '../logger';

-function userForSessionToken(sessionToken){
+function userIdForSessionToken(sessionToken) {
  var q = new Parse.Query("_Session");


Do we really need to change this method?

flovilmart · 2018-08-03T19:18:07Z

src/LiveQuery/SessionTokenCache.js

-      logger.verbose('Fetch userId %s of sessionToken %s from Cache', userId, sessionToken);
-      return Parse.Promise.as(userId);
+
+    if (this.cache.has(sessionToken)) {


This logic may be problematic, if we need to invalidate a session we should mark it INVALID_SESSION_TOKEN.

Also I’m not sure that the solution is the right one as every time a session will be missing, we’ll interpret as invalid. I’d rather add a Boolean onlyFromCache = false of we don’t want to poke the server.

codecov · 2018-08-07T04:25:47Z

Codecov Report

Merging #4926 into master will decrease coverage by 73.37%.
The diff coverage is 28.57%.

@@             Coverage Diff             @@
##           master    #4926       +/-   ##
===========================================
- Coverage   92.96%   19.58%   -73.38%     
===========================================
  Files         119      119               
  Lines        8837     8840        +3     
===========================================
- Hits         8215     1731     -6484     
- Misses        622     7109     +6487

Impacted Files	Coverage Δ
src/LiveQuery/SessionTokenCache.js	`34.61% <28.57%> (-52.35%)`	⬇️
src/cloud-code/HTTPResponse.js	`0% <0%> (-100%)`	⬇️
src/Adapters/Cache/NullCacheAdapter.js	`0% <0%> (-100%)`	⬇️
src/requiredParameter.js	`0% <0%> (-100%)`	⬇️
src/Push/utils.js	`4.76% <0%> (-95.24%)`	⬇️
src/LiveQuery/Subscription.js	`4.76% <0%> (-95.24%)`	⬇️
src/AccountLockout.js	`2.7% <0%> (-94.6%)`	⬇️
src/middlewares.js	`3.61% <0%> (-94.58%)`	⬇️
src/StatusHandler.js	`5.14% <0%> (-94.12%)`	⬇️
src/Options/parsers.js	`6.06% <0%> (-93.94%)`	⬇️
... and 92 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a61ef7e...378442a. Read the comment docs.

flovilmart

Thanks for the changes. Can we focus on:

fix the issues with the session token invalidation
pull out what you added for the roles cache as it is it’s onw PR (which has been started on Live query CLP #4387)

flovilmart · 2018-08-07T08:12:08Z

.eslintrc.json

@@ -22,6 +22,7 @@
        "no-multiple-empty-lines": 1,
        "prefer-const": "error",
        "space-infix-ops": "error",
-        "no-useless-escape": "off"
+        "no-useless-escape": "off",
+        "no-console":"off"


Please revert

flovilmart · 2018-08-07T08:12:47Z

spec/ParseLiveQueryServer.spec.js

@@ -1065,9 +1080,9 @@ describe('ParseLiveQueryServer', function() {
          //Return a role with the name "liveQueryRead" as that is what was set on the ACL
          const liveQueryRole = new Parse.Role();
          liveQueryRole.set('name', 'liveQueryRead');
-          return [
+          return Parse.Promise.as([


Parse.Promise is removed now, please use Promose

flovilmart · 2018-08-07T08:14:09Z

spec/SessionTokenCache.spec.js

@@ -2,49 +2,75 @@ const SessionTokenCache = require('../lib/LiveQuery/SessionTokenCache').SessionT

 describe('SessionTokenCache', function() {

-  beforeEach(function(done) {
-    const Parse = require('parse/node');
+  describe('Valid session', function() {


As mentioned before, can we please not change all tests this way? But add new tests as we go?

flovilmart · 2018-08-07T08:15:29Z

src/LiveQuery/RoleCache.js

+  user.id  = userId;
+  const rolesQuery = new Parse.Query(Parse.Role);
+  rolesQuery.equalTo("users", user);
+  return rolesQuery.find({useMasterKey:true});


This is invalid, as roles can be referenced in roles etc...

Also, the roles cache should /could be shared with the existing parse-server one

flovilmart · 2018-08-08T20:00:18Z

@flavionegrao I've just opened #4940 which refactors heavily Auth.js. With the refactor, you should be able to:

Authenticate a session token and get the user
Get all the roles for a particular user
Reuse a shared cache used by the server or a new in memory one.

flavionegrao · 2018-08-08T20:05:51Z

Sorry didn’t answer the changes you requested me on my last PR. I will review it asap including those last changes you mentioned. By the way have a look what happened with my Parse Servers after I have enabled Roles caching at the live query servers: <img width="720" alt="pastedgraphic-1" src="https://user-images.githubusercontent.com/1597236/43861498-69ac9682-9b2d-11e8-8128-fd74f43528d0.png"> Cheers.

…

Em 8 de ago de 2018, à(s) 17:00, Florent Vilmart ***@***.***> escreveu: @flavionegrao <https://github.com/flavionegrao> I've just opened #4940 <#4940> which refactors heavily Auth.js. With the refactor, you should be able to: Authenticate a session token and get the user Get all the roles for a particular user Reuse a shared cache used by the server or a new in memory one. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4926 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABhfNMhr9U5kVcPwbMFoZho5WmAOgxLLks5uO0NmgaJpZM4VuKJG>.

flovilmart · 2018-08-08T20:08:01Z

As I mentioned, I refactored the Auth.js module in order to help you achieve your goal. We can rip out the SessionTokenCache and use the Auth.js features with a custom cache controller or a shared one from Parse Server.

flavionegrao · 2018-08-08T20:54:22Z

Roger that.

…

Em 8 de ago de 2018, à(s) 17:08, Florent Vilmart ***@***.***> escreveu: As I mentioned, I refactored the Auth.js module in order to help you achieve your goal. We can rip out the SessionTokenCache and use the Auth.js features with a custom cache controller or a shared one from Parse Server. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4926 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABhfNIdaCsSKjXiZcZkVY-TOY7FXgNYzks5uO0UwgaJpZM4VuKJG>.

flovilmart · 2018-08-08T21:16:19Z

So just to sum up quickly:

I'M 💯 with you on this PR let me know if you need anything:
The user authentication in liveQuery should be refactored using Auth.js and Auth module refactoring in order to be reusable #4940
CLP + Roles support is provided with Live query CLP #4387, but this PR needs all the goodies of the step 2.
We can then provide a way to provide a cache adapter to the liveQuery server
And optionally, if liveQuery lives on the same server as Parse server, pass an config

What do you think?

flovilmart · 2018-08-09T01:49:55Z

@flavionegrao I went ahead and rebased the CLP for live query features with all the requirements that you may need:

#4387

Can you check it out an let me know if there's anything else we can do to improve it?

stale · 2018-09-23T02:22:52Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

flavionegrao and others added 4 commits August 3, 2018 01:46

invalid session tokens blacklisted

01b772a

Invalid sessions tokens tests

d7511a8

unit test adjusts

8062f49

better comments

4cd9511

flavionegrao changed the title ~~Master~~ SessionToken cache for invalid sessions tokens Aug 3, 2018

flovilmart suggested changes Aug 3, 2018

View reviewed changes

Cache for Parse.Roles added

3e9a0f9

flavionegrao added 3 commits August 7, 2018 01:43

defaut timeout adjusted

92f846a

bug fix

82ea7c6

bugfix

378442a

flovilmart suggested changes Aug 7, 2018

View reviewed changes

flovilmart mentioned this pull request Aug 8, 2018

Auth module refactoring in order to be reusable #4940

Merged

stale bot added the wontfix label Sep 23, 2018

stale bot closed this Sep 30, 2018

snyk-bot mentioned this pull request Apr 1, 2021

[Snyk] Upgrade apollo-server-express from 2.21.0 to 2.21.1 #7308

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SessionToken cache for invalid sessions tokens #4926

SessionToken cache for invalid sessions tokens #4926

flavionegrao commented Aug 3, 2018

flovilmart left a comment

flovilmart Aug 3, 2018

flovilmart Aug 3, 2018

flovilmart Aug 3, 2018

codecov bot commented Aug 7, 2018 •

edited

Loading

flovilmart left a comment

flovilmart Aug 7, 2018

flovilmart Aug 7, 2018

flovilmart Aug 7, 2018

flovilmart Aug 7, 2018

flovilmart Aug 7, 2018

flovilmart commented Aug 8, 2018

flavionegrao commented Aug 8, 2018 via email •

edited

Loading

flovilmart commented Aug 8, 2018

flavionegrao commented Aug 8, 2018 via email

flovilmart commented Aug 8, 2018

flovilmart commented Aug 9, 2018

stale bot commented Sep 23, 2018

SessionToken cache for invalid sessions tokens #4926

SessionToken cache for invalid sessions tokens #4926

Conversation

flavionegrao commented Aug 3, 2018

flovilmart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 7, 2018 • edited Loading

Codecov Report

flovilmart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flovilmart commented Aug 8, 2018

flavionegrao commented Aug 8, 2018 via email • edited Loading

flovilmart commented Aug 8, 2018

flavionegrao commented Aug 8, 2018 via email

flovilmart commented Aug 8, 2018

flovilmart commented Aug 9, 2018

stale bot commented Sep 23, 2018

codecov bot commented Aug 7, 2018 •

edited

Loading

flavionegrao commented Aug 8, 2018 via email •

edited

Loading