andrew-schofield / keepass2-haveibeenpwned

Simple Have I Been Pwned checker for KeePass
MIT License
423 stars 24 forks source link

"Double click to edit" possibly misleading #40

Open s-h-a-d-o-w opened 6 years ago

s-h-a-d-o-w commented 6 years ago

I just updated two of my throwaway passwords through the breached passwords dialog and when I was done, I lost access to those services because as it turned out... Keepass didn't actually store the edited passwords.

The odd thing is - after editing the passwords, I double clicked the entries again to double check whether the passwords had really been changed. And they were there. So... somehow, the passwords get updated in the breached passwords dialog but aren't transferred to the actual database. Might it have to do with the fact that Keepass locked itself in the background while I was editing the passwords?

Anyway, - if you can't ensure that the edited passwords actually are updated in the database on closing the dialog, I think that it would be great if you could warn the user in bold red text or not allow editing from that dialog from the get-go...

andrew-schofield commented 6 years ago

Double click to edit should be opening the entry in the database not a copy from the breached password dialog. Did you try opening the entries from the main keepass window after closing the breached entries dialog? Also, editing entries will not automatically save the database.

I'm unsure how you determined that keepass didn't store the passwords.

If the database locks itself in the background then this definitely is a problem, the dialog should ideally be disabled until then database is unlocked again.

s-h-a-d-o-w commented 6 years ago

Alright, my process step by step:

andrew-schofield commented 6 years ago

Interesting, so I just tried to replicate this with a dummy entry which I know has a breached password.

I'll need to test this again to see what happens when the database is locked while the breach dialog/editor dialog are open.

s-h-a-d-o-w commented 6 years ago

Well... that workflow makes you return quicker from the breached password dialog to the main window but it still assumes that you do that within your set "lock time". Which is 1 minute in my case... (And apparently, you can't assume that everybody does things the way you do them. ;) )

But I suppose the breach dialog getting disabled when the database locks would indeed be one way of resolving this... in my case, it obviously stayed enabled.