Skip to content

Commit

Permalink
Connect: Implement tshd event handlers for db cert renewal (#1383)
Browse files Browse the repository at this point in the history
* tshd events: Wait for listeners before responding

When tshd is sending the relogin event, it needs to be able to know when
the Electron app has finished relogging the user. I wanted to implement
this by simply waiting for the response from the RPC.

I'm so glad we did not use gRPC streams for tshd events as this would be
much harder to implement with streams.

* Return a function from ModalsService.openDialog which closes dialog

With the introduction of important and regular modals, this will help us
close the specific dialog if need arises.

* Make tshd events listeners aware of request cancellation

This will be useful in the upcoming commits. Basically, tshd is going to
ask the Electron app to relogin the user, with a 1 minute timeout.
The Electron app will show a login modal but if the user doesn't submit
it within 1 minute, tshd is going to cancel the request.

In that situation, we need to be able to close the modal.

* Add support for passing reason in DialogClusterConnect

* Add support for important modals

This will let us show the relogin modal on expired cert, even if the user
was using some other modal at that moment.

* Remove title attr from notification text

The user can read more by expanding the notification. The title attribute
persisted even after expanding the notification, making reading it harder.

* Add WindowsManager.forceFocusWindow

* Use IAppContext instead of AppContext

The next commit is going to add a private method to AppContext.
IAppContext is an interface which enables us to pass a mocked version of
AppContext in tests. That mock is not going have that private method,
so any place accepting AppContext wouldn't be able to accept the mocked
AppContext.

Instead, classes & functions should accept IAppContext rather than AppContext.

* Implement handlers for new tshd events

tshd needs to be able to do two things:

- Ask the user to log in again.
- Forward errors from goroutines running gateways to the Electron app
  in form of a notification. Otherwise those error would be visible
  only in the logs.

* Don't restart gateways after logging in

Restarting the gateways on login was a workaround from times where gateways
didn't manage their own certs.

In the new flow, a gateway takes care of refreshing the certs itself
through the middleware passed to alpnproxy.LocalProxy.

syncRootClusterAndRestartClusterGatewaysAndCatchErrors used to call two
functions:

- syncRootClusterAndCatchErrors
- restartClusterGatewaysAndCatchErrors

The second function is no longer necessary, so we can make any place that
was calling syncRootClusterAndRestartClusterGatewaysAndCatchErrors
call just syncRootClusterAndCatchErrors instead.
  • Loading branch information
ravicious authored Dec 6, 2022
1 parent dc0335a commit eafdaf0
Show file tree
Hide file tree
Showing 38 changed files with 1,814 additions and 286 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,6 @@ function getRenderedContent(
</div>
<Text
fontSize={13}
title={content.description}
lineHeight={20}
color="text.secondary"
css={longerTextCss}
Expand Down
1 change: 1 addition & 0 deletions web/packages/teleterm/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
"private": true,
"homepage": "https://goteleport.com",
"dependencies": {
"emittery": "^1.0.1",
"node-pty": "0.10.0"
},
"devDependencies": {
Expand Down
1 change: 1 addition & 0 deletions web/packages/teleterm/src/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ function initializeApp(): void {
logger,
configService,
fileStorage,
windowsManager,
});

app.on(
Expand Down
2 changes: 2 additions & 0 deletions web/packages/teleterm/src/mainProcess/fixtures/mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,4 +56,6 @@ export class MockMainProcessClient implements MainProcessClient {
removeKubeConfig(): Promise<void> {
return Promise.resolve(undefined);
}

forceFocusWindow() {}
}
8 changes: 8 additions & 0 deletions web/packages/teleterm/src/mainProcess/mainProcess.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,14 @@ import {
import { subscribeToTerminalContextMenuEvent } from './contextMenus/terminalContextMenu';
import { subscribeToTabContextMenuEvent } from './contextMenus/tabContextMenu';
import { resolveNetworkAddress } from './resolveNetworkAddress';
import { WindowsManager } from './windowsManager';

type Options = {
settings: RuntimeSettings;
logger: Logger;
configService: ConfigService;
fileStorage: FileStorage;
windowsManager: WindowsManager;
};

export default class MainProcess {
Expand All @@ -42,12 +44,14 @@ export default class MainProcess {
private sharedProcess: ChildProcess;
private fileStorage: FileStorage;
private resolvedChildProcessAddresses: Promise<ChildProcessAddresses>;
private windowsManager: WindowsManager;

private constructor(opts: Options) {
this.settings = opts.settings;
this.logger = opts.logger;
this.configService = opts.configService;
this.fileStorage = opts.fileStorage;
this.windowsManager = opts.windowsManager;
}

static create(opts: Options) {
Expand Down Expand Up @@ -195,6 +199,10 @@ export default class MainProcess {
})
);

ipcMain.handle('main-process-force-focus-window', () => {
this.windowsManager.forceFocusWindow();
});

subscribeToTerminalContextMenuEvent();
subscribeToTabContextMenuEvent();
subscribeToConfigServiceEvents(this.configService);
Expand Down
3 changes: 3 additions & 0 deletions web/packages/teleterm/src/mainProcess/mainProcessClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,5 +28,8 @@ export default function createMainProcessClient(): MainProcessClient {
removeKubeConfig(options) {
return ipcRenderer.invoke('main-process-remove-kube-config', options);
},
forceFocusWindow() {
return ipcRenderer.invoke('main-process-force-focus-window');
},
};
}
1 change: 1 addition & 0 deletions web/packages/teleterm/src/mainProcess/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ export type MainProcessClient = {
relativePath: string;
isDirectory?: boolean;
}): Promise<void>;
forceFocusWindow(): void;
};

export type ChildProcessAddresses = {
Expand Down
65 changes: 64 additions & 1 deletion web/packages/teleterm/src/mainProcess/windowsManager.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import path from 'path';

import { BrowserWindow, Menu, Rectangle, screen } from 'electron';
import { app, BrowserWindow, Menu, Rectangle, screen } from 'electron';

import { FileStorage } from 'teleterm/services/fileStorage';
import { RuntimeSettings } from 'teleterm/mainProcess/types';
Expand Down Expand Up @@ -78,6 +78,11 @@ export class WindowsManager {
this.window = window;
}

/**
* focusWindow is for situations where the app has privileges to do so, for example in a scenario
* where the user attempts to launch a second instance of the app – the same process that the user
* interacted with asks for its window to receive focus.
*/
focusWindow(): void {
if (!this.window) {
return;
Expand All @@ -90,6 +95,64 @@ export class WindowsManager {
this.window.focus();
}

/**
* forceFocusWindow if for situations where Connect wants to essentially steal focus.
*
* One example would be 3rd party apps interacting with resources exposed by Connect, e.g.
* gateways. If the user attempts to make a connection through a gateway but the certs have
* expired, Connect should receive focus and show an appropriate message to the user.
*/
forceFocusWindow(): void {
if (!this.window) {
return;
}

if (this.window.isFocused()) {
return;
}

// What follows is a special focus handler for windows.
//
// On Windows, app.focus() doesn't work as expected so instead we call win.focus().
// If the window is minimized, win.focus() will bring it to the front and give it focus.
// If the window is not minimized but simply covered by other another window, win.focus() will
// flash the icon of Connect in the task bar.
//
// Ideally, we'd like the not minimized window to receive focus too. We considered two
// workarounds to bring focus to a window that's not minimized:
//
// * win.minimized() followed by win.focus() – this reportedly doesn't work anymore (see the
// comment linked below) though it did work at the time of implementing forceFocusWindow.
// Admittedly, this seems like a hack and does cause the window to first minimize and then show
// up which feels weird.
// * win.setAlwaysOnTop(true) followed by win.show() – this does bring the window to the top
// but doesn't give it focus. Super awkward because Connect shows up over another app that you
// were using, you start typing to fill out whatever form Connect has shown you. But your
// keystrokes go to the app that the Connect window just covered.
//
// Since we cannot reliably steal focus, let's just not attempt to do it and instead defer to
// flashing the icon in the task bar.
//
// https://github.com/electron/electron/issues/2867#issuecomment-1080573240
//
// I don't understand why calling win.focus() on a minimized window gives it focus in the
// first place. In theory it shouldn't work, see the links below:
//
// https://stackoverflow.com/a/72620653/742872
// https://devblogs.microsoft.com/oldnewthing/20090220-00/?p=19083
// https://github.com/electron/electron/issues/2867#issuecomment-142480964
// https://github.com/electron/electron/issues/2867#issuecomment-142511956
if (this.settings.platform === 'win32') {
this.window.focus();
return;
}

app.dock?.bounce('informational');
// app.focus() alone doesn't un-minimize the window if the window is minimized.
this.window.show();
app.focus({ steal: true });
}

private saveWindowState(window: BrowserWindow): void {
const windowState: WindowState = {
...window.getNormalBounds(),
Expand Down
49 changes: 49 additions & 0 deletions web/packages/teleterm/src/services/relogin.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import { MainProcessClient } from 'teleterm/types';
import { ReloginRequest } from 'teleterm/services/tshdEvents';
import {
ModalsService,
ClusterConnectReason,
} from 'teleterm/ui/services/modals';
import { ClustersService } from 'teleterm/ui/services/clusters';

export class ReloginService {
constructor(
private mainProcessClient: MainProcessClient,
private modalsService: ModalsService,
private clustersService: ClustersService
) {}

relogin(
request: ReloginRequest,
onRequestCancelled: (callback: () => void) => void
): Promise<void> {
this.mainProcessClient.forceFocusWindow();
let reason: ClusterConnectReason;

if (request.gatewayCertExpired) {
const gateway = this.clustersService.findGateway(
request.gatewayCertExpired.gatewayUri
);
reason = {
kind: 'reason.gateway-cert-expired',
targetUri: request.gatewayCertExpired.targetUri,
gateway: gateway,
};
}

return new Promise((resolve, reject) => {
// GatewayCertReissuer in tshd makes sure that we only ever have one concurrent request to the
// relogin event. So at the moment, ReloginService won't ever call openImportantDialog twice.
const { closeDialog } = this.modalsService.openImportantDialog({
kind: 'cluster-connect',
clusterUri: request.rootClusterUri,
reason,
onSuccess: () => resolve(),
onCancel: () =>
reject(new Error('Login process was canceled by the user')),
});

onRequestCancelled(closeDialog);
});
}
}
13 changes: 0 additions & 13 deletions web/packages/teleterm/src/services/tshd/createClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -579,19 +579,6 @@ export default function createClient(
});
},

async restartGateway(gatewayUri = '') {
const req = new api.RestartGatewayRequest().setGatewayUri(gatewayUri);
return new Promise<void>((resolve, reject) => {
tshd.restartGateway(req, err => {
if (err) {
reject(err);
} else {
resolve();
}
});
});
},

async setGatewayTargetSubresourceName(
gatewayUri = '',
targetSubresourceName = ''
Expand Down
13 changes: 12 additions & 1 deletion web/packages/teleterm/src/services/tshd/fixtures/mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@ export class MockTshClient implements TshClient {
listGateways: () => Promise<Gateway[]>;
createGateway: (params: CreateGatewayParams) => Promise<Gateway>;
removeGateway: (gatewayUri: string) => Promise<undefined>;
restartGateway: (gatewayUri: string) => Promise<undefined>;
setGatewayTargetSubresourceName: (
gatewayUri: string,
targetSubresourceName: string
Expand Down Expand Up @@ -87,3 +86,15 @@ export class MockTshClient implements TshClient {
logout: (clusterUri: string) => Promise<undefined>;
transferFile: () => undefined;
}

export const gateway: Gateway = {
uri: '/gateways/gateway1',
targetName: 'postgres',
targetUri: '/clusters/teleport-local/dbs/postgres',
targetUser: 'alice',
targetSubresourceName: '',
localAddress: 'localhost',
localPort: '59116',
protocol: 'postgres',
cliCommand: 'psql postgres://alice@localhost:59116',
};
1 change: 0 additions & 1 deletion web/packages/teleterm/src/services/tshd/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,6 @@ export type TshClient = {
listGateways: () => Promise<Gateway[]>;
createGateway: (params: CreateGatewayParams) => Promise<Gateway>;
removeGateway: (gatewayUri: string) => Promise<void>;
restartGateway: (gatewayUri: string) => Promise<void>;
setGatewayTargetSubresourceName: (
gatewayUri: string,
targetSubresourceName: string
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading

0 comments on commit eafdaf0

Please sign in to comment.