Skip to content

Commit

Permalink
Prevent activation of previous workspace when launching Connect via d…
Browse files Browse the repository at this point in the history
…eep link to a different cluster (#50063) (#50732)

* Refactor `setActiveWorkspace` to async/await

* Keep all the logic that restores a single workspace state in `getWorkspaceDefaultState`

* Separate restoring state from setting active workspace

* Allow the dialog to reopen documents to be closed without any decision

* Add a test that verifies if the correct workspace is activated

* Docs improvements

* Return early if there's no restoredWorkspace

* Fix logger name

* Improve test name

* Make restored state immutable

* Fix comment

* Do not wait for functions to be called in tests

* Add tests for discarding documents reopen dialog

* Move setting `isInitialized` to a separate method

* Remove restored workspace when logging out so that we won't try to restore it when logging in again

* Do not try to restore previous documents if the user already opened new ones

* Do not close dialog in test

* Improve `isInitialized` comment

* Call `setActiveWorkspace` again when we fail to change the workspace

* Fix the logic around restoring previous documents

* Try to reopen documents also after `cluster-connect` is canceled

* canRestoreDocuments -> hasDocumentsToReopen

* Disallow parallel `setActiveWorkspace` calls (#50384)

* Allow passing abortSignal to `open*Modal` methods, allow closing everything at once

* Close all dialogs when activating a deep link

* Allow only one `setActiveWorkspace` execution at a time

* Review comments

* Always unregister event listeners when a dialog is closed

* Lint

* Add a diagram for launching a deep link

* Remove checking if the signal is aborted in `openImportantDialog`, rename it

(cherry picked from commit 8e9b004)
  • Loading branch information
gzdunek authored Jan 13, 2025
1 parent 1dbd00d commit b570e4e
Show file tree
Hide file tree
Showing 20 changed files with 959 additions and 303 deletions.
8 changes: 8 additions & 0 deletions web/packages/build/jest/jest-environment-patched-jsdom.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,14 @@ export default class PatchedJSDOMEnvironment extends JSDOMEnvironment {
dispatchEvent: () => {},
});
}

// TODO(gzdunek): JSDOM doesn't support AbortSignal.any().
// Overwriting only this function doesn't help much, something between
// AbortSignal and AbortController is missing.
if (!global.AbortSignal.any) {
global.AbortSignal = AbortSignal;
global.AbortController = AbortController;
}
}
}
export const TestEnvironment = PatchedJSDOMEnvironment;
45 changes: 45 additions & 0 deletions web/packages/teleterm/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -249,3 +249,48 @@ resource availability as possible.
### PTY communication overview (Renderer Process <=> Shared Process)

![PTY communication](docs/ptyCommunication.png)

### Overview of a deep link launch process

The diagram below illustrates the process of launching a deep link,
depending on the state of the workspaces.
It assumes that the app is not running and that the deep link targets a workspace
different from the persisted one.

<details>
<summary>Diagram</summary>

```mermaid
flowchart TD
Start([Start]) --> IsPreviousWorkspaceConnected{Is the previously active workspace connected?}
IsPreviousWorkspaceConnected --> |Valid certificate| PreviousWorkspaceReopenDocuments{Has documents to reopen from the previous workspace?}
IsPreviousWorkspaceConnected --> |Expired certificate| CancelPreviousWorkspaceLogin[Cancel the login dialog]
IsPreviousWorkspaceConnected --> |No persisted workspace| SwitchWorkspace
PreviousWorkspaceReopenDocuments --> |Yes| CancelPreviousWorkspaceDocumentsReopen[Cancel the reopen dialog without discarding documents]
PreviousWorkspaceReopenDocuments --> |No| SwitchWorkspace[Switch to a deep link workspace]
CancelPreviousWorkspaceDocumentsReopen --> SwitchWorkspace
CancelPreviousWorkspaceLogin --> SwitchWorkspace
SwitchWorkspace --> IsDeepLinkWorkspaceConnected{Is the deep link workspace connected?}
IsDeepLinkWorkspaceConnected --> |Valid certificate| DeepLinkWorkspaceReopenDocuments{Has documents to reopen from the deep link workspace?}
IsDeepLinkWorkspaceConnected --> |Not added| AddDeepLinkCluster[Add new cluster]
IsDeepLinkWorkspaceConnected --> |Expired certificate| LogInToDeepLinkWorkspace[Log in to workspace]
AddDeepLinkCluster --> AddDeepLinkClusterSuccess{Was the cluster added successfully?}
AddDeepLinkClusterSuccess --> |Yes| LogInToDeepLinkWorkspace
AddDeepLinkClusterSuccess --> |No| ReturnToPreviousWorkspace[Return to the previously active workspace and try to reopen its documents again]
LogInToDeepLinkWorkspace --> IsLoginToDeepLinkWorkspaceSuccess{Was login successful?}
IsLoginToDeepLinkWorkspaceSuccess --> |Yes| DeepLinkWorkspaceReopenDocuments
IsLoginToDeepLinkWorkspaceSuccess --> |No| ReturnToPreviousWorkspace
DeepLinkWorkspaceReopenDocuments --> |Yes| ReopenDeepLinkWorkspaceDocuments[Reopen documents]
DeepLinkWorkspaceReopenDocuments --> |No| End
ReopenDeepLinkWorkspaceDocuments --> End
ReturnToPreviousWorkspace --> End
```

</details>
248 changes: 248 additions & 0 deletions web/packages/teleterm/src/ui/AppInitializer/AppInitializer.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,248 @@
/**
* Teleport
* Copyright (C) 2024 Gravitational, Inc.
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

import 'jest-canvas-mock';

import { act, screen } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import { mockIntersectionObserver } from 'jsdom-testing-mocks';

import { render } from 'design/utils/testing';

import Logger, { NullService } from 'teleterm/logger';
import { MockedUnaryCall } from 'teleterm/services/tshd/cloneableClient';
import { makeRootCluster } from 'teleterm/services/tshd/testHelpers';
import { ResourcesContextProvider } from 'teleterm/ui/DocumentCluster/resourcesContext';
import { MockAppContextProvider } from 'teleterm/ui/fixtures/MockAppContextProvider';
import { MockAppContext } from 'teleterm/ui/fixtures/mocks';
import { ConnectionsContextProvider } from 'teleterm/ui/TopBar/Connections/connectionsContext';
import { IAppContext } from 'teleterm/ui/types';
import { VnetContextProvider } from 'teleterm/ui/Vnet';

import { AppInitializer } from './AppInitializer';

mockIntersectionObserver();
beforeAll(() => {
Logger.init(new NullService());
});

jest.mock('teleterm/ui/ClusterConnect', () => ({
ClusterConnect: props => (
<div
data-testid="mocked-dialog"
data-dialog-kind="cluster-connect"
data-dialog-is-hidden={props.hidden}
>
Connect to {props.dialog.clusterUri}
<button onClick={props.dialog.onSuccess}>Connect to cluster</button>
</div>
),
}));

test('activating a workspace via deep link overrides the previously active workspace', async () => {
// Before closing the app, both clusters were present in the state, with previouslyActiveCluster being active.
// However, the user clicked a deep link pointing to deepLinkCluster.
// The app should prioritize the user's intent by activating the workspace for the deep link,
// rather than reactivating the previously active cluster.
const previouslyActiveCluster = makeRootCluster({
uri: '/clusters/teleport-previously-active',
proxyHost: 'teleport-previously-active:3080',
name: 'teleport-previously-active',
connected: false,
});
const deepLinkCluster = makeRootCluster({
uri: '/clusters/teleport-deep-link',
proxyHost: 'teleport-deep-link:3080',
name: 'teleport-deep-link',
connected: false,
});
const appContext = new MockAppContext();
jest
.spyOn(appContext.statePersistenceService, 'getWorkspacesState')
.mockReturnValue({
rootClusterUri: previouslyActiveCluster.uri,
workspaces: {
[previouslyActiveCluster.uri]: {
localClusterUri: previouslyActiveCluster.uri,
documents: [],
location: undefined,
},
[deepLinkCluster.uri]: {
localClusterUri: deepLinkCluster.uri,
documents: [],
location: undefined,
},
},
});
appContext.mainProcessClient.configService.set(
'usageReporting.enabled',
false
);
jest.spyOn(appContext.tshd, 'listRootClusters').mockReturnValue(
new MockedUnaryCall({
clusters: [deepLinkCluster, previouslyActiveCluster],
})
);

render(
<MockAppContextProvider appContext={appContext}>
<ConnectionsContextProvider>
<VnetContextProvider>
<ResourcesContextProvider>
<AppInitializer />
</ResourcesContextProvider>
</VnetContextProvider>
</ConnectionsContextProvider>
</MockAppContextProvider>
);

expect(
await screen.findByText(`Connect to ${previouslyActiveCluster.uri}`)
).toBeInTheDocument();

// Launch a deep link and do not wait for the result.
act(() => {
void appContext.deepLinksService.launchDeepLink({
status: 'success',
url: {
host: deepLinkCluster.proxyHost,
hostname: deepLinkCluster.name,
port: '1234',
pathname: '/authenticate_web_device',
username: deepLinkCluster.loggedInUser.name,
searchParams: {
id: '123',
redirect_uri: '',
token: 'abc',
},
},
});
});

// The previous dialog has been replaced without a user interaction.
// In the real app, this happens fast enough that the user doesn't see the previous dialog.
expect(
await screen.findByText(`Connect to ${deepLinkCluster.uri}`)
).toBeInTheDocument();

// We confirm the current cluster-connect dialog.
const dialogSuccessButton = await screen.findByRole('button', {
name: 'Connect to cluster',
});
await userEvent.click(dialogSuccessButton);

// Check if the first activated workspace is the one from the deep link.
expect(await screen.findByTitle(/Current cluster:/)).toBeVisible();
expect(
screen.queryByTitle(`Current cluster: ${deepLinkCluster.name}`)
).toBeVisible();
});

test.each<{
name: string;
action(appContext: IAppContext): Promise<void>;
expectHasDocumentsToReopen: boolean;
}>([
{
name: 'closing documents reopen dialog via close button discards previous documents',
action: async () => {
await userEvent.click(await screen.findByTitle('Close'));
},
expectHasDocumentsToReopen: false,
},
{
name: 'starting new session in document reopen dialog discards previous documents',
action: async () => {
await userEvent.click(
await screen.findByRole('button', { name: 'Start New Session' })
);
},
expectHasDocumentsToReopen: false,
},
{
name: 'overwriting document reopen dialog with another regular dialog does not discard documents',
action: async appContext => {
act(() => {
appContext.modalsService.openRegularDialog({
kind: 'change-access-request-kind',
onConfirm() {},
onCancel() {},
});
});
},
expectHasDocumentsToReopen: true,
},
])('$name', async testCase => {
const rootCluster = makeRootCluster();
const appContext = new MockAppContext();
jest
.spyOn(appContext.statePersistenceService, 'getWorkspacesState')
.mockReturnValue({
rootClusterUri: rootCluster.uri,
workspaces: {
[rootCluster.uri]: {
localClusterUri: rootCluster.uri,
documents: [
{
kind: 'doc.access_requests',
uri: '/docs/123',
state: 'browsing',
clusterUri: rootCluster.uri,
requestId: '',
title: 'Access Requests',
},
],
location: undefined,
},
},
});
appContext.mainProcessClient.configService.set(
'usageReporting.enabled',
false
);
jest.spyOn(appContext.tshd, 'listRootClusters').mockReturnValue(
new MockedUnaryCall({
clusters: [rootCluster],
})
);

render(
<MockAppContextProvider appContext={appContext}>
<ConnectionsContextProvider>
<VnetContextProvider>
<ResourcesContextProvider>
<AppInitializer />
</ResourcesContextProvider>
</VnetContextProvider>
</ConnectionsContextProvider>
</MockAppContextProvider>
);

expect(
await screen.findByText(
'Do you want to reopen tabs from the previous session?'
)
).toBeInTheDocument();

await testCase.action(appContext);

expect(
appContext.workspacesService.getWorkspace(rootCluster.uri)
.hasDocumentsToReopen
).toBe(testCase.expectHasDocumentsToReopen);
});
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,13 @@ export const AppInitializer = () => {
await appContext.pullInitialState();
setShouldShowUi(true);
await showStartupModalsAndNotifications(appContext);
// If there's a workspace that was active before closing the app,
// activate it.
const rootClusterUri =
appContext.workspacesService.getRestoredState()?.rootClusterUri;
if (rootClusterUri) {
void appContext.workspacesService.setActiveWorkspace(rootClusterUri);
}
appContext.mainProcessClient.signalUserInterfaceReadiness({
success: true,
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,6 @@ export async function showStartupModalsAndNotifications(
// "User job role" dialog is shown on the second launch (only if user agreed to reporting earlier).
await setUpUsageReporting(configService, ctx.modalsService);

// If there's a workspace that was active before the user closed the app, restorePersistedState
// will block until the user interacts with the login modal (if the cert is not valid anymore) and
// the modal for restoring documents.
await ctx.workspacesService.restorePersistedState();

notifyAboutConfigErrors(configService, ctx.notificationsService);
notifyAboutDuplicatedShortcutsCombinations(
ctx.keyboardShortcutsService,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ export const Story = () => {
rootClusterUri="/clusters/foo.cloud.gravitational.io"
numberOfDocuments={8}
onConfirm={() => {}}
onCancel={() => {}}
onDiscard={() => {}}
/>
</MockAppContextProvider>
);
Expand All @@ -44,7 +44,7 @@ export const OneTab = () => {
rootClusterUri="/clusters/foo.cloud.gravitational.io"
numberOfDocuments={1}
onConfirm={() => {}}
onCancel={() => {}}
onDiscard={() => {}}
/>
</MockAppContextProvider>
);
Expand All @@ -57,7 +57,7 @@ export const LongClusterName = () => {
rootClusterUri="/clusters/foo.bar.baz.quux.cloud.gravitational.io"
numberOfDocuments={42}
onConfirm={() => {}}
onCancel={() => {}}
onDiscard={() => {}}
/>
</MockAppContextProvider>
);
Expand All @@ -73,7 +73,7 @@ export const LongContinuousClusterName = () => {
.join('')}`}
numberOfDocuments={680}
onConfirm={() => {}}
onCancel={() => {}}
onDiscard={() => {}}
/>
</MockAppContextProvider>
);
Expand Down
Loading

0 comments on commit b570e4e

Please sign in to comment.