search

farmOS / field-kit

A modular, offline-first companion app to farmOS.
https://farmOS.org
GNU General Public License v3.0
60 stars 39 forks source link

Sync fails silently if valid access and refresh tokens are missing #356

Closed jgaehring closed 4 years ago

jgaehring commented 4 years ago

I'm not sure if this behavior would be different if an actual invalid token was present, but below is how I simulated such a state on the current develop build using Chrome browser.

Steps to Reproduce

  1. Open the dev tools and navigate to the Application tab; open the site's Local Storage.
  2. Right-click on the token's value column and select "Edit Value".
  3. Replace both the access_token and refresh_token with an empty string.
  4. Sync logs.

Expected Result

The app should reroute the user to the Login page. (and maybe display the cause of the login error?)

Observed Result

Nothing. No error is displayed, and the user is not rerouted to login. If you open the console you see several failed network requests, which have 403'ed, followed by one request to /oauth2/token which has responded 400 (Bad Request).

I'm a little surprised that the 400 didn't trigger a reroute, b/c I'd actually encountered something similar before, which I believed I had fixed with 92f60be. So I guess we need to work on the error handling some more here.

paul121 commented 4 years ago

I think we need to expand this code to catch for "empty" tokens: https://github.com/farmOS/farmOS.js/blob/14bab48c10e6c3b8b96067268b3c0747d9258a16/index.js#L108

As far as the error handling, I haven't been able to take a close look, but I agree it seems like 400 errors should be caught?

jgaehring commented 4 years ago

Ok, so I think we found some things that can be improved, but practically speaking it looks like we don't really need to be on guard against tokens being deleted in this way, since it shouldn't happen in practice. We can, however, take some precautionary steps to make sure at least this error is propagated to the user by making sure we throw all errors so they can be caught and handled by the application code. I'm going to commit some changes to this effect and publish farmos@0.1.2 to npm, and pull those into Field Kit.

jgaehring commented 4 years ago

Resolved by 0845aec.