Bug 41269 - add python keyring support to webkit-patch
Summary: add python keyring support to webkit-patch
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Tony Chang
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-06-27 20:07 PDT by Tony Chang
Modified: 2010-09-28 15:56 PDT (History)
5 users (show)

See Also:


Attachments
Patch (1.76 KB, patch)
2010-06-27 20:48 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (2.17 KB, patch)
2010-06-27 21:10 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (7.66 KB, patch)
2010-09-28 15:00 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (7.84 KB, patch)
2010-09-28 15:34 PDT, Tony Chang
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Chang 2010-06-27 20:07:58 PDT
add python keyring support to webkit-patch
Comment 1 Tony Chang 2010-06-27 20:48:45 PDT
Created attachment 59871 [details]
Patch
Comment 2 Tony Chang 2010-06-27 20:51:20 PDT
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?
Comment 3 Tony Chang 2010-06-27 21:10:26 PDT
Created attachment 59872 [details]
Patch
Comment 4 Tony Chang 2010-06-27 21:11:41 PDT
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.
Comment 5 Eric Seidel (no email) 2010-06-27 21:25:52 PDT
See also bug 32728 and bug 36855.
Comment 6 Eric Seidel (no email) 2010-06-27 21:26:40 PDT
Should we just have autoinstall install the keyring module?
Comment 7 Tony Chang 2010-06-27 21:34:11 PDT
(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?
Comment 8 Adam Barth 2010-06-27 22:26:08 PDT
Love it.
Comment 9 Evan Martin 2010-06-28 11:20:40 PDT
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.
Comment 10 Adam Barth 2010-06-28 11:23:53 PDT
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.
Comment 11 Tony Chang 2010-09-28 15:00:46 PDT
Created attachment 69114 [details]
Patch
Comment 12 Eric Seidel (no email) 2010-09-28 15:14:59 PDT
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?
Comment 13 Tony Chang 2010-09-28 15:34:06 PDT
Created attachment 69118 [details]
Patch
Comment 14 Tony Chang 2010-09-28 15:37:59 PDT
(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.
Comment 15 Eric Seidel (no email) 2010-09-28 15:38:43 PDT
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.
Comment 16 Tony Chang 2010-09-28 15:56:09 PDT
Committed r68599: <http://trac.webkit.org/changeset/68599>