[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH v2] Add a credential-helper for KDE
From:       Jeff King <peff () peff ! net>
Date:       2011-09-30 10:21:11
Message-ID: 20110930102111.GA24507 () sigill ! intra ! peff ! net
[Download RAW message or body]

On Sun, Sep 18, 2011 at 04:52:58PM +0200, Lukas Sandström wrote:

> This Python script plugs into the credentials API
> of Git to ask the user for passwords with a nice
> KDE password dialog.
> 
> The password is saved in the KWallet.

So I managed to play with this a bit tonight. Overall, it seems pretty
nice.

Initially, it seemed somewhat clumsy. It asked me to open the wallet
(using a password) each time git ran. Which is about as annoying as just
typing my git password each time. :)

The magic trick was to configure kwallet to "keep the wallet open for 10
minutes after the last use" instead of "close when no applications have
the wallet open". Since git runs as many small programs, kwallet has no
real idea of how long a git session is.

This is totally not a kwallet thing, and nothing to do with your helper.
But since the helper is so annoyingly useless without that config, it
might be worth mentioning it in a README.

> Right. Multiple usernames per "unique" context is supported in this version.
> I looked at the git-credential-storage helper when I wrote the first patch,
> which didn't have obvious support for multiple usernames per unique context.

This part passed my tests just fine. Very nice.

> +class CredentialHelper(KApplication):
> +    def __init__(self, token, username = None, desc = None, reject = False):
> +        super(CredentialHelper, self).__init__()
> +        self.password = None
> +        self.username = username
> +        self.save_password = False
> +        self.token = token
> +        self.desc = desc
> +
> +        if not self.token:
> +            return

My tests complained about doing nothing when there is no token. As I've
mentioned elsewhere, this doesn't matter now (as git never invokes the
helper that way), but it would be nice to future-proof the helper by
just ignoring the wallet, but still doing the nice password dialog.

> +    def open_wallet(self):
> +        self.wallet = KWallet.Wallet.openWallet(
> +            KWallet.Wallet.LocalWallet(), 0, KWallet.Wallet.Synchronous)
> +        if not self.wallet.isOpen():
> +            return None
> +        if not self.wallet.hasFolder("GitCredentials"):
> +            self.wallet.createFolder("GitCredentials")
> +        self.wallet.setFolder("GitCredentials")

I peeked around the KWallet manager. There's a "passwords" folder in the
wallet, and I was surprised that the passwords didn't go there. But when
I tried using konqueror to store a password, I found that it also made
its own folder, and then stored a map within it for each URL.

So I'm not really sure if you're following kwallet best practices or
not, as I'm clearly confused about what the "passwords" folder is for.
;)

> +    def check_wallet(self):
> +        (res, data) = self.wallet.readMap(self.token)

So you're just using the token as a big blob. Which is how I
anticipated, but is the complete opposite of what OS X Keychain wants.
Which is leading me to think we should really just hand helpers both
forms: the information broken down by item (e.g., --host=github.com),
and a full URL (e.g., --url=https://github.com/). And then the helpers
can use whatever they like (where you would use "url" instead of the
current "unique").

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
[prev in list] [next in list] [prev in thread] [next in thread]