Connect tries to login to previous session instead of logging in to the DeviceWebToken's session

[codingllama]

This issue is within the context of device trust web. A small explanation: when doing device trust web, the Web UI attempts to open a teleport:// URL. That URL has most of the information required to start a login, namely the username and the proxy address.

Let's say Connect has an expired session for "cluster1", but doesn't know about "cluster2". We are running device web authentication against "cluster2", thus opening Connect via the corresponding "teleport://" URL.

Expected behavior:

Connect is expected to ignore the expired session for "cluster1" and attempt to login directly to "cluster2", as informed by the teleport:// URL.

Current behavior:

Connect attempts to re-connect to the expired session for "cluster1" first. After that is completed or canceled, then it shows the popup to authenticate for "cluster2".

To repro, do the following:

1. Remove all clusters from Connect
2. Login to "cluster1"
3. Close connect
4. Delete all keys for "cluster1" to simulate an expired credential (eg, rm -fr ~/Library/Application\ Support/Teleport\ Connect/tsh/keys/cluster1/)
5. Open a device authentication URL for "cluster2" (eg, open 'teleport://alpaca@cluster2:443/authenticate_web_device?id=4fbab9eb-568e-437b-ab89-ca45b843e8aa&token=YqkvcRfkZPsRHg4qiFz9UayN1zMJ97kcTwjjpiABEBQ')

Connect opens and immediately attempts to connect to "cluster1" (zarq in the screenshot):

After canceled (ESC), it shows the popup to connect to "cluster2":

Bug details:

• Teleport version: master
• Recreation steps: see above
• Debug logs: not necessary

[codingllama]

As @avatus put it, in much fewer words:

1. Connect opens and tries to open any sessions
2. Sees the expired session and tries to reauth
3. Now that its done "starting", it opens the deep link

[codingllama]

It's probably the same, but we should also consider what happens with distinct users for the same cluster.

[ravicious]

Launching a deep link waits until the frontend app is ready:

teleport/web/packages/teleterm/src/mainProcess/windowsManager.ts

Lines 180 to 184 in 606bcb1

	async launchDeepLink(
	deepLinkParseResult: DeepLinkParseResult
	): Promise<void> {
	try {
	await this.whenFrontendAppIsReady();

Currently the readiness is signaled after WorkspacesService.prototype.restorePersistedState resolves, which includes a call to restore the previous workspace. This is what triggers the login modal for the previous cluster and waits until that modal is closed.

teleport/web/packages/teleterm/src/ui/AppInitializer/AppInitializer.tsx

Lines 40 to 43 in 606bcb1

	await showStartupModalsAndNotifications(appContext);
	appContext.mainProcessClient.signalUserInterfaceReadiness({
	success: true,
	});

teleport/web/packages/teleterm/src/ui/AppInitializer/showStartupModalsAndNotifications.ts

Lines 28 to 51 in 606bcb1

	/**
	* Runs after the UI becomes visible.
	*/
	export async function showStartupModalsAndNotifications(
	ctx: IAppContext
	): Promise<void> {
	const { configService } = ctx.mainProcessClient;
	await askAboutUserJobRoleIfNeeded(
	ctx.statePersistenceService,
	configService,
	ctx.modalsService,
	ctx.usageService
	);
	// Setting up usage reporting after asking for a job role prevents a situation
	// where these dialogs are shown one after another.
	// Instead, on the first launch only "usage reporting" dialog shows up.
	// "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();

The solution would be to:

1. Move this.setActiveWorkspace out of WorkspacesService.prototype.restorePersistedState into showStartupModalsAndNotifications. (restorePersistedState is called only once in the app).
2. Signal readiness after restorePersistedState but before setActiveWorkspace.
   • Similarly, isInitialized of WorkspacesService should be set to true after restorePersistedState but before setActiveWorkspace. This affects VNet only though, not deep links.
3. Make setActiveWorkspace accept a signal.
4. WindowsManagerIpc.SignalUserInterfaceReadiness should return an AbortController that aborts the signal passed to setActiveWorkspace in showStartupModalsAndNotifications.
5. WindowsManager.prototype.launchDeepLink should pass the abort controller to DeepLinksService.prototype.launchDeepLink in the render process.
   • So far, each deep link handler already sets the correct workspace by calling loginAndSetActiveWorkspace. That method should abort the abort controller and show the modal for the right workspace. We cannot always abort the controller, because in the future there might be deep links that are not related to a particular workspace.

Instead of passing an abort controller back and forth between processes through WindowsManagerIpc.SignalUserInterfaceReadiness, perhaps we can create a single abort controller in AppContext (called, idk, restoreWorkspaceAbortController) and then pass it to both DeepLinksService and showStartupModalsAndNotifications.

[ravicious]

It's probably the same, but we should also consider what happens with distinct users for the same cluster.

I believe #45714 is now tracking this.

[gzdunek]

Thanks for the solution @ravicious, it really helped.

However, it seems to me that we can simplify it. The most important thing is separating restorePersistedState and setActiveWorkspace. restorePersistedState can now be called in AppContext.pullInitialState and setActiveWorkspace as the last instruction in AppInitializer.initializeApp (so no other code will wait for it to complete). This will unblock launchDeepLink, allowing it to display its dialogs without abort signals.

Why? Because ModalsService.openRegularDialog cancels a previous dialog (so we don't need to explicitly cancel setActiveWorkspace). I think this solution is actually better because it's more universal. Even now, if you open a dialog to connect to a cluster, and without closing it, launch a deep link, the dialog is replaced with a dialog from the deep link. We can reuse this existing mechanism, instead of introducing a separate one, used only when the app is launched.

[ravicious]

I think it makes sense. With this part of the codebase, it's hard to be 100% sure that it's not going to cause any unforeseen issues. But after going through the code it seems okay.

The only potential issue that could arise in the future would be two pieces of code that need to display a regular modal after the launch of the app. If we control when that code is fired, then it's not that big of a problem, because we can coordinate this in showStartupModalsAndNotifications. It'd be more of a problem if it's something the app doesn't directly control, like deep links that can be triggered at any time.

---

Though there's one piece of code which should run after setActiveWorkspace completes and that is appContext.mainProcessClient.signalUserInterfaceReadiness. As soon as UI readiness is signaled, VNet will be started. Unlike Connect My Computer, VNet is not dependent on any particular workspace. VNet doesn't need to wait for a user to log in to a workspace. It's just that if the user is not logged in to any workspace, VNet is kind of useless. So we just prefer to wait for the user to go through the modal first.

Once setActiveWorkspace completes (no matter if successfully or not), UI readiness should be signaled. If the initial dialog is closed by a deep link launch, then in this context this is not much different than the user manually closing the initial login dialog to log in to another workspace.

It'd be good to have test for that, as the correlation between this and VNet is rather convoluted. ;f

---

One other thing occurred to me while writing this: what about a situation where the app is launched from a deep link to workspace B, while the current workspace A has a valid cert and some tabs to reopen? In this case, there's no login dialog for workspace A, but there's a dialog asking about reopening documents from A, which would be immediately canceled (thus discarding any old tabs) in favor of displaying a login modal for B.

I suppose that's why I initially opted for "aborting" the process of setting a workspace instead of just straight up cancelling it (and thus running logic associated with cancelling a modal).

Maybe documents-reopen could be adjusted so that there are three actions: confirm, discard, and cancel. What currently happens in onCancel would happen in onDiscard. onCancel would merely cause the dialog to be closed or whatever and the user would see it again the next time they navigate to the workspace.

The user would never be able to call onCancel from the UI: interacting with the modal in any way should call onDiscard instead. This is to guarantee that after the dialog is closed through user interaction, we discard the documents from the previous session and no longer have to take them into consideration.

[gzdunek]

Though there's one piece of code which should run after setActiveWorkspace completes and that is appContext.mainProcessClient.signalUserInterfaceReadiness. As soon as UI readiness is signaled, VNet will be started. Unlike Connect My Computer, VNet is not dependent on any particular workspace. VNet doesn't need to wait for a user to log in to a workspace. It's just that if the user is not logged in to any workspace, VNet is kind of useless. So we just prefer to wait for the user to go through the modal first.

Once setActiveWorkspace completes (no matter if successfully or not), UI readiness should be signaled. If the initial dialog is closed by a deep link launch, then in this context this is not much different than the user manually closing the initial login dialog to log in to another workspace.

Hmm from what I see, appContext.mainProcessClient.signalUserInterfaceReadiness is used only by deep links, not by Vnet. Also, it can't wait until setActiveWorkspace completes since it would block changing the workspace to the one from a deep link.
I believe you meant workspaces.isIntialized which is used by VNet. But now I'm wondering, how important is to wait for setActiveWorkspace to complete?

It's just that if the user is not logged in to any workspace, VNet is kind of useless. So we just prefer to wait for the user to go through the modal first.

But does this code actually work? If I don't have any workspace, or just cancel the modal, VNet starts anyway. So what does waiting for setActiveWorkspace give us?

    this.setState(draftState => {
      draftState.workspaces = restoredWorkspaces;
    });

    if (persistedState.rootClusterUri) {
      await this.setActiveWorkspace(persistedState.rootClusterUri);
    }

    this.setState(draft => {
      draft.isInitialized = true;
    });
  }

Maybe we could simplify it to:

    this.setState(draftState => {
      draftState.workspaces = restoredWorkspaces;
      draftState.isInitialized = true;
    });