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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
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(
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't we wait here for the connect-cluster dialog to be opened with the correct content rather than doing waitFor for a mocked function? As in expect(screen.findByText(<whatever is shown in the modal>)). If possible, we shouldn't use toHaveBeenCalled as something that moves tests forward, but rather the actual results that the action has on the interface that the user sees.

() => {
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.

}

/**
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 directly. */
gzdunek marked this conversation as resolved.
Show resolved Hide resolved
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