Desktop, Mobile: Fixes #10645: Show notification in case Joplin Cloud credential is not valid anymore #10649

pedr · 2024-06-21T22:48:09Z

Description

The goal of this PR is to improve the user experience when using Joplin Cloud as synchronisation target.

Currently, when the Joplin Cloud credentials expire (because the user enabled MFA or deleted the application record from the Joplin Cloud website) and tries to synchronise, there is only a small error message above the synchronise button.

To make it clear what is happening when the credentials aren't valid anymore, we are redirecting the user to Joplin Cloud Login to guide to user to finish the login process to be able to continue connected.

This behaviour is similar to what we have for fresh installs: if Joplin Cloud is selected as the synchronisation target but the login process isn't finished, we redirect the user to Joplin Cloud Login when he presses 'Synchronisation' on the sidebar.

Shortcommings

While I think this implementation is an improvement by making the process easier for the user, there is the issue that we won't be able to show the error messages inside the interface. In the previous implementation checkConfig was called from inside the React component which allowed the error message to be displayed where it was called.

~~This might be confusing if there is an error happening and it is only possible to see inside the logs.~~

This is not really a problem anymore since we have a notification banner informing the user the state of the connection.

Clips

First login (application without any state):

first_login.mp4

User was logged in, but credentials turned invalid (mfa was enabled or application was deleted on website):

credential_invalid.mp4

When Joplin Cloud is offline:

joplin_cloud_offline.mp4

Testing

Several cases need to be tested:

Fresh install

Press 'Synchronise' button
Select Joplin Cloud as sync target
Should be redirected to Joplin Cloud Login
Connect to Joplin Cloud
Go back
Pressing 'Synchronise' button should start synchronise process again

Invalid credentials

Synchronise to Joplin Cloud
Delete application record that allow the synchronisation in Joplin Cloud website
Press 'Synchronise' button
Should be redirect to Joplin Cloud Login screen since credentials are invalid now

Valid credentials on Config screen

Synchronise to Joplin Cloud
On the Synchronisation screen in the Configurations press 'Check synchronisation configuration'
Should show a successful message below the button

Invalid credentials on Config screen

Synchronise to Joplin Cloud
Delete application record that allow the synchronisation in Joplin Cloud website
On the Synchronisation screen in the Configurations press 'Check synchronisation configuration'
Should redirect user to Joplin Cloud Login screen

laurent22 · 2024-06-22T08:20:03Z

Failing test on CI:

➤ YN0000: [@joplin/lib]: FAIL components/shared/config/shouldShowMissingPasswordWarning.test.js
➤ YN0000: [@joplin/lib]:   ● shouldShowMissingPasswordWarning › should return true when sync target requires a password and the password is missing
➤ YN0000: [@joplin/lib]: 
➤ YN0000: [@joplin/lib]:     expect(received).toBe(expected) // Object.is equality
➤ YN0000: [@joplin/lib]: 
➤ YN0000: [@joplin/lib]:     Expected: true
➤ YN0000: [@joplin/lib]:     Received: false
➤ YN0000: [@joplin/lib]: 
➤ YN0000: [@joplin/lib]:       20 | 			const expected = targetToRequiresPassword[targetName];
➤ YN0000: [@joplin/lib]:       21 |
➤ YN0000: [@joplin/lib]:     > 22 | 			expect(shouldShowMissingPasswordWarning(targetId, {})).toBe(expected);
➤ YN0000: [@joplin/lib]:          | 			                                                       ^
➤ YN0000: [@joplin/lib]:       23 |
➤ YN0000: [@joplin/lib]:       24 | 			// Should also consider an empty string to be missing
➤ YN0000: [@joplin/lib]:       25 | 			const settings = {
➤ YN0000: [@joplin/lib]: 
➤ YN0000: [@joplin/lib]:       at Object.toBe (components/shared/config/shouldShowMissingPasswordWarning.test.ts:22:59)

…-connection-ux

laurent22 · 2024-06-25T13:02:55Z

packages/app-desktop/gui/JoplinCloudLoginScreen.tsx

 		};
-	}, [intervalIdentifier]);
+	}, [intervalIdentifier, state, props.dispatch]);


We should only pass what's needed for the effect, so only state.active here

Also I don't really understand why this effect does and why it's modifying global state?

My initial idea was that the message in the sidebar might confuse the user if he just enters Joplin Cloud Login screen and leaves without taking any action, it might say that the synchronisation was complete, for example, while the credentials are invalid at the point he is reading it.

I think the notification alert addresses this by making it clear what is happening, I don't think this event is necessary anymore, I will remove it.

…-connection-ux

pedr · 2024-06-27T14:06:07Z

Remove messages about Joplin Cloud being offline

pedr · 2024-06-27T22:21:47Z

@laurent22 I removed the message about Joplin Cloud being offline.

I also changed the logic about the error catch inside isAuthenticated, I'm assuming any error.code that isn't > 500 should be a validation error (wrong credentials, or application credential doesn't exist yet) and anything else we assume it is an unknown error (like you said, it could be our server, but could also be user connection, proxy, DNS blocker, etc)

laurent22 · 2024-06-28T07:26:48Z

packages/lib/SyncTargetJoplinCloud.ts

+			if (error.code < 500) {
+				Setting.setValue(`sync.${SyncTargetJoplinCloud.id()}.isAuthenticated`, false);
+				return false;
+			}


I'm not sure we should have this logic here because when is it actually called? Also it's called "isAuthenticated" but it's clearly doing a lot more than this now, including saving settings.

Is it possible to add the code to lib/Synchronizer.ts instead, with the same logic as "MustUpgradeApp" error? If we catch an error, and if it's an auth error, and it's Joplin Cloud, then dispatch an action telling the user they must authenticate. That way we deal with the problem exactly where it happens.

I'm going to still test your suggestion, and I see how what I'm doing is really not necessary at this point (I'm removing the Setting.setValue and checkConfig), but I think we still need to have some error catch logic here.

It is necessary because when we call api.sessionId() and the user has invalid credentials it will throw an error (probably 403 or 404, depending on why the credentials are invalid).

The credentials can be invalid if the user:

Has enabled MFA and hasn't made a new login in the client yet.

Has deleted the application record from the Joplin Cloud page.

Edit: just to be clear, this happens when the user presses 'Synchronise', not when a schedule sync happens. I think your suggestion makes sense, I will try to implement it ASAP.

The credentials can be invalid if the user:

For now let's narrow it down to a specific case: we want to display a message when the session ID is cleared (when MFA is enabled). Whatever is the error code then we should display a message. Anything else we don't handle it at all.

pedr · 2024-06-28T17:01:56Z

I had to call 'MUST_AUTHENTICATE' in three places:

Inside the lib/register, after IsAutheticated is called because it is where the schedule sync checks if should start or not. If we don't have a sessionId (because it is a client that hasn't connected yet) or if we try to get a session but get an error of 'Invalid authentication code' (aka MFA was enabled) we set MUST_AUTHENTICATE to true, else is set to false

And inside the Synchronizer, because we might have a sesionId (so MUST_AUTHENTICATE was set to true) but when the Synchronize process start when we try to fetchSyncInfo() function we get an error because the session is not valid anymore (because MFA was enabled).

pedr · 2024-06-28T17:04:03Z

I hope this solution is aligned with what you expected, else feel free to ask for changes.

Sorry for taking so long on this, but this kind of interaction between the client and server, is authenticated or not, is mfa enabled or not, etc requires a lot of manual testing and I like to be on the safe side and test it two times if I'm not sure.

laurent22 · 2024-06-28T17:20:44Z

packages/app-mobile/components/ScreenHeader/WarningBox.tsx

@@ -6,7 +6,7 @@ import NavService from '@joplin/lib/services/NavService';

 interface Props {
 	themeId: number;
-	targetScreen: string;
+	targetScreen?: string;


Is that needed? It looks like targetScreen is not used in your code?

laurent22 · 2024-06-28T17:22:21Z

packages/lib/registry.ts

+		this.dispatch_ = dispatch;
+	}
+
+	private sendDispatch(event: AnyAction) {


Suggested change

private sendDispatch(event: AnyAction) {

private dispatch(event: AnyAction) {

laurent22 · 2024-06-28T17:25:24Z

packages/lib/Synchronizer.ts

@@ -524,6 +524,9 @@ export default class Synchronizer {
 					// await uploadSyncInfo(this.api(), remoteInfo);
 				}
 			} catch (error) {
+				if (error.message === 'Invalid authentication code') {


Isn't Joplin Cloud setting an http status code, and can we use this instead? We shouldn't check error message as they can change in the future.

laurent22 · 2024-06-28T17:26:09Z

packages/lib/registry.ts

+		this.dispatch_ = dispatch;
+	}
+
+	private sendDispatch(event: AnyAction) {


Suggested change

private sendDispatch(event: AnyAction) {

private sendDispatch(action: AnyAction) {

laurent22 · 2024-06-28T17:29:13Z

packages/lib/registry.ts

+	}
+
+	private sendDispatch(event: AnyAction) {
+		if (!this.dispatch_) throw new Error('Dispatch not set!');


Please make it optional, defaults to (action) => {}. I'm not keen that we have to add this dispatch thing to the registry now, but if there's no other way let's do it, but it should be optional.

laurent22 · 2024-06-28T17:32:43Z

packages/app-desktop/gui/MainScreen/MainScreen.tsx

@@ -684,6 +692,12 @@ class MainScreenComponent extends React.Component<Props, State> {
 			);
 		} else if (this.props.mustUpgradeAppMessage) {
 			msg = this.renderNotificationMessage(this.props.mustUpgradeAppMessage);
+		} else if (this.props.showInvalidJoplinCloudCredential) {
+			msg = this.renderNotificationMessage(
+				_('Your Joplin Cloud credentials are invalid., please re-authenticate.'),


Suggested change

_('Your Joplin Cloud credentials are invalid., please re-authenticate.'),

_('Your Joplin Cloud credentials are invalid, please login.'),

And please also update the other string

laurent22 · 2024-06-28T17:33:54Z

packages/app-desktop/gui/MainScreen/MainScreen.tsx

+		} else if (this.props.showInvalidJoplinCloudCredential) {
+			msg = this.renderNotificationMessage(
+				_('Your Joplin Cloud credentials are invalid., please re-authenticate.'),
+				_('Go to Joplin Cloud Login'),


Suggested change

_('Go to Joplin Cloud Login'),

_('Login to Joplin Cloud'),

laurent22 · 2024-06-29T09:03:21Z

packages/lib/registry.ts

+				// if (Setting.value('env') === 'dev') {
+				// 	this.logger().info('Recurrent sync operation DISABLED!!!');
+				// 	return;
+				// }


that should not be in the pr

laurent22 · 2024-06-29T09:04:40Z

packages/lib/Synchronizer.ts

+				if (error.code === 403) {
+					this.dispatch({ type: 'MUST_AUTHENTICATE', value: true });
+				}


Ok but other sync targets can set the 403 status code so it means we're going to show a message about Joplin Cloud when someone can't login to an unrelated service. I'm going to try to fix this issue

I'm checking the sync target in the place where it the notification is shown to avoid this problem

https://github.com/laurent22/joplin/pull/10649/files#diff-a400aeaf7c8882c4911f24f17d6e53c17f706ec22055ea21906882bb66590d7bR983

…rns a 404" This reverts commit f2e0429.

… credential is not valid anymore (#10649)

pedr added 2 commits June 21, 2024 18:46

desktop implementation

693dc5d

returning catch error to previous implementation

d0fe231

pedr requested a review from laurent22 June 21, 2024 22:48

pedr mentioned this pull request Jun 21, 2024

When MFA login fails, display an orange banner at the top of the screen #10645

Closed

pedr added 2 commits June 24, 2024 10:59

Merge remote-tracking branch 'upstream/dev' into improve-joplin-cloud…

23bfc4b

…-connection-ux

Merge remote-tracking branch 'upstream/dev' into improve-joplin-cloud…

ee1f4c9

…-connection-ux

laurent22 reviewed Jun 25, 2024

View reviewed changes

pedr added 7 commits June 25, 2024 13:07

adding warning banner to mobile

a27e4eb

adding notification to desktop

8ebb473

removing unnecessary state change

a160431

adding offline message to mobile

348e260

improving code readability

c142e8c

joplin cloud login method has changed

5aa9275

Merge remote-tracking branch 'upstream/dev' into improve-joplin-cloud…

465baae

…-connection-ux

pedr marked this pull request as ready for review June 25, 2024 18:18

pedr changed the title ~~Desktop, Mobile: Open Joplin Cloud login screen when credentials fail~~ Desktop, Mobile: Fixes #10645: Open Joplin Cloud login screen when credentials fail Jun 26, 2024

pedr added 2 commits June 27, 2024 16:20

removing isServerOffline functionality

b40400f

setting isAuthenticated false if it isn't a server error

1ea3c91

laurent22 reviewed Jun 28, 2024

View reviewed changes

simplify isAuthenticated, add dispatch to registry

80afa35

pedr marked this pull request as draft June 28, 2024 14:11

pedr added 3 commits June 28, 2024 13:34

add reducer state to show authentication message

2f32237

fix type

a4f5efd

fixing mobile

5c9709b

pedr marked this pull request as ready for review June 28, 2024 16:52

pedr added 2 commits June 28, 2024 13:53

removing test content

ddddaa9

removing unused setting

3c90529

pedr changed the title ~~Desktop, Mobile: Fixes #10645: Open Joplin Cloud login screen when credentials fail~~ Desktop, Mobile: Fixes #10645: Show notification in case Joplin Cloud credential is not valid anymore Jun 28, 2024

fixing test

a8118ae

laurent22 reviewed Jun 28, 2024

View reviewed changes

pedr added 4 commits June 28, 2024 14:36

reverting change to WarningBox since it is not necessary anymore

d2bf9bc

refactoring sendDIspatch

2f3453a

improving text

6b5970b

changing comparing message to code/http status

edc8259

laurent22 reviewed Jun 29, 2024

View reviewed changes

pedr added 3 commits June 29, 2024 17:51

removing comment from test code

dba4c93

when mfa is enabled application record is deleted, so it returns a 404

f2e0429

Revert "when mfa is enabled application record is deleted, so it retu…

ee139d1

…rns a 404" This reverts commit f2e0429.

laurent22 merged commit a074532 into laurent22:dev Jul 1, 2024
8 checks passed

laurent22 pushed a commit that referenced this pull request Jul 1, 2024

Desktop, Mobile: Fixes #10645: Show notification in case Joplin Cloud…

78d9a7e

… credential is not valid anymore (#10649)

pedr deleted the improve-joplin-cloud-connection-ux branch July 9, 2024 18:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Desktop, Mobile: Fixes #10645: Show notification in case Joplin Cloud credential is not valid anymore #10649

Desktop, Mobile: Fixes #10645: Show notification in case Joplin Cloud credential is not valid anymore #10649

pedr commented Jun 21, 2024 •

edited

Loading

laurent22 commented Jun 22, 2024

laurent22 Jun 25, 2024

laurent22 Jun 25, 2024

pedr Jun 25, 2024

pedr commented Jun 27, 2024 •

edited

Loading

pedr commented Jun 27, 2024

laurent22 Jun 28, 2024

pedr Jun 28, 2024 •

edited

Loading

laurent22 Jun 28, 2024

pedr commented Jun 28, 2024

pedr commented Jun 28, 2024 •

edited

Loading

laurent22 Jun 28, 2024

laurent22 Jun 28, 2024

laurent22 Jun 28, 2024

laurent22 Jun 28, 2024

laurent22 Jun 28, 2024

laurent22 Jun 28, 2024

laurent22 Jun 28, 2024

laurent22 Jun 29, 2024

laurent22 Jun 29, 2024

pedr Jun 29, 2024 •

edited

Loading

	private sendDispatch(event: AnyAction) {
	private dispatch(event: AnyAction) {

	private sendDispatch(event: AnyAction) {
	private sendDispatch(action: AnyAction) {

	_('Your Joplin Cloud credentials are invalid., please re-authenticate.'),
	_('Your Joplin Cloud credentials are invalid, please login.'),

Desktop, Mobile: Fixes #10645: Show notification in case Joplin Cloud credential is not valid anymore #10649

Desktop, Mobile: Fixes #10645: Show notification in case Joplin Cloud credential is not valid anymore #10649

Conversation

pedr commented Jun 21, 2024 • edited Loading

Description

Shortcommings

Clips

Testing

Fresh install

Invalid credentials

Valid credentials on Config screen

Invalid credentials on Config screen

laurent22 commented Jun 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedr commented Jun 27, 2024 • edited Loading

pedr commented Jun 27, 2024

Choose a reason for hiding this comment

pedr Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedr commented Jun 28, 2024

pedr commented Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedr Jun 29, 2024 • edited Loading

Choose a reason for hiding this comment

pedr commented Jun 21, 2024 •

edited

Loading

pedr commented Jun 27, 2024 •

edited

Loading

pedr Jun 28, 2024 •

edited

Loading

pedr commented Jun 28, 2024 •

edited

Loading

pedr Jun 29, 2024 •

edited

Loading