styra-link: correct password flow #81

msorens · 2024-04-17T16:27:09Z

What code changed, and why?

Several issues revolving around the password entry flow when installing the Styra CLI from the plugin.
DAS-770, DAS-771, DAS-772.

Definition of done

OLD:

NEW:

fix poor error reporting upon bad password.
allow password retries without re-downloading full binary: Add loop for retry but move the binary fetch outside the loop
cancel out of password entry closes progress bar.

How to test

You can install either from source code or from GitHub.

Install from source

Switch to current branch.
Run npm install.
Open project in VSCode.
In the debugger be sure you have "Run Extension" mode selected.
Invoke "run" (F5).

Install from GitHub

Go to the latest release on the release page.
Download the additional version piggybacked into that release, labelled vscode-styra-2.0.1-next.4.vsix.
Install via the standard command: code --install-extension vscode-styra-2.0.1-next.4.vsix

Exercise the Code

Related Resources

1. fix poor error reporting upon bad password. ``` ERROR: CLI installation failed Error: Password: Sorry, try again. Password: sudo: no password was provided sudo: 1 incorrect password attempt ``` 2. allow password retries without re-downloading full binary Add loop for retry but only fetch the binary on the first pass 3. cancel out of password entry closes progress bar Move `collectInputs` out of `await IDE.withProgress`.

Duh; do not need to include the binary download in the loop at all.

msorens · 2024-04-17T16:49:21Z

src/lib/styra-install.ts

+      // eslint-disable-next-line no-constant-condition
+      while (true) {


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

msorens · 2024-04-17T16:52:39Z

src/lib/styra-install.ts

+          if ((err as Error).message.includes('Sorry, try again')) {
+            info('invalid password; try again...');


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.

msorens · 2024-04-17T16:55:28Z

src/lib/styra-install.ts

-        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});


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.

msorens · 2024-04-17T18:35:49Z

src/lib/styra-install.ts

-    if (await StyraInstall.styraCmdExists()) {
+    if (await this.styraCmdExists()) {


Had a mix of class name vs. this. Making them all consistent.

msorens · 2024-04-17T18:36:44Z

src/lib/styra-install.ts

      info('Installing Styra CLI. This may take a few minutes...');
      try {
-        await this.installStyra();


Separating installStyra into downloadBinary and installOnPath.

msorens · 2024-04-17T18:38:08Z

src/lib/styra-install.ts

-      shouldResume
+      shouldResume // TODO: override and delete temp file


for later; low priority

msorens · 2024-04-17T18:38:50Z

src/test-jest/lib/styra-install.test.ts

+  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('downloadBinary', jest.fn().mockResolvedValue(''));


Introduced setPrivateMock to get rid of the smattering of eslint disablements.

srenatus · 2024-04-22T12:31:49Z

src/lib/styra-install.ts

+    } 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}'`];


[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.

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!

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.

msorens force-pushed the msorens/correct-pwd-flow branch from 56dc114 to f0674d4 Compare April 17, 2024 16:50

styra-link: cleanup

f3a5753

Duh; do not need to include the binary download in the loop at all.

msorens commented Apr 17, 2024

View reviewed changes

msorens marked this pull request as ready for review April 17, 2024 18:40

bookkeeping: v2.0.1-next.4

2609318

msorens requested a review from srenatus April 20, 2024 18:15

srenatus approved these changes Apr 22, 2024

View reviewed changes

msorens merged commit d517cb0 into main Apr 22, 2024
4 checks passed

msorens deleted the msorens/correct-pwd-flow branch April 22, 2024 16:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

styra-link: correct password flow #81

styra-link: correct password flow #81

msorens commented Apr 17, 2024 •

edited

Loading

msorens Apr 17, 2024

msorens Apr 17, 2024 •

edited

Loading

msorens Apr 17, 2024 •

edited

Loading

msorens Apr 17, 2024

msorens Apr 17, 2024

msorens Apr 17, 2024

msorens Apr 17, 2024

srenatus Apr 22, 2024

msorens Apr 22, 2024

srenatus Apr 22, 2024

		// eslint-disable-next-line no-constant-condition
		while (true) {

		if ((err as Error).message.includes('Sorry, try again')) {
		info('invalid password; try again...');

		if (await StyraInstall.styraCmdExists()) {
		if (await this.styraCmdExists()) {

		shouldResume
		shouldResume // TODO: override and delete temp file

styra-link: correct password flow #81

styra-link: correct password flow #81

Conversation

msorens commented Apr 17, 2024 • edited Loading

What code changed, and why?

Definition of done

How to test

Install from source

Install from GitHub

Exercise the Code

Related Resources

Choose a reason for hiding this comment

msorens Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

msorens Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msorens commented Apr 17, 2024 •

edited

Loading

msorens Apr 17, 2024 •

edited

Loading

msorens Apr 17, 2024 •

edited

Loading