vanilophp / framework

The truly Laravel E-commerce Framework
https://vanilo.io
MIT License
810 stars 102 forks source link

Failing Permissions for TaxonController #74

Closed S-DICKSON closed 4 years ago

S-DICKSON commented 4 years ago

Using Laravel 6.18.14.

image image

https://github.com/vanilophp/framework/blob/ac2f53d9e7c1599ae314a7134e858306aac1957f/src/resources/views/taxonomy/show.blade.php#L25

The permissions required should be create taxa instead of create taxons.

fulopattila122 commented 4 years ago

Likely comes from the buggy doctrine/inflector package. You can check it by typing

composer show doctrine/inflector

in the terminal. If the doctrine inflector version is either 1.4.0 or 2.0.0 then you have to upgrade it by

composer update doctrine/inflector

See: #72 and https://github.com/doctrine/inflector/pull/142

Let me know if it worked, thx!

fulopattila122 commented 4 years ago

Whoops, I wasn't completely correct, unlike media, this wasn't "fixed" with 1.4.1/2.01. See Sylius/Sylius#11440.

What you can do is to downgrade it to 1.3 by composer require doctrine/inflector '1.3.*'

Can you please check it on your end?

fulopattila122 commented 4 years ago

Update 3: Neither upgrade nor downgrade will work:

I'm about to roll out a fix for this shortly.

fulopattila122 commented 4 years ago

It's a bit tricky - I'm still on it. As a quick fix you can update your app's permissions table directly by executing these queries:

UPDATE permissions set name="list taxons" where name="list taxa";
UPDATE permissions set name="create taxons" where name="create taxa";
UPDATE permissions set name="view taxons" where name="view taxa";
UPDATE permissions set name="edit taxons" where name="edit taxa";
UPDATE permissions set name="delete taxons" where name="delete taxa";

Kudos to my ex-boss @ewgra who taught me to make migrations as static as possible. Would I've stuck to that rule, we would have avoided this issue 😕

fulopattila122 commented 4 years ago

Ah :hankey: it doesn't fully solve the problem, the ACL middleware is still using the pluralizer, thus it'll be denied during the HTTP request instead at the view level.

Working on it.

S-DICKSON commented 4 years ago

Having the permission set to taxa has worked on my side.

fulopattila122 commented 4 years ago

Having the permission set to taxa has worked on my side.

In the view, right?

fulopattila122 commented 4 years ago

For reference, I add the minimum required doctrine inflector versions based on Laravel versions:

Laravel Version Doctrine Inflector
Laravel 6.0 - 6.18.13 inflector ^1.1
Laravel 6.18.14+ inflector ^1.4 ^2.0
Laravel 7.0 - 7.10.3 inflector ^1.1
Laravel 7.11.0+ inflector ^1.4 ^2.0

See laravel/framework#32734 and laravel/framework#32730

S-DICKSON commented 4 years ago

Having the permission set to taxa has worked on my side.

In the view, right?

The view and the permissions in the database to be view taxa, create taxa etc. I have not tested this solution yet https://github.com/doctrine/inflector/issues/147#issuecomment-628807276

fulopattila122 commented 4 years ago

The solution is ready, and it's been released with Vanilo Framework v1.2.1.

Here's how you can upgrade:

composer update vanilo/framework konekt/appshell

This should update Vanilo to 1.2.1 and AppShell to 1.7.0.

Run the migrations afterwards, to ensure the permission names are the good ones:

php artisan migrate

Now your permission names should be "list taxons", "create taxons", etc instead of "taxa". All taxon related CRUD operations should work without problems.

Thank you for reporting this issue!

ewgRa commented 4 years ago

Kudos to my ex-boss @ewgRa who taught me to make migrations as static as possible. Would I've stuck to that rule, we would have avoided this issue confused

@fulopattila122 Thanks! Glad that it was helpful. Actually as far as I remember we also came to it through some experience :)