Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
On Drupal 8 administrator by default all permissions are ticked, this means password policy by pass for admin is also ticked by default.
Suggestion / Feature request is to move bypass password policy to config
Comment | File | Size | Author |
---|---|---|---|
#2 | password_policy-2862906-2.patch | 1.02 KB | badjava |
Comments
Comment #2
badjava CreditAttribution: badjava at Metasun for Pfizer, Inc. commentedWith the current code, you can't enforce the password policy for the user 1 account unfortunately. IMHO, this is the most critical account that needs to have a secure password since it bypasses all permission checks (which ironically is what allows it to bypass the password policy as well).
in _password_policy_show_policy():
The user 1 account will always return true on a hasPermission check. I've attached a patch which removes the above block and the bypass password policies as a temporary solution.
I think the full solution is to remove the permission and make the roles selection of a password policy to allow including and excluding of roles. Then the user 1 account (or any other account) could be added to a role that was excluded from a password policy.
Setting to needs review just to test the patch, this issue should be needs work if it doesn't fail.
Comment #3
badjava CreditAttribution: badjava at Metasun for Pfizer, Inc. commentedComment #4
daggerhart CreditAttribution: daggerhart commentedHi @badjava,
I think it's a great idea to remove the "bypass password policies" permission and your patch looks good to me. But I don't think we want to start tracking "included" and "excluded" roles on a password policy. That seems like a path to some very convoluted logic when dealing with users with multiple roles, and each role having included and excluded password policies.
I've updated the title of this issue to reflect exactly what the patch is trying to accomplish and re-queued your patch for testing. If it passes, this issue is ready to be reviewed imo. If it doesn't pass, I'll re-roll it against the current 8.x-3.x branch.
Comment #5
daggerhart CreditAttribution: daggerhart commentedTests passed. This is ready for review.
Comment #6
daggerhart CreditAttribution: daggerhart commentedVerified that this works.
As User 1, I tested editing a user that has a passworld policy. Before the patch I could change the password to anything regardless of the password policies. After the patch was applied, I was unable to save a new password that did not match the password policy.
Patch is simple and it works, markingRTBC
Comment #7
Rahulmaheshuni CreditAttribution: Rahulmaheshuni as a volunteer commentedThis patch doesn't work with composer
Comment #9
Rahulmaheshuni CreditAttribution: Rahulmaheshuni as a volunteer commentedI have updated the patch that works with composer style of applying patches.
Comment #10
johnzzonThe last patch won't work with composer, it has the paths all wrong. The one in #2 is correct however.
Comment #11
Rahulmaheshuni CreditAttribution: Rahulmaheshuni as a volunteer commentedPatch in #2 works with composer. My patch in #7 and #10 has the wrong path.
Comment #12
daggerhart CreditAttribution: daggerhart at Hook 42 commentedRequeued for testing against 8.7 & removing excess patches.
Comment #13
daggerhart CreditAttribution: daggerhart at Hook 42 commentedRe-tested and reviewed. Patch in #2 applies cleanly and does the job. The permission is gone from the system and as user-1 I am required to respect the policies that apply to my user.
Comment #14
nerdsteinI recognize that it's difficult to remove existing features, but I believe this is the right move for this permission. It confuses the system behavior. I've let this sit long enough to solicit feedback and I think, at this point in time, the right move is to commit the patch in #2. This will make it into the next release then, and we can see what kind of feedback, if any, arises. Thanks to all who participated in the discussion. I am not clear what the patches in 7 and 9 were trying to accomplish, since 2 seems to get the job done.
Comment #16
nerdsteinMarking as fixed