Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improving Invalid Session Handling #307

Closed
noahsilas opened this issue Jul 7, 2016 · 4 comments · May be fixed by #1803
Closed

Improving Invalid Session Handling #307

noahsilas opened this issue Jul 7, 2016 · 4 comments · May be fixed by #1803

Comments

@noahsilas
Copy link
Contributor

According to the documentation on Handling Invalid Session Token Error, the appropriate resolution is to attach an error handler to every call out to the Parse server.

Applying this change to an existing aplication is a bit of a chore in terms of finding all of the appropriate places to add the error handler. It also places a burden on developers to remember to include this handler whenever writing new code.

I can think of a couple ways to address this:

  • Always log the user out on INVALID_SESSION_TOKEN

    We build in to the framework a special case for this error that directly invokes Parse.User.logOut. This has the benefit of immediately changing the state of Parse.User.current to reflect the new information, which may be sufficient for some applications to understand the state change. The downside is that this may be a breaking change in the SDK for some applications that aren't expecting it.

  • Allow registration of a global error handling callback

    Imagine initializing the SDK could look something like:

    Parse.initialize(APP_ID);
    Parse.serverErrorHandler(function(err) {
      if (err.code === Parse.Error.INVALID_SESSION_TOKEN) {
        Parse.User.logOut();
      }
    });

    When this handler is present, it could be automatically invoked whenever we receive an error response from the Parse server. This has the downside of being extra configuration, but is extremely flexible in how it could be used. It also doesn't present any possibility of being an API breaking change, as the behavior would remain the same if the global handler is not registered.

How would folks feel about a pull request for either of these strategies? Are there other options to improve this that folks would rather see?

@andrewimm
Copy link
Contributor

andrewimm commented Jul 7, 2016

It's definitely important for app developers to handle invalid sessions in cases where they might want to allow users to log out specific devices. However, I don't believe there is a general catch-all solution for this, since the cases that result in such an error are broad.

The first solution you describe is a little complex, because applications can explicitly specify any token to make a request with (using the permissions options on any network request). Therefore, I could feasibly be logged in with one session for User A, and be using a different token for User A to make a request (or an unrelated token entirely). Just because that token fails doesn't mean that I should log out my current user, nor does it mean that I should specifically log out User A. You can see how such behavior would cause confusion for users being automatically logged out while making a simple query fetch (not to mention the breaking changes you mention). This solution is also hard to use in practice, because there is no universal log-out listener. Applications need to be able to redirect to the log-in screen whenever they are logged out, and if this behavior happens behind the scenes it's impossible to know this happened.

The second suggestion is closer to what we suggest, but we don't believe there should be a universal global handler for the entire SDK.
We previously explored the ideas of global handlers (for login, logout, session events mostly), but we found that they created confusion for developers, particularly around what callback fires when. Also, a catch-all was too powerful, and was being called when users didn't intend too -- implicit calls are risky. Our recommended solution is to implement your function as described, and explicitly use it as the callback to a final .catch() on any promise chain. Ignoring Parse for a second and just speaking as a JS developer, it's best practice to already ensure that all promise calls end with a standalone error handler, so that any errors aren't silently thrown away.

// BAD
doSomethingAync().then(() => {
  // ...
}).then(() => {
  // Any errors here aren't handled below
}, (err) => {
  // ...
});

// GOOD
doSomethingAync().then(() => {
  // ...
}).then(() => {
  // Errors here are still handled!
}).catch((err) => {
  // handle any and all errors
});

So I'd recommend to all that you use the second, but that it should not be a global property of the SDK. Having the freedom to implement this yourself also lets you chain together multiple handlers, should you choose to do so.

function handleInvalidSession(err) {
  if (err.code !== Parse.Error.INVALID_SESSION_TOKEN) {
    return Parse.Promise.reject(err);
  }
  // ... deal with the invalid session
}

new Parse.Query('Item')
  .find()
  .then(processItems)
  .catch(handleInvalidSession)
  .catch(handleOtherErrors);

In general, think of the SDK as moving towards a more explicit model with less "magic" or automatic behavior. These outcomes typically confuse developers, rather than making life easier.

@noahsilas
Copy link
Contributor Author

Thanks for the detailed response, @andrewimm.

My instinct is to say that "simple things should be simple, and complex things should be possible". It seems like the common use case is that you are making a request on behalf of an authenticated user, and that the SDK should understand when that state changes and provide a clean way for a developer to hook in to that event. While the explicit behavior you describe nicely follows the principle of least surprise, it also leaves a lot of that responsibility on the developer.

I would further argue that when a developer is opting in to more complex behaviors, such as making a request with an explicit session token, it's also acceptable for them to take on the burden of managing this kind of stateful session behavior instead of letting the SDK handle it for them.

Having said that, I understand the argument that "explicit is better than implicit", and a desire as a maintainer to avoid features that have been seen to confuse the developers using the SDK. Seeing as this is documented behavior with a clear rationale, I'm closing this issue.

Thanks!

@andrewimm
Copy link
Contributor

@noahsilas I do appreciate you taking the time to voice your concerns, and suggest improvements. I see this SDK as an evolving codebase, and it's crucial for the many users to continue contributing ideas to make it better each day.

@alansoliditydev
Copy link

alansoliditydev commented Nov 6, 2018

Hey, I have built a query builder.
You guys can have a look at this http://jsfiddle.net/toidibandao/b5qzfdt2/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants