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

Uses the Title of the entry as key Comment if key Comment is not available. #355

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

KuttKatrea
Copy link
Contributor

@KuttKatrea KuttKatrea commented Aug 24, 2022

Uses the Title of the entry as key Comment if key Comment is not available

@dlech
Copy link
Owner

dlech commented Sep 26, 2022

What do you think about the suggestion at #348 (comment)?

It could make sense to go with the KeePass entry title as the default but if we do that, we should add an option so that people can go back to the previous behavior if they want.

@KuttKatrea
Copy link
Contributor Author

As a person that always forget to set the right comment on key creation, I like the idea of using the Keepass entry title as the comment by default, as I feel it to be more "natural" for a Keepass workflow perspective (when using auto-type with the entry selection prompt, you also see the title of the entry).

Having it optional does make even more sense, as there should be people that is more careful when creating their keys.

@mplattner
Copy link

I'd like to see this being merged and yes, I think an option to not using the title field probably makes sense.

@KuttKatrea KuttKatrea force-pushed the feature/use-title-as-comment branch from ff9ee88 to 02c559f Compare April 5, 2023 12:16
@dlech dlech force-pushed the master branch 4 times, most recently from 8fa4e22 to a7af99c Compare December 24, 2023 20:47
@KuttKatrea KuttKatrea force-pushed the feature/use-title-as-comment branch 2 times, most recently from a7517ff to fb208fe Compare January 4, 2024 01:45
@dlech
Copy link
Owner

dlech commented Jan 21, 2024

Since it has been a while, can we get a recap of what problem this is solving (what are the places the user would want to see this)?

Would it be even better if we could show both the KeePass entry title and the SSH key comment instead of one or the other?

@KuttKatrea
Copy link
Contributor Author

The main place the comment is important to identify the key when requesting permission to use it and when the notification when a it is used.

The main use case of this PR is to prefer to manage the "comment" of the key directly using Keepass, because it's more natural for a KeePass user to just edit the Title instead of having to download the keys, running ssh-keygen commands to rename the key, and uploading it again. This also makes easier to identify the actual KeePass entry the used key belongs to.

Im my case, and I would like to hear others opinion on this, I always forgot to set a meaningful name on the key when creating it, so it always ends up being name@hostname. So most of my keys has probably the same comment, so I don't really care to see it. But YMMV.

Having said that, the PR as it is allows to switch between using Title of entry or comment in key, but I don't see any problem in including a third option to "use both".

@dlech
Copy link
Owner

dlech commented Jan 21, 2024

In my case, and I would like to hear others opinion on this,

here is one related opinion: #397

@dlech
Copy link
Owner

dlech commented Jan 21, 2024

In the KeeAgent window and the Select Key dialog, we already have a Source column already that shows the full "path" of the key. This includes the database name, the group and the entry title. It seems like it would be useful to reuse this pattern in case some users use the same title for all of their keys and need the group or database to differentiate.

image

@KuttKatrea
Copy link
Contributor Author

I agree that it makes sense to use the same pattern used in other parts of KeeAgent, of showing both Comment and Source

@dlech dlech force-pushed the master branch 2 times, most recently from bb23ee1 to 61eed65 Compare May 4, 2024 15:46
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