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

make sure email addresses used with 'tell' and 'killperson' exist in keyring #267

Merged
merged 8 commits into from
Sep 25, 2018
Merged

make sure email addresses used with 'tell' and 'killperson' exist in keyring #267

merged 8 commits into from
Sep 25, 2018

Conversation

joshrabinowitz
Copy link
Collaborator

@joshrabinowitz joshrabinowitz commented Sep 25, 2018

This PR a fix for #176.

#176 is a bug with tell which adds all keys with a substring matching the specified username. For example, git secret tell bob will add all keys with the string bob anywhere in the username or email address.

Here's the details:

  • each gpg key is associated with an email address, and optionally a name.
  • gpg does substring matching against both the email address and the username when looking for keys matching a string.

Also,

  • the nameassociated with a key is not guaranteed to be unique and
  • the email can be a substring of other names or emails

For the above reasons, this PR changes the code so you have to specify users by email address with git secret tell. (Edit: The docs always said to use email addresses so there is no doc change.)

@sobolevn @simbo1905 thoughts/suggestions/input?

* rename and add function to get emails from keyrings
* rename directories holding gpg test fixtures
@joshrabinowitz
Copy link
Collaborator Author

joshrabinowitz commented Sep 25, 2018

This PR does still have a bug where if

then

  • using git secret tell [email protected] will add keys for both users and not just the one you probably meant.

I don't see an easy way to fix that and figured we could tackle that in a separate PR.

If you see a good way to fix this issue, or have an other input on this PR, please let me know @sobolevn @simbo1905 @dave-pollock

Copy link
Collaborator

@simbo1905 simbo1905 left a comment

Choose a reason for hiding this comment

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

LGTM. I left a minor comment.

src/commands/git_secret_killperson.sh Outdated Show resolved Hide resolved
@joshrabinowitz joshrabinowitz changed the title make sure email addresses used with 'tell' exist in keyring make sure email addresses used with 'tell' and 'killperson' exist in keyring Sep 25, 2018
@joshrabinowitz
Copy link
Collaborator Author

I think this is ready for merge @sobolevn !

Copy link
Owner

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

🙌

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