Skip to content

fix: UI not reflecting 2FA TOTP and email status change immediately #35185

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.

Sign up for GitHub

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

Closed
dhairyashiil wants to merge 2 commits into from
Closed

fix: UI not reflecting 2FA TOTP and email status change immediately #35185

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e9d2bb9
Fix: Update UI to immediately reflect 2FA status change after enablin…
dhairyashiil Feb 11, 2025
f799be9
Add error handling and prevent multiple clicks for email 2FA enable/d…
dhairyashiil Feb 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/clever-shirts-talk.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@rocket.chat/meteor': patch
---

Written code to update UI to immediately reflect 2FA status change after enabling/disabling TOTP/email

@tiagoevanp tiagoevanp Feb 12, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Written code to update UI to immediately reflect 2FA status change after enabling/disabling TOTP/email
Fixes 2FA status change not reflected in UI when enabling/disabling TOTP/emailcode

@dhairyashiil dhairyashiil Feb 13, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, Sir. And yes, from next time, I will write the changeset summary in the manner you mentioned.

7 changes: 5 additions & 2 deletions apps/meteor/client/views/account/security/TwoFactorEmail.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { Box, Button, Margins } from '@rocket.chat/fuselage';
import { useUser } from '@rocket.chat/ui-contexts';
import type { ComponentProps } from 'react';
import { useCallback } from 'react';
import { useCallback, useState } from 'react';
import { useTranslation } from 'react-i18next';

import { useEndpointAction } from '../../../hooks/useEndpointAction';
Expand All @@ -10,7 +10,7 @@ const TwoFactorEmail = (props: ComponentProps<typeof Box>) => {
const { t } = useTranslation();
const user = useUser();

const isEnabled = user?.services?.email2fa?.enabled;
const [isEnabled, setIsEnabled] = useState(user?.services?.email2fa?.enabled);

@tiagoevanp tiagoevanp Feb 12, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, can you change the isEnabled and setIsEnabled for something like isEmail2faEnabled/setIsEmail2faEnabled for a better const description name? Thanks!

@dhairyashiil dhairyashiil Feb 13, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, Sir. I have renamed it to isEmail2faEnabled/setIsEmail2faEnabled. Thank you, and from now on, I will keep this in mind while defining and naming variables.


const enable2faAction = useEndpointAction('POST', '/v1/users.2fa.enableEmail', {
successMessage: t('Two-factor_authentication_enabled'),
Expand All @@ -21,9 +21,12 @@ const TwoFactorEmail = (props: ComponentProps<typeof Box>) => {

const handleEnable = useCallback(async () => {
await enable2faAction();
setIsEnabled(true);

@tiagoevanp tiagoevanp Feb 12, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about to use the same technique used into TOTP file for "is enabling" state control? This will avoid any double click into the action button if a connection drop/delay happens... ;)

@dhairyashiil dhairyashiil Feb 13, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, Sir. Thank you for the suggestion. I’ve added a registeringEmail2fa state to track the action process, preventing multiple clicks if there's a delay or connection issue. I’ve also included error handling, and right now, I'm only dispatching the error as a toast message, assuming we get an error message if enabling/disabling email 2FA fails.

If you prefer a different approach, please let me know, and I’ll make the changes accordingly

}, [enable2faAction]);

const handleDisable = useCallback(async () => {
await disable2faAction();
setIsEnabled(false);
}, [disable2faAction]);

return (
Expand Down
16 changes: 10 additions & 6 deletions apps/meteor/client/views/account/security/TwoFactorTOTP.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,20 +36,20 @@ const TwoFactorTOTP = (props: TwoFactorTOTPProps): ReactElement => {

const { register, handleSubmit } = useForm<TwoFactorTOTPFormData>({ defaultValues: { authCode: '' } });

const totpEnabled = user?.services?.totp?.enabled;
const [isTotpEnabled, setIsTotpEnabled] = useState(user?.services?.totp?.enabled);

const closeModal = useCallback(() => setModal(null), [setModal]);

useEffect(() => {
const updateCodesRemaining = async (): Promise<void | boolean> => {
if (!totpEnabled) {
if (!isTotpEnabled) {
return false;
}
const result = await checkCodesRemainingFn();
setCodesRemaining(result.remaining);
};
updateCodesRemaining();
}, [checkCodesRemainingFn, setCodesRemaining, totpEnabled]);
}, [checkCodesRemainingFn, setCodesRemaining, isTotpEnabled]);

const handleEnableTotp = useCallback(async () => {
try {
Expand All @@ -73,6 +73,9 @@ const TwoFactorTOTP = (props: TwoFactorTOTPProps): ReactElement => {
return dispatchToastMessage({ type: 'error', message: t('Invalid_two_factor_code') });
}

setIsTotpEnabled(false);
setRegisteringTotp(false);

dispatchToastMessage({ type: 'success', message: t('Two-factor_authentication_disabled') });
} catch (error) {
dispatchToastMessage({ type: 'error', message: error });
Expand All @@ -93,6 +96,7 @@ const TwoFactorTOTP = (props: TwoFactorTOTPProps): ReactElement => {
}

setModal(<BackupCodesModal codes={result.codes} onClose={closeModal} />);
setIsTotpEnabled(true);
} catch (error) {
dispatchToastMessage({ type: 'error', message: error });
}
Expand Down Expand Up @@ -121,15 +125,15 @@ const TwoFactorTOTP = (props: TwoFactorTOTPProps): ReactElement => {
<Box display='flex' flexDirection='column' alignItems='flex-start' {...props}>
<Margins blockEnd={8}>
<Box fontScale='h4'>{t('Two-factor_authentication_via_TOTP')}</Box>
{!totpEnabled && !registeringTotp && (
{!isTotpEnabled && !registeringTotp && (
<>
<Box>{t('Two-factor_authentication_is_currently_disabled')}</Box>
<Button primary onClick={handleEnableTotp}>
{t('Enable_two-factor_authentication')}
</Button>
</>
)}
{!totpEnabled && registeringTotp && (
{!isTotpEnabled && registeringTotp && (
<>
<Box>{t('Scan_QR_code')}</Box>
<Box>{t('Scan_QR_code_alternative_s')}</Box>
Expand All @@ -143,7 +147,7 @@ const TwoFactorTOTP = (props: TwoFactorTOTPProps): ReactElement => {
</Box>
</>
)}
{totpEnabled && (
{isTotpEnabled && (
<>
<Button danger onClick={handleDisableTotp}>
{t('Disable_two-factor_authentication')}
Expand Down