Allow logout with invalid session

[dblythy]

New Feature / Enhancement Checklist

• I am not disclosing a vulnerability.
• I am not just asking a question.
• I have searched through existing issues.

Current Limitation

Currently, if your session token becomes corrupted, even logging out returns "invalid session token", or "Session token is expired", meaning for most users there's no way to resolve this issue without force quitting / reinstalling.

Feature / Enhancement Description

Invalid Session Tokens shouldn't prevent users from logging out and Parse.User.current() from clearing. Even with an invalid session, logout should allow users to re-login.

Example Use Case

Alternatives / Workarounds

3rd Party References

Discussed here

[cbaker6]

Definitely can cause an issue on the client side. I did a fix in parse-community/Parse-Swift#73

which essentially clears the user from the Keychain ignoring any server errors (code):

public static func logout(options: API.Options = []) throws {
        let error = try? logoutCommand().execute(options: options)
        //Always let user logout locally, no matter the error.
        deleteCurrentContainerFromKeychain()
        BaseParseInstallation.deleteCurrentContainerFromKeychain()
        BaseConfig.deleteCurrentContainerFromKeychain()
        //Wait to throw error
        if let parseError = error {
            throw parseError
        }
    }

[dblythy]

Thanks @cbaker6 for your example, that's interesting because obviously a different error could occur here too.

My thoughts are that if an invalid session is provided to the logout request, it should return with a "success" so current user can be cleared internally, and developer can redirect to login screen. It would just mean logout cloud triggers wouldn't fire for invalid sessions, as there would be no way to tell who the "real" user is who making the request is.

What do you think?

[cbaker6]

That sounds good to me because it seems the user is "attempting" to discard their current session token by logging out.

If the server does what you mention, then the other client SDKs wouldn't have to implement something similar to what I have above.

The only issue they would run into is if a different error from the server occurs.

[cbaker6]

that's interesting because obviously a different error could occur here too.

Looks like we were thinking the same thing!

[dblythy]

Interestingly in the JS guide it recommends calling Parse.User.logOut() to handle invalid sessions, which won't currently work.

[cbaker6]

Interestingly in the JS guide it recommends calling Parse.User.logOut() to handle invalid sessions, which won't currently work.

It seems if the JS SDK isn't doing something similar to ParseSwift, the user will get stuck in limbo unless there's another method JS provides to clear the Keychain. Or, delete the app as you suggested...

[mtrezza]

My thoughts are that if an invalid session is provided to the logout request, it should return with a "success" so current user can be cleared internally

This should be handled by the client application. Making the server "lie" about the result of an operation prohibits the client SDK or client application to perform more complex flows, depending on the specific server response. We discussed a similar issue in https://github.com/parse-community/parse-server/security/advisories/GHSA-cfgm-jqhh-xmhr#advisory-comment-64808 and came to the same conclusion.

The communication between the server and the SDK should be truthful, the communication between the SDK and the client application as well. The communication between the client application and the user can be as opaque as the developer prefers and needs for their use case.

Interestingly in the JS guide it recommends calling Parse.User.logOut() to handle invalid sessions, which won't currently work.

This seems to be the actual bug the needs to be addressed, and maybe in other SDKs as well.

[dblythy]

This seems to be the actual bug the needs to be addressed, and maybe in other SDKs as well.

So you would suggest catching the error in the logout request in the JS SDK?

[mtrezza]

I suggest that the SDK properly cleans up the cached current user (if that makes sense technically), but reports to the client application truthfully, that the logout failed. It is up to the developer to catch these errors and deal with them, not up to the SDK to "lie" about the outcome of an operation in my opinion.

As I understand it, the logic that is mentioned above for the Swift SDK is the correct approach, it clears any internally cached current user (which the SDK needs technically), and truthfully throws the error to the client application.

[dblythy]

So, if I have:

await Parse.User.logOut();
window.location.href = '/login';

Would I have to wrap that in:

try {
  await Parse.User.logOut();
  window.location.href = '/login';
} catch (e) {
  if (e.code === 209) {
    window.location.href = '/login';
  }
  alert(e.message);
}

[mtrezza]

Yes. And this allows another developer to not redirect to /login but do something completely different, depending on their use case.

[dblythy]

Infact, that's how the code functions as is:

it('can logout with expired session token', async () => {
    await Parse.User.signUp('asdf', 'zxcv');
    const sessionQuery = new Parse.Query(Parse.Session);
    const session = await sessionQuery.first({ useMasterKey: true });
    const database = Config.get(Parse.applicationId).database;
    await database.update(
      '_Session',
      { objectId: session.id },
      { expiresAt: new Date().setFullYear(2010) },
      {}
    );
    try {
      await Parse.User.logOut();
      fail('should have returned invalid session');
    } catch (e) {
      expect(e.code).toBe(209);
      expect(Parse.User.current()).toBe(null);
    }
  });

It's my opinion that this is still a little clunky, and perhaps a response of "invalid session" can be passed if the logout request contains an invalid session. The logout is still successful, the invalid session token does not prevent the logout action from completing.

[dblythy]

Also the docs might need to updated to:

function handleParseError(err) {
  switch (err.code) {
    case Parse.Error.INVALID_SESSION_TOKEN: {
      try {
        Parse.User.logOut();
      } catch (e) {
        if (e.code !== 209) { 
          handleParseError(e);
        }
      }
      ... // If web browser, render a log in screen
      ... // If Express.js, redirect the user to the log in route
      break;

    ... // Other Parse API errors that you want to explicitly handle
  }
}

// For each API request, call the global error handler
query.find().then(function() {
  ...
}, function(err) {
  handleParseError(err);
});

[mtrezza]

It's my opinion that this is still a little clunky

How so? You mean because it is throwing but still clearing the local current user?

[dblythy]

If you're using an app, and all your errors are "your session has expired", you would think "ok, I need to log back out". Then you go and click "logout", and unless the Parse developer is aware of the way logout works with invalid sessions, you get the error message "your session has expired" (which you already know - that's why you're logging out).

I would assume that most developers aren't catching specifically for error code 209 in their logout calls, which I could imagine would lead to frustration with end users. I know I wasn't. Maybe this just requires a doco improvement.

[mtrezza]

The actual inconsistency for me is that the local cache is cleared if logout throws 209, but not if any other request throws 209. If the local cache was cleared on any 209 regardless of the type of request, there would be no need for the developer to call logout on invalid session token.

I see three possible approaches:

• a) Clear the local cache on any 209 regardless of request type
• b) Never clear the local cache unless the user logs in or the developer explicitly clears it with a method like Parse.clearLocalCache() (which may not exist yet)
• c) Clear the local cache only on logout with 209 but not on other requests with 209, documenting this "special behavior" (or "clunkyness" as you said) in the docs

I would go with a) unless we can think of any reason why a user should stay locally logged in although the authoritative entity (Parse Server) says the token is not and will never ever become valid again.

Now I can think of a use case where an app is offline, the user does some operations, then the app goes online and the server responds with 209. A developers could still want to see these operations being cached and uploaded after the user logs in and renews the token. Not sure is that is currently possible l, but if it is, we wouldn't want to break it, and that could speak for approach b) or c).

[dblythy]

The actual inconsistency for me is that the local cache is cleared if logout throws 209

I'm not sure if that's the case. I've been trying to figure out how it works in the JS SDK.

Now I can think of a use case where an app is offline, the user does some operations

Theoretically a developer could temporarily store the invalid user alongside the cached operations and ask the user to confirm the tasks once the session is revalidated. You're right, fundamentally changing the way this works could cause some unintended side effects.

Again, most of the documentation states:

// Option 1: Show a message asking the user to log out and log back in.

// If the user needs to finish what they were doing, they have the opportunity to do so.

// Option #2: Show login screen so user can re-authenticate.

// You may want this if the logout button is inaccessible in the UI.

I guess in the case of option 1, the could still pin objects to LDS with an invalid session?

Perhaps there's no need for any change on the server.

I probably still haven't explained properly but by clunky I mean it's sorta unintuitive that to handle invalid sessions, we recommend calling Parse.User.logOut(), which needs to be wrapped in a catch because an invalid session token error will be thrown. Effectively calling Parse.User.logOut() does nothing server side in this case.

So, I think the existing behaviour is fine, we just need to change Parse.User.logOut(); in the "handling invalid sessions" with Parse.User._clearCache() (as this does everything logout does locally without throwing). That way, if an invalid session error is thrown from the user clicking logout, the handleParseError will detect it, clear user cache, and redirect to login.

[dblythy]

We could also add something to the SDK such as Parse.User.logOut({muteInvalidSession: true})

[mtrezza]

So, I think the existing behaviour is fine, we just need to change Parse.User.logOut(); in the "handling invalid sessions" with Parse.User._clearCache() (as this does everything logout does locally without throwing). That way, if an invalid session error is thrown from the user clicking logout, the handleParseError will detect it, clear user cache, and redirect to login.

That sounds like approach b) and it seems to be a clean approach because it separates the logout functionality from the session clearing. However, if the session clearing is unrelated to pinning objects and resuming operations once the user reauths, I would still go with a) if the session cache is only managed and relevant internally for the SDK and opaque to the developer, because otherwise the question would be, why does the developer have to care about the SDK's session clearing?

Another thing to consider are server side hooks like before/afterLogout. I assume that a logout calls with an invalid session also does not trigger them? That sounds actually like a test case we should make sure to have.

We could also add something to the SDK such as Parse.User.logOut({muteInvalidSession: true})

Yes, that's a possibility. I suggest to evaluate what most developers would need and intuitively assume, define the default behavior and what should be configurable and see what changes are needed. For example, is this option you mention really something needed on request level, or does it make sense to define it in the SDK config whether to clear the session token on 209 for any request or should it happen automatically that the cache is cleared because the SDK's internal cache doesn't matter to the developer?

[dblythy]

Closing as I see this more as a JS SDK discussion. Will reopen and continue discussion shortly.