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

shell: account expiration and password expiration #2062

Merged
merged 2 commits into from Jul 26, 2017

Conversation

Projects
None yet
5 participants
@fridex
Copy link
Contributor

commented Mar 30, 2015

Based on #644

@andreasn andreasn self-assigned this Apr 7, 2015

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2015

I was going to try this one out again today, but I think it bitrotted :/
Would you be able to rebase it on top of master?

@fridex fridex force-pushed the fridex:account_expiration branch from 7a1d835 to fca9d63 Jun 9, 2015

@fridex

This comment has been minimized.

Copy link
Contributor Author

commented Jun 9, 2015

Rebased. It's been some time, hopefully I didn't break anything.

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2015

I created a user and selected "Force user to change password on next login", but when I tried to log in with that user I got "Authentication token is no longer valid; new one required"

The "Force user to change password on next login" is also a very long label. I would expect it to become even longer in some translations, so this needs to be shortened.

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2015

I think it will need to throw up some kind of dialog to change the password (and only that?). Allowing you to set it there, and then bring you back to the login screen.

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2015

Some previous art from FreeIPA

screenshot from 2015-06-24 15-57-07
screenshot from 2015-06-24 15-56-22

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2015

When the user account expires, does that mean that it gets deleted, or that the account gets locked out?

@fridex

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2015

When the user account expires, does that mean that it gets deleted, or that the account gets locked out?

Locking an account means disabling user's password (the user is unable to log in). When an account is expired, user is notified after entering a password (and logged out). "usermod" is used, you can find details in "man usermod"

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2015

Instead of "Force user to...." I would suggest the string "Require password change on next login". Force feels too aggressive.

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2015

Is the number 99999 days the system default for "Number of days between password change", or is that something custom that's part of your pull request?

@stefwalter

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2015

I think it will need to throw up some kind of dialog to change the password (and only that?). Allowing you to set it there, and then bring you back to the login screen.

The login screen would need to adapt, change its text to ask for a new password to be entered twice. You can see that Windows does this. I think GDM might do this too.

This happens at the PAM level. Therefore we can't log in or show any of the rest of Cockpit to do this. The logic to make this happen would need to happen at the login screen level, and related auth code in cockpit-ws. This part of the feature should also work with both cockpit-session and sshd logins in order to make it mergeable.

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2015

also, is that 99999 days starting with today as 0?

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2015

The login screen would need to adapt, change its text to ask for a new password to be entered twice. You can see that Windows does this. I think GDM might do this too.

I've checked out what GDM did. Will create some mockups for this.

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2015

Mockup for setting the password at the login screen:
Mockup

@stefwalter

This comment has been minimized.

Copy link
Contributor

commented Dec 22, 2015

So this pull request looks pretty dead. The blocker is that we don't support account expiration (which makes less sense for admins anyway) on our login screen, so this is incomplete as a feature.

But if we reduce scope, and just show the account and expiration information, then it could be considered complete.

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Jan 15, 2016

But if we reduce scope, and just show the account and expiration information, then it could be considered complete.

Sure, I think that could work.

@stefwalter

This comment has been minimized.

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2016

"Your password HAS expired" since it's an "it".

@stefwalter stefwalter removed the needsdesign label Sep 9, 2016

@stefwalter

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2016

Now that prompting for expired passwords is in Cockpit, this is unblocked and we can finish the users page parts of this.

@fridex

This comment has been minimized.

Copy link
Contributor Author

commented Jul 20, 2017

@stefwalter sure! checked.

@stefwalter stefwalter force-pushed the fridex:account_expiration branch 2 times, most recently from 2666a68 to 6e80a79 Jul 20, 2017

@stefwalter

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2017

@garrett or @andreasn Here's screenshots if you want to give advice on how this is laid out, wording, or design:

screenshot from 2017-07-24 10-31-29
screenshot from 2017-07-24 10-31-37
screenshot from 2017-07-24 10-31-40
screenshot from 2017-07-24 10-31-48
screenshot from 2017-07-24 10-31-51
screenshot from 2017-07-24 10-31-54
screenshot from 2017-07-24 10-31-57

@stefwalter stefwalter force-pushed the fridex:account_expiration branch from 6e80a79 to de3ffa6 Jul 24, 2017

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2017

From the screenshots, this looks good to me in general. I would only fix a couple of controls and some wording. I'll get some wireframes shortly.

When a password expires, you'll need to type a new password, right? What happens when an account expires?

@stefwalter

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2017

What happens when an account expires?

The account can no longer log in.

@stefwalter stefwalter force-pushed the fridex:account_expiration branch 2 times, most recently from 1d55757 to 729882e Jul 25, 2017

@larskarlitski
Copy link
Contributor

left a comment

Thanks for taking care of the old patches!

It all works, except that setting the expiration date from a time zone east of UTC results in a date one day before the desired one.

Also, I don't see an error message as nice as on the screenshot above. Instead, the username / password fields disappear and only the "account expired" error is shown. But I guess that's an unrelated problem?

else
prog.push("");
prog.push(account_id);
console.log(prog);

This comment has been minimized.

Copy link
@larskarlitski

larskarlitski Jul 25, 2017

Contributor

Is this a leftover from debugging?

This comment has been minimized.

Copy link
@stefwalter

stefwalter Jul 25, 2017

Contributor

Removed.

var account_id = $("#account-expiration").data("account-id");
if (!promise) {
if (date)
prog.push(date.toISOString().substr(0, 10));

This comment has been minimized.

Copy link
@larskarlitski

larskarlitski Jul 25, 2017

Contributor

This, in combination with new Date further up, depends on the Browser's time zone. I'm in CEST and the expiration date is always set to one day earlier than the day I entered in the dialog.

(new Date('Jul 20, 2017')).toISOString().substr(0, 10)
// -> "2017-07-19"

This comment has been minimized.

Copy link
@stefwalter

stefwalter Jul 25, 2017

Contributor

Fixed this. We display dates on the main page exactly as chage rendered them. Editable dates are always displayed as ISO 8601 ... and the datepicker is updated to make that the case. Fixed parsing and avoiding interpreting local time zones.

@stefwalter stefwalter force-pushed the fridex:account_expiration branch from 729882e to 0111a9e Jul 25, 2017

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2017

mockup

I wanted to address a couple of things in this design:

  • Explain what will happen when an account or password expire (I was a bit unsure myself if the account expiration would delete or lock the account)
  • Make it radio buttons rather than checkbox, to show the two different states
  • Reuse the wording "Lock account" and "require password change" as much as possible, since we have those concepts already. Still didn't manage to fully get rid of the word "expire", but hopefully this is good enough.

Thanks for the feedback from @garrett on IRC!

lib: Make disabled links work correctly
When we disable links, they should be gray, and should
be unclickable. We can use the .disabled class to do this.

This allows us to use the similar logic with explaining
why something is disabled.

stefwalter added a commit to stefwalter/cockpit that referenced this pull request Jul 25, 2017

users: Expiration for user accounts and passwords
Closes cockpit-project#2062

Signed-off-by: Stef Walter <stefw@redhat.com>
 * Rebased, brought over to users module.
 * Updated translations, indentation.
 * Reworked look to use links, fixed up dialogs
 * Implemented design from Andreas
 * Added tests, fixed behavior
users: Expiration for user accounts and passwords
Closes #2062

Signed-off-by: Stef Walter <stefw@redhat.com>
 * Rebased, brought over to users module.
 * Updated translations, indentation.
 * Reworked look to use links, fixed up dialogs
 * Implemented design from Andreas
 * Added tests, fixed behavior

@stefwalter stefwalter force-pushed the fridex:account_expiration branch from 0111a9e to a660667 Jul 25, 2017

@stefwalter

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2017

Updated for @andreasn and @larskarlitski design and review.

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2017

Works a lot better now!
I realized it is possible to lock the account in the past. Won't cause harm I think, but a bit odd. Can be a followup fix.

@stefwalter

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2017

I realized it is possible to lock the account in the past.

That's something Linux (chage command) allows in general. I think we don't need to worry about that.

@andreasn andreasn self-requested a review Jul 26, 2017

@larskarlitski

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2017

Approved, thanks for the changes.

I'm still wondering about the weird error message on the login screen, but that's not coming from this patch.

@larskarlitski larskarlitski merged commit 52d3620 into cockpit-project:master Jul 26, 2017

15 checks passed

avocado/fedora-24 Tests passed
Details
container/kubernetes Tests passed
Details
selenium/chrome Tests passed
Details
selenium/firefox Tests passed
Details
verify/centos-7 Tests passed
Details
verify/debian-stable Tests passed
Details
verify/debian-testing Tests passed
Details
verify/fedora-26 Tests passed
Details
verify/fedora-atomic Tests passed
Details
verify/fedora-i386 Tests passed
Details
verify/rhel-7 Tests passed
Details
verify/rhel-7-4 Tests passed
Details
verify/rhel-atomic Tests passed
Details
verify/ubuntu-1604 Tests passed
Details
verify/ubuntu-stable Tests passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.