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?