Skip to content
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.

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

Prevent activation of previous workspace when launching Connect via deep link to a different cluster #50063

Merged
merged 23 commits into from
Dec 31, 2024
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
51c37e9
Refactor `setActiveWorkspace` to async/await
gzdunek Dec 10, 2024
f43b0ae
Keep all the logic that restores a single workspace state in `getWork…
gzdunek Dec 10, 2024
dc70bdf
Separate restoring state from setting active workspace
gzdunek Dec 10, 2024
a6bb953
Allow the dialog to reopen documents to be closed without any decision
gzdunek Dec 11, 2024
e941385
Add a test that verifies if the correct workspace is activated
gzdunek Dec 11, 2024
9fc2003
Docs improvements
gzdunek Dec 13, 2024
cc2644a
Return early if there's no restoredWorkspace
gzdunek Dec 13, 2024
3dde323
Fix logger name
gzdunek Dec 13, 2024
8c4dbec
Improve test name
gzdunek Dec 13, 2024
8776788
Make restored state immutable
gzdunek Dec 16, 2024
f5be332
Fix comment
gzdunek Dec 17, 2024
35194aa
Do not wait for functions to be called in tests
gzdunek Dec 17, 2024
706b0f0
Add tests for discarding documents reopen dialog
gzdunek Dec 17, 2024
4bb34e3
Move setting `isInitialized` to a separate method
gzdunek Dec 17, 2024
23f82b8
Remove restored workspace when logging out so that we won't try to re…
gzdunek Dec 17, 2024
26c1dac
Do not try to restore previous documents if the user already opened n…
gzdunek Dec 17, 2024
9e810c1
Do not close dialog in test
gzdunek Dec 20, 2024
0753223
Improve `isInitialized` comment
gzdunek Dec 20, 2024
e275bbb
Call `setActiveWorkspace` again when we fail to change the workspace
gzdunek Dec 20, 2024
e339d09
Fix the logic around restoring previous documents
gzdunek Dec 20, 2024
c20ec38
Try to reopen documents also after `cluster-connect` is canceled
gzdunek Dec 30, 2024
a9bec27
canRestoreDocuments -> hasDocumentsToReopen
gzdunek Dec 30, 2024
aef61d0
Disallow parallel `setActiveWorkspace` calls (#50384)
gzdunek Dec 31, 2024
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
183 changes: 183 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,183 @@
/**
* 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 { render } from 'design/utils/testing';
import { screen, act, waitFor } from '@testing-library/react';
import userEvent from '@testing-library/user-event';

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

import { AppInitializer } from './AppInitializer';

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}
>
<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],
})
);
jest.spyOn(appContext.modalsService, 'openRegularDialog');
const userInterfaceReady = withPromiseResolver();
jest
.spyOn(appContext.mainProcessClient, 'signalUserInterfaceReadiness')
.mockImplementation(() => userInterfaceReady.resolve());

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

// Wait for the app to finish initialization.
await act(() => userInterfaceReady.promise);
// 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 cluster-connect dialog should be opened two times.
// The first one comes from restoring the previous session, but it is
// immediately canceled and replaced with a dialog to the cluster from
// the deep link.
await waitFor(
gzdunek marked this conversation as resolved.
Show resolved Hide resolved
() => {
expect(appContext.modalsService.openRegularDialog).toHaveBeenCalledTimes(
2
);
},
// A small timeout to prevent potential race conditions.
{ timeout: 10 }
);
expect(appContext.modalsService.openRegularDialog).toHaveBeenNthCalledWith(
1,
expect.objectContaining({
kind: 'cluster-connect',
clusterUri: previouslyActiveCluster.uri,
})
);
expect(appContext.modalsService.openRegularDialog).toHaveBeenNthCalledWith(
2,
expect.objectContaining({
kind: 'cluster-connect',
clusterUri: deepLinkCluster.uri,
})
);

// We blindly 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();
});

//TODO(gzdunek): Replace with Promise.withResolvers after upgrading to Node.js 22.
function withPromiseResolver() {
let resolver: () => void;
const promise = new Promise<void>(resolve => (resolver = resolve));
return {
resolve: resolver,
promise,
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,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 @@ -33,7 +33,7 @@ export const Story = () => {
rootClusterUri="/clusters/foo.cloud.gravitational.io"
numberOfDocuments={8}
onConfirm={() => {}}
onCancel={() => {}}
onDiscard={() => {}}
/>
</MockAppContextProvider>
);
Expand All @@ -46,7 +46,7 @@ export const OneTab = () => {
rootClusterUri="/clusters/foo.cloud.gravitational.io"
numberOfDocuments={1}
onConfirm={() => {}}
onCancel={() => {}}
onDiscard={() => {}}
/>
</MockAppContextProvider>
);
Expand All @@ -59,7 +59,7 @@ export const LongClusterName = () => {
rootClusterUri="/clusters/foo.bar.baz.quux.cloud.gravitational.io"
numberOfDocuments={42}
onConfirm={() => {}}
onCancel={() => {}}
onDiscard={() => {}}
/>
</MockAppContextProvider>
);
Expand All @@ -75,7 +75,7 @@ export const LongContinuousClusterName = () => {
.join('')}`}
numberOfDocuments={680}
onConfirm={() => {}}
onCancel={() => {}}
onDiscard={() => {}}
/>
</MockAppContextProvider>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ import { useAppContext } from 'teleterm/ui/appContextProvider';
export function DocumentsReopen(props: {
rootClusterUri: RootClusterUri;
numberOfDocuments: number;
onCancel(): void;
onDiscard(): void;
onConfirm(): void;
hidden?: boolean;
}) {
Expand All @@ -50,7 +50,7 @@ export function DocumentsReopen(props: {
<DialogConfirmation
open={!props.hidden}
keepInDOMAfterClose
onClose={props.onCancel}
onClose={props.onDiscard}
dialogCss={() => ({
maxWidth: '400px',
width: '100%',
Expand All @@ -70,7 +70,7 @@ export function DocumentsReopen(props: {
<H2 mb={4}>Reopen previous session</H2>
<ButtonIcon
type="button"
onClick={props.onCancel}
onClick={props.onDiscard}
color="text.slightlyMuted"
>
<Cross size="medium" />
Expand Down Expand Up @@ -105,7 +105,7 @@ export function DocumentsReopen(props: {
<ButtonPrimary autoFocus mr={3} type="submit">
Reopen
</ButtonPrimary>
<ButtonSecondary type="button" onClick={props.onCancel}>
<ButtonSecondary type="button" onClick={props.onDiscard}>
Start New Session
</ButtonSecondary>
</DialogFooter>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ const documentsReopenDialog: DialogDocumentsReopen = {
rootClusterUri: '/clusters/foo',
numberOfDocuments: 1,
onConfirm: () => {},
onDiscard: () => {},
onCancel: () => {},
};

Expand Down
4 changes: 2 additions & 2 deletions web/packages/teleterm/src/ui/ModalsHost/ModalsHost.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -120,9 +120,9 @@ function renderDialog({
hidden={hidden}
rootClusterUri={dialog.rootClusterUri}
numberOfDocuments={dialog.numberOfDocuments}
onCancel={() => {
onDiscard={() => {
handleClose();
dialog.onCancel();
dialog.onDiscard();
}}
onConfirm={() => {
handleClose();
Expand Down
2 changes: 1 addition & 1 deletion web/packages/teleterm/src/ui/StatusBar/StatusBar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ export function StatusBar() {
css={`
white-space: nowrap;
`}
title={clusterBreadcrumbs}
title={clusterBreadcrumbs && `Current cluster: ${clusterBreadcrumbs}`}
>
{clusterBreadcrumbs}
</Text>
Expand Down
3 changes: 2 additions & 1 deletion web/packages/teleterm/src/ui/appContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -176,8 +176,9 @@ export default class AppContext implements IAppContext {

this.subscribeToDeepLinkLaunch();
this.notifyMainProcessAboutClusterListChanges();
this.clustersService.syncGatewaysAndCatchErrors();
void this.clustersService.syncGatewaysAndCatchErrors();
await this.clustersService.syncRootClustersAndCatchErrors();
this.workspacesService.restorePersistedState();
Copy link
Member

Choose a reason for hiding this comment

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

If I close the app while I'm in workspace A with a valid cert and open it again through a deep link to workspace B, I can close the login modal and continue to use workspace A without making a decision wrt restored state. The user should be forced to make an action before interacting with a workspace.

valid-cert-on-launch.mov

How do we fix this? I almost feel like DeepLinksService should always bail out to the "Connect" screen. As in, when loginAndSetActiveWorkspace is called, we check if the current workspace is equal to the workspace from the URL. If not, then we always zero rootClusterUri in WorkspacesService to force the user back to cluster choice. Otherwise, I feel it kind of makes little sense to launch the app through a link to workspace B, interact with a modal about workspace B (the login modal), then close it and be back at workspace A, just because it was the last used workspace before the app was closed.

I'm just not sure at this point how this is going to interact with the rest of the app.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At first it seemed like a good idea, but then I realized that setting rootClusterUri to undefined unmounts the workspaces, so all the state kept in tabs will be lost :/ And since a deep link can be opened while you work on something, you would loose it.

Because of that, I decided to keep it as is.

Copy link
Member

Choose a reason for hiding this comment

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

At first it seemed like a good idea, but then I realized that setting rootClusterUri to undefined unmounts the workspaces, so all the state kept in tabs will be lost :/

That sounds like a behavior that we can change. But I'll look at the changes in this PR and the other PR first.

}

/**
Expand Down
2 changes: 2 additions & 0 deletions web/packages/teleterm/src/ui/services/modals/modalsService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,8 @@ export interface DialogDocumentsReopen {
rootClusterUri: RootClusterUri;
numberOfDocuments: number;
onConfirm?(): void;
onDiscard?(): void;
/** Cancels the dialog, without discarding documents. */
onCancel?(): void;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,10 @@ export type WorkspacesPersistedState = Omit<
WorkspacesState,
'workspaces' | 'isInitialized'
> & {
workspaces: Record<string, Omit<Workspace, 'accessRequests'>>;
workspaces: Record<
string,
Omit<Workspace, 'accessRequests' | 'documentsRestoredOrDiscarded'>
>;
};

export interface StatePersistenceState {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,19 +85,12 @@ export class DocumentsService {
};
}

/** @deprecated Use createClusterDocument function instead of the method on DocumentsService. */
createClusterDocument(opts: {
clusterUri: uri.ClusterUri;
queryParams?: DocumentClusterQueryParams;
}): DocumentCluster {
const uri = routing.getDocUri({ docId: unique() });
const clusterName = routing.parseClusterName(opts.clusterUri);
return {
uri,
clusterUri: opts.clusterUri,
title: clusterName,
kind: 'doc.cluster',
queryParams: opts.queryParams || getDefaultDocumentClusterQueryParams(),
};
return createClusterDocument(opts);
}

/**
Expand Down Expand Up @@ -509,6 +502,21 @@ export class DocumentsService {
}
}

export function createClusterDocument(opts: {
clusterUri: uri.ClusterUri;
queryParams?: DocumentClusterQueryParams;
}): DocumentCluster {
const uri = routing.getDocUri({ docId: unique() });
const clusterName = routing.parseClusterName(opts.clusterUri);
return {
uri,
clusterUri: opts.clusterUri,
title: clusterName,
kind: 'doc.cluster',
queryParams: opts.queryParams || getDefaultDocumentClusterQueryParams(),
};
}

export function getDefaultDocumentClusterQueryParams(): DocumentClusterQueryParams {
return {
resourceKinds: [],
Expand Down
Loading
Loading