Problem/Motivation

A changed in #3464639: Fix Validate pipeline - specifically this change https://git.drupalcode.org/project/password_policy/-/commit/c02ec557f44d... causes a fatal error.

Input bag will never return an array - it throws an exception if the param value is a not a scalar.

This exception is not caught, which means users get a 500 error and can not change their password.

Steps to reproduce

  1. Enabled password_policy_history
  2. Either create/edit user
  3. Change a password through the UI
  4. Get a fatal error
CommentFileSizeAuthor
#9 3465364-1.patch911 bytesyeniatencio
Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

ericgsmith created an issue. See original summary.

ericgsmith’s picture

ericgsmith’s picture

Status: Active » Needs review
ericgsmith’s picture

Priority: Major » Critical
Issue summary: View changes
rollins’s picture

Status: Needs review » Reviewed & tested by the community

I can confirm that changes from the MR make sense here
It will prevent us to have the fatal error

ankitv18’s picture

Left a comment otherwise looks good

ericgsmith’s picture

Fixed the comment, thanks for spotting that

yeniatencio’s picture

StatusFileSize
new911 bytes

Creating patch based on @ericgsmith as need an urgent solution in the meantime.

ankitv18’s picture

RTBC++

kosa ilma’s picture

I get the same error, the patch from #9 fixed the error.
Core 10.3.1, Password policy 4.0.2.

Thank you!

vishalkhode’s picture

Changes looks good to me. However, If we add test coverage for the same, that would be great.

rajab natshah’s picture

Title: Fatal error when changing password when password_policy_history is enabled » Fix Fatal error when changing password when password_policy_history is enabled

Facing the same issue.
Thanks, the MR and patch in #9 are working.

pooja_sharma made their first commit to this issue’s fork.

pooja_sharma’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs review

Added test coverage in the newly created testPasswordResetBehaviorsUi() as it covers specific scenario, however reused the existing test file as most of code can be leverage from it rather than creating separate test file & applied a super nitpicky :void return on the existing test function as well. Apart form it nothing seems to be left.

Please review, moving NR

ankitv18’s picture

Status: Needs review » Needs work

Thanks for covering the test but it would be great if it is in password_policy_history sub-module.

pooja_sharma’s picture

Thanks for reviewing, have the similar thought, however kept the test coverage in the main module by considering some couple of things like:

  • if issue is from the password_policy module itself then still error 'll be caught, no additional code needs to write in that scenario
  • More or less same code can be leverage as it added in the existing test file from the optimisation perspective

Any specific concerns/suggestions over it?

pooja_sharma’s picture

Status: Needs work » Needs review
vishalkhode’s picture

Status: Needs review » Needs work

@pooja_sharma: Thanks for the adding test coverage, let's not add tests in PasswordResetBehaviorsTest as that test is specifically to validating password reset functionality. And here we want verify password_history functionality, so that it doesn't accept repeated password. Lets see if we can add test coverage for following:

  • Password History constraint works as expected.
  • When user try to provide the same password again from UI, it throws an error.

For this, we can either update the existing tests i.e PasswordPolicyInterfaceTest.php or we can add a new tests in password_history submodule similar to PasswordCharacterOperationsTest.php

ankitv18’s picture

Status: Needs work » Reviewed & tested by the community

Looks good ~~RTBC

pooja_sharma’s picture

Thanks @vishalkhode for providing detailed suggestions, chosen the first opt (to update the existing tests i.e PasswordPolicyInterfaceTest.php), as I need to add only minimal single line code change for the test coverage along with leverage complete developed code, I believe if similar issue encounter by any other sub-module that can also be cover similarly (test coverage perspective).

vishalkhode’s picture

Status: Reviewed & tested by the community » Fixed

Thanks @pooja_sharma, changes looks good to me. Verified and it's seems to be working fine. However, I also noticed that we don't have test coverage which checks for Password Policy history constraint, but that's fine, we can create a separate ticket for it and fix it there. Hence, merged.
Thanks everyone for working on this.

Status: Fixed » Closed (fixed)

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