Problem/Motivation

We needed to rebuilt a site built with Drupal 6 to Drupal 8 and while trying to migrate the user restrictions I noticed the type "Hostname/IP" was missing entirely in version 8. So I had a look on how to add this with custom hooks and it would be really painful :/

Proposed resolution

Switch from hardcoded restriction types to plugins.
Bonus (to be discussed): rename the entity from "User restrictions" (plural) to "User restriction" (singular) including the entity type name itself because the entity itself represents a single restriction.

Remaining tasks

Add patch.

User interface changes

None

API changes

Well, a lot :) After applying the patch, developers can add various custom restriction types using plugins instead of altering the form/submit/validation.

Data model changes

None.

CommentFileSizeAuthor
#18 user_restrictions-refactoring_plugins-2936400-final.patch69.03 KBadammalone
#16 user_restrictions-refactoring_plugins-2936400-16.patch64.82 KBadammalone
#15 user_restrictions-refactoring_plugins-2936400-15.patch64.27 KBadammalone
#14 user_restrictions-refactoring_plugins-2936400-14.patch64.24 KBadammalone
#13 user_restrictions-refactoring_plugins-2936400-13.patch64.36 KBadammalone
#12 user_restrictions-refactoring_plugins-2936400-12.patch62.44 KBadammalone
#11 user_restrictions-refactoring_plugins-2936400-10.patch62.03 KBadammalone
#9 user_restrictions-refactoring_plugins-2936400-9.patch62 KBadammalone
#8 user_restrictions-refactoring_plugins-2936400-8.patch0 bytesadammalone
#7 user_restrictions-2936400-interdiff-4-7.diff2.46 KBadammalone
#7 user_restrictions-refactoring_plugins-2936400-7.patch60.62 KBadammalone
#5 user_restrictions-refactoring_plugins-2936400-5.patch60.24 KBadammalone
#4 user_restrictions-refactoring_plugins-2936400-4.patch59.48 KBstBorchert
#4 interdiff-2936400-3-4.txt6.32 KBstBorchert
#3 user_restrictions-refactoring_plugins-2936400-3.patch58.73 KBstBorchert
#2 user_restrictions-refactoring_plugins-2936400-2.patch58.68 KBstBorchert
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

stBorchert created an issue. See original summary.

stBorchert’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
58.68 KB
stBorchert’s picture

Fixed some errors in the patch.

stBorchert’s picture

Added more fixes (including default expiry date when adding rules).
To maintain backwards compatibility with old restriction masks (using "%" as wildcard) we have to replace all "%" with ".*" in the patterns. Unfortunately you cannot use "LIKE" to query config entities so the pattern have to be a real regular expression ...

adammalone’s picture

Status: Needs review » Needs work
FileSize
60.24 KB

This is fantastic thank you for your time and effort from this. I'm uploading a new patch that addresses a couple of very minor issues I found.

One of the things I noticed was that this doesn't work as expected for email addresses due to the '.' character and how preg_quote() changes a rule like ".*@com.au" to "\.\*@com\.au" so the preg_match() doesn't work. I'm wondering if we need to remove the preg_quote here?

The block in question is in src/Plugin/UserRestrictionType/UserRestrictionTypeBase.php

    foreach ($rules as $rule) {
      $pattern = strtr(preg_quote($rule->getPattern()), ['%' => '.*']);
      if (preg_match('/' . $pattern . '/i', $value)) {
        // Exit loop after first matching pattern.
        return ($rule->getAccessType() == UserRestrictions::BLACKLIST) ? $rule : FALSE;
      }
adammalone’s picture

Also noting here I think we need to look at the usecase where we might want to blacklist all email addresses apart from one mask. Currently the functionality introduced in the patch exits as soon as it finds a match so won't allow us to have black/white list combos.

adammalone’s picture

Adding new patch to address issue in #6 and interdiff between 4 and 7.

adammalone’s picture

Further work here to add:

  • Config schema
  • Fixing tests up a bit more
  • Work on validation
adammalone’s picture

Uploaded wrong file - trying again.

adammalone’s picture

adammalone’s picture

Amended tests further and fixed coding standards violations.

adammalone’s picture

adammalone’s picture

Fixed an issue where permissions checking for users able to bypass user permissions occurred before they had logged in and therefore had said permissions. Also further work on tests.

adammalone’s picture

adammalone’s picture

adammalone’s picture

adammalone’s picture

Status: Needs review » Fixed
FileSize
69.03 KB

Thanks heaps @stBorchert I've committed this to the 8.1.x dev branch with some minor changes and tests that now work. This is a great base to move forward from. The final patch is attached here.

Status: Fixed » Closed (fixed)

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