Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: allow certain keychain operations without a password #726

Merged
merged 3 commits into from
Aug 5, 2020

Conversation

achingbrain
Copy link
Member

Listing, removing, renaming etc keys do not require a password so
the user should not be required to provide one.

This means we don't have to prompt the user to create a password
when they aren't going to do any operations that require a password.

@jacobheun jacobheun marked this pull request as ready for review August 5, 2020 15:56
@jacobheun
Copy link
Contributor

I've update this to not require a password. I'll create an issue for supporting a password rotate feature. We should be able to set new passwords on they keychain. This will give us the ability to start without one and then "rotate" to one later.

@vasco-santos
Copy link
Member

@achingbrain is this enough for js-ipfs to use libp2p's keychain without a password?

I believe that this needs to be changed: https://github.com/libp2p/js-libp2p/blob/master/src/index.js#L85

achingbrain and others added 3 commits August 5, 2020 18:19
Listing, removing, renaming etc keys do not require a password so
the user should not be required to provide one.

This means we don't have to prompt the user to create a password
when they aren't going to do any operations that require a password.
@jacobheun jacobheun force-pushed the feat/do-not-require-password-for-keychain branch from 088b782 to 386c3d3 Compare August 5, 2020 16:21
@jacobheun
Copy link
Contributor

I believe that this needs to be changed: https://github.com/libp2p/js-libp2p/blob/master/src/index.js#L85

Good catch, I rebased with master and added a test for libp2p creation without a keychain pass.

Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

LGTM! 🚢

@jacobheun
Copy link
Contributor

FYI I verified it fixes js-ipfs, ipfs/js-ipfs#3212. I'm working on bubbling it up there.

@jacobheun jacobheun merged commit 8c56ec0 into master Aug 5, 2020
@jacobheun jacobheun deleted the feat/do-not-require-password-for-keychain branch August 5, 2020 17:03
@achingbrain
Copy link
Member Author

Thanks for pulling this across the line!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants