Download & Extend

Allow the user edit form to only ask for the current password when necessary (in a followup confirmation step)

Project:Drupal core
Version:8.x-dev
Component:user system
Category:task
Priority:normal
Assigned:sun
Status:needs work
Issue tags:API clean-up, form API

Issue Summary

erm, first attempt - sounded easy, but apparently, form rebuilding, caching, re-using, restoring, and alteration after #process on $complete_form doesn't work out (and I had to code to find this out :P )

Don't try this at home.

AttachmentSizeStatusTest resultOperations
drupal.paranoia.patch6.29 KBIdleFailed: 13232 passes, 116 fails, 12 exceptionsView details | Re-test

Comments

#1

To provide some clues:

The idea was to rebuild the user account (and user cancellation form) in case the user tries to change the e-mail or password, or wants to kill his account.

The screenshot shows the form after submission. Actually, only the current password field should have been there, but apparently, #process can't alter the complete form during processing of child elements.

AttachmentSizeStatusTest resultOperations
current-password-0.png49.55 KBIgnored: Check issue status.NoneNone

#2

This should give you some more clues:

current-password-2a.png

current-password-2b.png

current-password-2c.png

Almost done.

AttachmentSizeStatusTest resultOperations
current-password-2a.png25.19 KBIgnored: Check issue status.NoneNone
current-password-2b.png12.35 KBIgnored: Check issue status.NoneNone
current-password-2c.png12.46 KBIgnored: Check issue status.NoneNone

#3

Status:needs work» needs review

well, "almost". ;)

I hope chx, eaton, and/or frando are able to kick in.

AttachmentSizeStatusTest resultOperations
drupal.paranoia.3.patch7.45 KBIdleFailed: 13342 passes, 2 fails, 0 exceptionsView details | Re-test

#4

Solving this === killing confirm_form(), btw.

#5

Status:needs review» needs work

The last submitted patch failed testing.

#6

Issue tags:+form API

Die confirm_form(), DIE! +1 Subscribe!... Love that paranoia patch.

#7

Concept.

Problem

  1. Users can change their password and e-mail address without any confirmation. The expected confirmation for both would be to provide the current password.
  2. Users have to go through heaps to cancel their account, wait for an e-mail, click on a specially crafted link in that e-mail to get back to the site, and on there, click another confirmation form button to confirm the action. Providing the current password would be sufficient to confirm the action.
  3. Confirmation forms, in general, are a very ugly construct,

Details

  • My primary interest is to kill the ugly user account cancellation process. If that means that we can kill the ugly confirm_form() construct (which dates back to Drupal 4.x or even earlier), awesome. (And it's very likely that it will.)
  • There are 2 ways to let a user enter/confirm an action by entering his current password:
    1. Just squeeze a damn 'password' field into the form, using a #element_validate handler that checks against the user's current password.

      This, however, doesn't really work out. On the user account form, we have two form elements (e-mail and password) that should require the current password to confirm. So we'd need either a separate "Current password" field for each of those, or we'd move both fields into a new fieldset, which also contains a new "Current password" field to confirm one of both or both changes. To do that, we'd probably use a custom #confirm_elements property on that "Current password" form element, which references to the other elements in the form that need the advanced validation/confirmation in case their value has been changed.

      However, that sounds a bit awkward.

    2. Use a #process or #element_validate form element handler on form elements that need confirmation. Whenever the user submits a value for that element that differs from the #default_value, display a confirmation form that replaces the original form. The confirmation form may additionally contain a "Current password" form element that requires the user to type in his current password to confirm the action (or any other form elements). When the confirmation form is submitted, take the submitted values, merge the originally submitted form values on top of it, and execute form submit handlers.

      This would be the very first multi-step form implementation in core, AFAICS. So, ++DX.

  • On the UX-side, both approaches are seen on the net. For Drupal, the second one seems a lot more suitable, because we already have that confirmation form construct.

Problems (current patch)

  1. All top-level form elements are set to #access = FALSE in case a form element value requiring the current password is altered, because the idea was to just hide them, but keep their submitted values. However, when the confirmation form containing the current password is submitted, I'm trying to merge back the original form values from $form_state['storage'] in a #element_validate handler of the "Current password" element.

    That would work, if there were not elements like #type = 'password_confirm', which apparently seem to be processed somewhere from their original array-based values in POST into $form_state['input']. "somewhere" means I have no clue where that happens.

  2. Not sure whether it's a good idea to set #access = FALSE for all original form elements (only regarding hook_form_alter(), not UX). I tried to think of alternative ways, but did not have a better idea.

Amendments

  • Current confirm_form() signature:
    function confirm_form($form, $question, $path, $description = NULL, $yes = NULL, $no = NULL, $name = 'confirm') {
  • Current example of confirmation form usage:
    <?php
    function node_form($form, &$form_state, $node) {
      if (!empty(
    $node->nid) && node_access('delete', $node)) {
       
    $form['buttons']['delete'] = array(
         
    '#type' => 'submit',
         
    '#value' => t('Delete'),
         
    '#weight' => 15,
         
    '#submit' => array('node_form_delete_submit'),
        );
      }
    }

    function
    node_form_delete_submit($form, &$form_state) {
     
    $destination = '';
      if (isset(
    $_GET['destination'])) {
       
    $destination = drupal_get_destination();
        unset(
    $_GET['destination']);
      }
     
    $node = $form['#node'];
     
    $form_state['redirect'] = array('node/' . $node->nid . '/delete', $destination);
    }

    function
    node_delete_confirm($form, &$form_state, $node) {
     
    $form['nid'] = array(
       
    '#type' => 'value',
       
    '#value' => $node->nid,
      );

      return
    confirm_form($form,
       
    t('Are you sure you want to delete %title?', array('%title' => $node->title)),
        isset(
    $_GET['destination']) ? $_GET['destination'] : 'node/' . $node->nid,
       
    t('This action cannot be undone.'),
       
    t('Delete'),
       
    t('Cancel')
      );
    }

    function
    node_delete_confirm_submit($form, &$form_state) {
      if (
    $form_state['values']['confirm']) {
       
    $node = node_load($form_state['values']['nid']);
       
    node_delete($form_state['values']['nid']);
       
    watchdog('content', '@type: deleted %title.', array('@type' => $node->type, '%title' => $node->title));
       
    drupal_set_message(t('@type %title has been deleted.', array('@type' => node_type_get_name($node), '%title' => $node->title)));
      }

     
    $form_state['redirect'] = '<front>';
    }
    ?>
  • Possible scenario:
    <?php
    function node_form($form, &$form_state, $node) {
     
    $form['buttons']['delete'] = array(
       
    '#type' => 'submit',
       
    '#value' => t('Delete'),
       
    '#weight' => 15,
       
    '#access' => !empty($node->nid) && node_access('delete', $node),
       
    '#process' => array('form_process_confirm'),
       
    '#confirm_title' => t('Are you sure you want to delete %title?', array('%title' => $node->title)),
       
    '#confirm_description' => t('This action cannot be undone.'),
       
    '#confirm_submit' => t('Delete'),
       
    '#submit' => array('node_form_delete_submit'),
      );
    }

    function
    node_delete_confirm_submit($form, &$form_state) {
      if (
    $form_state['values']['confirm']) {
       
    $node = node_load($form_state['values']['nid']);
       
    node_delete($form_state['values']['nid']);
       
    watchdog('content', '@type: deleted %title.', array('@type' => $node->type, '%title' => $node->title));
       
    drupal_set_message(t('@type %title has been deleted.', array('@type' => node_type_get_name($node), '%title' => $node->title)));
      }

     
    $form_state['redirect'] = '<front>';
    }
    ?>

#8

This is an advanced use-case: A #process and/or #element_validate form handler (not sure yet) tries to turn a form into a multi-step form.

#9

how about we create seperate pages for changing password (user/edit/password) and email-address (user/edit/email)?

on the password page, we request the current password, along with the new password. (MENU_LOCAL_ACTION?)
on the email page we request the new email along with the current password. (again MENU_LOCAL_ACTION?)

on user/edit we show the current values of those fields (or ******** in case of the password.)

#10

Thanks, Alex! But if you want that, you can subscribe to a flood of other issues - just search for "user password" (and similar) in Drupal core. ;)

This issue is, indeed, about bringing Form API from Drupal 4 to Drupal 7 regarding confirmation forms. The [theme_]confirm_form() construct using a separate page callback actually dates back to pre-form-API, i.e. before Drupal 4.7.

#11

.

#12

Version:7.x-dev» 8.x-dev

can we handle the simple case for D7? #86299: Add "current password" field to "change password form"

#13

Version:8.x-dev» 7.x-dev

Sure, you can try to handle all of these...

#86299: Add "current password" field to "change password form"
#75924: Include fields for resetting password on the one-time password reset page
#486544: system_settings_form does not support password / password_confirm fields well
#430780: User account hijack prevention
#85494: Verify changing user email addresses
#64483: permissions & variable for disallowing editing of user variables

They all share the same common denominator if you start to think about it, and especially if you start to think about usability.

This one is in my top 10 of API clean-up issues. Because, as mentioned before, we're carrying forward this crappy construct of confirm_form() since years, instead of implementing our existing APIs properly.

#14

I'd suggest updating the title cos this is hard to follow. Is the issue "remove the confirm form foo"? (Which in turn is a pre-requisite for fixing password confirm?)

#15

Version:7.x-dev» 8.x-dev

#16

Re-rolled against HEAD. Due to other Form API fixes, this should be easier now, but I couldn't make it work yet though.

AttachmentSizeStatusTest resultOperations
drupal.paranoia.16.patch4.33 KBIgnored: Check issue status.NoneNone

#17

Priority:critical» major

Downgrading all D8 criticals to major per http://drupal.org/node/45111

#18

Status:needs work» closed (duplicate)

This sure seems like it was fixed already as part of #86299: Add "current password" field to "change password form" (see screenshot of D7's account form).

The major part of this issue was certainly corrected (a password confirmation). In any case, this issue is very hard to follow (considering the summary was finally added in #7). If we want to make a new task for removing confirmation forms, it should be in a dedicated issue.

AttachmentSizeStatusTest resultOperations
current-password.png57.21 KBIgnored: Check issue status.NoneNone

#19

Priority:major» normal
Status:closed (duplicate)» needs work

#20

@sun: So what's the goal of this issue now? Remove confirmation forms? We've already added the field for password confirmation, which I assume is why this issue is titled "Drupal paranoia"?

#21

Title:Drupal paranoia» Allow the user edit form to only ask for the current password when necessary (in a followup confirmation step)

I think the goal here is to improve the user experience (by not showing a confusing "Current password" field on the user edit form every time, but rather only showing it on a followup confirmation screen when it's actually needed).

And potentially to do it via the same form (in a built-in multi-step fashion), rather than resorting to confirm_form().

Attempting to give it a more descriptive title :)

#22

And potentially to do it via the same form (in a built-in multi-step fashion), rather than resorting to confirm_form().

A potential concern with this approach is that any form that uses $form_state['storage'] or is multiple steps will make an entry in the cache_form table, potentially *with* the users plain-text password stored in the database. Actually now that I think about it, if you add a FileField to the user form, this could already be the case. These "cache" entries are both insecure and long-lived (for at least 6 hours).