Allow interoperability with 3rd-party conventions for keyring item names

[adobeDan]

This is intended to fix #60. Originally I thought of this as a Windows-only fix, but then I noticed that Mac and Linux have their own conventions to segregate "user" and "service" as well (e.g., application on Linux, keychain on Mac). So I'm going to take a slightly more general approach and introduce an extension mechanism for mapping user and service to the target item that can be varied appropriately on a per-platform basis for those who care to.

[adobeDan]

Hi, @1Dragoon, this code now works on all platforms and is ready for you to try.

Hi, @hwchen, this code refactors the platform code into much smaller chunks and makes the library have platform-independent code that provides all the right documentation. So I am going to go ahead and add error-handling improvements to a branch based off of this one rather than off the updated-depedencies PR.

[1Dragoon]

How can you retrieve the username from the windows key store if you don't already know it and/or don't want to store it in your source code? That was one of the things I addressed with my fork.

FWIW third party applications might count on both the username and passwords being stored in the exact same credential object, so workarounds of using one credential object for the username and another credential object for the password wouldn't work in that case. I drew inspiration from the way the python keyring module does it when implementing mine.

[adobeDan]

@1Dragoon Thanks for the review!

I am trying to understand what scenario of use you are trying to support. I can come up with two of them:

1. (based on how @jaraco's windows implementation works) You are given a username and service. The password has been stored by a 3rd party app that has used one of two "service" names (actually called the "target name" on Windows): either the keyring "service" name or a concatenated name such as "username@service", but you don't know which form it has used. In that case you would have two different IdentityMapper functions: one that ignores the username and one that uses the username. First you would try retrieving the password by creating a keyring using the one that ignores the username, and then if that finds no entry you would create a second keyring using the one that concatenates the username and service. One of those two would succeed.
2. (based on a different way of reading what you wrote above) You aren't given a username or service at all; instead you are given the actual target name of the credential as stored by the 3rd party service, and you know that the 3rd-party service has stored the username in some other field in the credential (such as the comment). In that case you can use a custom IdentityMapper to retrieve the password, but at that point you can't get the username unless we enhance keyring to return those other fields.

Can you clarify which of these two scenarios you are asking about? (I'm guessing the first.)

[adobeDan]

@hwchen This code is now solid and ready for your review. I am basing my error code work on this platform decomposition, so please let me know what you think.

[1Dragoon]

Actually the scenario I'm dealing with is where you just have the service (target) name, and you need to retrieve both the username and the password from it, as both are unknown

[adobeDan]

@1Dragoon I'm happy to enhance the functionality to return more fields than just the password, but there are a bunch of different fields in a credential. Can you be explicit about which ones you care about? And this is only on Windows, right?

[1Dragoon]

Basically just the username and the password. I can't think of a use for any of the other fields. The hard part is figuring out how that would work if you wanted to keep the application code unified across different platforms. This was the reason I created the credential struct in my PR, which basically stored the username if it was passed to the API call. The idea being that you could optionally read from that field, or if you already knew the username beforehand, then you simply pass it through your function calls. I'm pretty new to programming in general though, (been doing it as a hobby for about a year) so I really can't fathom what the ideal approach would be.

[phillipCouto]

@adobeDan great work and I am reviewing the changes a few things I noticed immediately that are of concern:

• Rework of the CI configuration
• Dependency updates and additions

There is #55 that is already introducing dependency updates and I am wondering is it better to have the updates posted in this change posted in that one and merged ahead of this one?

I think it would be best to pull the CI changes out into a separate PR for review so they can be merged ahead of the breaking changes introduced by this change.

[phillipCouto]

Basically just the username and the password. I can't think of a use for any of the other fields. The hard part is figuring out how that would work if you wanted to keep the application code unified across different platforms. This was the reason I created the credential struct in my PR, which basically stored the username if it was passed to the API call. The idea being that you could optionally read from that field, or if you already knew the username beforehand, then you simply pass it through your function calls. I'm pretty new to programming in general though, (been doing it as a hobby for about a year) so I really can't fathom what the ideal approach would be.

So this change would be a stepping stone in adding the ability to store multiple credentials for a service. Keyring will need its API expanded to allow for searching for credentials by service only. So far I think it is possible but I haven't read through thoroughly how this would be implemented in the different OSes.

@adobeDan it may be best to get this change merged first and then we can expand the Keyring API in separate PR to address the functionality requirements of @1Dragoon.

[hwchen]

It looks like some of this work was based on #66 (ci and dependency rework), so it would be a good idea to rebase so that it'll be easier to review

[adobeDan]

Hi @phillipCouto, thanks for the review! To the issues you raised:

There is #55 that is already introducing dependency updates and I am wondering is it better to have the updates posted in this change posted in that one and merged ahead of this one?

Yes, I think @hwchen has now done a release (0.10.4) of #66 that has just the dependency updates and the CI changes. (I have now closed #55.) That one has no API changes (a true patch/minor, depending on how you view a leading 0 in the server world :), so this one now builds on that.

@adobeDan it may be best to get this change merged first and then we can expand the Keyring API in separate PR to address the functionality requirements of @1Dragoon.

Yes, I agree that we can get theses changes without the additional enhancement. My current thinking is:

• The error handling changes are the most urgent to get in, and they will introduce API breaks, so will require a major version change.
• This PR also introduces API changes and, although they are backward-compatible (extensions), they require a minor version change.
• If I understand @1Dragoon's request properly, it will also require an API change. That one may also be fully backward-compatible, but I can't be sure until I work it through.
• If I can close off on the enhancement in just a few days, which I think I can, it will probably be worth just releasing all the changes in the major release.

Does this make sense to you?

[adobeDan]

It looks like some of this work was based on #66 (ci and dependency rework), so it would be a good idea to rebase so that it'll be easier to review

Hi @hwchen, good point, but because of the later CI fixes doing the rebase now will require a force-push which I am loathe to do. Given that you've now released #66, all the merge changes are resolved and this is now clean against master. Are you OK if I just leave this as is?

[hwchen]

Ah, since you merged in master, that's fine it's the same to me.

[phillipCouto]

From a code point of view everything looks good, I didn't see anything of concern with the changes and the changes will allow for more advanced features to be added without disturbing the core features originally added.

I also tested the changes in our application on the macOS side and it worked without any issues but I wasn't able to test on the windows side as the update to the security-framework introduced recently impacted other dependencies but I am confident there will be no issues.

[adobeDan]

All of the work in this PR is now in #70 as well, plus some more enhancements (like naming keychains). It definitely looks like we are headed for a 1.0 release that has all of this in it (and changes the API).

I'm going to close this PR in favor of #70. @phillipCouto I've fixed the security issues on Windows so please try it that PR out there now. @hwchen please focus your review on #70. Thanks!