Decouple display of policy table and policy validation

I performed a code review for the changes suggested here and they look good to me.

I have been reviewing this issue and need to some clarity before proceeding. I am all for providing a higher level of configuration for the current adopted behavior, so I'm sure this will be fine. I especially like the "decoupling" of separating discrete things (tables, errors, validation logic, etc) from the actions (user edit, user reset, etc) such that there can be more flexibility.

From reading above, it seems like my interpretation of the goal may differ from the proposed implementation. I'm not clear on how decoupling (which I view as advantageous for the technical implementation) led to new configuration for a policy. I don't fully understand what this configuration is affording either (I didn't relate to the analogy cleanly). Can you describe to me a user story in which this configuration helps? I'm leary about "turning off" a display of policy validation which may make it less clear to the user why their password failed.

It may be something like, "As an authenticated user, I want to be able to turn off table validation and only receive an error message". Is this correct?

RE: "3. Removed the changes of the element weights. This seemed like an overreach of this module, and offered no functional advantage. If there's a compelling reason this should not be changed, please let me know." -> This is fine as long as as the module-related data showed up in a relevant place on the respective forms. I had to reorder it before by adding weights to ensure the policy-related information showed up near/close to the password.

Code review comments:

1. Shouldn't this always run? I doubt we want users to not have any validation. We may tailor *where* or *how* the errors get rendered (see comments above) but the validation still occurs, no?

+  // Check whether the policy validation should run.
+  if (_password_policy_validation_should_run()) {

2. I notice we are removing the permission for "bypass password policies" which is good for the other admin user role issue. Is the other issue updated?

-  if ($account->hasPermission('bypass password policies')) {

-bypass password policies:
-  title: 'Bypass password policies'
-  description: "Be able to bypass password policies. Users with this permission will not have enforced password policies."

I like both recommendations you have in points 1 and 2.

For the name of the function, maybe we should just call it "checkPasswordPolicyInvocation" or "checkPasswordPolicyContext" something similar. In lieu of a function callback (which is very procedural), I'm wondering if this should be put into a service (new or existing). If this service is in the Password Policy module, this would simplify the naming convention to something like, "checkInvocation", "checkContext", or even something as simple as "shouldRun". Again, these are just suggestions but hopefully they trigger some idea you like (or it leads to something).

In terms of decoupling, we should verify there are three separate functions: one that performs the checks (and returns an array of potential issues), a processor for the error message generation, and a processor for the table generation. The new feature you are proposing would be a lot simpler (a condition wrapping the table generation function) and both functions for the optional table and the error message would leverage the same checking function that returns the errors.

I'm going to divert my attention to the other "needs review" issues but will circle back to this if you are able to submit a new patch.

Hi
I have been using this patch for a while, but it needs to be updated for password_policy version 3.0-alpha4. The patch cannot be applied on this version.

Thanks

reroll and some other tests, maybe further work is needed, i'm assigning it to me to work on it

As a first result, i've rolled back the patch on 8.x-3.0-alpha4, also the highlighted issue with 'and' in conditions i've replaced it everywhere to the standards suggested && - and i did the same with 'or' changing it to ||.
I'm still checking the rest of the things from your comments guys, also will check the tests and adapt the new property to them.
Cheers,
Balazs.

A few notes that I had seen from that patch:

1. +++ b/password_policy.module
   @@ -85,16 +76,58 @@ function _password_policy_show_policy() {
   + *   TRUE if the current user has a role captured in a policy, FALSE otherwise.
   + */
   +function _password_policy_validation_should_run() {
   +  $account = \Drupal::currentUser();
   +  $config = \Drupal::config('user.settings');
   

   Should this function stored in the .module file?
   I prefer to move this functionality into the service with the related service injection.

2. +++ b/password_policy.module
   @@ -57,7 +45,10 @@ function password_policy_form_user_form_alter(&$form, &$form_state) {
   +  // Check whether the policy validation should run.
   +  if (_password_policy_validation_should_run()) {
        $form['#validate'][] = '_password_policy_user_profile_form_validate';
   

   This should be updated regarding previous suggestion.

3. +++ b/config/schema/password_policy.schema.yml
   @@ -20,9 +20,13 @@ password_policy.password_policy.*:
        policy_constraints:
          type: sequence
          label: 'Constraints'
   -      sequence:
   -        type: password_policy.constraint.plugin.[id]
   -        label: 'Constraint'
   +    sequence:
   +      type: password_policy.constraint.plugin.[id]
   +      label: 'Constraint'
   

   Changes for property of policy_constraints configuration schema. Should be reverted.

4. +++ b/password_policy.module
   @@ -85,16 +76,58 @@ function _password_policy_show_policy() {
    
   ...
   +  if ($account->isAnonymous() && $config->get('verify_mail')) {
   
   +++ b/password_policy.permissions.yml
   @@ -1,6 +1,3 @@
   -bypass password policies:
   -  title: 'Bypass password policies'
   -  description: "Be able to bypass password policies. Users with this permission will not have enforced password policies."
   

   Should 'bypass password policies' permission check be removed from the functionality?

Hey folks,
sorry for disappearing in the last days, now I've made the mentioned changes from comment#12, as code got improved for showing the table with a different condition then checking the permission that we want to remove from the module, for now I've put it back and will follow it under https://www.drupal.org/project/password_policy/issues/2947985

Cheers,
Balazs.

i just update the interdiff to a correct one (removed the header)

A little bit changes here.
Added plugin manager to handle visibility rules. I also had added the abstraction of weight to have a possibility to change rules order based on the context.

Missed one thing.

Thanks, all for working on this. Here are some comments:

1. I am curious why we're switching all the "and" and "ors" in this patch? Is this a coding standard recommendation? Could this please be separated from this patch?

2. I like the cleanup of the interface documents, thank you.

3. In the new plugin manager, this comment doesn't seem to be accurate for what the code is doing:

+  /**
+   * Determine if there is a role within any given policy for the current user.
+   *
+   * @return bool
+   *   TRUE if the current user has a role captured in a policy,
+   *   FALSE otherwise.
+   */

Isn't the function to process the plugins by weight to determine if the table should be shown?

4. Can the new visibility plugins be turned on / off (enabled / disabled)?

5. Is there a config form to weight the new visibility plugins? How is the weight configured?

6. Can we please have some automated tests added to test the behavior of the new plugin system?

Reroll of #16 for Alpha 5

Whoops! For real this time. :)

Neither of the last two patches (#16 and #19) work on the most recent version I downloaded. Is there any chance of getting a more up-to-date update?

Hello,

Are the patches being considered for the following scenario?
=======================================
As you can see in the image (Repeated_Policy.png), we have two separate password policies, "Strong" and "Loose" - that are causing repetition when a certain type of user is created, because "authenticated user" is checked by default.

So, let's say a password policy "A" is created for a type of role: "role_A" and another password policy "B" created for "authenticated user" and other types of roles.
Whenever a user is created of type "role_A", it picks up the policies of both "A" and "B" because "authenticated user" is checked by default.

This is demonstrated in the screenshot - "Loose" and "Strong" being the password policies.

Is there a way around this repetition? We would like different policies for different roles. Basic Authenticated users should not have the same requirements as Admin and Content Editors.

Patches from comments #16 and #19 are failing to apply.

In any case, could some one please answer my question from #21?

Patch attached fixes the current failing tests.

17.1: Fixed.
17.3: Seems correct. This still needs to be fixed.
17.4: There is no user interface to do this. But it can using some custom code.
17.5: The weight is configured in code. But instead of a method I would suggest moving this to the annotation.

+++ b/src/Plugin/PasswordPolicyVisibility/BypassVisibility.php
@@ -0,0 +1,77 @@
+  protected $weight = 1000;

17.6: Automated tests still need to be added.

Also, is there a reason why we're using the plugin pattern to show/hide the table? It seems to add a lot of complexity, to replace a function that used to be 20 lines of code...

Provided some test coverage.

Hmm, seems something was wrong with the test-only patch.

Removed the plugin manager for the visibility rules since imo this is adding too much complexity for a small piece of code.

Needs reroll due to #3064853: Refactor _password_policy_constraints_validate().

#31 doesn't apply correctly.

Re-roll patch created, Please review.

The patch does apply and I'm seeing the configuration setting, but Password Policy itself has disappeared from the forms.
I'm not seeing any errors so I can't pinpoint the exact problem. Unless I forgot something...

The previous patches uses the result of \Drupal\password_policy\PasswordPolicyValidator::buildPasswordPolicyConstraintsTableRows as the #items key for the item_list render array.

The structure of the data returned by the method of the password_policy.validator service does not match with the structure expected by the template_preprocess_item_list function showing innecessary information to the user. For example, the row classes will be listed.

The attached patch extracts the ['data']['status'] value of each row and then only the status of each policy will be shown.

I also attached an interdiff with #34.

Table display does show, but checking it off removes the policy as a whole.
Someone else has to test this and say that I am wrong. Maybe I've overidden something without my own knowledge...

Hi @antoineh!

We're using this patch on our developments and the policies works as expected.

Maybe the selected roles does not match your users? Or there is a bug on one of the policies you're using?

Regards
Cristian

I misinterpreted this issue, that's why I expected a different result and thought the patch didn't work.
My apologies for that.

Testing the patch does seem to fix this issue.

#37 worked for me.

I created anothe patch to based on #37 to fix some warnings in case of Account Field split module is used.

Wrong patch added in the previous comment . Uploading new patch

For some reason this patch from #43 doesn't apply for me with composer. I created another one.
Sorry for the mess.

The interdiff is the same.

Added also the possibility to alter the Constraints table rows

This patch worked great, except for I found that validationShouldRun() never ran for a rule we had setup for our site. This was because our rule was setup to target only the "authenticated" role. However when a user is logged out, all they have is the anonymous role, which can never match a password policy rule, as you can't target the anonymous role.

To fix, I think we should always search for rules that match the authenticated role, as all users will have this post register.

Here is a reroll of the patch against current 8.x-3.x (526f77e2ffa7f7dd6a6c51591460928edb16d758).

Hi all!
I changed the approach to MR from patch #48.

Patch #48 works great for my use case, on 9.2.7. Thanks! This has saved me loads of time.

Lets postpone this issue while #3054412: Make policy summary table easier to customise is not committed.

As #3054412: Make policy summary table easier to customise was committed, I'm updating the issue status.

Another rebase.

I'll try a manual test to review the last MR

It seems that all previous comments were addressed except for the question in #21 that wasn't answered. So answering @apathak, in my opinion the patch works as designed. If we repeat the same constraint in more than one policy and apply both policies to the same user than it's natural they would appear repeated, for example:
Policy A has constraints 1 and 2 and is applied to Admin roles
Policy B has constraint 1 and is applied to Authenticated users
The table of constraints for an Admin user would look like this:

Policy A - Constraint 1
Policy A - Constraint 2
Policy B - Constraint 1

Instead of changing this patch logic (and run the risk of making it even more complex) the site owner can solve it by simply managing better the constraints of each role and removing constraint 1 from Policy A (since it will already be applied to all authenticated users through policy B) so the Policies and the table would look like this:
Policy A has constraint 2 and is applied to Admin roles
Policy B has constraint 1 and is applied to Authenticated users

Policy A - Constraint 2
Policy B - Constraint 1

That being said I manually tested the code from the MR20 and it seems to be running well. I tested the case of two Policies to the same role with only one, both and none of them checked to be displayed, and the result was the expected in all cases (it displays the complete table if one or more policies that were applied to the role is checked to be displayed).

@NigelCunningham:

Merge blocked: the source branch must be rebased onto the target branch.

Only removed some deprecated code added before in the MR 20!

I'm working in this issue!

I tested and the patch works fine! Rebase still needed.

I've now rebased the branch, thanks.

Tests are failing.

I will work on this issue.

Please review.

I'll do the review

While running the phpcs i recieved 1 warning in src/PasswordPolicyValidator.php to use dependency injection instead of \Drupal call on line 155.
For the automated tests everything worked fine.
For the manual tests it worked as commented on #58. When one or more policy is checked to show it shows all the policies related to the role, and when all policies are not checked it didn't shows.

Addressed #69: Fixed warning in src/PasswordPolicyValidator.php to use dependency injection instead of \Drupal call on line 155.

Please review this.

I will do the review.

I reviewed the last MR. Dependency injection has been handled by the commit of @lucassc, tests are passing and working as mentioned earlier by @sophiavs. There are only two details which were a use statement not being used and a parameter type missing, I fixed it and I'm moving to RTBC.

FILE: /password_policy/src/Plugin/migrate/process/PolicyConfiguration.php
---------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
---------------------------------------------------------------------------------------------------------------------------
 124 | ERROR | Missing parameter type
---------------------------------------------------------------------------------------------------------------------------


FILE: /password_policy/password_policy.module
-----------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-----------------------------------------------------------------------------------------------
 8 | WARNING | [x] Unused use statement
-----------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-----------------------------------------------------------------------------------------------