Permissions for viewing list types in the CP are incorrectly initialised

[martyspain]

Describe the bug

I have an issue where the final list type is unable to be viewed by non-admin CP users even though they have the correct permissions set to view this list type. I've dug into the source code to figure out why, and I think I've found the cause.

The permissions for each list type are set up using an array index as the suffix for the permission in the _registerPermissions function in the main Wishlist.php plugin file (line 194):

foreach ($listTypes as $id => $listType) {
    $suffix = ':' . $id;
    $listTypePermissions['wishlist-manageListType' . $suffix] = ['label' => Craft::t('wishlist', 'Manage “{type}” lists', ['type' => $listType->name])];
}

$id in this case is the index of the list type in the object, which will be a 0-based index so the permissions to view each list type are set up like this:

wishlist-manageListType:0
wishlist-manageListType:1
wishlist-manageListType:2

and so on.

However, when the 'Lists' sidebar for the Wishlist CP section is rendered in the defineSources function of ListElement.php, the permissions are set up using the ID of each list type as the suffix (line 77):

foreach ($listTypes as $listType) {
    $key = 'listType:' . $listType->id;
    $canEditLists = Craft::$app->getUser()->checkPermission('wishlist-manageListType:' . $listType->id);
}

The ID of each list type uses a 1-based auto-increment index from the database, so the the $listType->id suffix in the Craft::$app->getUser()->checkPermission('wishlist-manageListType:' . $listType->id); line will be 1-based and therefore permissions for each list type will be offset by 1.

This seems to be why my users cannot view lists for the last list type that was created, because it's checking for a permission like this:

wishlist-manageListType:6 (6 being the ID of the last list type that was created)

But the stored permission that the user has looks like this:

wishlist-manageListType:5 (5 being the index of that list type in the $listTypes object that's used to set up permissions during the plugin's initialisation.

I haven't tested this yet, but I think the fix could be updating line 195 of Wishlist.php to use the list type's ID instead:

foreach ($listTypes as $listType) {
    $suffix = ':' . $listType->id;
    $listTypePermissions['wishlist-manageListType' . $suffix] = ['label' => Craft::t('wishlist', 'Manage “{type}” lists', ['type' => $listType->name])];
}

Steps to reproduce

1. Set up more than one wishlist type.
2. Grant a non-admin user CP access and permission to view both list types.
3. Log in as that user.
4. Go to the Wishlist section in the CP and click on 'Lists'.
5. Note that you can only view one list type in the 'List Types' subnav, not both.

Craft CMS version

4.4.9

Plugin version

2.0.4

Multi-site?

No

Additional context

No response

[engram-design]

Actually, we shouldn't be using the ID of list types at all, as they aren't consistent across multiple environments. We should be using UIDs instead.

Updated for the next release. To get this early, run composer require verbb/wishlist:"dev-craft-4 as 2.0.4

[martyspain]

@engram-design Thanks for looking at this so quickly. I've tried the fix and the migration to update the user permissions table runs but doesn't update the permissions on my system so after installing the update, non-admin users now can't see any lists at all! It looks like something is failing in the conditional so it never hits the update statement on line 33 of the migration.

[engram-design]

Good call, actually there's some minor fixes I've made to the migration. Rather than adding a new one (which I can do if it's too much trouble), would it be possible for you to roll back the last migration and re-apply it? You can do this by removing the migration row in migrations database table, and changing the schemaVersion of the Wishlist plugin back on minor version. This should let Craft know that the migration needs to run again.

[martyspain]

Sure, no worries. I have a backup of the database pre-migration anyway, so I'll reset everything and try again.

[martyspain]

OK, tried again with the latest update and it updated the permissions for some but not all of the Wishlist rows in the userpermissions table. I can fix these manually, but would it be useful to send over a database dump to debug why some were missed?

[engram-design]

That’d be helpful if you can, but yeah manually updating those should be fine. Send through to web@verbb.io

[martyspain]

I've emailed a link to the DB file through to you - it's a bit large so I didn't want to send directly.

I've also found that the ListsController.php file needs updating on line 76 to use the UID as well, so that users can view the list item.

[engram-design]

Thanks, updated those references too.

[engram-design]

Fixed in 2.0.5