Skip to content

command/login: Require "yes" to confirm#25379

Merged
alisdair merged 1 commit intomasterfrom
alisdair/yes
Jun 25, 2020
Merged

command/login: Require "yes" to confirm#25379
alisdair merged 1 commit intomasterfrom
alisdair/yes

Conversation

@alisdair
Copy link
Copy Markdown
Contributor

@alisdair alisdair commented Jun 24, 2020

This is for consistency with other commands which use prompts, all of which require "yes" rather than "y" to confirm. Fixes #24514.

After review, this change also includes adding secret/password entry to the UIInput interface and implementation, using the same logic as in mitchellh/cli:

if opts.Secret && isatty.IsTerminal(os.Stdin.Fd()) {
line, err = speakeasy.Ask("")
} else {
buf := bufio.NewReader(r)
line, err = buf.ReadString('\n')
}

Also includes an exciting new horizontal rule in the UI to fix some confusing spacing. Screenshot:

Screenshots

Password/secret prompt (iTerm uses the 🔑 icon to indicate that echo is off):
password-prompt

Login completed, without the token being displayed:
done

@alisdair alisdair requested a review from a team June 24, 2020 18:54
@codecov
Copy link
Copy Markdown

codecov bot commented Jun 24, 2020

Codecov Report

Merging #25379 into master will increase coverage by 0.00%.
The diff coverage is 86.20%.

Impacted Files Coverage Δ
command/ui_input.go 61.03% <57.14%> (-1.47%) ⬇️
command/login.go 50.95% <95.45%> (+2.54%) ⬆️
command/cli_ui.go 87.50% <0.00%> (-12.50%) ⬇️
backend/remote/backend_common.go 54.57% <0.00%> (-0.68%) ⬇️

@mildwonkey
Copy link
Copy Markdown
Contributor

We have an existing pattern for requesting user input (that I only encountered recently myself):
https://github.com/hashicorp/terraform/blob/master/command/013_config_upgrade.go#L149-L166

I genuinely do not know, but I presume there are benefits to this method that might be worth looking into.

@alisdair
Copy link
Copy Markdown
Contributor Author

I genuinely do not know, but I presume there are benefits to this method that might be worth looking into.

There are some benefits! Most obvious is that the UI prompts are more helpful, and I think it's better to be consistent with the UI throughout where we can.

But also, testing using UIInput is clearer. We can specify IDs for queries, and a map of ID to answers for test responses. Order then isn't important, and missing IDs in the test map throw useful errors. For cli.Ui.Ask/AskSecret, we have to supply a string of input, each response separated by newlines.

However, UIInput does not support disabling echo for asking passwords, and from my attempts it doesn't seem to be possible to mix the two under test—they have incompatible ways of mocking user input.

So in my latest commit, I added support for a Secret option to UIInput, and migrated the login command across. This does make the change more invasive, but I think it's probably worth it.

Screenshots

Password/secret prompt (iTerm uses the 🔑 icon to indicate that echo is off):
password-prompt

Login completed, without the token being displayed:
done

This is for consistency with other commands which use prompts, all of
which require "yes" rather than "y" to confirm.

We also migrate the login command to use UIInput, which now supports
securely asking for passwords or secrets via the speakeasy library.
Copy link
Copy Markdown
Contributor

@mildwonkey mildwonkey left a comment

Choose a reason for hiding this comment

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

This looks good, thanks for digging into command.UIInput!

PS: It looks like you ran go mod tidy, which I completely forgot to run before merging my recent library-bumping PR. Thanks and sorry for the oversight!

Copy link
Copy Markdown
Contributor

@pselle pselle left a comment

Choose a reason for hiding this comment

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

I also appreciate this branch name

@alisdair alisdair merged commit b4cc77b into master Jun 25, 2020
@alisdair alisdair deleted the alisdair/yes branch June 25, 2020 17:00
@ghost
Copy link
Copy Markdown

ghost commented Jul 26, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Jul 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Terraform login prompt should require yes for approval

3 participants