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

Adding option to exit OTP progress bar #2041

Merged
merged 6 commits into from
Dec 17, 2021
Merged

Adding option to exit OTP progress bar #2041

merged 6 commits into from
Dec 17, 2021

Conversation

AnomalRoil
Copy link
Member

As discussed in #2019

@AnomalRoil AnomalRoil mentioned this pull request Nov 17, 2021
Merged
@AnomalRoil
RELEASE_NOTES=[ENHANCEMENT] Adding option to exit OTP progress bar
63df0f1 
Signed-off-by: Yolan Romailler <yolan@romailler.ch>
dominikschulz
dominikschulz requested changes Nov 18, 2021
View reviewed changes
Copy link
Member

@dominikschulz dominikschulz left a comment

Choose a reason for hiding this comment

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

I wonder why we need the go-tty lib. Also I think gocycle rightfully complains. If you need to keep the nesting at least move the implementation of these goroutines to explicit functions.

internal/action/otp.go Outdated Show resolved Hide resolved
internal/action/otp.go Outdated Show resolved Hide resolved
internal/action/otp.go Outdated Show resolved Hide resolved
@AnomalRoil
Copy link
Member Author

AnomalRoil commented Nov 18, 2021 •
edited

Yeah, this is still a work in progress since the Linux CI isn't passing, but I was trying to code on Windows for once, which made my life a little bit more complicated. And didn't have time today to go back to it yet.

Excellent idea @dominikschulz about putting the Done in a defer.

For the go-tty, we need it in order to read a keystroke on all platforms, without requiring to press Enter after entering our input...
If we're fine with requiring a Enter after the keystroke I could get rid of it and use the Context Cancel syntax, I guess. Wdyt?

@AnomalRoil AnomalRoil marked this pull request as draft November 19, 2021 17:07
@dominikschulz
Copy link
Member

@AnomalRoil Hmm, I thought I had replied to this one. But pressing enter after q sounds very un-intuitive to me. So let's go with go-tty instead.

@AnomalRoil
applying code review comments
d0eccf6 
RELEASE_NOTES=n/a

Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com>
@AnomalRoil
fix merge conflict
7206681 
RELEASE_NOTES=n/a

Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com>
@AnomalRoil
patching CI
3f06a5b 
RELEASE_NOTES=n/a

Signed-off-by: Yolan Romailler <yolan@romailler.ch>
@AnomalRoil AnomalRoil marked this pull request as ready for review December 3, 2021 14:28
@AnomalRoil
Copy link
Member Author

At long last! I shouldn't try and dev when I cannot run the full test suite and codequality checks locally xD

AnomalRoil
AnomalRoil commented Dec 3, 2021
View reviewed changes
internal/action/otp.go
return // returning not to leak the goroutine
default:
}
r, err := tty.ReadRune()
Copy link
Member Author

Choose a reason for hiding this comment

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

Actually I wonder if this is not waiting until the next keystroke, and thus this goroutine isn't returning when it should... (basically it's always in iteration 1 of the for loop...)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, the goroutine is leaking when the OTP Bar finishes on line 52, I think.
Related: https://benjamincongdon.me/blog/2020/04/23/Cancelable-Reads-in-Go/

Seems like there isn't an easy way out...

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I'll have to refactor to use a cancelable reader, using a io.Reader on os.Stdin and test it on Windows hoping it will work fine without all that boilerplate since we are just caring about key presses...

dominikschulz
dominikschulz previously approved these changes Dec 3, 2021
View reviewed changes
Copy link
Member

@dominikschulz dominikschulz left a comment

Choose a reason for hiding this comment

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

The change looks fine and seems to work well.
But I was immedeately thinking that we should maybe keep generating new OTP codes, just like the Autenticator apps do.

@AnomalRoil
Copy link
Member Author

That seems like a nice idea, and quite easy to implement I guess :D
I'll add it to this PR then and try to solve the above Go routine leakage

@dominikschulz
Copy link
Member

Don't feel obliged to add that. I'm not a huge fan of the OTP feature anyway, but since we have it we could try to make it as nice and useful as it can be. Up to you.

@AnomalRoil
Adding a loop for OTP
59f84a4 
RELEASE_NOTES=[UX] OTP code now runs in loop until canceled or used with -o

Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com>
@AnomalRoil
Merge conflict
b5945a7 
RELEASE_NOTES=n/a

Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com>
@AnomalRoil
Copy link
Member Author

It was actually easier to "fix" the Go routine leak by not fixing it and having a loop so that it's mandatory to cancel the loop and there's no risk of having a character being eaten by the leaked go routine when we're back to gopass prompt view.
So I've implemented your suggestion, let me know what you think.

@dominikschulz
Copy link
Member

Seems to work fine. Thank you.

@dominikschulz dominikschulz merged commit adf6254 into master Dec 17, 2021
@dominikschulz dominikschulz deleted the fix-otpbar branch December 17, 2021 19:16
@colemickens
Copy link
Contributor

Looks like another major behavior change, change of default, and no global config for this? Or maybe I skimmed the diff too quickly?

@AnomalRoil
Copy link
Member Author

@colemickens I'm not sure how that's a change of behaviour?
We used to have a "ugly progress bar" for OTP, and we now have a nicer looking one, which has a prompt to quit it.

@colemickens
Copy link
Contributor

It's a big change for anyone that had scripts that relied on "show --clip" and "totp --clip" being one-shot behavior. AFAICT there's no way for me to have the same keyboard shortcut working before without doing something like chaining until with gopass so that it gets killed after a few seconds.

Not a huge deal but it is a change in UX and sadly I just hit another (unrelated) change in gopass yesterday and I'm just tired. No other mission critical tool "moves" under me as much as gopass has over the past couple of years.

@dominikschulz
Copy link
Member

@colemickens I still don't get it.

Before this PR we had a progress bar that you couldn't cancel.
After this PR we still have a progress bar. That you can quit by pressing q.

How would that break show --clip?

We can discuss if we should print the progress bar when --clip is given or not - but that didn't change this PR.

@colemickens
Copy link
Contributor

Unless I'm bad at reading Go lately. the PR seems to add a loop that wasn't there before that causes the OTP generation to loop. I have a keybinding that pops up a terminal, runs gopass [show|totp] --clip and then expects it to exit. Maybe the entirety of this behavior isn't just encapsulated in this PR, but it definitely exacerbated it.

It does seem like there's a short-circuit for when it's being piped, so I can probably workaround this.

@dominikschulz
Copy link
Member

The actual loop was already present before this PR. Try it yourself: git checkout adf6254add0cfd5dd56141bcb14247e43089898b^ (this will get you the state before this was merged) and then go build.

I'm not sure if we should be printing the progress bar at all when --clip is specified, but I don't think this PR should be able to break any workflow.

@dominikschulz dominikschulz mentioned this pull request May 30, 2022
Merged
@AnomalRoil
Copy link
Member Author

Yeah, the fact the progress bar now prints when using --clip is totally a bug.

@AnomalRoil
Copy link
Member Author

BTW, any unexpected behaviour change warrants an issue, don't hesitate to open issue whenever you run into something breaking. Don't hesitate to open issues if you run into anything that breaks your workflow after any update.

kpitt pushed a commit to kpitt/gopass that referenced this pull request Jul 21, 2022
@AnomalRoil
Adding option to exit OTP progress bar (gopasspw#2041)
3e97a29 
* RELEASE_NOTES=[ENHANCEMENT] Adding option to exit OTP progress bar

Signed-off-by: Yolan Romailler <yolan@romailler.ch>

* applying code review comments

RELEASE_NOTES=n/a

Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com>

* patching CI

RELEASE_NOTES=n/a

Signed-off-by: Yolan Romailler <yolan@romailler.ch>

* Adding a loop for OTP

RELEASE_NOTES=[UX] OTP code now runs in loop until canceled or used with -o

Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dominikschulz dominikschulz dominikschulz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@AnomalRoil @dominikschulz @colemickens