Skip to content
This repository was archived by the owner on Feb 8, 2024. It is now read-only.

Add commands for linking tsh on macOS #1480

Merged
ravicious merged 11 commits into from
Jan 4, 2023
Merged

Add commands for linking tsh on macOS #1480

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
2bcadc7
Expand story for QuickInput
ravicious Dec 22, 2022
1ba8dad
Make command suggestions stay in place
ravicious Dec 23, 2022
ed7cdc0
Align suggestion icons to the top rather than center
ravicious Dec 23, 2022
5c5f8c5
Add install & uninstall cmds to command bar
ravicious Dec 23, 2022
f717bf6
Exclude new commands from OSes other than macOS
ravicious Dec 23, 2022
7600ae8
Implement commands for symlinking tsh
ravicious Jan 3, 2023
ad11fd7
Add macos suffix to symlink IPC handlers
ravicious Jan 4, 2023
68f0a69
Refactor QuickInput story into two separate stories
ravicious Jan 4, 2023
905bd19
Use assertUnreachable
ravicious Jan 4, 2023
acf1c9f
Refactor autocomplete commands
ravicious Jan 4, 2023
b006f1f
Add MacOs suffix to MainProcessClient methods
ravicious Jan 4, 2023
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
69 changes: 40 additions & 29 deletions packages/teleterm/src/mainProcess/fixtures/mocks.ts
View file
Open in desktop
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';

@ravicious ravicious Jan 3, 2023

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 symlinkTsh() {
return true;
}
async removeTshSymlink() {
return true;
}
}

const defaultRuntimeSettings = {
platform: 'darwin' as const,
dev: true,

@avatus avatus Jan 3, 2023

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?

@ravicious ravicious Jan 4, 2023

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
View file
Open in desktop
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', async () => {

@zmb3 zmb3 Jan 3, 2023

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.

Since this is specific to macOS, should we indicate that in the name of the event?

Suggested change
ipcMain.handle('main-process-symlink-tsh', async () => {
ipcMain.handle('main-process-symlink-tsh-macos', async () => {

@ravicious ravicious Jan 4, 2023

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.

Since this is specific to macOS, should we indicate that in the name of the event?

I was thinking about it but in the end I didn't do it for some reason, I'll add that.

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', async () => {

@ravicious ravicious Jan 3, 2023

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 just copy-pasted the other handler instead of adding some abstractions here, IMHO once we have another use case for osascript we can think about abstracting osascript prompts.

@avatus avatus Jan 3, 2023

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 love this thinking. Always reminds me of this.

const target = '/usr/local/bin/tsh';
const prompt =
'Teleport Connect wants to remove a symlink for tsh from /usr/local/bin.';

@avatus avatus Jan 3, 2023

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

@ravicious ravicious Jan 4, 2023

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

@gzdunek gzdunek Jan 4, 2023

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.

@ravicious ravicious Jan 4, 2023

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.

@gzdunek gzdunek Jan 4, 2023

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
View file
Open in desktop
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');
},
symlinkTsh() {

@gzdunek gzdunek Jan 4, 2023

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 somehow forgot to send a comment earlier, but I'd also add macOs part to symlinkTsh/removeTshSymlink.

return ipcRenderer.invoke('main-process-symlink-tsh');
},
removeTshSymlink() {
return ipcRenderer.invoke('main-process-remove-tsh-symlink');
},
};
}
10 changes: 10 additions & 0 deletions packages/teleterm/src/mainProcess/types.ts
View file
Open in desktop
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.
*/
symlinkTsh(): 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.
*/
removeTshSymlink(): Promise<boolean>;
};

export type ChildProcessAddresses = {
Expand Down
179 changes: 164 additions & 15 deletions packages/teleterm/src/ui/QuickInput/QuickInput.story.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,35 @@ export default {
};

export const Story = () => {
return (
<>
{/* Extra bottom margin to accommodate the suggestions displayed under the input. */}
<QuickInputDemo description="Pristine state" mb={350} />

@gzdunek gzdunek Jan 4, 2023

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.

Nit: I'd consider creating separate stories for each demo. The "scroll into viewport" mechanism in QuickInput causes the whole story to jump when you want to use an input at the bottom of the viewport.

@ravicious ravicious Jan 4, 2023

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.

The "scroll into viewport" mechanism in QuickInput causes the whole story to jump when you want to use an input at the bottom of the viewport.

Right, it's annoying but the reason I created the story in the first place is that I wanted to have an easy way to inspect all available suggestions.

But I suppose a better way would be to create a separate story which uses QuickInputList alone rather than setting up the whole QuickInput component. I'll make that change soon.

<QuickInputDemo description="Command suggestions" inputValue="ts" />
<QuickInputDemo
description="Login suggestions"
inputValue="tsh ssh "
mb={160}
/>
<QuickInputDemo
description="Server suggestions"
inputValue="tsh ssh root@"
mb={350}
/>
<QuickInputDemo
description="Database suggestions"
inputValue="tsh proxy db "
mb={350}
/>
</>
);
};

const QuickInputDemo = (props: {
description: string;
inputValue?: string;
mb?: number;
}) => {
const appContext = new MockAppContext();

appContext.workspacesService.state = {
Expand All @@ -45,30 +74,150 @@ export const Story = () => {
rootClusterUri: '/clusters/localhost',
};

const cluster = {
uri: '/clusters/localhost' as const,
name: 'Test',
leaf: false,
connected: true,
proxyHost: 'localhost:3080',
loggedInUser: {
activeRequestsList: [],
name: 'admin',
acl: {},
sshLoginsList: [
'root',
'ubuntu',
'ansible',
'lorem-ipsum-dolor-sit-amet-lorem-ipsum-dolor-sit-amet-lorem-ipsum-dolor-sit-amet',
],
rolesList: [],
},
};

appContext.clustersService.getClusters = () => {
return [
return [cluster];
};

appContext.clustersService.setState(draftState => {
draftState.clusters = new Map([[cluster.uri, cluster]]);
});

appContext.resourcesService.fetchServers = async () => ({
agentsList: [
{
uri: '/clusters/localhost',
name: 'Test',
leaf: false,
connected: true,
proxyHost: 'localhost:3080',
loggedInUser: {
activeRequestsList: [],
name: 'admin',
acl: {},
sshLoginsList: [],
rolesList: [],
},
uri: '/clusters/localhost/servers/foo',
tunnel: false,
name: '2018454d-ef3b-4b15-84f7-61ca213d37e3',
hostname: 'foo',
addr: 'foo.localhost',
labelsList: [
{ name: 'env', value: 'prod' },
{ name: 'kernel', value: '5.15.0-1023-aws' },
],
},
];
};
{
uri: '/clusters/localhost/servers/bar',
tunnel: false,
name: '24c7aebe-4741-4464-ab69-f076fe467ebd',
hostname: 'bar',
addr: 'bar.localhost',
labelsList: [
{ name: 'env', value: 'staging' },
{ name: 'kernel', value: '5.14.1-1058-aws' },
],
},
{
uri: '/clusters/localhost/servers/lorem',
tunnel: false,
name: '24c7aebe-4741-4464-ab69-f076fe467ebd',
hostname:
'lorem-ipsum-dolor-sit-amet-lorem-ipsum-dolor-sit-amet-lorem-ipsum-dolor-sit-amet',
addr: 'lorem.localhost',
labelsList: [
{ name: 'env', value: 'staging' },
{ name: 'kernel', value: '5.14.1-1058-aws' },
{
name: 'lorem',
value:
'lorem-ipsum-dolor-sit-amet-lorem-ipsum-dolor-sit-amet-lorem-ipsum-dolor-sit-amet',
},
{ name: 'kernel2', value: '5.14.1-1058-aws' },
{ name: 'env2', value: 'staging' },
{ name: 'kernel3', value: '5.14.1-1058-aws' },
],
},
],
totalCount: 3,
startKey: 'foo',
});

appContext.resourcesService.fetchDatabases = async () => ({
agentsList: [
{
uri: '/clusters/localhost/dbs/postgres',
name: 'postgres',
desc: 'A PostgreSQL database',
protocol: 'postgres',
type: 'self-hosted',
hostname: 'postgres.localhost',
addr: 'postgres.localhost',
labelsList: [
{ name: 'env', value: 'prod' },
{ name: 'kernel', value: '5.15.0-1023-aws' },
],
},
{
uri: '/clusters/localhost/dbs/mysql',
name: 'mysql',
desc: 'A MySQL database',
protocol: 'mysql',
type: 'self-hosted',
hostname: 'mysql.localhost',
addr: 'mysql.localhost',
labelsList: [
{ name: 'env', value: 'staging' },
{ name: 'kernel', value: '5.14.1-1058-aws' },
],
},
{
uri: '/clusters/localhost/dbs/lorem',
name: 'lorem-ipsum-dolor-sit-amet-lorem-ipsum-dolor-sit-amet-lorem-ipsum-dolor-sit-amet',
desc: 'Lorem ipsum dolor sit amet lorem ipsum dolor sit amet lorem ipsum dolor sit amet lorem ipsum dolor sit amet',
protocol: 'mysql',
type: 'self-hosted',
hostname: 'lorem.localhost',
addr: 'lorem.localhost',
labelsList: [
{ name: 'env', value: 'staging' },
{ name: 'kernel', value: '5.14.1-1058-aws' },
{
name: 'lorem',
value:
'lorem-ipsum-dolor-sit-amet-lorem-ipsum-dolor-sit-amet-lorem-ipsum-dolor-sit-amet',
},
{ name: 'kernel2', value: '5.14.1-1058-aws' },
{ name: 'env2', value: 'staging' },
{ name: 'kernel3', value: '5.14.1-1058-aws' },
],
},
],
totalCount: 2,
startKey: 'foo',
});

if (props.inputValue !== undefined) {
appContext.quickInputService.setInputValue(props.inputValue);
appContext.quickInputService.show();
appContext.quickInputService.hide = () => {};
}

return (
<AppContextProvider value={appContext}>
<p>{props.description}</p>
<div
css={`
height: 40px;
margin-bottom: ${props.mb || 150}px;
`}
>
<QuickInput />
Expand Down
Loading