search

200ok-ch / organice

An implementation of Org mode without the dependency of Emacs - built for mobile and desktop browsers
https://organice.200ok.ch/
GNU Affero General Public License v3.0
2.43k stars 151 forks source link

Unable to log into gitlab backend when stale localstorage hangs around #845

Open tarnung opened 2 years ago

tarnung commented 2 years ago

TLDR: If you are in a state where you're not logged into a backend but the localstorage of your last session did not get cleared out, you can not log in. (On gitlab, i did not reproduce it on other backends.)

A while ago I got logged out of organice on a machine i rarely use it on. So when my attempt to log back in failed, I took the easy way out and used other devices. Now I tried it again. I even revoked the application privileges on gitlab to get a clean slate. The authorize form showed up as expected but while being redirected to the organice page a GET call was made to https://gitlab.com/oauth/undefined. The organice site loaded with a popup "Error loading files". On pressing "ok" i was redirected to the gitlab authorize form a second time. Again after clicking "Authorize" the bad oauth request happened before i finally got redirected to the organice landing page. After a bit of searching i found that my localstorage still knew about the application state from when i last was logged in. After whiping it with localStorage.clear() i managed to regularly log in.

localStorage gets whiped on logout so the problem state is when the landing page loads while there is still some stale state in localStorage. You can not log out while on the loading page.

Possible Solution: Whipe localStorage (or parts of it like backend credentials) as the first thing that happens when you load the landing page (so after the decison is made that you will not be forwarded to the app but get to see the landing page)?

BTW: I love to see the recent burst of activity around here!

munen commented 2 years ago

I confirm the bug; I have seen it before, as well.

I didn't check it out in detail yet, because I don't (can't) use GitLab as my primary sync back-end. The reason is that I have shared Org files with different parties residing in separate repositories.

Your possible solution sounds correct to me. One small addendum: Wiping the localStorage will certainly fix the problem. But on the edge case that the user had some state that wasn't synchronized, yet, this will be lost. When making the conscious choice to sign out, it's a different story. So, maybe it is enough to wipe storage.syncBackend, localStorage.gitLabProject, localStorage.authenticatedSyncService, and localStorage.oauth2authcodepkce-state. Potentially some more.

However, it's an edge case that would require this kind of cherry picking. I'm fine with either solution.

Would you like to work on it? It might be easier for you to confirm the fix since you're using GitLab as a back-end. If not, it certainly is a high prio bug anyway and I'll label it as such. Not all users will have the technical ability and organice knowledge to clear the localStorage and start from a clean slate.

Thank you for the detailed report and it's nice to hear from you, again :wave:

BTW: I love to see the recent burst of activity around here!

Me, too! It's a lot of fun, Phil got really involved now, as well. There's some great stuff coming to master that's currently being tested on develop, too(;

munen commented 2 years ago

After a tiny bit more thought.. I can see some upside to completely clearing localStorage when the user gets redirected to the landing page and not cherry-pick for GitLab. The reasons are:

  1. It's easier to understand code.
  2. This cleanup code will not have to be kept in sync with GitLab.
  3. It will work for any sync back-end that has a similar kind of issue.

I'm tending to think that these arguments outweigh the edge case outlined above.

munen commented 2 years ago

At the very least, I have created a PR that improves the error logging for the GitLab backend. I noticed that there are some caught exceptions that are not logged.

https://github.com/200ok-ch/organice/pull/846

munen commented 2 years ago

Hah, what do you know. Getting to work this morning, I actually found this screen:

image

Tbh, I cannot even remember which backend I was connected to last yesterday evening. Maybe it was GitLab, but it could be WebDAV or Dropbox. I was doing tests on all of them.

Looks like the landing page should be rendered, but it still wants to fetch files from a sync backend. However, the redux store has no information regarding a sync backend.

Redux store looks like this:

image

tarnung commented 2 years ago

I'll try to look into it!

munen commented 2 years ago

Nice, good luck and have fun^^

munen commented 2 years ago

Since introducing the new LP, we have a bit of conflation in the routing. I think this could also introduce side effects for the storage back-ends.

Here's a draft PR to refactor and clean this up nicely. Unfortunately, I cannot finish the PR this week, because the monthly retreat is about to start. I'll leave the PR here in case it helps you, maybe you event want to pick it up. If it does not help, just ignore it(;

munen commented 2 years ago

Potentially, https://github.com/200ok-ch/organice/pull/847 fixes https://github.com/200ok-ch/organice/issues/845#issuecomment-1156066653. I'm not 100% on that, though. Unfortunately, I cleared the localStorage 'by accident' after I think I saw that it worked once. So now I don't have any browser left in this broken state.

tarnung commented 2 years ago

If I understand correctly you opted against clearing localStorage when deciding to show the landing page. Instead you chose to rely on isAuthenticated to decide if you should show the landing page. As far as i can see isAuthenticated is only set to true if a connection to a sync backend is established successfully. If i delete my dropbox access token and refresh, i'm redirected to the landing page instead of seeing an error loading files.

For dropbox loging in again works fine on my local running app. I can not test it locally for gitlab. I'm not sure if there still is a possible state where you get redirected to the landing page but some stale state prohibits you from loging into the backend again.

I'll happily test it once #847 is live.

munen commented 2 years ago

If I understand correctly you opted against clearing localStorage when deciding to show the landing page.

If you mean in #847, then yes, because #847 is not specifically a fix for this (#845) issue. It's a refactor and a cleanup that also potentially solved https://github.com/200ok-ch/organice/issues/845#issuecomment-1156066653.

To tackle #845, I'm in favour of clearing the localStorage with the arguments outlined here.

tarnung commented 2 years ago

Ah, i missed that you referenced the comment, not the issue as a whole.

munen commented 2 years ago

No worries. I kind of conflated your issue by adding a 'possibly related and similar, but not the same issue' issue(;