Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Adds a findCredentials method which yields an array of account/password objects of all matching saved credentials #56 #85

Merged
merged 1 commit into from
Dec 13, 2017

Conversation

siebertm
Copy link
Contributor

@siebertm siebertm commented Nov 7, 2017

This PR addresses the need to find all accounts/password should you want to save multiple accounts in your keychain. Fixes #56

  • tests pass
  • lint passes
  • documentation added

If I missed anything, please tell me and I'll try to fix it.

@siebertm siebertm force-pushed the add-find-credentials branch 3 times, most recently from b965b80 to 7d417db Compare November 8, 2017 08:19
@iolsen
Copy link

iolsen commented Nov 28, 2017

@BinaryMuse would you be willing to review this?

@BinaryMuse BinaryMuse self-assigned this Nov 29, 2017
Copy link
Contributor

@BinaryMuse BinaryMuse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @siebertm! This looks great. I've requested a few small changes, then I think this will be good to merge.

@@ -1,7 +1,8 @@
{
"name": "keytar",
"version": "4.0.4",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind checking out the changes to package-lock.json so that the file isn't modified in this PR?

@@ -68,4 +72,25 @@ describe("keytar", function() {
assert.equal(await keytar.findPassword(service), null)
})
})

describe('findCredentials(service)', function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a test here that checks the behavior when no credentials are found.

};
callback->Call(2, argv);
} else {
v8::Local<v8::Value> argv[] = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd prefer to see an empty array returned here, so users don't have to do null checking on the result of the operation.

@siebertm
Copy link
Contributor Author

@BinaryMuse thanks for your input! I incorporated your comments in my changes now - I actually fell over the null vs [] thing yesterday, too!

This should be ready now.

@BinaryMuse
Copy link
Contributor

Thanks very much! This is great!

@BinaryMuse BinaryMuse merged commit 435129d into atom:master Dec 13, 2017
@BinaryMuse
Copy link
Contributor

Version 4.1.0 has been released, incorporating this change. Thanks again!

@siebertm siebertm deleted the add-find-credentials branch December 14, 2017 08:20
@siebertm siebertm restored the add-find-credentials branch December 14, 2017 08:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Method to retrieve a list of services from the password manager.
3 participants