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

[New feature] Show expired passwords on database open #4624

Closed
ba32107 opened this issue Apr 18, 2020 · 42 comments · Fixed by #7290
Closed

[New feature] Show expired passwords on database open #4624

ba32107 opened this issue Apr 18, 2020 · 42 comments · Fixed by #7290

Comments

@ba32107
Copy link
Contributor

ba32107 commented Apr 18, 2020

Summary

  • As a user, I would like to be notified by the application about my expired passwords, so that I can fix/regenerate them whenever required.
  • As a user, I would like the application to proactively notify me about the expired passwords in some visual way, so that I don't have to keep in mind that I periodically need to check if there are any expired ones.

I know that this has been discussed multiple times (see #2664, #3092 and #623), and those issues were closed in favor of #551.

I have now just pulled the latest develop branch to check out the Password Analyzer feature, and I don't see how the feature helps me with this problem. According to @wolframroesler, this notification is implemented in the feature (see #551 (comment)), but I don't understand how it works. If I go into Database -> Reports... -> Health Check, I indeed see my expired passwords, but, this is not a proactive notification. I need to go into a menu and open a separate dialog to see this.

Don't get me wrong, I think the Health Check feature is great, but I do believe that it does not solve this issue. If I misunderstood how this feature works, then please let me know.

Since I really really need this feature, I am happy to implement it, but would like to start only if the maintainers agree that this is something that can be merged.

Recommended implementation

Two possible implementations:

  • I like the way the original KeePass does this. Whenever the database is unlocked,
    it groups together all expired entries into a temporary, "virtual" group, which is displayed by default, so you see all your expired entries. If you navigate away to another group, the virtual group disappears, and the application behaves as normal from that point on.
  • A simpler approach is just showing an alert window or some sort of notification (only once) which says "You have expired entries!", after the database is unlocked. The notification should not be shown again, so there is no continuous notification service involved.

For either approach, we will add an option to the settings called "Warn me about expired passwords on database open", which is disabled by default, so that the existing functionality is completely unchanged, and users who don't want this will not be warned.

Context

I have a lot of passwords, organized into multiple groups, and I set relatively short expiry times (1-3 months). My root group is empty, it does not contain any passwords, only subgroups. This means that whenever I open and unlock my database, by default I don't see any of my passwords (hence I don't see the expired ones being crossed out). I usually just unlock the application and minimize it.

Sometimes I notice one expired password, then I go through my whole database only to realize there are actually 10 passwords that have long expired.

@droidmonkey
Copy link
Member

I'd prefer to implement this with the new notification panel that I intend to implement for 2.7.0.

@droidmonkey droidmonkey added this to the v2.7.0 milestone Apr 18, 2020
@paddylandau
Copy link

I'm adding my voice to this, please.

I previously used KeePass2, and its automatic notification when unlocking was useful and valuable.

This is particularly important with a large database. I have hundreds (!) of passwords gathered over years of using the internet.

Thank you

@LetMeR00t
Copy link

Hi,
For the moment, a good workaround is to use reports :)
Under "Database" -> "Database Reports"
On this view, you can have a list of possible issues with your passwords, including the fact that they are expired :)

@paddylandau
Copy link

… a good workaround is to use reports … > Under "Database" -> "Database Reports"

I've tried this, but unfortunately the expired entries are simply not listed.

@wolframroesler
Copy link
Contributor

I've tried this, but unfortunately the expired entries are simply not listed.

They should be. Do they show up when you set the "Also show entries that have been excluded from reports" check mark?

Screenshot from 2020-10-26 19-23-21

@ba32107
Copy link
Contributor Author

ba32107 commented Oct 26, 2020

For the moment, a good workaround is to use reports :)

I disagree, personally this does not help me at all. I can search for expired passwords at any time if it bumps into my mind - but the whole point is that I shouldn't have to keep this in mind, the application should notify me in some shape or form.

I'd prefer to implement this with the new notification panel that I intend to implement for 2.7.0.

@droidmonkey could you elaborate on this notification panel? I searched the issues but found nothing. How does it work under the hood, and how does the user interact with it? Is there an open issue for it? How will the alert about the expired passwords be integrated into that panel?

@droidmonkey
Copy link
Member

A workaround is not a direct solution to a problem, but something you can get by with in the interim.

The notification system hasn't been designed yet except for in my mind.

@ba32107
Copy link
Contributor Author

ba32107 commented Oct 26, 2020

I know what @LetMeR00t meant by workaround and I know it wasn't supposed to be a complete solution. I wasn't trying to dismiss that recommendation, sorry if it came across that way - I am sure that it's valuable for some people. I was trying to point out that for me personally, it does not even qualify as a workaround. From my perspective, using the Database Report function is no different than manually going through all my groups and check for crossed out entries.

I'm not sure if you have any rough timelines on when the notification system would be completed, but I would just like to repeat my offer of delivering a simple notification just for expired passwords (as described in my top comment). Implementing the whole notification system is beyond my current C++ skills and free time I can dedicate to this, but I'd be happy to implement a simple version. This issue was first raised more than 3 years ago and I've seen a lot of people adding their vote to it, so it would be great to have it in the next release.

Since currently this issue is planned for 2.7.0, I guess doing the simple version doesn't make sense, however if there is any chance that the notification system might be pushed back to the future, then I think my recommendation is definitely worth considering.

@LetMeR00t
Copy link

I'm not saying that this workaround could fit this feature and say "well, we can close it as we have the information in the reports".
We need a true solution of course, but this workaround is enough to work with for the moment until a solution is implemented.

@paddylandau
Copy link

I've tried this, but unfortunately the expired entries are simply not listed.

They should be. Do they show up when you set the "Also show entries that have been excluded from reports" check mark?

Curious. I don't have that option — not at the bottom where yours is, or anywhere else that I can find.

My KeepassXC version is 2.6.2 (from the snap package), which is the latest version according to the download pages.

@droidmonkey
Copy link
Member

Are you sure you are running 2.6.2, verified in the about dialog?

@paddylandau
Copy link

Are you sure you are running 2.6.2, verified in the about dialog?

Yes. The snap listing shows 2.6.2, and here's the screenshot of the About screen.

KeepassXC About

@rschulzeLE
Copy link

I'm just here to let you know that I'm also desperately be waiting for this feature. Important usecase: I'm keeping track of a few HTTPS certificates that can't be updated automatically, so I've set an entry in KeePassXC with the expiring date of the certificate. With former KeePass2, this worked very well since I got proactive notifications if a cert needed renewal. With KeePassXC, I've missed to update certificates multiple times, which is nasty since I can't update expired certs remotely but must access the hardware of the concerned machine. This caused me a lot of extra work, so I can no longer track this data with KeePassXC.

To cut a long story short: I'd be so happy to see this feature implemented sooner or later. Thanks a million for your great work.

@droidmonkey
Copy link
Member

Recommend using a calendar reminder for important events like that. We are not a reminder service. You can also use the database reports to see all expired entries.

@rschulzeLE
Copy link

That's true, but since I'm storing the private key in KeePassXC it seems logical to use the "expire" function to mark the key expiration date. Now, I need to set an additional reminder, as long as I've used KeePass2 this wasn't necessary - I simply unlocked my database in the morning and had a wonderful overview about entries that need attention.

It was wonderful that KeePass2 showed not only expired entries but also those who would expire within the next 10 (or 7?) days. For me, this was really helpful and simplified my workflows. So I agree this isn't the main task of KeePassXC but it would add value to it from my point of view.

@paddylandau
Copy link

I disagree. Passwords are the core function of KeePassXC. Therefore, alerting you to problems — such as expired entries — is a valid and important function of a password manager. I truly feel that this one addition would add relevant value to KeePassXC.

@michaelk83
Copy link

FYI, new alternative solution: Automatic password renewal.

@Glandos
Copy link

Glandos commented Aug 10, 2021

This alternative solution isn't able to cover the use case of expired Windows password, since it can't renew it by itself.

Once my enterprise Windows password expires, I can't login to anything, so I have to change it before it expires. And manually. So I must be notified.

@droidmonkey droidmonkey removed this from the v2.7.0 milestone Oct 2, 2021
@ba32107
Copy link
Contributor Author

ba32107 commented Jan 15, 2022

Hello, it is now 1.5+ years after I opened this issue, but still there is no solution for this. I would again like to offer my help to implement this feature, but I will only start if I can get agreement from the maintainers.

@droidmonkey you said you wanted to implement this as part of some new notification panel. How is that going? Is there a GitHub issue I can follow?

@droidmonkey
Copy link
Member

We have not worked on this

@ba32107
Copy link
Contributor Author

ba32107 commented Jan 15, 2022

Okay - so what do you think about my suggestion?

@droidmonkey
Copy link
Member

If you make it tasteful and an optional feature (or a don't show me this again type thing) then sure. You should also honor the exclude from reports flag.

@tleepa
Copy link

tleepa commented Jan 20, 2022

I can see the PR is about showing expired entries on database unlock.
I am curious if you thought about entries that will expire in the coming days? Like in this comment?

@ba32107
Copy link
Contributor Author

ba32107 commented Jan 20, 2022

I agree it would be useful, but not sure how it would fit into what I implemented. Would it mix expired entries with "almost expired" ones? Or does it show two separate lists? I think anything more complex than what I implemented might be for the notification system that droidmonkey is planning to implement.

@droidmonkey
Copy link
Member

droidmonkey commented Jan 20, 2022

The setting could be changed to a drop down choice:

Show expiring entries on database unlock:

  1. Disabled/Never
  2. 5 days before expiration
  3. 2 days before expiration
  4. When expired

Something like that.

@tleepa
Copy link

tleepa commented Jan 21, 2022

[...] Would it mix expired entries with "almost expired" ones? Or does it show two separate lists? [...]

KeePass is showing expired and soon-to-expire entries on the same list, so I guess this would be fine.

2. 5 days before expiration
3. 2 days before expiration

Could it be configurable? I think, in some cases, 5 days may not be enough? Maybe 7 and 3 days before?
I know, it is a matter of personal choice and we will have hard time agreeing on just two fixed options :)

@paddylandau
Copy link

The setting could be changed to a drop down choice:

A choice is always good.

  1. Disabled/Never
  2. 5 days before expiration
  3. 2 days before expiration
  4. When expired

Please don't make the mistake of making decisions for the user. Rather have a simple choice:

"Remind ___ days beforehand"

0 means on the day of expiry. Blank means don't remind at all. I would suggest that a negative number be allowed (which means days after expiry), because who knows who would want it and why?

@michaelk83
Copy link

Blank means don't remind at all.

That's unintuitive. Just use a checkbox before:

[✓] Show expiring entries on database unlock [ 5][±] days before expiration.

@paddylandau
Copy link

That's unintuitive. Just use a checkbox before:
[✓] Show expiring entries on database unlock [ 5][±] days before expiration.

Yes, that's much cleaner, thank you.

@ba32107
Copy link
Contributor Author

ba32107 commented Jan 21, 2022

I agree these would be useful, and I'm happy for someone to iterate over my feature to implement this (or maybe I could do it at a later date). However, I'd prefer to have my PR merged as it is for now, so it could make the next release. I am very short on personal free time and can't easily spend much on this.

@droidmonkey
Copy link
Member

We're all in the same boat with free time 😭

@ba32107
Copy link
Contributor Author

ba32107 commented Jan 22, 2022

I took a stab at it after all:
image

@paddylandau
Copy link

I'd like to add a caveat.

It's a good time to display the expired entries when I'm unlocking in order to view the main window of KeePassXC.

But, it's not a good time to display the expired entries when I've activated an auto-type, and I have to unlock due to timeout (Tools > Settings > Security > Lock databases…).

This is probably obvious, but I'd like to make it explicit.

@droidmonkey
Copy link
Member

droidmonkey commented Jan 22, 2022

That's a great point. There should be a timeout (perhaps once a day) where the expired entries are shown if re-unlocking the database.

@ba32107 not a fan of two checkmarks and two settings for this. One checkmark and the days picker is sufficient.

[ ] Show expired entries on unlock. Include entries that expire within: [ X Days]

@ba32107
Copy link
Contributor Author

ba32107 commented Jan 22, 2022

I did it like that first, but for me, a 0 day timeout didn't make it sufficiently explicit that it means "already expired".

IMO this way it's very clear: the first checkbox refers to already expired, and the second sub-setting refers to future expires. The UI has a tiny bit more text, but it's more user friendly IMHO.
(I wouldn't be storing 3 settings in the DB for this, it's just a UI thing).

I can change it if you feel strongly about it.

@droidmonkey
Copy link
Member

You could use this when the minimum is set to 0: https://doc.qt.io/qt-5/qabstractspinbox.html#specialValueText-prop

When the value is at minimum it will display the text instead of the number. So you could show "Expired" for example. (I don't agree that we need negative days, that makes no sense as Expired is Expired)

@ba32107
Copy link
Contributor Author

ba32107 commented Jan 22, 2022

Cool idea! Current state:

image

image

I like this much better. Thoughts?

@droidmonkey
Copy link
Member

Oh man that is perfect!

@tleepa
Copy link

tleepa commented Jan 22, 2022

Just to clarify: if one set 'show entries that will expire in X days' - the already expired entries would be shown as well?

@ba32107
Copy link
Contributor Author

ba32107 commented Jan 22, 2022

Correct

@Glandos
Copy link

Glandos commented Jan 22, 2022

So maybe the wording could be "will expire in less than 5 days"

@michaelk83
Copy link

So maybe the wording could be "will expire in less than 5 days

within 5 days

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

Successfully merging a pull request may close this issue.

9 participants