Always clear sessions when user password is updated

[flovilmart]

No description provided.

[codecov]

Codecov Report

Merging #3821 into master will decrease coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3821      +/-   ##
==========================================
- Coverage   90.18%   90.13%   -0.06%     
==========================================
  Files         114      114              
  Lines        7531     7532       +1     
==========================================
- Hits         6792     6789       -3     
- Misses        739      743       +4

Impacted Files	Coverage Δ
src/RestWrite.js	93.06% <100%> (+0.01%)	⬆️
...rc/Adapters/Storage/Mongo/MongoSchemaCollection.js	94.44% <0%> (-2.78%)	⬇️
src/Controllers/DatabaseController.js	94.36% <0%> (-0.42%)	⬇️

---

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 9dbb89a...51501c7. Read the comment docs.

[acinader]

so, I've adopted a new pattern in my promise shiz.

I used to do this:

async.do()
  .then(done)
  .then(null, done.fail)

the reason for the second then was to prevent an error in the first then block from getting swallowed.

But then I started using bluebird and adopted this:

async.do()
  .then(done)
  .catch(done.fail)

So testing it out a bit....

const Parse = require('parse/node');

const p = new Parse.Promise((resolve, reject) => {
  setTimeout(resolve, 100);
});

p
  .then(() => console.log('boo'))
  .catch(() => console.error('bingo'));

prints 'boo' as you'd expect. If I then mangle the then to create an error:

const Parse = require('parse/node');

const p = new Parse.Promise((resolve, reject) => {
  setTimeout(resolve, 100);
});

p
  .then(() => console.bogusFunction('boo'))
  .catch((e) => console.error(e, 'bingo'));

I get:

TypeError: console.bogusFunction is not a function
    at ParsePromise.p.then (/Users/arthur/code/followstyle-bot/testing.js:8:23)
    at ParsePromise.wrappedResolvedCallback (/Users/arthur/code/followstyle-bot/node_modules/parse/lib/node/ParsePromise.js:153:43)
    at ParsePromise.value (/Users/arthur/code/followstyle-bot/node_modules/parse/lib/node/ParsePromise.js:89:36)
    at ontimeout (timers.js:365:14)
    at tryOnTimeout (timers.js:237:5)
    at Timer.listOnTimeout (timers.js:207:5) 'bingo'

so far so good! now to demonstrate why we bother, if i change it to:

const Parse = require('parse/node');

const p = new Parse.Promise((resolve, reject) => {
  setTimeout(resolve, 100);
});

p
  .then(() => console.bogusFunction('boo'), (e) => console.error(e, 'bingo'));

then it just silently fails! no bueno.

and just to complete the circle, here's how the old style I used worked:

const Parse = require('parse/node');

const p = new Parse.Promise((resolve, reject) => {
  setTimeout(resolve, 100);
});

p
  .then(() => console.bogusFunction('boo'))
  .then(null, (e) => console.error(e, 'bingo'));

which will print out the exact same error as when using the catch

[flovilmart]

that's interesting, having last handler like that, me likey :)

[acinader]

in case it isn't clear, my current preferred is:

async.do()
  .then(done)
  .catch(done.fail);

[flovilmart]

@acinader makes sense, do you want me to update the code for that style? Or we'll do it at a later time?

[acinader]

no need to change this. just an fyi :)

[acinader]

looks good. i left a long comment on promises though :)

[MBDeveloper]

This change caused my users get "invalid session token" error when connecting from different device.
Can you add a flag (server config or environment variable) to control this behaviour ? I would like to turn it off on my server.

[natanrolnik]

@MBDeveloper this should only trigger "invalid session token" errors when users update their passwords.

[MBDeveloper]

We add some level of security that change the password dynamically (the details are not relevant) and since we change the password it invalidate the user token on the other device.
IMHO this kind of changes should be set with flags. (we had a similar issue when the user email was cut off from user details - userSensitiveFields)

[flovilmart]

@MBDeveloper this was a security issue, when the password was reset by a masterKey, the sessions were not invalidated. You should be thankful some people report those issues and make the platform more secure. If this doesn't fit your use case, feel free to fork the project.

[MBDeveloper]

Can you please direct me to the location of the change in the sources so I can submit a pull request that add a flag to override the default behaviour.

[flovilmart]

the location of the change is the PR itself... Not sure what more you need. And as mentioned, this is a security issue and I won't budge on this behaviour.

[henrikperrochon]

Hello @flovilmart ,

There is (or was?) a parameter called revokeSessionOnPasswordReset: true/false when initializing Parse Server

new ParseServer ({appId: "XXX", ..... revokeSessionOnPasswordReset: false});

Is it still taken into account or does the v2.4.1 ignores it?

Thanks!

[flovilmart]

@Helmikku the flag should be still taken into account as properly covered by tests :)