parse-community / parse-server

Parse Server for Node.js / Express
https://parseplatform.org
Apache License 2.0
20.85k stars 4.77k forks source link

Wrongly deleted sessions #6749

Closed SebC99 closed 3 years ago

SebC99 commented 4 years ago

Issue Description

We do log a big number of INVALID_SESSION_TOKEN errors for our users, appearing without any apparent logic: sessions are not expired, the user hasn't changed the password, sometimes it happens multiple times in the same day, and we can't figure out why. Is there any way for us to better detect the reason for this? Are we the only one with this weird behavior?

I know there's multiple places in the code where Sessions are deleted (or session caches) but I can't see why it could explain that.

Any help would be appreciated :)

mtrezza commented 4 years ago
SebC99 commented 4 years ago

Unfortunately not, we can't find any pattern It's been a while, but maybe slightly more during last the last weeks, but we also had an increase of users coming back after a few weeks off because of the covid. But with a 1 year session token, that should not happen. We even have cases with users being disconnected each day (through FB login)

dplewis commented 4 years ago

@SebC99 @mtrezza Can I add you to the Parse Team? I would have sent an email sorry about that.

mtrezza commented 4 years ago

@dplewis Sure, please go ahead.

@SebC99 Could it be some external reason? I am thinking of an iOS setting that uninstalls apps automatically when they are not used for some time, and then restores them from cloud. Or auto-backup on Android, see https://github.com/parse-community/Parse-SDK-Android/issues/970#issuecomment-545656891. If a user opens the app again after 2 months, maybe it gets restored and that causes some issues, hence you see more errors after covid?

SebC99 commented 4 years ago

@dplewis not sure it was addressed to me, but if so yes for sure

@mtrezza We have seen the same on test devices where we know for sure this iOS setting is not activated... So I'm guessing it's not linked. It seems to be somewhat proportional with the number of devices used by an account, but it's not even sure.

BTW we also know that we have an issue each time a user restore its phone as Parse always fails to restore the currentUser (we have activated the anonymous account, so it's hard to detect when this occurs). But I think that's an issue with Parse iOS SDK, and it's another subject.

mtrezza commented 4 years ago

I guess the first step would be determining whether the issue is server or client side. The error can be thrown in many different places on the server, can you tell from the stack trace where it is thrown?

SebC99 commented 4 years ago

I know. A hint is that it seems to occur on both iOS and Android apps, so it seems to be server side. But as we are 90% iOS, it's much more frequent on iOS and it's hard to be sure.

On client side, we only see the 209 Invalid Session Token error, but with AWS X-Ray we were able to see more details, and it seems it's always this trace:

Error: Invalid session token
at Object.getAuthForSessionToken (/var/app/current/node_modules/parse-server/lib/Auth.js:106)
at runMicrotasks (<anonymous>)
at processTicksAndRejections (internal/process/task_queues.js:97)

Which is why I supposed it was an issue with wrongly deleted sessions.

mtrezza commented 4 years ago

/lib/Auth.js:106

I assume thats this? https://github.com/parse-community/parse-server/blob/de79b70cbce7abd3b8bae1ef66d5a15ab0a5f144/src/Auth.js#L104-L109

SebC99 commented 4 years ago

I think so... And we have a unique index, and we always have the 'user' field, so it's really because of a deletion of the session object... but why?

mtrezza commented 4 years ago

That really seems like either the session was removed or the token was incorrect. Did you check whether the requests may be malicious, maybe trying to brute force the session token? See https://github.com/parse-community/parse-server/issues/6755

SebC99 commented 4 years ago

We don't see any weird requests, and it also happens with our own devices / sessions. We even found cases where all the sessions token of one user were removed without reasons.

mtrezza commented 4 years ago

Have you set a TTL index on the session collection expireAt field?

mtrezza commented 4 years ago

We even found cases where all the sessions token of one user were removed without reasons.

Interesting that you say "all the session tokens". There should ever only be one session object per user per installation. If all session objects of the user of different installations are deleted, that could point to an issue in your custom code.

If I understand this correctly, before a new session object is created, all existing sessions objects of that user/installation combination are destroyed:

https://github.com/parse-community/parse-server/blob/288e74688848f067cb1a98a3875296189b66b44c/src/RestWrite.js#L975

Interestingly, this is executed before runDatabaseOperation which runs the actual query. If creating the new session fails for some reason, all existing sessions would already be deleted and there would be not any session object left for that user/installation combination, if I understood the flow correctly. The new session is actually created before that in handleSession.

SebC99 commented 4 years ago

No TTL index I think, and we have a lots of old session token, so I guess it wouldn't be the case with a TTL index. (It could be nice to have one haha) All sessions tokens because or tests accounts are used on many devices, and we often logout/login from these accounts, and they are also used on simulators... so they do have lots of installationId :)

But the issue of deleted sessions is for all users.

mtrezza commented 4 years ago

(It could be nice to have one haha)

I added functionality to build a TTL index in https://github.com/parse-community/parse-server/pull/6748 so someone could add that feature easily after merge

All sessions tokens because or tests accounts are used on many devices, and we often logout/login from these accounts, and they are also used on simulators... so they do have lots of installationId :)

I cannot think of a debugging scenario in which a session would be deleted while it is still in use on another device, given that each device has its own installation ID. I also have never experienced that when debugging. Maybe the issue is related to your custom code where you manipulate sessions / installations / users?

Could it be that you try to execute some server calls on the client after the user logged out? I think that would cause the same error?

SebC99 commented 4 years ago

I cannot think of a debugging scenario in which a session would be deleted while it is still in use on another device, given that each device has its own installation ID. I also have never experienced that when debugging. Maybe the issue is related to your custom code where you manipulate sessions / installations / users?

Agreed. But we don’t manipulate sessions/installations. We use standard logout/login, and the only thing we do is saving the user in the installation. We do have triggers on Users but that’s all.

Could it be that you try to execute some server calls on the client after the user logged out? I think that would cause the same error? We do unfortunately as on the iOS SDK the notification for invalid session is not blocking. So other requests can be fired but they have the same error result.

Do you think we could patch the server code to add some debug logs or hints that could explain anything? Thanks

mtrezza commented 4 years ago

Do you think we could patch the server code to add some debug logs or hints that could explain anything?

Definitely, it would be interesting to see which tokens are sent by the clients that are invalid. Now you don't want tokens in your logs, but these are invalid tokens anyway. Then you could log when a token gets deleted, in Parse Server or with even with a mongodb trigger on the DB level. Then you would know:

You may want to log logout / password change / password reset operations as well; I don't know off the top of my head whether these trigger a session delete as well.

SebC99 commented 4 years ago

Yes I'll try to add some logs thanks

mtrezza commented 4 years ago

Sure, let us know what you found.

mtrezza commented 4 years ago

@SebC99 Are there any new insights on this?

SebC99 commented 4 years ago

Unfortunately no, we still have the issue, and we can't figure out why. Our logs in our app are not helping as we only see that people are "loosing" their session token at one point, and that those session tokens were valid just before (with an expiration date in 2021). So we still don't know why those tokens are being deleted by the server

mtrezza commented 4 years ago

Interesting. I will also take a look into logs. For which platforms (SDKs) do you obverse this?

SebC99 commented 4 years ago

We use the iOS, Android and JS SDK and we mostly see that on the iOS one, but that's also because most of our users are using iOS

mtrezza commented 4 years ago

So it does occur with all 3 SDKs?

Can you check that to be sure? For example for Android you'd want to make sure a user does not login with iOS as well and therefore the session is deleted somehow. In other words, if you have users who only use the Android SDK or only the JS SDK, whether they are affected as well.

Did you patch the server to get more logs as discussed?

SebC99 commented 4 years ago

Really hard to say as it's seems to be random. 100% sure it occurs on iOS. 50% sure it occurs on Android (but there's other issues in the Android SDK so it might not be the same reason). The web app can't be compared as there's no real local storage.

We have almost no users using both Android and iOS, but we do have lots of users with multiple iOS devices, and some of them are also using the web app on their computer.

As far as we can see, we see more of that issue with users with multiple devices.

As for the logs, not for now as I don't know what kind of logs to add in parse-server.... We did add logs in the clients app but it didn't help. We could add a beforeDelete hook in the cloud code, but I don't know what to display in the logs that could be helpful.

mtrezza commented 4 years ago

Let's analyze a bit more where we observe it and what we observe, so we don't overlook anything.

we only see that people are "loosing" their session token at one point

50% sure it occurs on Android... there's other issues in the Android SDK so it might not be the same reason... The web app can't be compared as there's no real local storage.

SebC99 commented 4 years ago

By "loosing" I understand that you log INVALID_SESSION_TOKEN on the app? Exactly

Do you also get user feedback about session issues or is that only something you technically observe? Yes they are disconnected without any reason and we have to reconnect them

What are users usually doing before / after INVALID_SESSION_TOKEN? For example, if users log out manually in the app (maybe in the process when they delete their accounts), but some requests are still queued, these requests will fail and would log INVALID_SESSION_TOKEN. Most of the time they are just using the app, and that’s where is the real issue! They open the app and get that error randomly (it’s rare of course but it is clearly too often)

Can you add logs to the web app to see whether you log INVALID_SESSION_TOKEN there as well? Sure!

First, we want to make sure that /lib/Auth.js:106 is actually I think it is sure as there’s only one error in Auth.js with this comment ´Invalid session token´ (We can see the comment in IOS logs)

I’ll add those logs yes! (But again, it appears to ourselves with our test devices where we don’t deauthorize anything)

Thanks !

mtrezza commented 4 years ago

it appears to ourselves with our test devices

Well, if you can reproduce it, maybe you can just run Parse Server in a debugger to analyze the issue.

SebC99 commented 4 years ago

Well no, we can't reproduce it when we want to. We just see it happening from times to times but without any clue letting us reproduce it :(

mtrezza commented 4 years ago

That's unpleasant, then the server logs should hopefully give a hint.

SebC99 commented 4 years ago

Ok I've tried... but in Auth.js, as it fails on line 106 because the token is no more in the _Session collection, I can't have any info about that session. The only thing I have is the old token which doesn't mean anything...

mtrezza commented 4 years ago

I think the key here is to create a hook before a session is deleted. That is currently not possible but you could easily patch Parse Server to allow hooks on the Session class. Otherwise you could create a hook on the DB side, but that may be a bit more complicated.

I think all you would need to do is comment out this check:

https://github.com/parse-community/parse-server/blob/44015c3e351fc7cf0ac3e99bc64738d35ddc3cba/src/triggers.js#L65-L69

(I'm further thinking whether we should remove this restriction on the Session class and at least allow before/after create/delete)

mtrezza commented 4 years ago

Just for reference I'm adding https://github.com/parse-community/parse-server/issues/6726 because it is somewhat related.

SebC99 commented 4 years ago

@mtrezza we might have found something... On iOS, when we run saveInBackground() on the currentUser, it sometimes send the user's password with the request, as if it has been changed (which had not), and the transformUser method of the RestWrite.js file of Parse Server marks the request with generateNewSession: true, which will invalidate every sessions of that user, even the active one. And it's not the case on the web as far as we can see. Now we need to figure out why the password is still sent sometimes.

mtrezza commented 4 years ago

it sometimes send the user's password with the request, as if it has been changed (which had not)

Now that's interesting.

Parse Server marks the request with generateNewSession: true, which will invalidate every sessions of that user

Is it expected that changing a password causes invalidates all other sessions? I know some services do that (e.g. Google) while others do not (e.g. Instagram). I could see that as a feature, but it would have to be consistent across the SDKs of course. If it's currently inconsistent maybe we can take this as a opportunity to introduce a Parse Server configuration parameter for this, unless there is a best practice we should abide to.

SebC99 commented 4 years ago

Well, on one device, when we do a login then just a save, it occurs each time (transforming the session from a login one to a signup one) On another device, never... We investigate!

(I think there’s already a setting in parse server for that: revokeSessionOnPasswordReset, and it's true by default)

SebC99 commented 4 years ago

BTW, this code (in Auth.js) is weird:

RestWrite.prototype.createSessionToken = async function() {
  // cloud installationId from Cloud Code,
  // never create session tokens from there.
  if (this.auth.installationId && this.auth.installationId === 'cloud') {
    return;
  }
  const { sessionData, createSession } = Auth.createSession(this.config, {
    userId: this.objectId(),
    createdWith: {
      action: this.storage['authProvider'] ? 'login' : 'signup',
      authProvider: this.storage['authProvider'] || 'password',
    },
    installationId: this.auth.installationId,
  });
  if (this.response && this.response.response) {
    this.response.response.sessionToken = sessionData.sessionToken;
  }
  return createSession();
};

It means each time a new sessionToken is created (because of a save with a password, even the same as before), the action is set at signup. It shouldn't have any impact, but it's clearly not a signup when a user changes his password.

mtrezza commented 4 years ago

I think there’s already a setting in parse server for that: revokeSessionOnPasswordReset, and it's true by default

You are right, Parse Server never stops to amaze.

mtrezza commented 4 years ago

Can we close this issue or do you have any further info?

SebC99 commented 4 years ago

No more info except it's still happening...

mtrezza commented 4 years ago

I'm reluctant to close this, because if it is indeed a bug, it may be an important one that affects user experience.

You wrote

Well, on one device, when we do a login then just a save, it occurs each time (transforming the session from a login one to a signup one)

Can you still observe this? Could you just debug the iOS app and Parse Server and step through the code to see why this is happening?

SebC99 commented 4 years ago

Unfortunately as soon as we compiled a debug version on the device, it stopped on that one. So we're back with the bug happening randomly, with no clue for now

nopol10 commented 3 years ago

Not sure if it is related to this issue but I've been experiencing a strange problem with invalid session tokens as well. Here's what happens:

  1. Login with Google Auth on my web app
  2. App gets a session token, it matches with what's in the database. Login success.
  3. Logout of Parse and google auth. No issues.

But if I do this

  1. Refrsh the page and login again with google auth. Don't perform a logout
  2. Refresh the page
  3. Since I don't store the session token, I'll have to login again. I login with google again. No issues, apparently...
  4. Try to logout. Invalid session token gets returned instead. So I go ahead and inspect the session token received in the login POST request vs the session token returned in getSessionToken in the User object from loginWith. Turns out they are different, the one in the POST request is what's in the database but the one returned to me by the function is the previous session token. Am I doing something wrong or is there an actual issue here? I'm on 4.2.0 btw as 4.3.0 has the issue which prevents google auth from working .
oli107 commented 3 years ago

I am experiencing a similar issue and am uncertain if this is directly related, but does this only occur when directAccess is set to true? I've tracked down missing session tokens in my app (entirely using the JS SDK) to when there is an update of the user record and directAccess is enabled. It triggers a logout call which removes the session token. I can't quite work out how this is called? I've tried putting some logging into the transformUser method of RestWrite.js but that doesn't seem to get fired when direct access is enabled? Disabling direct access prevents the logout from being called and the sessions are not deleting. I've searched the code until my eyeballs bleed, could either of you @SebC99 or @mtrezza point me in the direction for where a logout call might be made after modifying the user object? It definitely seems to occur, by adding some logging into the ParseServerRESTController.js I can see that a POST to logout is called straight after a PUT to /classes/_User.

mtrezza commented 3 years ago

by adding some logging into the ParseServerRESTController.js I can see that a POST to logout is called straight after a PUT to /classes/_User.

@oli107 This is certainly an interesting observation and to my knowledge the only clearly reproducible case so far. Can you reproduce this with a clean local installation of Parse Server? If so, you could debug Parse Server while running into this scenario and find the stack trace that calls logout.

mtrezza commented 3 years ago

@oli107 @nopol10 @SebC99 Did anyone have any luck in reproducing this issue?

nopol10 commented 3 years ago

Haven't had the time to reproduce in a brand new installation yet unfortunately. Not sure if this question

I am experiencing a similar issue and am uncertain if this is directly related, but does this only occur when directAccess is set to true?

was meant for me but I haven't explicity set directAccess so I guess it should default to false.

mtrezza commented 3 years ago

@SebC99 I stumbled upon this comment and it reminded me of this unsolved mystery - maybe related?

SebC99 commented 3 years ago

@mtrezza the last comment reminds me of the issue we had with the password being sent again, which leads to the parse-server thinking it's a change of password and thus deleting all sessions

mtrezza commented 3 years ago

@SebC99 That's what I was thinking too, sounds remarkably related. Is this still an issue you're investigating?