-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Desktop: Change Joplin Cloud login process to allow MFA via browser #9445
Conversation
I'm struggling to avoid a problem in the new SyncTargetJoplinCloud login. I'm writing this down so you maybe can help me think about it, or maybe I just to help myself think what are the solutions here. I think this is more of a rambling than a thoughtful explanation of what I'm struggling with, I'm sorry! The scenario that I'm dealing with is what happens when the user is logged-in with an application credentials and his credentials get revoked (excluded via the website interface, for example). If this happens while the user is synching its file, we will get a request a What I'm trying to rethink right now is the process, thinking in two steps of verifying if the credentials are correct (would be something similar to checkConfig) and the process of renewing the token when it expires. What I'm struggling is to understand what should happen when/where. What I thought as a strategy:
I don't think this used to be a problem because until now user only had one credential, but now with application credentials I think this might turn out to be an issue. |
A similar error happens when
Again, it can be fixed by going to the synchronisation screen and login-in into Joplin Cloud, but might not be obvious (there is no error message) besides console logs |
I pushed another "feature": When Joplin Cloud is selected as the synchronisation target and the users clicks "Check synchronisation configuration", we check if the user is Authenticated, if it is not we redirect to JoplinCloudLogin screen, if he is we show the happy message: joplin/packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx Lines 63 to 74 in f6f8787
|
I update the branch with the fixes and merged with upstream dev. |
I changed the implementation to be able to cancel the setInterval and show the error message when the "Failed to fetch" is the error message that is returned when the server is offline, this part will change depending in the error that Joplin Cloud throws. |
This PR is ready for review again. |
|
||
isWaitingResponse = false; | ||
const jsonBody = await response.json(); | ||
logger.warn('Server could not retrieve application credential', jsonBody); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it mean that when trying to fetch unique_login_code
but it's not ready yet the server throws an error? If that's what it is I think it should be changed - instead it should tell the status like "in progress", "done", etc. with 200 status code.
Errors should be only when there's an actual error, for example if the code doesn't exist or the server is down. And we need to report these errors to the user so that function should throw an exception with the statusText or whatever info the server provides.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made the change to follow this principle. I wasn't sure if it was ok to keep the message log, I guess it doesn't make sense from what you saying.
Last changes:
I also add images about how the feature looks right now at the PR description |
Last changes:
|
This is a working-in-progress PR for the new login flow that we are going to use for Joplin Cloud.
This change is required to make the 2FA work in the Desktop.
With this PR, when the user tries to synchronise to Joplin Cloud we will show a new page that will instruct the user on how to authorise the application over Joplin Cloud. This process will involve the creation of a unique value that Joplin Cloud will use to identify the application that allowed the use of the recently created credentials.
Images
There are some missing things for now:
unique_login_code
api/applications
with an interval, but a lot of code here is platform-specific./api/sessions
Testing
I still need to add automated tests, maybe I will need to mock so much stuff that won't really be very useful?
Some manual tests that are good to be done:
api/applications
after one of the buttons is clickedTesting upgrading version without 2FA
Testing upgrading version and enabling 2FA