feat: handle revoked tokens #862

acezard · 2023-07-05T07:38:02Z

This PR introduces a new feature to handle user token revocation in a more seamless and user-friendly manner. When a user's token is no longer valid (e.g., due to expiration or a revocation by the server), the user is redirected to the welcome page, and a modal dialog is displayed to inform them about the issue.

Changes

AuthService Update: The authentication service was updated to include a new state, userRevoked, which is set to true when a token error is detected, indicating that the user has been revoked due to an invalid token.
Event-Based Status Updates: The AuthService now uses an event-based approach to communicate status updates to other parts of the app. This ensures a clear separation of concerns and improves code maintainability.
User Revocation Modal: A new modal dialog has been created to inform the user when they have been revoked due to an invalid token. This modal is displayed on the welcome screen and can be dismissed by the user.
Custom Hook: A new custom hook, useRevokedStatus, has been introduced to encapsulate the logic of handling the user revoked status. This makes the logic reusable and helps keep the component code clean and focused on rendering.
WelcomeScreen Update: The WelcomeScreen component has been updated to use the useRevokedStatus hook and show the user revocation modal when necessary.

Testing

to be added

Screenshots

Ldoppea

Sorry I made a lot of comments but I'm a bit worried by the over-complexity of this PR.

The only business logic seems to be handleOnUnrevoked that effectively clean the local state. The rest seems to be unnecessary layers between client.on('revoked') and the dialog's validation button. So there seems overkill to have 2 states (one in useRevokedStatus and one in AuthService) where only one seems enough and 2 new events with a new listener where the client.on('revoked') seems enough.

But maybe I'm missing something so feel free to explain why you did this.

src/app/domain/authentication/services/AuthService.ts

src/libs/client.js

src/app/domain/authentication/services/AuthService.ts

src/app/view/Auth/ErrorTokenModal.spec.tsx

src/app/domain/authentication/services/AuthService.ts

acezard · 2023-07-11T06:06:17Z

Sorry I made a lot of comments but I'm a bit worried by the over-complexity of this PR.

The only business logic seems to be handleOnUnrevoked that effectively clean the local state. The rest seems to be unnecessary layers between client.on('revoked') and the dialog's validation button. So there seems overkill to have 2 states (one in useRevokedStatus and one in AuthService) where only one seems enough and 2 new events with a new listener where the client.on('revoked') seems enough.

But maybe I'm missing something so feel free to explain why you did this.

Well, my goal was to make this feature as explicit and compartmentalised as possible, separating concerns for clean and testable code. But it's possible that I disregarded too many practical considerations while doing so.

I guess it could be simplified a lot by eliminating both the mirrored state and using a more declarative approach in the handling of errors and hiding dialogue windows. But since I didn't want to use a Context, it's somehow complex to send information to UI from a service. Maybe I could use react-navigation params when redirecting, which will show the modal, and remove these params when the user confirms.

Ldoppea

This is way cleaner and easier to understand, great refacto 👍

Ldoppea · 2023-07-11T12:38:13Z

src/locales/fr-FR/translation.json

+    },
+    "ErrorToken": {
+      "title": "Accès révoqué",
+      "body": "Vous semblez avoir révoqué cet appareil depuis votre Cozy. Pour accéder à nouveau à vos données, veuillez vous reconnecter.Si cette action ne vient pas de vous, n’hésitez pas à nous contacter à [email protected].",


Suggested change

"body": "Vous semblez avoir révoqué cet appareil depuis votre Cozy. Pour accéder à nouveau à vos données, veuillez vous reconnecter.Si cette action ne vient pas de vous, n’hésitez pas à nous contacter à [email protected].",

"body": "Vous semblez avoir révoqué cet appareil depuis votre Cozy. Pour accéder à nouveau à vos données, veuillez vous reconnecter. Si cette action ne vient pas de vous, n’hésitez pas à nous contacter à [email protected].",

Should [email protected] be clickable in the UI?

src/app/domain/authentication/services/AuthService.ts

Ldoppea · 2023-07-11T12:55:19Z

src/app/domain/authentication/services/AuthService.ts

+export const authLogger = Minilog('[AuthService]')
+let clientInstance: CozyClient | null = null
+
+const asyncLogoutNoClient = async (): Promise<void> => {


nit: it would be great to move this code near asyncLogout so we can easily see that both version exist and we don't forget to edit both if we add an item to clear on logout.

Or event better: find a way to mutualize them.

yeah but asyncLogout isn't in the greatest place right now. I have to check if it's easy to move it now

Ldoppea · 2023-07-11T12:58:23Z

src/screens/welcome/WelcomeScreen.jsx

+
+      {route.params?.options === 'showTokenError' && (
+        <ErrorTokenModal
+          onClose={() => navigation.setParams({ options: '' })}


oh wait, it didn't know about navigation.setParams(). Does this trigger a navigation (remount components and add history)? Or does it just rewrite params?

If the later it can be a good alternative for consumeRouteParameter

yes unfortunately it seems to trigger a rerender from what I know. Maybe it doesn't always do that? But I remember fixing a huge bug linked to that

At getClient(), we are checking if the user has a valid token. If it's not the case, we send him to the welcome page with a modal. This provides better UX. The feature has been implemented in a decoupled manner

It will emit the revoked event

acezard force-pushed the feat--handle-revoked-tokens-at-init branch 5 times, most recently from 98ece40 to c57aa40 Compare July 6, 2023 13:18

acezard changed the title ~~feat: handle revoked tokens at init~~ feat: handle revoked tokens Jul 6, 2023

acezard force-pushed the feat--handle-revoked-tokens-at-init branch 2 times, most recently from 34d3c0f to f1226e2 Compare July 7, 2023 09:01

acezard marked this pull request as ready for review July 7, 2023 09:01

acezard requested review from Ldoppea and zatteo as code owners July 7, 2023 09:01

acezard force-pushed the feat--handle-revoked-tokens-at-init branch 2 times, most recently from e3fe8be to e12932b Compare July 7, 2023 12:13

Ldoppea requested changes Jul 10, 2023

View reviewed changes

Ldoppea approved these changes Jul 11, 2023

View reviewed changes

acezard force-pushed the feat--handle-revoked-tokens-at-init branch 2 times, most recently from 5978a69 to 6ab862d Compare July 12, 2023 08:43

acezard force-pushed the feat--handle-revoked-tokens-at-init branch 6 times, most recently from 5f5bc83 to 900aabe Compare July 26, 2023 12:46

acezard added 4 commits July 27, 2023 10:17

feat: handle revoked tokens at init

307ce56

At getClient(), we are checking if the user has a valid token. If it's not the case, we send him to the welcome page with a modal. This provides better UX. The feature has been implemented in a decoupled manner

fix: rework data flow in functional style

c00e55c

feat: update strings to use i18n

6e41b6c

feat: Update cozy-client to latest

57b516b

It will emit the revoked event

acezard force-pushed the feat--handle-revoked-tokens-at-init branch from 231d438 to 57b516b Compare July 27, 2023 08:20

acezard merged commit 1a198d8 into master Jul 27, 2023
1 check passed

acezard deleted the feat--handle-revoked-tokens-at-init branch July 27, 2023 08:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: handle revoked tokens #862

feat: handle revoked tokens #862

acezard commented Jul 5, 2023 •

edited

Loading

Ldoppea left a comment

acezard commented Jul 11, 2023 •

edited

Loading

Ldoppea left a comment

Ldoppea Jul 11, 2023

Ldoppea Jul 11, 2023

acezard Jul 11, 2023 •

edited

Loading

Ldoppea Jul 11, 2023

acezard Jul 11, 2023

Ldoppea Jul 11, 2023

acezard Jul 11, 2023

	"body": "Vous semblez avoir révoqué cet appareil depuis votre Cozy. Pour accéder à nouveau à vos données, veuillez vous reconnecter.Si cette action ne vient pas de vous, n’hésitez pas à nous contacter à [email protected].",
	"body": "Vous semblez avoir révoqué cet appareil depuis votre Cozy. Pour accéder à nouveau à vos données, veuillez vous reconnecter. Si cette action ne vient pas de vous, n’hésitez pas à nous contacter à [email protected].",

feat: handle revoked tokens #862

feat: handle revoked tokens #862

Conversation

acezard commented Jul 5, 2023 • edited Loading

Changes

Testing

Screenshots

Ldoppea left a comment

Choose a reason for hiding this comment

acezard commented Jul 11, 2023 • edited Loading

Ldoppea left a comment

Choose a reason for hiding this comment

Ldoppea Jul 11, 2023

Choose a reason for hiding this comment

Ldoppea Jul 11, 2023

Choose a reason for hiding this comment

acezard Jul 11, 2023 • edited Loading

Choose a reason for hiding this comment

Ldoppea Jul 11, 2023

Choose a reason for hiding this comment

acezard Jul 11, 2023

Choose a reason for hiding this comment

Ldoppea Jul 11, 2023

Choose a reason for hiding this comment

acezard Jul 11, 2023

Choose a reason for hiding this comment

acezard commented Jul 5, 2023 •

edited

Loading

acezard commented Jul 11, 2023 •

edited

Loading

acezard Jul 11, 2023 •

edited

Loading