Problem/Motivation

A third-party module, such as CAS, may deny the access to the user password form element. In such cases, the password policy status table is still displayed on the form even the user is not able anymore to manage its password.

Proposed resolution

  • Set the #access property of password policy status table element to be the same as the $form['account']['pass']['#access'].
  • Because this might lead to a race condition, as the third-party will use the form alter hook too, and the module that sets $form['account']['pass']['#access'] might run after password_policy_form_user_form_alter(), just add a #after_build property with a custom callback to the password policy status table element.
  • In the custom callback do:
    $element['#access'] = !empty($form_state->getCompleteForm()['account']['pass']['#access']);
  • Add a test.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

N/A

CommentFileSizeAuthor
#6 interdiff.txt2.55 KBpfrenssen
#6 3068891-5.patch4.67 KBpfrenssen
#6 3068891-5-test-only.patch3.14 KBpfrenssen
#3 3068891-3.patch4.68 KBpfrenssen
#3 3068891-3-test-only.patch3.15 KBpfrenssen

Comments

claudiu.cristea created an issue. See original summary.

pfrenssen’s picture

pfrenssen
Location Sofia
commented
Assigned: Unassigned » pfrenssen
Category: Bug report » Feature request

Assigning this, I am going to implement this. I think it is rather a feature request than a bug.

pfrenssen’s picture

pfrenssen
Location Sofia
commented
Status: Active » Needs review
StatusFileSize
new 3068891-3-test-only.patch3.15 KB
new 3068891-3.patch4.68 KB

The last submitted patch, 3: 3068891-3-test-only.patch, failed testing. View results

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Needs review » Needs work 
    1. +++ b/password_policy.module
@@ -125,6 +126,32 @@ function password_policy_check_constraints_password_confirm_process(array $eleme
+  $password_visible = !empty($form['account']['pass']) && isset($form['account']['pass']['#access']) ? $form['account']['pass']['#access'] : TRUE;

      I think the solution proposed in issue summary covers also the case when a 3rd party module is suppressing the password in this way:

      function mymodule_form_user_form_alter(&$form, FormStateInterface $form_state, $form_id) {
  unset($form['account']['pass']);
  unset($form['account']['current_pass']);
}

      EDIT: The check would be !empty($form['account']['pass']['#access']). 

    2. +++ b/tests/src/Functional/PasswordPolicyStatusVisibilityTest.php
@@ -0,0 +1,45 @@
+  public static $modules = ['password_policy', 'password_policy_test'];

      s/public/protected 

    3. +++ b/tests/src/Functional/PasswordPolicyStatusVisibilityTest.php
@@ -0,0 +1,45 @@
+    $this->drupalGet('user/' . $user->id() . '/edit');
...
+    $this->drupalGet('user/' . $user->id() . '/edit');

      Nit: Can be written in a more "API oriented" style: $this->drupalGet($user->toUrl('edit-form')); 

    4. +++ b/tests/src/Functional/PasswordPolicyStatusVisibilityTest.php
@@ -0,0 +1,45 @@
+    $this->container->get('state')->set('password_policy_test.user_form.hide_password', TRUE);

      In BrowserTestBase tests is safe to use \Drupal::service() (in this case\Drupal::state()) as, in some circumstances, $this->container is out-of-sync. In kernel tests $this->container->get() is always safe.

    Setting to NR, especially for 1.

pfrenssen’s picture

pfrenssen
Location Sofia
commented
Status: Needs work » Needs review
StatusFileSize
new 3068891-5-test-only.patch3.14 KB
new 3068891-5.patch4.67 KB
new interdiff.txt2.55 KB

Thanks for the review! I addressed all the remarks. The code is a bit more complex than just !empty($form['account']['pass']['#access']) because #access is optional and defaults to TRUE when omitted.

The last submitted patch, 6: 3068891-5-test-only.patch, failed testing. View results

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Needs review » Reviewed & tested by the community

The code is a bit more complex than just !empty($form['account']['pass']['#access']) because #access is optional and defaults to TRUE when omitted.

Indeed! Thank you for the work.

  • AohRveTPV committed 701adb4 on 8.x-3.x authored by pfrenssen 
    Issue #3068891 by pfrenssen, claudiu.cristea: Display pass policy status...
aohrvetpv’s picture

aohrvetpv commented
Status: Reviewed & tested by the community » Fixed

Looks great, thank you. Committed/pushed, with two reservations:
1. If a contributed module hides the password fields after build, wouldn't there be the same problem? (Is this just moving the problem to another level?)
2. This conditional seems a bit hard to fully understand (to me at least):

$password_invisible = empty($form['account']['pass']) || (isset($form['account']['pass']['#access']) ? !$form['account']['pass']['#access'] : FALSE);

I'm not sure I would've understood it without pfrenssen's comment that #access defaults to TRUE when omitted.

Maybe an explanatory comment like this would be helpful?:

  // The password field is invisible when either (a) the 'pass' form element is
  // FALSE or unset, or (b) when the '#access' property of 'pass' is set and
  // FALSE.  (When the '#access' property is unset, it defaults to TRUE and the
  // password field is visible.)
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

If a contributed module hides the password fields after build, wouldn't there be the same problem? (Is this just moving the problem to another level?)

It's true, but is less probable. Modules are usually altering in hook_form_FORM_ID_alter() or hook_form_alter(). That is the "official" API to alter forms. Of course, a race condition might happen also in after build. We cannot anticipate all cases, but this covers 99.99% of the cases.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

pfrenssen’s picture

pfrenssen
Location Sofia
commented
Assigned: pfrenssen » Unassigned