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

allow automatic expiration date extension on pwd modification (Addresses issue #2010) #6456

Draft
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

xvallspl
Copy link
Contributor

@xvallspl xvallspl commented Apr 27, 2021

Addresses issue #2010

Creates a new Expiration Settings area in the Edit Entry widget, composed
of the expiration date settings and a new optional feature that allows for
automatic extension of the expiration date on password modification.

Allows for randomization of the expiration date, within an interval (set by the user) of days from the deadline

Tasks left:

  • Discuss and gather feedback
  • Persistify the changes
  • Change the time menu to quantity - time unit pickers
  • Fix UI spacing
  • Document the new features

Screenshots

Extension

Testing strategy

Manual testing (?)

Type of change

  • ✅ New feature (change that adds functionality)
  • ✅ Documentation (non-code change)

@xvallspl xvallspl force-pushed the feature/update-pwd-expiration-date branch 2 times, most recently from 3a31a80 to 34a7024 Compare April 27, 2021 16:16
@droidmonkey
Copy link
Member

Oh that's gorgeous

@xvallspl
Copy link
Contributor Author

xvallspl commented May 4, 2021

I coded myself into a hole a couple of times and had to pluck a lot of code afterwards, but I updated the PR with a commit that provides better granularity for the extension by switching to two pickers, one for the quantity and another one for the period of time (updating the screenshots right now).

I need to store the extension information for the entry between sessions. I added it first into the TimeInfo class, but @phoerious pointed out to me that there is a kdbx spec to follow and I can't just add more fields and expect them to be stored. I was suggested to use Custom data, which until now I (think) was only been used for the browser extension.

The commit introduces several states more into the application, but I think I tested all of them manually.

@michaelk83
Copy link

michaelk83 commented May 7, 2021

After some more thought on my #2010 comment, I think the "Auto assign new ..." checkbox can just say "Update when password is changed.", and should be checked by default. The expiration settings can have a label "Interval:", and can default to the what you get when you subtract the last password update date from the current expiry date, capped to some some sensible minimum like 24 hours.

Or maybe reformat the whole Expiration settings block like so:

Last updated:    2021-04-04 07:34
[✓] Expires after: [  30|±] [days   |▼]  [ Presets |▼]
[✓] Repeat when password is changed.
Next expiration: 2021-05-04 07:34

And move this right under the password field, so it's more clearly associated with it.

edit: If we want #6451, then after the "Expires after:" line, we can add a similar "Randomize by:" line. And when expiry date passes, change "Next expiration:" to a bold red "Expired on:".

@droidmonkey , what do you think?

@xvallspl
Copy link
Contributor Author

xvallspl commented May 7, 2021

I like the rewording of the checkbox but I'm not sure about checking it by default, sounds like a user choice and would require assuming defaults.

In the reformatting you miss being able to set a specific date for the first expiration, which is useful when you have a password that is halfway through its expiration time but you are introducing it in KeePassXC for the first time.

@michaelk83
Copy link

I like the rewording of the checkbox but I'm not sure about checking it by default, sounds like a user choice and would require assuming defaults.

See my comment on #2010 for the reasoning. The default can be calculated as: <expiry date> minus <last password update date>. The reformatted version doesn't need a default, since the expiry interval becomes the primary setting.

In the reformatting you miss being able to set a specific date for the first expiration, which is useful when you have a password that is halfway through its expiration time but you are introducing it in KeePassXC for the first time.

If you have the "Last updated" label, then one can fairly easily figure out the interval for whatever date they want. The "Next expiration" label gives immediate feedback, and if it comes out wrong, the user can adjust the interval.

I agree this is less comfortable for this particular use case, but OTOH, how often do users actually want a specific expiry date, vs an interval? (Note: the interval is always from the last update date, not from when the interval is set.)

@xvallspl
Copy link
Contributor Author

Added randomization, updated the screenshot

@xvallspl xvallspl force-pushed the feature/update-pwd-expiration-date branch from a7a4c78 to 70f2862 Compare May 10, 2021 18:01
Addressed issue #2010

Creates a new Expiration Settings area in the Edit Entry widget, composed
of the expiration date settings and a new optional feature that allows for
automatic extension of the expiration date on password modification.
Switches presets for quantity and unit of time pickers. Stores auto extension
on modification data as custom data for the entry to respect the spec
@tunbridgep
Copy link

tunbridgep commented Sep 4, 2021

I had an alternate idea for how to handle this, see here, and I think the UI in my suggestion is more streamlined in a usability sense.

#6890

Basically, instead of "extending" the date, you instead set the expiry to be today + some relative date upon updating the password field of the entry (and no other field, since we don't want things like updating the browser settings to extend password expiry).

It would also be good to be able to have a preset expiry interval. That is, the expiry date will always be set to, say, the X of the current month, or if it's in the past, the X of next month. This would be useful for, say, employers with rules like "passwords must be changed on the first of every month"

@michaelk83
Copy link

I'm still in favor of the reorganization I suggested in my comment above.

If wanted, the "Next expiration" can be made editable too, so the user can pick which way to update: if they change the "Expires after" fields, it automatically recalculates the next expiration; if they edit the next expiration field, then the "Expires after" are recalculated. Just need to be careful to avoid recursion (don't recalculate the active field).

@shemeshg
Copy link
Contributor

shemeshg commented Oct 13, 2021

I'm still in favor of the reorganization I suggested in my comment above.

If wanted, the "Next expiration" can be made editable too, so the user can pick which way to update: if they change the "Expires after" fields, it automatically recalculates the next expiration; if they edit the next expiration field, then the "Expires after" are recalculated. Just need to be careful to avoid recursion (don't recalculate the active field).

See: #7029,

A spinner between the QDate and the Dropdown might be better then using
"On password change" logic for "current expiration date"
this will distinguish between current expiration value, and expiration On password change

Maybe even adding a thin <hr/> line between the first and the other lines (2nd and 3rd) would make it even more clearer

#7029 can be added to this PR by @xvallspl and #7029 would be deleted/closed

@droidmonkey
Copy link
Member

Concur with combining the two

@michaelk83
Copy link

The last update to this PR was 5 months ago, and it's still marked as draft, so looks like it may be stuck..
@xvallspl , do you need someone to take over here?

@xvallspl
Copy link
Contributor Author

@michaelk83 No, I still intend to merge it in by the end of the year.

@jab416171
Copy link

@xvallspl what's the status on this PR?

@xvallspl
Copy link
Contributor Author

It's stale and looking for someone to take over. I couldn't get it in before the end of the year and I shouldn't lie to myself about finding time for it anymore.

@OdinZhang
Copy link

greate feature, when will be merged? I wanna this feature too.

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

Successfully merging this pull request may close these issues.

None yet

7 participants