Skip to content
This repository was archived by the owner on Feb 8, 2024. It is now read-only.
69 changes: 40 additions & 29 deletions packages/teleterm/src/mainProcess/fixtures/mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,55 +5,66 @@ import { createMockFileStorage } from 'teleterm/services/fileStorage/fixtures/mo
// Importing electron breaks the fixtures if that's done from within storybook.
import { createConfigService } from 'teleterm/services/config/configService';

const platform = 'darwin';
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I had to remove the hardcoded platform in order to be able to test config service for different platforms.


export class MockMainProcessClient implements MainProcessClient {
configService: ReturnType<typeof createConfigService>;

constructor(private runtimeSettings: Partial<RuntimeSettings> = {}) {
this.configService = createConfigService(
createMockFileStorage(),
this.getRuntimeSettings().platform
);
}
getRuntimeSettings(): RuntimeSettings {
return {
platform,
dev: true,
userDataDir: '',
binDir: '',
certsDir: '',
kubeConfigsDir: '',
defaultShell: '',
tshd: {
insecure: true,
requestedNetworkAddress: '',
binaryPath: '',
homeDir: '',
flags: [],
},
sharedProcess: {
requestedNetworkAddress: '',
},
tshdEvents: {
requestedNetworkAddress: '',
},
installationId: '123e4567-e89b-12d3-a456-426614174000',
};
return { ...defaultRuntimeSettings, ...this.runtimeSettings };
}

getResolvedChildProcessAddresses = () =>
Promise.resolve({ tsh: '', shared: '' });

openTerminalContextMenu() {}

openClusterContextMenu() {}

openTabContextMenu() {}

showFileSaveDialog() {
return Promise.resolve({ canceled: false, filePath: '' });
}

configService = createConfigService(createMockFileStorage(), platform);

fileStorage = createMockFileStorage();

removeKubeConfig(): Promise<void> {
return Promise.resolve(undefined);
}

forceFocusWindow() {}

async symlinkTshMacOs() {
return true;
}
async removeTshSymlinkMacOs() {
return true;
}
}

const defaultRuntimeSettings = {
platform: 'darwin' as const,
dev: true,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I know this is a mock, but what are the consequences of having our mock be in dev mode instead of not? Could this lead to any false positives/negatives?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I know this is a mock, but what are the consequences of having our mock be in dev mode instead of not? Could this lead to any false positives/negatives?

dev is mostly used during the build & startup of the app, it shouldn't have much effect overall on tests.

In general I'm of the opinion that we shouldn't use dev for business logic, I was talking with Grzegorz about this recently. And for any other scenario, our setup code in tests should probably be closer to the dev build than the prod build (for example in terms of what's getting logged and what's not, etc.).

userDataDir: '',
binDir: '',
certsDir: '',
kubeConfigsDir: '',
defaultShell: '',
tshd: {
insecure: true,
requestedNetworkAddress: '',
binaryPath: '',
homeDir: '',
flags: [],
},
sharedProcess: {
requestedNetworkAddress: '',
},
tshdEvents: {
requestedNetworkAddress: '',
},
installationId: '123e4567-e89b-12d3-a456-426614174000',
};
50 changes: 48 additions & 2 deletions packages/teleterm/src/mainProcess/mainProcess.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { ChildProcess, fork, spawn } from 'child_process';
import { ChildProcess, fork, spawn, exec } from 'child_process';
import path from 'path';

import fs from 'fs/promises';

import { promisify } from 'util';
import {
app,
dialog,
Expand Down Expand Up @@ -203,6 +203,52 @@ export default class MainProcess {
this.windowsManager.forceFocusWindow();
});

// Used in the `tsh install` command on macOS to make the bundled tsh available in PATH.
// Returns true if tsh got successfully installed, false if the user closed the osascript
// prompt. Throws an error when osascript fails.
ipcMain.handle('main-process-symlink-tsh-macos', async () => {
const source = this.settings.tshd.binaryPath;
const target = '/usr/local/bin/tsh';
const prompt =
'Teleport Connect wants to create a symlink for tsh in /usr/local/bin.';
const command = `osascript -e "do shell script \\"mkdir -p /usr/local/bin && ln -sf '${source}' '${target}'\\" with prompt \\"${prompt}\\" with administrator privileges"`;

try {
await promisify(exec)(command);
this.logger.info(`Created the symlink to ${source} under ${target}`);
return true;
} catch (error) {
// Ignore the error if the user canceled the prompt.
// https://developer.apple.com/library/archive/documentation/AppleScript/Conceptual/AppleScriptLangGuide/reference/ASLR_error_codes.html#//apple_ref/doc/uid/TP40000983-CH220-SW2
if (error instanceof Error && error.message.includes('-128')) {
return false;
}
this.logger.error(error);
throw error;
}
});

ipcMain.handle('main-process-remove-tsh-symlink-macos', async () => {
const target = '/usr/local/bin/tsh';
const prompt =
'Teleport Connect wants to remove a symlink for tsh from /usr/local/bin.';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder if this language could be changed at all. I know this is exactly what the script is doing, and I know our audience is technical enough to understand thing, but it just seems so... destructive? Idk what the word is. I'm ok with not changing it, but I wonder if there is simpler terms to describe what is happening. Maybe I'm overreacting

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm ok with not changing it, but I wonder if there is simpler terms to describe what is happening.

We could say "Connect wants to uninstall tsh from PATH" but personally I'd prefer the app to tell me exactly what it wants to do when it asks me for a password.

Another option would be "Connect wants to modify the contents of /usr/local/bin" which doesn't use the notion of removing/uninstalling but again fails at specifying what exactly is going to happen and potentially sounds even more dangerous.

I'd probably keep this, the way I look at it is we ask the user for permission to do something so we better explain that as succinctly as possible.

const command = `osascript -e "do shell script \\"rm '${target}'\\" with prompt \\"${prompt}\\" with administrator privileges"`;

try {
await promisify(exec)(command);
this.logger.info(`Removed the symlink under ${target}`);
return true;
} catch (error) {
// Ignore the error if the user canceled the prompt.
// https://developer.apple.com/library/archive/documentation/AppleScript/Conceptual/AppleScriptLangGuide/reference/ASLR_error_codes.html#//apple_ref/doc/uid/TP40000983-CH220-SW2
if (error instanceof Error && error.message.includes('-128')) {
return false;
}
this.logger.error(error);
throw error;
Comment on lines +244 to +248
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I have mixed feelings about this, it would be nice to return the errors in the same way.
Maybe we could create a new custom error, like PromptCancelledByUserError and throw (or even return) it instead of false? Then on the renderer side we would have to create our own instanceOf (as the the classes are serialized) that would compare errors by name.

IMO this would make the code more elegant, but we can live with what we have, it is quite simple and documented.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I have mixed feelings about this, it would be nice to return the errors in the same way.

The way I think about this is that closing the prompt is not really an error/exception so I wanted to reserve rejecting the promise for actual errors that might happen during the execution of the script.

As seen in your suggestion, using a custom error would be much more complicated. But even more important is that it wouldn't work because errors are not only stripped of any custom properties, they lose their name as well and become simple Error instances. So we couldn't inspect name AFAIR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So the only way is to return plain objects, but if we don't think of closing the prompt as an error then it's fine to stay with the current solution.

}
});

subscribeToTerminalContextMenuEvent();
subscribeToTabContextMenuEvent();
subscribeToConfigServiceEvents(this.configService);
Expand Down
6 changes: 6 additions & 0 deletions packages/teleterm/src/mainProcess/mainProcessClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,5 +31,11 @@ export default function createMainProcessClient(): MainProcessClient {
forceFocusWindow() {
return ipcRenderer.invoke('main-process-force-focus-window');
},
symlinkTshMacOs() {
return ipcRenderer.invoke('main-process-symlink-tsh-macos');
},
removeTshSymlinkMacOs() {
return ipcRenderer.invoke('main-process-remove-tsh-symlink-macos');
},
};
}
10 changes: 10 additions & 0 deletions packages/teleterm/src/mainProcess/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,16 @@ export type MainProcessClient = {
isDirectory?: boolean;
}): Promise<void>;
forceFocusWindow(): void;
/**
* The promise returns true if tsh got successfully symlinked, false if the user closed the
* osascript prompt. The promise gets rejected if osascript encountered an error.
*/
symlinkTshMacOs(): Promise<boolean>;
/**
* The promise returns true if the tsh symlink got removed, false if the user closed the osascript
* prompt. The promise gets rejected if osascript encountered an error.
*/
removeTshSymlinkMacOs(): Promise<boolean>;
};

export type ChildProcessAddresses = {
Expand Down
Loading