Bug 41269 – add python keyring support to webkit-patch

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tony Chang]

add python keyring support to webkit-patch

[Tony Chang]

Created attachment 59871 [details]
Patch

[Tony Chang]

This patch isn't that awesome.  It requires that the user have keyring installed and that they have set bugzilla.username in their git config.  Otherwise, it does nothing.  It also doesn't provide a way to set a password and because of different URL formats, I'm not sure if this will work out of the box if you already have gnome-keyring or kwallet with your password.  That is, gnome-keyring seems to use the full URL, and self.host is only "bugs.webkit.org".  Maybe I should try to normalize the host?

[Tony Chang]

Created attachment 59872 [details]
Patch

[Tony Chang]

Comment on attachment 59872 [details]
Patch

Here's the same patch with a prompt to store the password.  I'm not sure I like it because if you don't want to use keyring, you have to say no every time.  Maybe that's not a big deal.

[Eric Seidel (no email)]

See also bug 32728 and bug 36855.

[Eric Seidel (no email)]

Should we just have autoinstall install the keyring module?

[Tony Chang]

(In reply to comment #6)
> Should we just have autoinstall install the keyring module?

I can't seem to get the gnome-keyring integration to work on Hardy which causes it to silently fallback to an encrypted text file.  I'm not sure you want that to happen automatically.

Also, how does autoinstall work with binary python modules?  Will it checkout the source and build it?

[Adam Barth]

Love it.

[Evan Martin]

BTW, the weird syntax in the pw prompt that it seems you copied from my other patch was to match the other prompts in that program -- may wanna consult the surrounding code for how to properly do a y/n prompt.

Realistically, it's fine to ask every time, especially if the default is N.  There's no good reason to not use your keyring and it's not too hard to hit enter a second time.

[Adam Barth]

Comment on attachment 59872 [details]
Patch

Good point Evan.

Please use User.confirm.  Feel free to extend it to support reversing the default or to add another method that does the reverse.

[Tony Chang]

Created attachment 69114 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 69114 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69114&action=review

I think this is great, but there are some nits above.  I'm happy to look again.  It would be OK to land as is, but probably best to go one more round.

> WebKitTools/Scripts/webkitpy/common/net/credentials.py:45
> +    import keyring

Should we just autoinstall this instead?

> WebKitTools/Scripts/webkitpy/common/net/credentials.py:57
> +        self.keyring = keyring

Seems odd to hang this off of self, especially since it's not passed in via the constructor.

Also, despite our earlier lack of understanding of python design.  Adam and I know realize that we should be naming private members to _foo, so this should be _keyring and the other members will eventually get fixed.

> WebKitTools/Scripts/webkitpy/common/net/credentials.py:126
>          if not username or not password:
>              (username, password) = self._credentials_from_keychain(username)

Would we need this apple-specific code if the keyring module was autoinstalled?  If you don't feel comfortable removing it, we could add a FIXME about removing it.

> WebKitTools/Scripts/webkitpy/common/net/credentials.py:139
> +                    "Store password in system keyring?", User.DEFAULT_NO)

I wonder if keyring exposes some platform-specific naming here.  Since on OS X its called the keychain, and I expect windows has a specific name, etc.

> WebKitTools/Scripts/webkitpy/common/net/credentials_unittest.py:124
> +                self.keyring = MockKeyring()

Seems it would be better to pass it as an optional parameter to the constructor, which default to None and then set it in __init__ as self._keyring = keyring or keyring_module?

[Tony Chang]

Created attachment 69118 [details]
Patch

[Tony Chang]

(In reply to comment #12)
> > WebKitTools/Scripts/webkitpy/common/net/credentials.py:45
> > +    import keyring
> 
> Should we just autoinstall this instead?

I looked into how autoinstall is implemented and it won't work for python modules that need to be compiled like this one.  Also, I mentioned above that the default settings on oldering Linuxes is crummy, so I don't think we should autoinstall even if worked.

> > WebKitTools/Scripts/webkitpy/common/net/credentials.py:57
> > +        self.keyring = keyring
> 
> Seems odd to hang this off of self, especially since it's not passed in via the constructor.
> 
> Also, despite our earlier lack of understanding of python design.  Adam and I know realize that we should be naming private members to _foo, so this should be _keyring and the other members will eventually get fixed.

I switched to _keyring and made it a param (which is overridden during testing).  I can switch the other variables over in a separate change since mixing styles seems bad.

> > WebKitTools/Scripts/webkitpy/common/net/credentials.py:139
> > +                    "Store password in system keyring?", User.DEFAULT_NO)
> 
> I wonder if keyring exposes some platform-specific naming here.  Since on OS X its called the keychain, and I expect windows has a specific name, etc.

Doesn't look like it.  This module is pretty simple and doesn't expose many hooks.  You could do something like printing the class name of the backend ('GnomeKeyring', 'KDEKWallet', 'KeyringBackend', 'OSXKeychain', 'UncryptedFileKeyring', 'Win32CryptoKeyring') but that seems hacky.

[Eric Seidel (no email)]

Comment on attachment 69118 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69118&action=review

OK.  We should consider autoinstalling keyring in a later patch.

Thanks again!

> WebKitTools/Scripts/webkitpy/common/net/credentials.py:53
> +                 keyring=keyring):

I'm told that foo=None followed by a self._foo = foo or default_foo  is better here, since keyring will be a singleton and thus shared between multiple instances of the object.

In this case, I think it's OK.  But for example foo=[] would be bad. :)  You wouldn't actually end up with a new [] for each object.

[Tony Chang]

Committed r68599: <http://trac.webkit.org/changeset/68599>