1196134 - add ability for admins to force a user to change their password on next login

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: General, defect, P1)

Description

[:glob ✱]

most systems have the ability for admins to force a user to change their password the next time they log in.

- add boolean "password_change_required" field to profiles
- add lazy-loaded "password_change_reason" field
  - default to "You are required to change your password" with a placeholder
- check on every page load
- user/pass/token api requests must fail if password_change_required is true
- when someone loads a page with password_change_required set:
  - if the method is not GET then throw an error
  - show a page which
    - shows the password_change_reason
    - requires their existing password
    - provide a link to the 'forgot my password' page with their username prefilled
    - prompt for a new password (twice, yadda yadda)
    - once the password has been updated redirect to their originally requested page

Comment 1

[:glob ✱]

Attachment: 1196134_1.patch

Comment 2

[David Lawrence [:dkl]]

Comment on attachment 8650557 [details] [diff] [review]
1196134_1.patch

Review of attachment 8650557 [details] [diff] [review]:
-----------------------------------------------------------------

Mostly cosmetic. Functionally works fine.

::: Bugzilla.pm
@@ +374,5 @@
> +    if ($authenticated_user->password_change_required) {
> +        # We cannot show the password reset UI for API calls, so treat those as
> +        # a disabled account.
> +        if (i_am_webservice()) {
> +            ThrowUserError("account_disabled", { disabled_reason => $authenticated_user->password_change_reason });

ThrowUserError("account_disabled",
               { disabled_reason => $authenticated_user->password_change_reason });

::: Bugzilla/DB/Schema.pm
@@ +944,5 @@
> +            extern_id                => { TYPE => 'varchar(64)' },
> +            is_enabled               => { TYPE => 'BOOLEAN',       NOTNULL => 1, DEFAULT => 'TRUE' },
> +            last_seen_date           => { TYPE => 'DATETIME' },
> +            password_change_required => { TYPE => 'BOOLEAN',       NOTNULL => 1, DEFAULT => 'FALSE' },
> +            password_change_reason   => { TYPE => 'varchar(64)' },

I feel this should be at least 255. I can imagine some cases where we would need a more detailed explanation of why we need to require a reset. Example, mass reset due to a security issue.

Plus all of the above/below cleanup puts most of this past 80 columns and is unnecessary. Are you purposely trying to make my upstream merging work more difficult? ;)

::: Bugzilla/Install/DB.pm
@@ +739,5 @@
>      $dbh->bz_add_column('user_api_keys', 'last_used_ip',
>                          {TYPE => 'varchar(40)'});
>  
> +    $dbh->bz_add_column('profiles', 'password_change_required', { TYPE => 'BOOLEAN', NOTNULL => 1, DEFAULT => 'FALSE' });
> +    $dbh->bz_add_column('profiles', 'password_change_reason', { TYPE => 'varchar(64)' });

ditto extreme line length.

::: Bugzilla/User.pm
@@ +122,5 @@
> +    login_name               => \&check_login_name_for_creation,
> +    realname                 => \&_check_realname,
> +    extern_id                => \&_check_extern_id,
> +    is_enabled               => \&_check_is_enabled,
> +    password_change_required => \&_check_password_change_required,

password_change_required => \&Bugzilla::Object::check_boolean

::: reset_password.cgi
@@ +59,5 @@
> +
> +    # done
> +    print $cgi->header();
> +    $template->process('global/message.html.tmpl', { message => 'password_changed' })
> +        || ThrowTemplateError($template->error());

Redirect back to index.cgi at least with the password changed message. Having just the message and a blank page looks bad. Bonus points if we can somehow get back to the page where they came from (show_bug.cgi, etc.). May be too complex depending on original query params.

::: template/en/default/account/reset-password.html.tmpl
@@ +8,5 @@
> +
> +[% inline_style = BLOCK %]
> +
> +#password-reset {
> +}

Remove

@@ +150,5 @@
> +<input type="hidden" name="do_save" value="1">
> +
> +<div id="password-reset">
> +  <div class="field-hr">
> +    <hr>

I would leave out the <hr>s as they do not add any value IMO and clutter the form. Especially ones that are full width. Rather just add extra spacing between un-related sections.

@@ +193,5 @@
> +</div>
> +
> +</form>
> +
> +<p id="errors"></p>

Please place this in a float div next to the password fields. When typing the new password(s), it keeps pushing the #complexity_rules div up and down and looks unprofessional.

::: template/en/default/admin/users/userdata.html.tmpl
@@ +86,5 @@
> +        <label for="password_change_required">User must change their password immediately</label>
> +        <div id="password_change_reason_container">
> +          Password change reason (will be displayed to the user):<br>
> +          <input type="text" size="64"
> +            name="password_change_reason" id="password_change_reason"

nit: line up attributes

::: template/en/default/global/user-error.html.tmpl
@@ +1349,5 @@
>      You must enter a new password.
>  
> +  [% ELSIF error == "new_password_same" %]
> +    [% title = "Password Unchanged" %]
> +    Your new password cannot be the same as your old password.

This can be circumvented by just changing it again in the userprefs.cgi?tab=account screen. We really need to log old passwords somehow and require completely new ones. Another bug for later I know.

Comment 3

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #2)
> I feel this should be at least 255. I can imagine some cases where we would
> need a more detailed explanation of why we need to require a reset. Example,
> mass reset due to a security issue.

because this is on the profiles table i want to keep this as small as possible.
i can't picture a scenario where we'd need to write more than a simple reason, let alone >255 chars.
it's really only there to allow us to differentiate between "password reset required" and "password has expired".

> Plus all of the above/below cleanup puts most of this past 80 columns and is
> unnecessary. Are you purposely trying to make my upstream merging work more
> difficult? ;)

80 columns has never been a limit, on bmo or upstream.
i'm not trying to make the merging work more difficult, and we shouldn't avoid make changes because that's in the pipeline.

> Redirect back to index.cgi at least with the password changed message.
> Having just the message and a blank page looks bad. Bonus points if we can
> somehow get back to the page where they came from (show_bug.cgi, etc.). May
> be too complex depending on original query params.

redirecting back to the original page is very complex, especially for POSTs.  i'll redirect to index.

Comment 4

[:glob ✱]

Attachment: 1196134_2.patch

Comment 5

[David Lawrence [:dkl]]

Comment on attachment 8651616 [details] [diff] [review]
1196134_2.patch

r=dkl

Comment 6

[:glob ✱]

schema only:

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   c63ecc8..59f9641  master -> master

Comment 7

[:glob ✱]

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   90a6182..e6d45b6  master -> master