Re: [PATCH] wincred: improve compatibility with windows versions

[Erik Faye-Lund <kusmabite@gmail.com>]

On Tue, Jan 8, 2013 at 5:20 PM, Karsten Blees <karsten.blees@gmail.com> wrote:
> Am 04.01.2013 22:57, schrieb Erik Faye-Lund:
>> The only reason why I used Cred[Un]PackAuthenticationBuffer, were that
>> I wasn't aware that it was possible any other way. I didn't even know
>> there was a Windows Credential Manager in Windows XP.
>>
>
> I believe the Cred* API was introduced in Win2k. The XP control panel applet supports domain credentials only, but cmdkey.exe from Windows Server 2003 can be used on XP to manage generic credentials.
>

Thanks for the background-info.

>> The credential attributes were because they were convenient, and I'm
>> not sure I understand what you mean about the Win7 credential manager
>> tools. I did test my code with it - in fact, it was a very useful tool
>> for debugging the helper.
>>
>> Are you referring to the credentials not *looking* like normal
>> HTTP-credentials?
>
> No, I was referring to creating / editing git credentials with Windows tools manually. For example, changing your password in control panel removes all credential attributes, so the current wincred won't find them any longer...same for git credentials created e.g. via 'cmdkey /generic:git:http://me@example.com /user:me /pass:secret'.
>
> The 'puzzling' part is that those credentials *look* exactly the same as if created by wincred, but they don't work. And wincred isn't exactly verbose in its error messages :-)
>

Right, thanks for clearing that up.

>> But, if we do any of these changes, does this mean I will lose my
>> existing credentials? It's probably not a big deal, but it's worth a
>> mention, isn't it?
>>
>
> Yes, existing stored credentials are lost after this patch. Will add a note to the commit message.
>
> We _could_ try to detect the old format, but I don't think it's worth the trouble.
>

Nah, I don't think it's worth the trouble. It's a bit unfortunate that
people might get stale credentials clogging up the system, but I don't
really thing this is a big deal.

>>>  static int match_cred(const CREDENTIALW *cred)
>>>  {
>>> -       return (!wusername || !wcscmp(wusername, cred->UserName)) &&
>>> -           match_attr(cred, L"git_protocol", protocol) &&
>>> -           match_attr(cred, L"git_host", host) &&
>>> -           match_attr(cred, L"git_path", path);
>>> +       LPCWSTR target = cred->TargetName;
>>> +       if (wusername && wcscmp(wusername, cred->UserName))
>>> +               return 0;
>>> +
>>> +       return match_part(&target, L"git", L":") &&
>>> +               match_part(&target, protocol, L"://") &&
>>> +               match_part(&target, wusername, L"@") &&
>>> +               match_part(&target, host, L"/") &&
>>> +               match_part(&target, path, L"");
>>>  }
>>>
>>
>> Ugh, it feels a bit wrong to store and verify the username twice. Do
>> we really have to?
>>
>> The target needs to be unique, even if two different usernames are
>> stored for the same site under the same conditions. So probably. It
>> just doesn't feel quite right.
>>
>
> I don't really see why you would need several usernames to connect to the same target. I can imagine different credentials for reading / writing, but then the protocol would usually be different as well, wouldn't it? (e.g. http vs. ssh)
>

I can kind of make up some theoretical reasons, but they are a bit exotic ;)

> One of my interim solutions was to remove the username part from the URL entirely. That enabled me to change credentials in control panel (including the username), and wincred would use them. However, that version failed the 'helper can store multiple users' test, so I ended up with storing / checking username twice.
>

I don't think breaking this is a good idea. It just feels a bit silly,
but I see now that other applications does the same duplication. So
let's just stick to it, even if it's a bit icky ;)