From c367f88acf73707c897c626992ac138da8df3d99 Mon Sep 17 00:00:00 2001 From: Grzegorz Zdunek Date: Fri, 15 Nov 2024 12:24:47 +0100 Subject: [PATCH] Allow displaying hardware keys prompts when relogin is in progress (#48813) * Convert `DialogConfirmation` to TS * Allow `DialogConfirmation` and `Modal` to be hidden using CSS while closed * Allow hiding all dialogs that are displayed as important * Allow displaying multiple important dialogs, separate regular and important dialogs and get rid of `DialogNone` * Pass `hidden` prop to important dialogs * Rename `importantModalSemaphore` to `singleImportantModalSemaphore` * Remove semaphores from hardware key prompts * `keepMounted` -> `keepInDOMAfterClose` * Remove the explicit value from `keepInDOMAfterClose` * Revert splitting dialogs into regular and important ones, pass `hidden` to all of them, hide regular modal when important one is visible * Use random id as a modal key * Improve `singleImportantModalSemaphore` comment * Improve `NewHardwareKeyPromptConstructor` comment * Do not acquire important modal semaphore in MFA prompt and relogin, give each prompt its own mutex/semaphore (cherry picked from commit 742812889709b1322db34945085a4c514aef653c) --- lib/teleterm/daemon/daemon.go | 41 +++-- lib/teleterm/daemon/daemon_headless.go | 4 +- lib/teleterm/daemon/daemon_test.go | 43 +++-- lib/teleterm/daemon/hardwarekeyprompt.go | 32 ++-- lib/teleterm/daemon/mfaprompt.go | 6 +- ...story.jsx => DialogConfirmation.story.tsx} | 4 +- ...n.test.jsx => DialogConfirmation.test.tsx} | 4 +- ...onfirmation.jsx => DialogConfirmation.tsx} | 32 +++- .../DialogConfirmation/{index.js => index.ts} | 2 +- web/packages/design/src/Modal/Modal.test.tsx | 170 ++++++++++-------- .../src/ui/ClusterConnect/ClusterConnect.tsx | 8 +- .../src/ui/ClusterLogout/ClusterLogout.tsx | 17 +- .../ui/DocumentsReopen/DocumentsReopen.tsx | 10 +- .../HeadlessAuthn/HeadlessAuthentication.tsx | 2 + .../HeadlessPrompt/HeadlessPrompt.tsx | 6 +- .../src/ui/ModalsHost/ModalsHost.story.tsx | 40 ++++- .../src/ui/ModalsHost/ModalsHost.test.tsx | 94 ++++++++-- .../teleterm/src/ui/ModalsHost/ModalsHost.tsx | 66 +++++-- .../AuthenticateWebDevice.tsx | 18 +- .../ChangeAccessRequestKind.tsx | 5 +- .../ModalsHost/modals/HardwareKeys/AskPin.tsx | 4 +- .../modals/HardwareKeys/ChangePin.tsx | 4 +- .../modals/HardwareKeys/OverwriteSlot.tsx | 4 +- .../ModalsHost/modals/HardwareKeys/Touch.tsx | 4 +- .../modals/ReAuthenticate/ReAuthenticate.tsx | 4 +- .../ModalsHost/modals/UsageData/UsageData.tsx | 12 +- .../modals/UserJobRole/UserJobRole.tsx | 15 +- .../src/ui/Search/ResourceSearchErrors.tsx | 4 +- .../src/ui/services/modals/modalsService.ts | 61 ++++--- 29 files changed, 451 insertions(+), 265 deletions(-) rename web/packages/design/src/DialogConfirmation/{DialogConfirmation.story.jsx => DialogConfirmation.story.tsx} (94%) rename web/packages/design/src/DialogConfirmation/{DialogConfirmation.test.jsx => DialogConfirmation.test.tsx} (92%) rename web/packages/design/src/DialogConfirmation/{DialogConfirmation.jsx => DialogConfirmation.tsx} (52%) rename web/packages/design/src/DialogConfirmation/{index.js => index.ts} (94%) diff --git a/lib/teleterm/daemon/daemon.go b/lib/teleterm/daemon/daemon.go index 8ff6aa94f141d..63d776a3dcf15 100644 --- a/lib/teleterm/daemon/daemon.go +++ b/lib/teleterm/daemon/daemon.go @@ -114,11 +114,6 @@ func (s *Service) relogin(ctx context.Context, req *api.ReloginRequest) error { } defer s.reloginMu.Unlock() - if err := s.importantModalSemaphore.Acquire(ctx); err != nil { - return trace.Wrap(err) - } - defer s.importantModalSemaphore.Release() - const reloginUserTimeout = time.Minute timeoutCtx, cancelTshdEventsCtx := context.WithTimeout(ctx, reloginUserTimeout) defer cancelTshdEventsCtx() @@ -912,7 +907,7 @@ func (s *Service) UpdateAndDialTshdEventsServerAddress(serverAddress string) err client := api.NewTshdEventsServiceClient(conn) s.tshdEventsClient = client - s.importantModalSemaphore = newWaitSemaphore(maxConcurrentImportantModals, imporantModalWaitDuraiton) + s.headlessAuthSemaphore = newWaitSemaphore(maxConcurrentImportantModals, imporantModalWaitDuraiton) return nil } @@ -1217,20 +1212,32 @@ type Service struct { gateways map[string]gateway.Gateway // tshdEventsClient is a client to send events to the Electron App. tshdEventsClient api.TshdEventsServiceClient - // The Electron App can only display one important Modal at a time. tshd events - // that trigger an important modal (relogin, headless login) should use this - // lock to ensure it doesn't overwrite existing tshd-initiated important modals. - // - // We use a semaphore instead of a mutex in order to cancel important modals that - // are no longer relevant before acquisition. + + // The Electron App can display multiple important modals by showing the latest one and hiding the others. + // However, we should be careful with it, and generally try to limit the number of prompts on the tshd side, + // to avoid flooding the app. + // Multiple prompts of the same type may also conflict with each other. This is currently possible with + // MFA prompts (see the mfaMu comment for details). + // Generally, only one prompt of each type (e.g., re-login, MFA) should be allowed at the same time. // - // We use a waitSemaphore in order to make sure there is a clear transition between modals. - importantModalSemaphore *waitSemaphore + // But why do we allow multiple important modals at all? It is necessary in specific scenarios where one + // modal action requires completing in another. Currently, there are two cases: + // 1. A hardware key prompt may appear during a re-login process. + // 2. An MFA prompt may appear during a re-login process. + + // We use a waitSemaphore to make sure there is a clear transition between modals. + // We allow a single headless auth prompt at a time. + headlessAuthSemaphore *waitSemaphore + // mfaMu prevents concurrent MFA prompts. These can happen when using VNet with per-session MFA. + // Issuing an MFA prompt starts the Webauthn goroutine which prompts for key touch on hardware level. + // Webauthn does not support concurrent prompts, so without this semaphore, one of the prompts would fail immediately. + mfaMu sync.Mutex + // reloginMu is used when a goroutine needs to request a relogin from the Electron app. + // We allow a single relogin prompt at a time. + reloginMu sync.Mutex + // usageReporter batches the events and sends them to prehog usageReporter *usagereporter.UsageReporter - // reloginMu is used when a goroutine needs to request a relogin from the Electron app. Since the - // app can show only one login modal at a time, we need to submit only one request at a time. - reloginMu sync.Mutex // headlessWatcherClosers holds a map of root cluster URIs to headless watchers. headlessWatcherClosers map[string]context.CancelFunc headlessWatcherClosersMu sync.Mutex diff --git a/lib/teleterm/daemon/daemon_headless.go b/lib/teleterm/daemon/daemon_headless.go index 3ea8b5755d678..2662791ca17bb 100644 --- a/lib/teleterm/daemon/daemon_headless.go +++ b/lib/teleterm/daemon/daemon_headless.go @@ -272,10 +272,10 @@ func (s *Service) sendPendingHeadlessAuthentication(ctx context.Context, ha *typ HeadlessAuthenticationClientIp: ha.ClientIpAddress, } - if err := s.importantModalSemaphore.Acquire(ctx); err != nil { + if err := s.headlessAuthSemaphore.Acquire(ctx); err != nil { return trace.Wrap(err) } - defer s.importantModalSemaphore.Release() + defer s.headlessAuthSemaphore.Release() _, err := s.tshdEventsClient.SendPendingHeadlessAuthentication(ctx, req) return trace.Wrap(err) diff --git a/lib/teleterm/daemon/daemon_test.go b/lib/teleterm/daemon/daemon_test.go index 00bf1176ce03f..9c46057f7ac30 100644 --- a/lib/teleterm/daemon/daemon_test.go +++ b/lib/teleterm/daemon/daemon_test.go @@ -489,7 +489,7 @@ func TestRetryWithRelogin(t *testing.T) { } } -func TestImportantModalSemaphore(t *testing.T) { +func TestConcurrentHeadlessAuthPrompts(t *testing.T) { t.Parallel() ctx := context.Background() @@ -522,54 +522,54 @@ func TestImportantModalSemaphore(t *testing.T) { // Claim the important modal semaphore. customWaitDuration := 10 * time.Millisecond - daemon.importantModalSemaphore.waitDuration = customWaitDuration - err = daemon.importantModalSemaphore.Acquire(ctx) + daemon.headlessAuthSemaphore.waitDuration = customWaitDuration + err = daemon.headlessAuthSemaphore.Acquire(ctx) require.NoError(t, err) - // relogin and sending pending headless authentications should be blocked. + // Pending headless authentications should be blocked. - reloginErrC := make(chan error) + headlessPromptErr1 := make(chan error) go func() { - reloginErrC <- daemon.relogin(ctx, &api.ReloginRequest{}) + headlessPromptErr1 <- daemon.sendPendingHeadlessAuthentication(ctx, &types.HeadlessAuthentication{}, "") }() - sphaErrC := make(chan error) + headlessPromptErr2 := make(chan error) go func() { - sphaErrC <- daemon.sendPendingHeadlessAuthentication(ctx, &types.HeadlessAuthentication{}, "") + headlessPromptErr2 <- daemon.sendPendingHeadlessAuthentication(ctx, &types.HeadlessAuthentication{}, "") }() select { - case <-reloginErrC: - t.Error("relogin completed successfully without acquiring the important modal semaphore") - case <-sphaErrC: - t.Error("sendPendingHeadlessAuthentication completed successfully without acquiring the important modal semaphore") + case <-headlessPromptErr1: + t.Error("sendPendingHeadlessAuthentication for the first prompt completed successfully without acquiring the semaphore") + case <-headlessPromptErr2: + t.Error("sendPendingHeadlessAuthentication for the second prompt completed successfully without acquiring the semaphore") case <-time.After(100 * time.Millisecond): } - // if the request's ctx is canceled, they will unblock and return an error instead. + // If the request's ctx is canceled, they will unblock and return an error instead. cancelCtx, cancel := context.WithCancel(ctx) cancel() - err = daemon.relogin(cancelCtx, &api.ReloginRequest{}) + err = daemon.sendPendingHeadlessAuthentication(cancelCtx, &types.HeadlessAuthentication{}, "") require.Error(t, err) err = daemon.sendPendingHeadlessAuthentication(cancelCtx, &types.HeadlessAuthentication{}, "") require.Error(t, err) - // Release the semaphore. relogin and sending pending headless authentication should + // Release the semaphore. Pending headless authentication should // complete successfully after a short delay between each semaphore release. releaseTime := time.Now() - daemon.importantModalSemaphore.Release() + daemon.headlessAuthSemaphore.Release() var otherC chan error select { - case err := <-reloginErrC: + case err := <-headlessPromptErr1: require.NoError(t, err) - otherC = sphaErrC - case err := <-sphaErrC: + otherC = headlessPromptErr2 + case err := <-headlessPromptErr2: require.NoError(t, err) - otherC = reloginErrC + otherC = headlessPromptErr1 case <-time.After(time.Second): t.Error("important modal operations failed to acquire unclaimed semaphore") } @@ -589,8 +589,7 @@ func TestImportantModalSemaphore(t *testing.T) { t.Error("important modal semaphore should not be acquired before waiting the specified duration") } - require.EqualValues(t, 1, service.reloginCount.Load(), "Unexpected number of calls to service.Relogin") - require.EqualValues(t, 1, service.sendPendingHeadlessAuthenticationCount.Load(), "Unexpected number of calls to service.SendPendingHeadlessAuthentication") + require.EqualValues(t, 2, service.sendPendingHeadlessAuthenticationCount.Load(), "Unexpected number of calls to service.SendPendingHeadlessAuthentication") } type mockTSHDEventsService struct { diff --git a/lib/teleterm/daemon/hardwarekeyprompt.go b/lib/teleterm/daemon/hardwarekeyprompt.go index 4c8b5838a3b01..ad75ccf6f699b 100644 --- a/lib/teleterm/daemon/hardwarekeyprompt.go +++ b/lib/teleterm/daemon/hardwarekeyprompt.go @@ -30,6 +30,22 @@ import ( // NewHardwareKeyPromptConstructor returns a new hardware key prompt constructor // for this service and the given root cluster URI. +// +// TODO(gzdunek): Improve multi-cluster and multi-hardware keys support. +// The code in yubikey.go doesn't really support using multiple hardware keys (like one per cluster): +// 1. We don't offer a choice which key should be used on the initial login. +// 2. Keys are cached per slot, not per physical key - it's not possible to use different keys with the same slot. +// +// Additionally, using the same hardware key for two clusters is not ideal too. +// Since we cache the keys per slot, if two clusters specify the same one, +// the user will always see the prompt for the same cluster URI. For example, if you are logged into both +// cluster-a and cluster-b, the prompt will always say "Unlock hardware key to access cluster-b." +// It seems that the better option would be to have a prompt per physical key, not per cluster. +// But I will leave that for the future, it's hard to say how common these scenarios will be in Connect. +// +// Because the code in yubikey.go assumes you use a single key, we don't have any mutex here. +// (unlike other modals triggered by tshd). +// We don't expect receiving prompts from different hardware keys. func (s *Service) NewHardwareKeyPromptConstructor(rootClusterURI uri.ResourceURI) keys.HardwareKeyPrompt { return &hardwareKeyPrompter{s: s, rootClusterURI: rootClusterURI} } @@ -41,10 +57,6 @@ type hardwareKeyPrompter struct { // Touch prompts the user to touch the hardware key. func (h *hardwareKeyPrompter) Touch(ctx context.Context) error { - if err := h.s.importantModalSemaphore.Acquire(ctx); err != nil { - return trace.Wrap(err) - } - defer h.s.importantModalSemaphore.Release() _, err := h.s.tshdEventsClient.PromptHardwareKeyTouch(ctx, &api.PromptHardwareKeyTouchRequest{ RootClusterUri: h.rootClusterURI.String(), }) @@ -56,10 +68,6 @@ func (h *hardwareKeyPrompter) Touch(ctx context.Context) error { // AskPIN prompts the user for a PIN. func (h *hardwareKeyPrompter) AskPIN(ctx context.Context, requirement keys.PINPromptRequirement) (string, error) { - if err := h.s.importantModalSemaphore.Acquire(ctx); err != nil { - return "", trace.Wrap(err) - } - defer h.s.importantModalSemaphore.Release() res, err := h.s.tshdEventsClient.PromptHardwareKeyPIN(ctx, &api.PromptHardwareKeyPINRequest{ RootClusterUri: h.rootClusterURI.String(), PinOptional: requirement == keys.PINOptional, @@ -74,10 +82,6 @@ func (h *hardwareKeyPrompter) AskPIN(ctx context.Context, requirement keys.PINPr // The Electron app prompt must handle default values for PIN and PUK, // preventing the user from submitting empty/default values. func (h *hardwareKeyPrompter) ChangePIN(ctx context.Context) (*keys.PINAndPUK, error) { - if err := h.s.importantModalSemaphore.Acquire(ctx); err != nil { - return nil, trace.Wrap(err) - } - defer h.s.importantModalSemaphore.Release() res, err := h.s.tshdEventsClient.PromptHardwareKeyPINChange(ctx, &api.PromptHardwareKeyPINChangeRequest{ RootClusterUri: h.rootClusterURI.String(), }) @@ -93,10 +97,6 @@ func (h *hardwareKeyPrompter) ChangePIN(ctx context.Context) (*keys.PINAndPUK, e // ConfirmSlotOverwrite asks the user if the slot's private key and certificate can be overridden. func (h *hardwareKeyPrompter) ConfirmSlotOverwrite(ctx context.Context, message string) (bool, error) { - if err := h.s.importantModalSemaphore.Acquire(ctx); err != nil { - return false, trace.Wrap(err) - } - defer h.s.importantModalSemaphore.Release() res, err := h.s.tshdEventsClient.ConfirmHardwareKeySlotOverwrite(ctx, &api.ConfirmHardwareKeySlotOverwriteRequest{ RootClusterUri: h.rootClusterURI.String(), Message: message, diff --git a/lib/teleterm/daemon/mfaprompt.go b/lib/teleterm/daemon/mfaprompt.go index 82404ea5d5010..a055c1a00d827 100644 --- a/lib/teleterm/daemon/mfaprompt.go +++ b/lib/teleterm/daemon/mfaprompt.go @@ -61,10 +61,8 @@ func (s *Service) NewMFAPrompt(resourceURI uri.ResourceURI, cfg *libmfa.PromptCo } func (s *Service) promptAppMFA(ctx context.Context, in *api.PromptMFARequest) (*api.PromptMFAResponse, error) { - if err := s.importantModalSemaphore.Acquire(ctx); err != nil { - return nil, trace.Wrap(err) - } - defer s.importantModalSemaphore.Release() + s.mfaMu.Lock() + defer s.mfaMu.Unlock() return s.tshdEventsClient.PromptMFA(ctx, in) } diff --git a/web/packages/design/src/DialogConfirmation/DialogConfirmation.story.jsx b/web/packages/design/src/DialogConfirmation/DialogConfirmation.story.tsx similarity index 94% rename from web/packages/design/src/DialogConfirmation/DialogConfirmation.story.jsx rename to web/packages/design/src/DialogConfirmation/DialogConfirmation.story.tsx index ef9260cf9ac82..51e5e25ed7b8a 100644 --- a/web/packages/design/src/DialogConfirmation/DialogConfirmation.story.jsx +++ b/web/packages/design/src/DialogConfirmation/DialogConfirmation.story.tsx @@ -16,9 +16,7 @@ * along with this program. If not, see . */ -import React from 'react'; - -import { ButtonPrimary } from './../Button'; +import { ButtonPrimary } from '../Button'; import DialogConfirmation, { DialogHeader, diff --git a/web/packages/design/src/DialogConfirmation/DialogConfirmation.test.jsx b/web/packages/design/src/DialogConfirmation/DialogConfirmation.test.tsx similarity index 92% rename from web/packages/design/src/DialogConfirmation/DialogConfirmation.test.jsx rename to web/packages/design/src/DialogConfirmation/DialogConfirmation.test.tsx index a1974fe1fc247..4887037fd3974 100644 --- a/web/packages/design/src/DialogConfirmation/DialogConfirmation.test.jsx +++ b/web/packages/design/src/DialogConfirmation/DialogConfirmation.test.tsx @@ -16,11 +16,9 @@ * along with this program. If not, see . */ -import React from 'react'; - import { render, fireEvent } from 'design/utils/testing'; -import DialogConfirmation from './DialogConfirmation'; +import { DialogConfirmation } from './DialogConfirmation'; test('onClose is respected', () => { const onClose = jest.fn(); diff --git a/web/packages/design/src/DialogConfirmation/DialogConfirmation.jsx b/web/packages/design/src/DialogConfirmation/DialogConfirmation.tsx similarity index 52% rename from web/packages/design/src/DialogConfirmation/DialogConfirmation.jsx rename to web/packages/design/src/DialogConfirmation/DialogConfirmation.tsx index f15f1b5c8604c..80b0c33ead6d7 100644 --- a/web/packages/design/src/DialogConfirmation/DialogConfirmation.jsx +++ b/web/packages/design/src/DialogConfirmation/DialogConfirmation.tsx @@ -16,22 +16,36 @@ * along with this program. If not, see . */ -import React from 'react'; +import { ReactNode } from 'react'; +import { StyleFunction } from 'styled-components'; import Dialog from 'design/Dialog'; -function DialogConfirmation(props) { - const { children, open, onClose, dialogCss } = props; +export function DialogConfirmation(props: { + open: boolean; + /** + * Prevent unmounting the component and its children from the DOM when closed. + * Instead, hides it with CSS. + */ + keepInDOMAfterClose?: boolean; + /** @deprecated This props has no effect, it was never passed down to `Dialog`. */ + disableEscapeKeyDown?: boolean; + children?: ReactNode; + onClose?: ( + event: KeyboardEvent | React.MouseEvent, + reason: 'escapeKeyDown' | 'backdropClick' + ) => void; + dialogCss?: StyleFunction; +}) { return ( - {children} + {props.children} ); } - -export default DialogConfirmation; diff --git a/web/packages/design/src/DialogConfirmation/index.js b/web/packages/design/src/DialogConfirmation/index.ts similarity index 94% rename from web/packages/design/src/DialogConfirmation/index.js rename to web/packages/design/src/DialogConfirmation/index.ts index 4a2b0c46d6057..62a4d56409401 100644 --- a/web/packages/design/src/DialogConfirmation/index.js +++ b/web/packages/design/src/DialogConfirmation/index.ts @@ -16,7 +16,7 @@ * along with this program. If not, see . */ -import DialogConfirmation from './DialogConfirmation'; +import { DialogConfirmation } from './DialogConfirmation'; import { DialogTitle, DialogContent, diff --git a/web/packages/design/src/Modal/Modal.test.tsx b/web/packages/design/src/Modal/Modal.test.tsx index f8785a5294bd8..041786aa61037 100644 --- a/web/packages/design/src/Modal/Modal.test.tsx +++ b/web/packages/design/src/Modal/Modal.test.tsx @@ -35,110 +35,126 @@ const escapeKey = { key: 'Escape', }; -describe('design/Modal', () => { - it('respects open prop set to false', () => { - render( - -
Hello
- - ); - - expect(screen.queryByRole('dialog')).not.toBeInTheDocument(); - }); +test('respects open prop set to false', () => { + render( + +
Hello
+ + ); - it('respects onBackdropClick prop', () => { - const mockFn = jest.fn(); + expect(screen.queryByRole('dialog')).not.toBeInTheDocument(); +}); - renderModal({ - onBackdropClick: mockFn, - }); +test('respects onBackdropClick prop', () => { + const mockFn = jest.fn(); - // handlebackdropClick - fireEvent.click(screen.getByTestId('backdrop')); - expect(mockFn).toHaveBeenCalled(); + renderModal({ + onBackdropClick: mockFn, }); - it('respects onEscapeKeyDown props', () => { - const mockFn = jest.fn(); + // handlebackdropClick + fireEvent.click(screen.getByTestId('backdrop')); + expect(mockFn).toHaveBeenCalled(); +}); - const { container } = renderModal({ - onEscapeKeyDown: mockFn, - }); +test('respects onEscapeKeyDown props', () => { + const mockFn = jest.fn(); - // handleDocumentKeyDown - fireEvent.keyDown(container, escapeKey); - expect(mockFn).toHaveBeenCalled(); + const { container } = renderModal({ + onEscapeKeyDown: mockFn, }); - it('respects onClose prop', () => { - const mockFn = jest.fn(); + // handleDocumentKeyDown + fireEvent.keyDown(container, escapeKey); + expect(mockFn).toHaveBeenCalled(); +}); + +test('respects onClose prop', () => { + const mockFn = jest.fn(); + + const { container } = renderModal({ + onClose: mockFn, + }); - const { container } = renderModal({ - onClose: mockFn, - }); + // handlebackdropClick + fireEvent.click(screen.getByTestId('backdrop')); + expect(mockFn).toHaveBeenCalled(); - // handlebackdropClick - fireEvent.click(screen.getByTestId('backdrop')); - expect(mockFn).toHaveBeenCalled(); + // handleDocumentKeyDown + fireEvent.keyDown(container, escapeKey); + expect(mockFn).toHaveBeenCalled(); +}); - // handleDocumentKeyDown - fireEvent.keyDown(container, escapeKey); - expect(mockFn).toHaveBeenCalled(); +test('respects hideBackDrop prop', () => { + renderModal({ + hideBackdrop: true, }); - it('respects hideBackDrop prop', () => { - renderModal({ - hideBackdrop: true, - }); + expect(screen.queryByTestId('backdrop')).not.toBeInTheDocument(); +}); - expect(screen.queryByTestId('backdrop')).not.toBeInTheDocument(); +test('respects disableBackdropClick prop', () => { + const mockFn = jest.fn(); + renderModal({ + disableBackdropClick: true, + onClose: mockFn, }); - it('respects disableBackdropClick prop', () => { - const mockFn = jest.fn(); - renderModal({ - disableBackdropClick: true, - onClose: mockFn, - }); + // handleBackdropClick + fireEvent.click(screen.getByTestId('backdrop')); + expect(mockFn).not.toHaveBeenCalled(); +}); - // handleBackdropClick - fireEvent.click(screen.getByTestId('backdrop')); - expect(mockFn).not.toHaveBeenCalled(); +test('respects disableEscapeKeyDown prop', () => { + const mockFn = jest.fn(); + const { container } = renderModal({ + disableEscapeKeyDown: true, + onClose: mockFn, }); - it('respects disableEscapeKeyDown prop', () => { - const mockFn = jest.fn(); - const { container } = renderModal({ - disableEscapeKeyDown: true, - onClose: mockFn, - }); + // handleDocumentKeyDown + fireEvent.keyDown(container, escapeKey); + expect(mockFn).not.toHaveBeenCalled(); +}); - // handleDocumentKeyDown - fireEvent.keyDown(container, escapeKey); - expect(mockFn).not.toHaveBeenCalled(); +test('unmount cleans up event listeners and closes modal', () => { + const mockFn = jest.fn(); + const { container, unmount } = renderModal({ + onEscapeKeyDown: mockFn, }); - test('unmount cleans up event listeners and closes modal', () => { - const mockFn = jest.fn(); - const { container, unmount } = renderModal({ - onEscapeKeyDown: mockFn, - }); + unmount(); - unmount(); + expect(screen.queryByTestId('Modal')).not.toBeInTheDocument(); - expect(screen.queryByTestId('Modal')).not.toBeInTheDocument(); + fireEvent.keyDown(container, escapeKey); + expect(mockFn).not.toHaveBeenCalled(); +}); - fireEvent.keyDown(container, escapeKey); - expect(mockFn).not.toHaveBeenCalled(); +test('respects backdropProps prop invisible', () => { + renderModal({ + BackdropProps: { invisible: true }, }); - test('respects backdropProps prop invisible', () => { - renderModal({ - BackdropProps: { invisible: true }, - }); - - expect(screen.getByTestId('backdrop')).toHaveStyle({ - 'background-color': 'transparent', - }); + expect(screen.getByTestId('backdrop')).toHaveStyle({ + 'background-color': 'transparent', }); }); + +test('closing dialog when keepInDOMAfterClose is set only hides it with DOM', async () => { + const { rerender } = render( + +
Hello
+ + ); + + expect(screen.getByText('Hello')).toBeVisible(); + + rerender( + +
Hello
+ + ); + + expect(screen.getByText('Hello')).not.toBeVisible(); +}); diff --git a/web/packages/teleterm/src/ui/ClusterConnect/ClusterConnect.tsx b/web/packages/teleterm/src/ui/ClusterConnect/ClusterConnect.tsx index cc92a0a57d466..250cc15a688f3 100644 --- a/web/packages/teleterm/src/ui/ClusterConnect/ClusterConnect.tsx +++ b/web/packages/teleterm/src/ui/ClusterConnect/ClusterConnect.tsx @@ -27,7 +27,10 @@ import { RootClusterUri } from 'teleterm/ui/uri'; import { ClusterAdd } from './ClusterAdd'; import { ClusterLogin } from './ClusterLogin'; -export function ClusterConnect(props: { dialog: DialogClusterConnect }) { +export function ClusterConnect(props: { + dialog: DialogClusterConnect; + hidden?: boolean; +}) { const [createdClusterUri, setCreatedClusterUri] = useState< RootClusterUri | undefined >(); @@ -52,7 +55,8 @@ export function ClusterConnect(props: { dialog: DialogClusterConnect }) { })} disableEscapeKeyDown={false} onClose={props.dialog.onCancel} - open={true} + open={!props.hidden} + keepInDOMAfterClose > {!clusterUri ? ( ({ maxWidth: '400px', diff --git a/web/packages/teleterm/src/ui/DocumentsReopen/DocumentsReopen.tsx b/web/packages/teleterm/src/ui/DocumentsReopen/DocumentsReopen.tsx index 531c0a2f5c4d6..a29795dc55e84 100644 --- a/web/packages/teleterm/src/ui/DocumentsReopen/DocumentsReopen.tsx +++ b/web/packages/teleterm/src/ui/DocumentsReopen/DocumentsReopen.tsx @@ -29,14 +29,13 @@ import { pluralize } from 'shared/utils/text'; import { RootClusterUri, routing } from 'teleterm/ui/uri'; import { useAppContext } from 'teleterm/ui/appContextProvider'; -interface DocumentsReopenProps { +export function DocumentsReopen(props: { rootClusterUri: RootClusterUri; numberOfDocuments: number; onCancel(): void; onConfirm(): void; -} - -export function DocumentsReopen(props: DocumentsReopenProps) { + hidden?: boolean; +}) { const { rootClusterUri } = props; const { clustersService } = useAppContext(); // TODO(ravicious): Use a profile name here from the URI and remove the dependency on @@ -47,7 +46,8 @@ export function DocumentsReopen(props: DocumentsReopenProps) { return ( ({ maxWidth: '400px', diff --git a/web/packages/teleterm/src/ui/HeadlessAuthn/HeadlessAuthentication.tsx b/web/packages/teleterm/src/ui/HeadlessAuthn/HeadlessAuthentication.tsx index d395dd8f9f03d..1cb1a0e5e20c4 100644 --- a/web/packages/teleterm/src/ui/HeadlessAuthn/HeadlessAuthentication.tsx +++ b/web/packages/teleterm/src/ui/HeadlessAuthn/HeadlessAuthentication.tsx @@ -36,6 +36,7 @@ interface HeadlessAuthenticationProps { skipConfirm: boolean; onCancel(): void; onSuccess(): void; + hidden?: boolean; } export function HeadlessAuthentication(props: HeadlessAuthenticationProps) { @@ -81,6 +82,7 @@ export function HeadlessAuthentication(props: HeadlessAuthenticationProps) { return (