[MERGE] NotATerminal considered harmful

[Jelmer Vernooij]

Vincent Ladeuil wrote:
> Keeping the first cover letter as it still applies.
>   
>>>>>> "vila" == Vincent Ladeuil <v.ladeuil+lp at free.fr> writes:
>>>>>>             
>
>     vila> We need to query the user name to fix
>     vila> https://bugs.launchpad.net/bzr/+bug/256612.
>
>     vila> Jelmer is working on implementing ui.get_username() and while
>     vila> discussing his patch we came to the conclusion that a bit of
>     vila> refactoring was needed at the CLIUIFactory level.
>
>     vila> Currently, CLIUIFactory mandates that stdin is a tty and raise
>     vila> NotATerminal otherwise.
>
>     vila> But it does so only when trying to get a password, not when
>     vila> asking for a 'y/n' boolean.
>
>     vila> It also doesn't care about the terminal encoding *except* when
>     vila> querying for the password. It doesn't care about the terminal
>     vila> encoding to encode the queried password either (but I don't
>     vila> address that here).
>
>     vila> Checking the terminal and encoding the prompt sounds logically
>     vila> like prompt() responsibility so I did that first.
>
>     vila> Some tests began to fail then because they expected to be able to
>     vila> query for booleans even when stdin was redirected.
>
>     vila> At that point I realized that redirecting stdin was more useful
>     vila> than requiring the use of a real terminal (even for password
>     vila> handling)
>
>     vila> So unless somebody can show me a security problem doing so, I'd
>     vila> like to get rid of that constraint since it isn't implemented for
>     vila> all cases (the alternative is to fix the offending tests, err,
>     vila> test (blackbox test break-lock).)
>
>     vila> The attached patch does:
>
>     vila> - delete the NotATerminal exception,
>
>     vila> - factors out the terminal encoding handling in prompt()
>
>     vila> - simplify get_non_echoed_password() which is CLIUIFactory
>     vila>   specific,
>
>     vila> - clean up some duplications.
>
>     vila> I checked bzr-gtk and qbzr and they are not impacted by these
>     vila> changes so I don't think I need the deprecation dance for
>     vila> NotATerminal, get_prompt and get_non_echoed_password.
>
> Yet, this patch didn't allow bzr to reliably query a password if run
> with no terminal.
>
> The attached one does.
>
> I couldn't find a way to reliably simulate running without a terminal
> while running from a terminal while still making getpass.gepass believes
> it was running without a terminal (simulating isatty() is not enough
> since getpass() then uses termios tricks and 2.5 and 2.6 do that
> differently...) and I thought it wasn't really worth the effort because,
> in the end, people running bzr with no controlling terminals are
> unlikely to do so with branches requiring authentication, and if they
> do, they'll get a authentication error anyway (giving them a
> NotATerminal traceback is just hiding the real cause).
>   
bb:approve

Cheers,

Jelmer