[MERGE] Add a get_login method to UIFactory and test get_password

[Vincent Ladeuil]

>>>>> "john" == John Arbash Meinel <john at arbash-meinel.com> writes:

    john> Vincent Ladeuil wrote:

<snip/>

    Martin> Rather than passing the prompt and kwargs to these
    Martin> methods why not let the caller combine them?
    >> 
    >> I think the original intent in get_password (which I duplicated)
    >> was to "let different front ends display different things". I'm
    >> +0.5 on this until I see useful examples.
    >> 
    >> Note that get_boolean add a '[y\n] ?' and that get_login and
    >> get_password add a ': ' at the end of the prompt which seems
    >> appropriate for CLI.
    >> 
    >> But that's a different point than kwargs though.

    john> That was the original intent. It means that the string
    john> being used is relatively fixed, so if you want to
    john> replace it with something else because of a GUI, it is
    john> easy to do, while inspecting an already expanded string
    john> is difficult.

Agreed.

    john> At this point it was a bit of "future proofing" that
    john> hasn't done anything yet, so it may not be the best way
    john> to go about it. But since ui_factory was meant to be a
    john> way to change the interface from CLI to something else,
    john> it seemed reasonable.

Agreed too.

    john> To me, the alternative is to make the interface more
    john> specific and not allow a prompt at all. Rather the
    john> prompt is defined by the function. So we would have
    john> "get_password_for_user_at_host()" (which may just be
    john> get_password()).

I may have to rethink about how I wanted to address bug #72792.

The idea was that if the http server replies with a 401
Unauthorized, bzr may request a username and a password.

Another approach may be to *require* the username in the url so
that we don't need get_login anymore.

This approach will be more in line with the other transports
which do not (or cannot) handle changing the username.  And
anyway, as the username is required to connect, typing it in the
url provides the additional benefits that the right url will be
recorded in locations.conf or branch/parent, those avoiding
typing the username at each use.

In that case the 401 handling will issue a 'Authorization
required, use user at host syntax in your url' message to the user
and just abort.

Thoughts ?

Additionally to the host, http may specify a realm.

So in the existing code we have:

- ssh/ftp/sftp: host fixed, user fixed, password typed
  prompt='SSH %(user)s@%(host)s password'
  prompt='FTP %(user)s@%(host)s password',

- http/https: host fixed, realm fixed, user fixed, password typed.
  prompt='HTTP %(user)s@%(host)s password'

So I'll go with:

get_password(prompt, scheme, host, user, realm=None)

with a default value of prompt:
%(scheme)s %(user)s@%(host)s  %(realm)s password'

with realm='for realm (blabla)' or empty if no realm is
required.

And I keep the idea of letting the other GUI change the prompt if
they desire.

Thoughts ?

Note that I intent to address that point as part of bug #72792
fix, not this one.

    john> ...

    john> v- I'm concerned about this being tested on multiple
    john> platforms, since it seems pretty OS specific. What
    john> would happen on Windows, Mac or FreeBSD?

Good point. They all can be tricked about the standard input,
except... that windows has a different point of view and indeed
the tests failed because the fallback getpass.default_getpass
implementation was used and that's the one I was trying to avoid
because it issued a warning message.

    john> Also, if we do something like this, it might be better
    john> if we exposed a fake file descripter (tempfile.mkstemp
    john> comes to mind).

That didn't work, a tty is awaited.

But even windows check for sys.stdin in the getpass.getpass
implementation, so I think the cleanest way is to wrap getpass
call into a get_non_echoed_password CLIUIFactory and override it
in TestUIFactory (we don't care about the echo being disabled
during tests with is the main point of getpass.getpass) by using
the equivalent of getpass._raw_input.

>>>>> "Martin" == Martin Pool <mbp at sourcefrog.net> writes:

    martin> Also I did a search and see that we already have a
    martin> couple of factories for testing, so perhaps we can
    martin> reuse them:

    martin> tests/workingtree_implementations/test_commit.py|32| class
    martin> CapturingUIFactory(ui.UIFactory):

This one address a very specific need (capturing first-level
update messages) and I think it's better to leave it alone.

    martin> tests/blackbox/__init__.py|124| class
    martin> TestUIFactory(ui.CLIUIFactory):

This one is promising ;-)

By default, TestCase use a SilentUIFactory, but

tests.blackbox.TestUIFactory 
        # hide the progress bar but emit note()s.
    also redirect stdout and stderr

This is also used in TestCase.run_bzr_captured which also provide
stdin redirection to a StringIO.

I think the more pragmatic solution is to:
- move tests.blackbox.TestUIFactory to tests.TestUIFactory,
- add stdin wrapping,
- provide a specific get_non_echoed_password implementation.

That's what this new patch implements, './bzr selftest UI' ok
on Linux, MacOS and Windows.

Thanks for the past and coming remarks ;)

       Vincent

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 185 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20070221/f1b24bd6/attachment-0001.pgp 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fakestdin.patch
Type: text/x-patch
Size: 43211 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20070221/f1b24bd6/attachment-0001.bin