fix: Wildcard permissions algorithm performance (ALERT: Breaking Changes)

[danharrin]

This week we were investigating performance improvements for an app. It uses wildcard permissions. A specific page checks 3000 wildcard permissions, and was taking ~3.5s to load.

We searched the issues, and found #1424, which explained that the cache was not being used for wildcard permissions. It ended up being closed without a solution. We checked our cache, and sure enough there were no permissions there.

We tested this discovery by disabling wildcard permission checks in the config file, and found that our page only took ~600ms to load after that.

I started off this PR investigating why permissions were not being cached, but then stumbled upon the real reason why these checks were so slow:

• Each time a permission is checked, a collection of "all permissions" is constructed from the user's direct permissions and their role permissions.
• This collection was being looped over, and an algorithm was being used to check if any of the patterns satisfied the permission that was currently being checked.

I probably don't need to explain why doing this for 3000 permission checks is not a good idea, but just to give you an idea, if you store the "all permissions" collection in memory instead of generating it every time, it does shave ~1s off of our loading time for this page. Which is a great improvement for us, but not enough.

This was a tricky problem to solve, so I had to make a lot of changes to how wildcard permissions work. I believe this PR introduces breaking changes, since I modified a public interface (Wildcard) and how the implementation (WildcardPermission) is used.

Now, with the solution, our page speed is down to ~600ms with 3000 checks instead of ~3.5s.

All existing tests are passing, but I have targeted this PR at the main branch instead, ready for the (upcoming?) major 6.x release.

The solution

To start with, I have introduced a new "wildcard permission index" on the existing PermissionRegistrar class. This index is a deeply nested array that is optimised for wildcard permission checks.

The top level of the index array is keyed by the class name of the model which has permissions (Role or User). The second level of the index array is keyed by the key of the model which has permissions (Role or User). This allows us to optimally store the index of a particular model in memory, so it is not built more than once per request. When the index is requested for a particular model, we will check if it has been built already, and retrieve it. If not, we will build it, and store it in the PermissionRegistrar object of the container for next time. If a permission or role is added or removed from a user, the index for that user is flushed and rebuilt. If a permission is added or removed from a role, the index for all users is flushed.

The third level of the index array is keyed by the guard name of the permission that has been cached. That allows us to quickly eliminate any permissions from being checked that have the wrong guard.

The rest of the index array past the third level is responsible for the actual permission "parts". When a permission is indexed, it is split into parts. Each part name is used as the index of the array. If a part has "subparts" (separated by a comma usually), then multiple array entires are created, one for each subpart. Once all part names are in the index array, we add a final entry with a null key and true value. This signifies that the permission is granted to the user.

By structuring the index array in this way, we can more easily check if there is a wildcard permission that matches or not.

Example 1

Permissions assigned to user - articles

Permission checked - articles

Index array:

{
    articles: {
        null: true,
    },
}

Checks required for wildcard permission - index.articles

Example 2

Permissions assigned to user - articles.*

Permission checked - articles.1

Index array:

{
    articles: {
        *: {
            null: true,
        },
    },
}

Checks required for wildcard permission - index.articles.1, index.articles.*

Example 3

Permissions assigned to user - articles.*.edit

Permission checked - articles.1.edit

Index array:

{
    articles: {
        *: {
            edit: {
                null: true,
            },
        },
    },
}

Checks required for wildcard permission - index.articles.1.edit, index.articles.*.edit

Example 4

Permissions assigned to user - articles.*.create,edit

Permission checked - articles.1.edit

Index array:

{
    articles: {
        *: {
            create: {
                null: true,
            },
            edit: {
                null: true,
            },
        },
    },
}

Checks required for wildcard permission - index.articles.1.edit, index.articles.*.edit

Example 5

Permissions assigned to user - articles.1,2,3.edit

Permission checked - articles.2.edit

Index array:

{
    articles: {
        1: {
            edit: {
                null: true,
            },
        },
        2: {
            edit: {
                null: true,
            },
        },
        3: {
            edit: {
                null: true,
            },
        },
    },
}

Checks required for wildcard permission - index.articles.2.edit

Example 6

Permissions assigned to user - articles.1,2,3

Permission checked - articles.2.edit

Index array:

{
    articles: {
        1: {
            null: true,
        },
        2: {
            null: true,
        },
        3: {
            null: true,
        },
    },
}

Checks required for wildcard permission - index.articles.2

[danharrin]

Hi @stefanriehl @erikn69, sorry for the @, but since you both contributed to parts of this wildcard permission system then it's good to loop you in and see what you think about these changes. If you are not interested or no longer using this package, please ignore :)

[stefanriehl]

Hi @danharrin,
Unfortunately, I don't have the time to deal with this more intensively at the moment. But basically this was still an open point, especially when it comes to a lot of permission checks. I will have a closer look when I have more time. Thank you for your work and at first glance it looks promising.
Best,
Stefan

[freekmurze]

@drbyte For me this change seems ok, what do you think?

Are there any more things we'd like to do before tagging a new major version?

[drbyte]

Thanks. I will review this week...

[drbyte]

@danharrin Thanks!

[drbyte]

@danharrin I see now that phpstan is flagging these concerns ...

  Line   src/PermissionRegistrar.php
 ------ -------------------------------------------------------------------------------------
  172    Call to an undefined method Illuminate\Database\Eloquent\Model::getWildcardClass().
 ------ -------------------------------------------------------------------------------------

 ------ --------------------------------------------------------------------------------------
  Line   src/WildcardPermission.php
 ------ --------------------------------------------------------------------------------------
  32     Call to an undefined method Illuminate\Database\Eloquent\Model::getAllPermissions().

Aside: while reviewing I noted the use of Eloquent/Model in WildcardPermission constructor. But I was paying more attention to the choice of $record for variable name, which I eventually satisfied myself that I was okay with. And then I didn't go back and think about this.

I'm undecided whether to flag exceptions for phpstan, or to do something different. Thoughts?

[danharrin]

@drbyte If I were to design this library from scratch, I would add a required interface to the model to allow for this sort of type safety. But since you don't have that and I doubt you would do such a large breaking change, I don't know if there is really another option apart from flagging these exceptions, unless you use method_exists().

[drbyte]

@danharrin

1. Agreed on the breaking-change factor. Worth separately exploring pros and cons of forcing an interface for all models capable of permissions handling.

2. By any chance, have you run this new code on Octane/Swoole? I'm wondering if a call to the new forgetWildcardPermissionIndex() should be included in clearPermissionsCollection()?

[danharrin]

Worth separately exploring pros and cons of forcing an interface for all models capable of permissions handling.

This is ultimately down to maintainer personal preference, are you ok with enforcing an interface in the upgrade guide?

By any chance, have you run this new code on Octane/Swoole? I'm wondering if a call to the new forgetWildcardPermissionIndex() should be included in clearPermissionsCollection()?

I have not tested it. However, I do not believe it needs to be cleared after every request, since the index is not user-specific and should still be useful across sessions and requests. It may even provide a further performance boost, as the index doesn't need to be regenerated. Let me know if you see any dissonance in this logic.