search

tannercollin / standardnotes-fs

Mount your Standard Notes as a filesystem.
GNU General Public License v3.0
148 stars 12 forks source link

MFA users prompted for code every time #27

Closed ghost closed 4 years ago

ghost commented 4 years ago

Users who have MFA enabled are prompted for an MFA code every time they mount a directory, preventing the process from being automated (e.g. mounting a dir on login). This is inconsistent with the behavior of the SN desktop app, which doesn't reprompt every time it's reopened.

I've written a patch that addresses the issue by caching the JWT auth token alongside the other creds at felixauscent/standardnotes-fs@d9d10f003532239fadd8666f1809c5be835c67e3 - care to take a look at it? The main problem that I'm not sure yet how to address is that it won't fail gracefully if the JWT becomes invalid (not sure under what circumstances that would happen under, but it seems possible). Maybe there's some way to make a simple API request and check if it fails, and clear the cached JWT if so?

Thanks!

tannercollin commented 4 years ago

Thanks for the patch!

I suspect the jwt would be expired if the user does something to their credentials or 2fa. It would be wise to check if this is the case. What happens if you call /auth/sign_in with the correct auth header present?

ghost commented 4 years ago

That's a good point - cred changes should invalidate current sessions.

No luck, unfortunately - hitting it with the jwt header still just returns the mfa-required error. I'll throw a question in the SN Slack, see if they have any suggestions for checking a jwt's validity

ghost commented 4 years ago

Alright, I've got something in felixauscent/standardnotes-fs@43133416b5d0d19da2f54b967c0b1d146acbaeac. It hits the sync API endpoint to see if the jwt is still valid, and clears it/re-authenticates if not.

I modified the post() function with some error checking, because, unlike the MFA error (returned as JSON), hitting /items/sync with an invalid JWT returns a response with an 500 response code with an empty body, causing the JSON decode to fail.

I'm very open to any code style feedback you have as well - this is my first open-source contribution, so I'm sure it could stand some improvement.

tannercollin commented 4 years ago

I've tested your patch and pulled in the changes. Thanks so much for your help!

I just changed one thing in 9f07935c1198c0876ee77d6992b52183b1467de5 to make it simpler. It doesn't seem to matter if the jwt doesn't get removed. Let me know if you've found different.

ghost commented 4 years ago

Looks good - yeah, that change seems fine. Thanks!