Add explicit privilege prompt to improve sudo UX #138

grayside · 2018-01-29T21:14:44Z

Fixes #134

This change identifies that we need to sudo and adjusts logging/spinning behaviors accordingly. Needs some more testing (e.g., with verbose) and a bit more smarts in sudo detection, hence the WIP flag. Also some UI issues around the asynchronous nature of the spinner.

I am pausing here for approach feedback.

Here's what it looks like when it operates as intended:

Paused for Input

Success!

Corrected a few more references to rig, and elaborated the build script.

Release 1.2.0

1.2.1 release

1.2.2 Release

1.2.3 Release

Version 1.3.0

Version 1.3.1

1.3.2 Release

2.0.0 release

2.1.0

febbraro · 2018-01-29T21:27:06Z

So I would simply say Administrative privileges needed and nothing about a password, since the Password prompt does that already. Look at this example, I ran rig dns twice. Once to get the initial prompt because I was unprivileged...

https://www.dropbox.com/s/m8twc771il6llhq/Screenshot%202018-01-29%2013.25.10.png?dl=0

And then once again, immediately, while I still had privs....

https://www.dropbox.com/s/vtx768cuhep9xk2/Screenshot%202018-01-29%2013.25.52.png?dl=0

It gave me "please enter your password" even though it just blasted right past it. Claiming privs needed is a good plan and it gives us the newline needed for an unobscured prompt. Claiming they are needed and blowing past it is really just a heads up and it is not confusing.

grayside · 2018-01-29T21:45:24Z

Unfortunately here are some of the other results I'm getting, with a password needed:

grayside · 2018-01-30T23:44:52Z

Simplification helped tremendously. This is ready for final review.

I would appreciate if someone took it for a spin to confirm the timing issues are not still lurking.

My test has been:

./build/darwin/rig dns

tekante · 2018-01-31T12:10:34Z

Looks like it's working as intended. First run through didn't need password as I had recently used sudo, second time includes the password prompt.

tekante · 2018-01-31T12:14:05Z

util/logger.go

+	// This newline ensures the last status before escallation is preserved
+	// on-screen.
+	fmt.Println()
+	log.Spin("Evaluating administrative action...")


Just noting that things go by so quickly on my machine I never see the "Evaluating administrative action..." message.

I wouldn't expect it to. I need the log.Spin() to start the spinner and the log.Warning() for the real message... let me test just using the same message for both, since that would avoid coder confusion.

tekante · 2018-01-31T12:56:33Z

util/slices.go

+
+// IndexOfString is a general utility function that can find the index of a value
+// present in a string slice. The second value is true if the item is found.
+func IndexOfString(slice []string, value string) (int, bool) {


This is probably due to general newness to go and it's structures and syntax but it took me a while to realize that this was testing for the presence of an element in a collection of elements rather than the presence of a substring within a string. Probably baggage from character arrays and the concept of a slice of a string from Perl combined with previous indexOf type functions. I don't think it has to change I just thought I'd mention it.

This is relevant because commands of the form of util.Command("bash", "-c", fmt.Sprintf("echo 'nameserver %s' | sudo tee /etc/resolver/vm", bridgeIP)).Run() on 178 of dns.go would fail this check. It's not really an issue for Macs because that one is guaranteed to be proceeded by a command which will already trigger the Password prompt and stop the spinner (line 175 of dns.go). I think it might affect Linux however as the configureLinuxResolver function has no such preceding command.

I've attached an example of what this looks like on Linux.

One potential avenue around this would be to import the strings package and change the conditional on line 7 to something like
if elem == value || strings.Contains(elem, value + " ") {

This appears to function properly for both mac and linux though it changes the nature of this function such that you'll probably want to rename it.

There's a convention with exec.Cmd that the tokens all be individual array elements. I see that subshell situations might be different. Will adjust.

grayside · 2018-01-31T23:04:46Z

In addition to addressing CJ's feedback, I took the following steps:

Refactored Executor.ToString() to Executor.String() to be more in-line with Golang built-ins.
Added the privilege escalation prompt to more of our command execution methods.
When resuming the spinner, it now flashes "resuming operation", this is likely to be a flicker in many cases, but if the calling code doesn't do a good job of spinner management it may be confusing to see the spinning without context. I think this is a good idea, but could be convinced to leave it as an empty string.

febbraro · 2018-02-01T20:47:56Z

I'm getting prompt overlap on rig stop

Using sudo -k first to remove privs

febbraro · 2018-02-01T21:29:04Z

Prompt overlap is only when stopping a running machine. If I stop an already stopped machine, the prompt works fine.

grayside · 2018-02-06T17:25:48Z

I haven't found a way to crack the rig stop spinner yet. It appears that the route command does something unusual in how it interacts with newlines or the password prompt, and may require much deeper research. Is this change a sufficient improvement to move forward anyway?

Should I output an extra message on rig stop to provide instruction on dealing with the confusion?

tekante · 2018-02-06T17:37:22Z

I could go with proceeding if it is just the stop portion that overlap is wonky on. If it's something with route would an initial sudo cat /dev/null just so that the password gets prompted for alleviate that without causing ill effects?

grayside · 2018-02-06T20:30:22Z

cat /dev/null seems to have helped. Will need to research the implications of that trick.

* Add explicit privilege prompt to improve sudo UX (#138) * Explicitly prompt for privilege escallation * Remove password prompt part of privilege message * Expand sudo detection. * Tidy up timing issues. * Consolidate messaging and avoid newline in verbose. * Cleanup ToString, sudo contains, cover more exec methods. * Lint does not catch all of fmt. * Remove unnecessary password prompt from networking cleanup. * Remove color reset and cat /dev/null to clear route text. * trying a different approach to requesting for admin privs (#144)

febbraro and others added 11 commits February 16, 2017 14:44

Merge pull request #2 from phase2/develop

cc38caa

Corrected a few more references to rig, and elaborated the build script.

Merge pull request #15 from phase2/develop

fd8c63f

Release 1.2.0

Merge pull request #17 from phase2/develop

f628762

1.2.1 release

Merge pull request #24 from phase2/develop

73afa6e

1.2.2 Release

Merge pull request #27 from phase2/develop

d89b894

1.2.3 Release

Merge pull request #46 from phase2/develop

a55d874

Version 1.3.0

Merge pull request #63 from phase2/develop

05c5997

Version 1.3.1

Merge pull request #77 from phase2/develop

d1a9e76

1.3.2 Release

Merge pull request #121 from phase2/develop

9794851

2.0.0 release

Merge pull request #131 from phase2/develop

b1a93f6

2.1.0

Explicitly prompt for privilege escallation

b22dba9

grayside requested review from tekante and febbraro January 29, 2018 21:14

Adam Ross added 3 commits January 29, 2018 13:49

Remove password prompt part of privilege message

4f45751

Expand sudo detection.

ce49a7f

Tidy up timing issues.

38069be

grayside changed the title ~~[WIP] Add explicit privilege prompt to improve sudo UX~~ Add explicit privilege prompt to improve sudo UX Jan 30, 2018

grayside changed the base branch from master to develop January 31, 2018 04:08

tekante reviewed Jan 31, 2018

View reviewed changes

Adam Ross added 2 commits January 31, 2018 14:58

Consolidate messaging and avoid newline in verbose.

26bc45a

Cleanup ToString, sudo contains, cover more exec methods.

92afb06

Lint does not catch all of fmt.

550c257

Remove unnecessary password prompt from networking cleanup.

a7d7bd1

Remove color reset and cat /dev/null to clear route text.

2a8e3be

febbraro merged commit 41cad0c into develop Feb 6, 2018

febbraro deleted the privilege-prompt branch February 6, 2018 23:26

febbraro mentioned this pull request Feb 7, 2018

Release 2.1.2 #143

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add explicit privilege prompt to improve sudo UX #138

Add explicit privilege prompt to improve sudo UX #138

Uh oh!

grayside commented Jan 29, 2018

Uh oh!

febbraro commented Jan 29, 2018

Uh oh!

grayside commented Jan 29, 2018

Uh oh!

grayside commented Jan 30, 2018

Uh oh!

tekante commented Jan 31, 2018

Uh oh!

tekante Jan 31, 2018

Uh oh!

grayside Jan 31, 2018

Uh oh!

tekante Jan 31, 2018

Uh oh!

tekante Jan 31, 2018

Uh oh!

grayside Jan 31, 2018

Uh oh!

grayside commented Jan 31, 2018

Uh oh!

febbraro commented Feb 1, 2018

Uh oh!

febbraro commented Feb 1, 2018

Uh oh!

grayside commented Feb 6, 2018

Uh oh!

tekante commented Feb 6, 2018

Uh oh!

grayside commented Feb 6, 2018

Uh oh!

Uh oh!

Add explicit privilege prompt to improve sudo UX #138

Add explicit privilege prompt to improve sudo UX #138

Uh oh!

Conversation

grayside commented Jan 29, 2018

Paused for Input

Success!

Uh oh!

febbraro commented Jan 29, 2018

Uh oh!

grayside commented Jan 29, 2018

Uh oh!

grayside commented Jan 30, 2018

Uh oh!

tekante commented Jan 31, 2018

Uh oh!

tekante Jan 31, 2018

Choose a reason for hiding this comment

Uh oh!

grayside Jan 31, 2018

Choose a reason for hiding this comment

Uh oh!

tekante Jan 31, 2018

Choose a reason for hiding this comment

Uh oh!

tekante Jan 31, 2018

Choose a reason for hiding this comment

Uh oh!

grayside Jan 31, 2018

Choose a reason for hiding this comment

Uh oh!

grayside commented Jan 31, 2018

Uh oh!

febbraro commented Feb 1, 2018

Uh oh!

febbraro commented Feb 1, 2018

Uh oh!

grayside commented Feb 6, 2018

Uh oh!

tekante commented Feb 6, 2018

Uh oh!

grayside commented Feb 6, 2018

Uh oh!

Uh oh!