search

debtcollective / parent

1 stars 0 forks source link

SSO failure does not fail gracefully #143

Closed berleant closed 6 years ago

berleant commented 6 years ago

Reproduce:

  1. Follow this link: http://localhost:8080/dispute-tools/wage-garnishment-dispute?sso=notavalidvalue

The browser just hangs indefinitely. On the server side, the only indication that something happened is UnhandledPromiseRejectionWarning: Error and a stack trace.

This is happening in my dev environment. Not sure about prod. Refreshing the webpage results in this behavior.

Acceptance criteria

sarayourfriend commented 6 years ago

After https://github.com/debtcollective/dispute-tools/pull/40 is merged this will stop happening when you refresh a page after the sso handshake is completed. That PR wipes the SSO query parameters by redirecting you after the SSO process is completed and the JWT is created so the browser never lands on the page with the query parameters.

Before, if you navigated to /disputes/my in the tools and you were not logged into the tools you would end up at /disputes/my?sso=blahblahblah. If you refreshed that, the data inside of the sso parameter would be stale and the server wasn't handling that correctly (as this issue points out). After the PR you'll just end up at the route you requested without any additional sso parameters.

The following is the expected SSO handshake

  1. User hits tools page that requires authentication
  2. nonce is reacted and saved in memory in the tools service
  3. User is redirected to discourse sso endpoint w/ the created nonce
  4. User is redirected back to the tools w/ the sso payload containing the same nonce
  5. Tools service verifies the nonce matches then logs the user into the tools
  6. User finally lands on the page they requested, free of sso query parameters.

In this process, if the dispute-tools service is restarted between steps 3 and 4, the tools will have lost the nonce's referenced in the SSO payload and will not be able to validate the SSO payload. This would be very rare in production, but is, of course, quite possible.

With that in mind, I propose that the final solution to the hanging request described in the issue is to just redirect the user to the page again so that the SSO handshake is attempted again. However, this could theoretically cause a loop of infinite redirects, but that's even less likely than the server restarting exactly between steps 3 and 4 for a given user.

An alternative solution that avoids the possible (but hard to imagine as every happening) infinite redirects is to simply redirect the user to a 404 page.

berleant commented 6 years ago

I like your proposal, provided we either a) prevent infinite redirects or b) log such that we will be notified if that ever happens.

We also should come up with a strategy for error handling in general that will result in good logs and a better user experience. @marcondag since you are most familiar with the dispute tools, could you write something up about that?

sarayourfriend commented 6 years ago

Agreed that it should be logged in Sentry (which would send email & slack alerts) regardless of the solution that gets implemented.

I'll write up a wiki page or something about error handling. It's fairly straight forward w/ Raven and how things are currently set up in the dispute tools.

berleant commented 6 years ago

I would put it in the readme--I don't think people (read: me) reliably look at the wiki for info

berleant commented 6 years ago

Closing in favor of https://github.com/debtcollective/parent/issues/184