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.

Sign up for GitHub

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

Jump to bottom

styra-link: correct password flow #81

Merged
merged 3 commits into from
Apr 22, 2024
Merged

styra-link: correct password flow #81

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f0674d4
styra-link: correct password flow
msorens Apr 17, 2024
f3a5753
styra-link: cleanup
msorens Apr 17, 2024
2609318
bookkeeping: v2.0.1-next.4
msorens Apr 17, 2024
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
62 changes: 39 additions & 23 deletions src/lib/styra-install.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,13 +61,22 @@ export class StyraInstall {

if (selection === 'Install') {
info('Installing Styra CLI. This may take a few minutes...');
try {
await this.installStyra();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Separating installStyra into downloadBinary and installOnPath.

teeInfo(`CLI ${operation} completed.`);
return true;
} catch (err) {
teeError(`CLI ${operation} failed: ${err}`);
return false;
let trials = 0;
// eslint-disable-next-line no-constant-condition
while (true) {
Comment on lines +71 to +72
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Surprised it is not smart enough to recognize the loop does have exits.

try {
const tempFile = await this.installStyra(trials++);
await StyraInstall.installOnPath(tempFile);
teeInfo(`CLI ${operation} completed.`);
return true;
} catch (err) {
if ((err as Error).message.includes('Sorry, try again')) {
info('invalid password; try again...');
Comment on lines +78 to +79
Copy link
Collaborator Author

@msorens msorens Apr 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Add in a specific check for a password failure and, if found:
(a) print a clean, short message instead of the cryptic, verbose one (see image above), and
(b) do not return from the function yet, so it loops.

} else {
teeError(`CLI ${operation} failed: ${(err as Error).message}`);
return false;
}
}
}
} else {
infoFromUserAction(`CLI ${operation} cancelled`);
Expand Down Expand Up @@ -163,37 +172,44 @@ export class StyraInstall {
: `${prefix}/linux/amd64/styra`;
}

private static async installStyra(): Promise<void> {
private static async installStyra(trials = 0): Promise<string> {

const tempFileLocation = path.join(os.homedir(), this.BinaryFile);
const url = this.getDownloadUrl();

return await IDE.withProgress({
await IDE.withProgress({
location: IDE.ProgressLocation.Notification,
title: 'Installing Styra CLI',
cancellable: false
}, async () => {
await this.getBinary(url, tempFileLocation);
info(` Platform: ${process.platform}`);
info(` Architecture: ${process.arch}`);
info(` Executable: ${this.ExeFile}`);
fs.chmodSync(tempFileLocation, '755');
if (this.isWindows()) {
await moveFile(tempFileLocation, this.ExeFile);
await this.adjustWindowsPath(this.ExePath);
} else {
const state = await this.collectInputs();
// see https://stackoverflow.com/q/39785436/115690 for ideas on running sudo
const args = ['-c', `echo ${state.pwd} | sudo -S bash -c 'mv ${tempFileLocation} ${this.ExeFile}'`];
// vital to run in quiet mode so password does not display
await new CommandRunner().runShellCmd('sh', args, {progressTitle: '', quiet: true});
Comment on lines -185 to -189
Copy link
Collaborator Author

@msorens msorens Apr 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

By moving collectInputs outside of the IDE.withProgress block, the user can now abort the operation cleanly (by pressing Escape in the text input box prompting for the password). Inside the IDE.withProgress block, a progress bar appears until finished, and that progress bar did not clean up when aborting with Escape.

if (trials === 0) {
await this.getBinary(url, tempFileLocation);
info(` Platform: ${process.platform}`);
info(` Architecture: ${process.arch}`);
info(` Executable: ${this.ExeFile}`);
fs.chmodSync(tempFileLocation, '755');
}
});
return tempFileLocation;
}

private static async installOnPath(tempFileLocation: string) {
if (this.isWindows()) {
await moveFile(tempFileLocation, this.ExeFile);
await this.adjustWindowsPath(this.ExePath);
} else {
const state = await this.collectInputs();
// see https://stackoverflow.com/q/39785436/115690 for ideas on running sudo
const args = ['-c', `echo ${state.pwd} | sudo -S bash -c 'mv ${tempFileLocation} ${this.ExeFile}'`];
Copy link
Member

Choose a reason for hiding this comment

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

[nit] I know this is copied code, but I still don't quite like this. I'm always getting nervous when any tool asks me to input my password. Be that as it may, I suppose we're not here to discuss this. Anyway...

This particular method seems icky, too -- multi-user machines are no longer the norm, but any other process on the machine could read the user's password from ps output if they're eager to. Can we not just open a shell with the sudo call? if the user inputs their password there it'll always be safe.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I certainly agree with your point but I am not sure there is a way to give a user a shell prompt from within a VSCode command. I like the idea though, so will put it on my backlog but merge as is for now. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

The only winning move is not to play -- we can install the binary anywhere, and use it from anywhere. So we can use a location where an unprivileged user can write to. .styra/bin/ or something. We'll add a note saying that they need to add that to their PATH to have it be available on the command line. If they only interact with the link tool using the VSCode plugin, there's no need for updating the $PATH even.

// vital to run in quiet mode so password does not display
await new CommandRunner().runShellCmd('sh', args, {progressTitle: '', quiet: true});
}
}

private static async getBinary(url: string, tempFileLocation: string): Promise<void> {
// adapted from https://stackoverflow.com/a/69290915
const response = await fetch(url);

if (!response.ok) {
throw new Error(
response.status === 404 ? `Bad URL for downloading styra - ${url}`
Expand Down
43 changes: 34 additions & 9 deletions src/test-jest/lib/styra-install.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,13 @@ describe('StyraInstall', () => {

const spy = new OutputPaneSpy();

function setPrivateMock(functionName: string, mock: jest.Mock) {
(StyraInstall as any)[functionName] = mock;
}

beforeEach(() => {
// eslint-disable-next-line dot-notation
StyraInstall['installStyra'] = jest.fn().mockResolvedValue('');
setPrivateMock('installStyra', jest.fn().mockResolvedValue(''));
setPrivateMock('installOnPath', jest.fn().mockResolvedValue(''));
StyraInstall.styraCmdExists = jest.fn().mockResolvedValue(false);
IDE.getConfigValue = mockVSCodeSettings();
});
Expand Down Expand Up @@ -91,13 +95,36 @@ describe('StyraInstall', () => {
});
});

test('returns false if installStyra throws an error', async () => {
test('returns true and succeeds if installStyra gets a bad pwd then a good pwd', async () => {
IDE.showInformationMessageModal = jest.fn().mockReturnValue('Install');
setPrivateMock('installStyra', jest.fn()
.mockRejectedValueOnce({message: 'Sorry, try again. Bad password'})
.mockResolvedValueOnce(''));

expect(await StyraInstall.checkCliInstallation()).toBe(true);
expect(spy.content).toMatch(/invalid password/);
expect(spy.content).toMatch(/CLI installation completed/);
});

test('returns false and fails if installStyra gets bad pwd, then some other error', async () => {
IDE.showInformationMessageModal = jest.fn().mockReturnValue('Install');
setPrivateMock('installStyra', jest.fn()
.mockRejectedValueOnce({message: 'Sorry, try again. Bad password'})
.mockRejectedValue({message: 'some error'}));

expect(await StyraInstall.checkCliInstallation()).toBe(false);
expect(spy.content).toMatch(/invalid password/);
expect(spy.content).toMatch(/CLI installation failed/);
expect(spy.content).toMatch(/some error/);
});

test('returns false and fails if installStyra throws an error other than bad pwd', async () => {
IDE.showInformationMessageModal = jest.fn().mockReturnValue('Install');
// eslint-disable-next-line dot-notation
StyraInstall['installStyra'] = jest.fn().mockRejectedValue('error');
setPrivateMock('installStyra', jest.fn().mockRejectedValue({message: 'some error'}));

expect(await StyraInstall.checkCliInstallation()).toBe(false);
expect(spy.content).toMatch(/CLI installation failed/);
expect(spy.content).toMatch(/some error/);
});
});

Expand Down Expand Up @@ -200,8 +227,7 @@ describe('StyraInstall', () => {
DAS.runQuery = jest.fn().mockResolvedValue({cliVersion: available});
CommandRunner.prototype.runShellCmd = jest.fn().mockResolvedValue(installed);
const installMock = jest.fn();
// eslint-disable-next-line dot-notation
StyraInstall['promptForInstall'] = installMock;
setPrivateMock('promptForInstall', installMock);

await StyraInstall.checkForUpdates();

Expand All @@ -221,8 +247,7 @@ describe('StyraInstall', () => {
DAS.runQuery = jest.fn().mockImplementation(available);
CommandRunner.prototype.runShellCmd = jest.fn().mockImplementation(installed);
const installMock = jest.fn();
// eslint-disable-next-line dot-notation
StyraInstall['promptForInstall'] = installMock;
setPrivateMock('promptForInstall', installMock);

await StyraInstall.checkForUpdates();

Expand Down
Loading