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

App doesn't recognize when it has been logged out #219

Closed mstenta closed 4 years ago

mstenta commented 5 years ago

This is an issue with https://farmOS.app - I'm not sure if it's also an issue with the native apps.

Steps to reproduce:

  1. Log into a farmOS instance directly in your browser (eg: https://test.farmOS.net)
  2. Log into farmOS.app using the same credentials in a new tab.
  3. Log out of farmOS in the first tab.
  4. Attempt to sync in farmOS.app - nothing happens - console errors.
jgaehring commented 5 years ago

Hmm, interesting. Yea, I don't know how we're going to reconcile the fact that we're using the same cookies for auth that farmOS uses. Might be tricky, or might involve changing some of how auth is handled by the server.

mstenta commented 5 years ago

Yea that's the cause - and that probably won't happen often.

But I think regardless of that, we may also have the same issue when a session expires? Or are we handling that case already? It seems like it would be the same thing.

jgaehring commented 5 years ago

Attempt to sync in farmOS.app - nothing happens - console errors.

Just console errors? Our error handler isn't picking it up then? At the very least I would hope it would reroute you to the login page.

mstenta commented 5 years ago

Just console errors? Our error handler isn't picking it up then? At the very least I would hope it would reroute you to the login page.

Tested this, and the error handler is picking it up and displaying "Unable to sync because the network is currently unavailable. Please try syncing again later."

Screenshot from 2019-08-01 11-19-02

However it is not redirecting to the login screen. Presumably because the app thinks it is already logged in?

There is not way to get to the login screen when the app thinks it is already logged in. All you can do is logout and then login again. And logging out destroys unsynced logs, right? :-/

I think we definitely need to add logic that handles this gracefully. The app needs to detect when either a cookie is no longer available, or the session has expired (same mechanism?) and ideally it would automatically try to re-authenticate using stored credentials.

mstenta commented 5 years ago

Another thing to note (with regard to #222): if you are not in the "My Logs" screen, you do not see the error message "Unable to sync because the network is currently unavailable. Please try syncing again later."

Is that only displayed in "My Logs"?

To reproduce: destroy your cookie while still "logged in" to farmOS.app, create a new log, and try to click the "Sync" button when selecting assets/areas... no error.

mstenta commented 5 years ago

FYI: #222 is not a Field Kit issue, it seems, but rather a PostgreSQL compatibility issue. I've moved it to the farmOS queue.

However, everything I said above still stands for this issue.

jgaehring commented 5 years ago

FYI: #222 is not a Field Kit issue

Ah, good to know.

Our auth procedures definitely need overhaul. I guess I was always hoping we'd migrate to OAuth and that would alleviate some of the issues, and in the course of migrating we could address the other issues.

I'm not sure the best way to wade into all the potential issues. Some of this would be related to the error-handling label I've created for tracking those issues. I'd maybe like to create some kind of roadmap for looking further into authentication from the top level down, rather than running around to squash a bunch of bugs individually without really addressing the overall design, which I think may be flawed.

mstenta commented 5 years ago

Our auth procedures definitely need overhaul. I guess I was always hoping we'd migrate to OAuth and that would alleviate some of the issues, and in the course of migrating we could address the other issues.

Agreed - and I think there's some common stuff that would be necessary whether or not we have OAuth (speaking of - might be looking into that soon...)

jgaehring commented 5 years ago

This should be resolved by some of the new bug fixes I just pushed, specifically 8de0278. Now it should reroute you to the login screen.