From 4e8aa575a6da8c2961661bc88f7b21ebf3ead0f7 Mon Sep 17 00:00:00 2001 From: Tiago Evangelista Pinto Date: Fri, 19 Jul 2024 16:36:34 -0300 Subject: [PATCH] fix: Blocked login when dismissed 2FA modal (#32482) Co-authored-by: Tasso Evangelista <2263066+tassoevan@users.noreply.github.com> --- .changeset/soft-donkeys-thank.md | 8 + .../GenericModal/GenericModal.spec.tsx | 87 +++++++++ .../components/GenericModal/GenericModal.tsx | 35 +++- apps/meteor/client/lib/imperativeModal.tsx | 10 +- apps/meteor/client/portals/ModalPortal.tsx | 30 ++- .../ModalProvider/ModalProvider.spec.tsx | 177 ++++++++++-------- .../providers/ModalProvider/ModalProvider.tsx | 2 +- .../meteor/client/views/modal/ModalRegion.tsx | 9 +- .../hooks/useVideoConfOpenCall.spec.tsx | 11 +- .../actions/useRedirectModerationConsole.ts | 2 +- .../mock-providers/src/MockedModalContext.tsx | 4 +- packages/web-ui-registration/package.json | 3 + 12 files changed, 270 insertions(+), 108 deletions(-) create mode 100644 .changeset/soft-donkeys-thank.md create mode 100644 apps/meteor/client/components/GenericModal/GenericModal.spec.tsx diff --git a/.changeset/soft-donkeys-thank.md b/.changeset/soft-donkeys-thank.md new file mode 100644 index 000000000000..7273ddcffca4 --- /dev/null +++ b/.changeset/soft-donkeys-thank.md @@ -0,0 +1,8 @@ +--- +"@rocket.chat/meteor": patch +"@rocket.chat/mock-providers": patch +"@rocket.chat/ui-contexts": patch +"@rocket.chat/web-ui-registration": patch +--- + +Fixed an issue with blocked login when dismissed 2FA modal by clicking outside of it or pressing the escape key diff --git a/apps/meteor/client/components/GenericModal/GenericModal.spec.tsx b/apps/meteor/client/components/GenericModal/GenericModal.spec.tsx new file mode 100644 index 000000000000..0ef7235729c4 --- /dev/null +++ b/apps/meteor/client/components/GenericModal/GenericModal.spec.tsx @@ -0,0 +1,87 @@ +import { useSetModal } from '@rocket.chat/ui-contexts'; +import { act, screen } from '@testing-library/react'; +import { renderHook } from '@testing-library/react-hooks'; +import userEvent from '@testing-library/user-event'; +import type { ReactElement } from 'react'; +import React, { Suspense } from 'react'; + +import ModalProviderWithRegion from '../../providers/ModalProvider/ModalProviderWithRegion'; +import GenericModal from './GenericModal'; + +import '@testing-library/jest-dom'; + +const renderModal = (modalElement: ReactElement) => { + const { + result: { current: setModal }, + } = renderHook(() => useSetModal(), { + wrapper: ({ children }) => ( + + {children} + + ), + }); + + act(() => { + setModal(modalElement); + }); + + return { setModal }; +}; + +describe('callbacks', () => { + it('should call onClose callback when dismissed', async () => { + const handleClose = jest.fn(); + + renderModal(); + + expect(await screen.findByRole('heading', { name: 'Modal', exact: true })).toBeInTheDocument(); + + userEvent.keyboard('{Escape}'); + + expect(screen.queryByRole('heading', { name: 'Modal', exact: true })).not.toBeInTheDocument(); + + expect(handleClose).toHaveBeenCalled(); + }); + + it('should NOT call onClose callback when confirmed', async () => { + const handleConfirm = jest.fn(); + const handleClose = jest.fn(); + + const { setModal } = renderModal(); + + expect(await screen.findByRole('heading', { name: 'Modal', exact: true })).toBeInTheDocument(); + + userEvent.click(screen.getByRole('button', { name: 'Ok', exact: true })); + + expect(handleConfirm).toHaveBeenCalled(); + + act(() => { + setModal(null); + }); + + expect(screen.queryByRole('heading', { name: 'Modal', exact: true })).not.toBeInTheDocument(); + + expect(handleClose).not.toHaveBeenCalled(); + }); + + it('should NOT call onClose callback when cancelled', async () => { + const handleCancel = jest.fn(); + const handleClose = jest.fn(); + + const { setModal } = renderModal(); + + expect(await screen.findByRole('heading', { name: 'Modal', exact: true })).toBeInTheDocument(); + + userEvent.click(screen.getByRole('button', { name: 'Cancel', exact: true })); + + expect(handleCancel).toHaveBeenCalled(); + + act(() => { + setModal(null); + }); + + expect(screen.queryByRole('heading', { name: 'Modal', exact: true })).not.toBeInTheDocument(); + + expect(handleClose).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/meteor/client/components/GenericModal/GenericModal.tsx b/apps/meteor/client/components/GenericModal/GenericModal.tsx index 914928d4d423..d371e1ff4ef2 100644 --- a/apps/meteor/client/components/GenericModal/GenericModal.tsx +++ b/apps/meteor/client/components/GenericModal/GenericModal.tsx @@ -1,9 +1,9 @@ import { Button, Modal } from '@rocket.chat/fuselage'; -import { useUniqueId } from '@rocket.chat/fuselage-hooks'; +import { useEffectEvent, useUniqueId } from '@rocket.chat/fuselage-hooks'; import type { Keys as IconName } from '@rocket.chat/icons'; import { useTranslation } from '@rocket.chat/ui-contexts'; import type { ComponentProps, ReactElement, ReactNode, ComponentPropsWithoutRef } from 'react'; -import React from 'react'; +import React, { useEffect, useRef } from 'react'; import type { RequiredModalProps } from './withDoNotAskAgain'; import { withDoNotAskAgain } from './withDoNotAskAgain'; @@ -78,6 +78,31 @@ const GenericModal = ({ const t = useTranslation(); const genericModalId = useUniqueId(); + const dismissedRef = useRef(true); + + const handleConfirm = useEffectEvent(() => { + dismissedRef.current = false; + onConfirm?.(); + }); + + const handleCancel = useEffectEvent(() => { + dismissedRef.current = false; + onCancel?.(); + }); + + const handleCloseButtonClick = useEffectEvent(() => { + dismissedRef.current = true; + onClose?.(); + }); + + useEffect( + () => () => { + if (!dismissedRef.current) return; + onClose?.(); + }, + [onClose], + ); + return ( @@ -86,7 +111,7 @@ const GenericModal = ({ {tagline && {tagline}} {title ?? t('Are_you_sure')} - + {children} @@ -94,7 +119,7 @@ const GenericModal = ({ {annotation && !dontAskAgain && {annotation}} {onCancel && ( - )} @@ -104,7 +129,7 @@ const GenericModal = ({ )} {!wrapperFunction && onConfirm && ( - )} diff --git a/apps/meteor/client/lib/imperativeModal.tsx b/apps/meteor/client/lib/imperativeModal.tsx index 28db6fa107ed..3740eb1ebc9c 100644 --- a/apps/meteor/client/lib/imperativeModal.tsx +++ b/apps/meteor/client/lib/imperativeModal.tsx @@ -1,15 +1,15 @@ import { Emitter } from '@rocket.chat/emitter'; import React, { Suspense, createElement } from 'react'; -import type { ComponentProps, ElementType, ReactNode } from 'react'; +import type { ComponentProps, ComponentType, ReactNode } from 'react'; import { modalStore } from '../providers/ModalProvider/ModalStore'; -type ReactModalDescriptor = { +type ReactModalDescriptor = ComponentType> = { component: TComponent; props?: ComponentProps; }; -type ModalDescriptor = ReactModalDescriptor | null; +type ModalDescriptor = ReactModalDescriptor | null; type ModalInstance = { close: () => void; @@ -41,11 +41,11 @@ class ImperativeModalEmmiter extends Emitter<{ update: ModalDescriptor }> { this.store = store; } - open = (descriptor: ReactModalDescriptor): ModalInstance => { + open = >(descriptor: ReactModalDescriptor): ModalInstance => { return this.store.open(mapCurrentModal(descriptor as ModalDescriptor)); }; - push = (descriptor: ReactModalDescriptor): ModalInstance => { + push = >(descriptor: ReactModalDescriptor): ModalInstance => { return this.store.push(mapCurrentModal(descriptor as ModalDescriptor)); }; diff --git a/apps/meteor/client/portals/ModalPortal.tsx b/apps/meteor/client/portals/ModalPortal.tsx index d7c9ae9caa2d..6b2210d56926 100644 --- a/apps/meteor/client/portals/ModalPortal.tsx +++ b/apps/meteor/client/portals/ModalPortal.tsx @@ -1,18 +1,32 @@ -import type { ReactElement, ReactNode } from 'react'; -import React, { memo, useEffect, useState } from 'react'; +import type { ReactNode } from 'react'; +import { memo } from 'react'; import { createPortal } from 'react-dom'; -import { createAnchor } from '../lib/utils/createAnchor'; -import { deleteAnchor } from '../lib/utils/deleteAnchor'; +const createModalRoot = (): HTMLElement => { + const id = 'modal-root'; + const existing = document.getElementById(id); + + if (existing) return existing; + + const newOne = document.createElement('div'); + newOne.id = id; + document.body.append(newOne); + + return newOne; +}; + +let modalRoot: HTMLElement | null = null; type ModalPortalProps = { children?: ReactNode; }; -const ModalPortal = ({ children }: ModalPortalProps): ReactElement => { - const [modalRoot] = useState(() => createAnchor('modal-root')); - useEffect(() => (): void => deleteAnchor(modalRoot), [modalRoot]); - return <>{createPortal(children, modalRoot)}; +const ModalPortal = ({ children }: ModalPortalProps) => { + if (!modalRoot) { + modalRoot = createModalRoot(); + } + + return createPortal(children, modalRoot); }; export default memo(ModalPortal); diff --git a/apps/meteor/client/providers/ModalProvider/ModalProvider.spec.tsx b/apps/meteor/client/providers/ModalProvider/ModalProvider.spec.tsx index f77933337456..ea062c324807 100644 --- a/apps/meteor/client/providers/ModalProvider/ModalProvider.spec.tsx +++ b/apps/meteor/client/providers/ModalProvider/ModalProvider.spec.tsx @@ -1,115 +1,138 @@ -// import type { IMessage } from '@rocket.chat/core-typings'; -import { Emitter } from '@rocket.chat/emitter'; import { useSetModal } from '@rocket.chat/ui-contexts'; -import { render, screen } from '@testing-library/react'; -import { expect } from 'chai'; -import type { ReactNode } from 'react'; -import React, { Suspense, createContext, useContext, useEffect } from 'react'; +import { act, render, screen } from '@testing-library/react'; +import type { ForwardedRef, ReactElement } from 'react'; +import React, { Suspense, createContext, createRef, forwardRef, useContext, useImperativeHandle } from 'react'; import GenericModal from '../../components/GenericModal'; import { imperativeModal } from '../../lib/imperativeModal'; import ModalRegion from '../../views/modal/ModalRegion'; import ModalProvider from './ModalProvider'; import ModalProviderWithRegion from './ModalProviderWithRegion'; +import '@testing-library/jest-dom'; -const TestContext = createContext({ title: 'default' }); -const emitter = new Emitter(); +const renderWithSuspense = (ui: ReactElement) => + render(ui, { + wrapper: ({ children }) => {children}, + }); -const TestModal = ({ emitterEvent, modalFunc }: { emitterEvent: string; modalFunc?: () => ReactNode }) => { - const setModal = useSetModal(); - const { title } = useContext(TestContext); +describe('via useSetModal', () => { + const ModalTitleContext = createContext('default'); - useEffect(() => { - emitter.on(emitterEvent, () => { - setModal(modalFunc || undefined}>); - }); - }, [emitterEvent, setModal, title, modalFunc]); + type ModalOpenerAPI = { open: () => void }; - return <>; -}; + const ModalOpener = forwardRef((_: unknown, ref: ForwardedRef) => { + const setModal = useSetModal(); + const title = useContext(ModalTitleContext); + useImperativeHandle(ref, () => ({ + open: () => { + setModal(); + }, + })); + + return null; + }); -describe('Modal Provider', () => { it('should render a modal', async () => { - render( - + const modalOpenerRef = createRef(); + + renderWithSuspense( + + + , + ); + + act(() => { + modalOpenerRef.current?.open(); + }); + + expect(await screen.findByRole('dialog', { name: 'default' })).toBeInTheDocument(); + }); + + it('should render a modal that consumes a context', async () => { + const modalOpenerRef = createRef(); + + renderWithSuspense( + - + - , + , ); - emitter.emit('open'); - expect(await screen.findByText('default')).to.exist; + act(() => { + modalOpenerRef.current?.open(); + }); + + expect(await screen.findByRole('dialog', { name: 'title from context' })).toBeInTheDocument(); }); - it('should render a modal that is passed as a function', async () => { - render( - + it('should render a modal in another region', async () => { + const modalOpener1Ref = createRef(); + const modalOpener2Ref = createRef(); + + renderWithSuspense( + - undefined} />} /> + - , + + + + + + , ); - emitter.emit('open'); - expect(await screen.findByText('function modal')).to.exist; + + act(() => { + modalOpener1Ref.current?.open(); + }); + + expect(await screen.findByRole('dialog', { name: 'modal1' })).toBeInTheDocument(); + + act(() => { + modalOpener2Ref.current?.open(); + }); + + expect(await screen.findByRole('dialog', { name: 'modal2' })).toBeInTheDocument(); }); +}); + +describe('via imperativeModal', () => { + it('should render a modal through imperative modal', async () => { + renderWithSuspense( + + + , + ); - it('should render a modal through imperative modal', () => { - async () => { - render( - - - - - , - ); - - const { close } = imperativeModal.open({ + act(() => { + imperativeModal.open({ component: GenericModal, - props: { title: 'imperativeModal' }, + props: { title: 'imperativeModal', open: true }, }); + }); - expect(await screen.findByText('imperativeModal')).to.exist; + expect(await screen.findByRole('dialog', { name: 'imperativeModal' })).toBeInTheDocument(); - close(); + act(() => { + imperativeModal.close(); + }); - expect(screen.queryByText('imperativeModal')).to.not.exist; - }; + expect(screen.queryByText('imperativeModal')).not.toBeInTheDocument(); }); it('should not render a modal if no corresponding region exists', async () => { // ModalProviderWithRegion will always have a region identifier set // and imperativeModal will only render modals in the default region (e.g no region identifier) - render( - - - , - ); - - imperativeModal.open({ - component: GenericModal, - props: { title: 'imperativeModal' }, - }); - expect(screen.queryByText('imperativeModal')).to.not.exist; - }); + renderWithSuspense(); - it('should render a modal in another region', () => { - render( - - - - - - - - - - , - ); + act(() => { + imperativeModal.open({ + component: GenericModal, + props: { title: 'imperativeModal', open: true }, + }); + }); - emitter.emit('openModal1'); - expect(screen.getByText('modal1')).to.exist; - emitter.emit('openModal2'); - expect(screen.getByText('modal2')).to.exist; + expect(screen.queryByRole('dialog', { name: 'imperativeModal' })).not.toBeInTheDocument(); }); }); diff --git a/apps/meteor/client/providers/ModalProvider/ModalProvider.tsx b/apps/meteor/client/providers/ModalProvider/ModalProvider.tsx index 6c3f1026bc51..27092ea602b6 100644 --- a/apps/meteor/client/providers/ModalProvider/ModalProvider.tsx +++ b/apps/meteor/client/providers/ModalProvider/ModalProvider.tsx @@ -33,7 +33,7 @@ const ModalProvider = ({ children, region }: ModalProviderProps) => { }, region, }), - [currentModal, region, setModal], + [currentModal?.node, currentModal?.region, region, setModal], ); return ; diff --git a/apps/meteor/client/views/modal/ModalRegion.tsx b/apps/meteor/client/views/modal/ModalRegion.tsx index 5cbad2b52bc1..284c460ee043 100644 --- a/apps/meteor/client/views/modal/ModalRegion.tsx +++ b/apps/meteor/client/views/modal/ModalRegion.tsx @@ -1,6 +1,7 @@ -import { useModal, useCurrentModal } from '@rocket.chat/ui-contexts'; +import { useEffectEvent } from '@rocket.chat/fuselage-hooks'; +import { useCurrentModal, useModal } from '@rocket.chat/ui-contexts'; import type { ReactElement } from 'react'; -import React, { lazy, useCallback } from 'react'; +import React, { lazy } from 'react'; import ModalBackdrop from '../../components/ModalBackdrop'; import ModalPortal from '../../portals/ModalPortal'; @@ -10,7 +11,9 @@ const FocusScope = lazy(() => import('react-aria').then((module) => ({ default: const ModalRegion = (): ReactElement | null => { const currentModal = useCurrentModal(); const { setModal } = useModal(); - const handleDismiss = useCallback(() => setModal(null), [setModal]); + const handleDismiss = useEffectEvent(() => { + setModal(null); + }); if (!currentModal) { return null; diff --git a/apps/meteor/client/views/room/contextualBar/VideoConference/hooks/useVideoConfOpenCall.spec.tsx b/apps/meteor/client/views/room/contextualBar/VideoConference/hooks/useVideoConfOpenCall.spec.tsx index ed14eacb7191..c64dd19b2dbf 100644 --- a/apps/meteor/client/views/room/contextualBar/VideoConference/hooks/useVideoConfOpenCall.spec.tsx +++ b/apps/meteor/client/views/room/contextualBar/VideoConference/hooks/useVideoConfOpenCall.spec.tsx @@ -1,20 +1,18 @@ import { faker } from '@faker-js/faker'; import { ModalContext } from '@rocket.chat/ui-contexts'; -import type { WrapperComponent } from '@testing-library/react-hooks'; import { renderHook } from '@testing-library/react-hooks'; +import type { ReactNode } from 'react'; import React from 'react'; import { useVideoConfOpenCall } from './useVideoConfOpenCall'; describe('with window.RocketChatDesktop set', () => { - const wrapper: WrapperComponent = ({ children }) => ( + const wrapper = ({ children }: { children: ReactNode }) => ( { - return null; - }, + setModal: () => null, }, currentModal: { component: null }, }} @@ -54,7 +52,8 @@ describe('with window.RocketChatDesktop set', () => { describe('with window.RocketChatDesktop unset', () => { const setModal = jest.fn(); - const wrapper: WrapperComponent = ({ children }) => ( + + const wrapper = ({ children }: { children: ReactNode }) => ( { - const [currentModal, setCurrentModal] = React.useState(null); + const [currentModal, setCurrentModal] = useState(null); return (