awcodes / filament-curator

A media picker plugin for Filament Panels.
MIT License
343 stars 89 forks source link

Column not found: 1054 #379

Closed aeq-dev closed 11 months ago

aeq-dev commented 11 months ago

Filament Version

3.1.20

Plugin Version

3.2.4

PHP Version

8.2

Problem description

After upgrading to the latest version, I got this error on trying to open the curator : SQLSTATE[42S22]: Column not found: 1054 Unknown column 'store_id' in 'where clause' I have activated the filament tenant as Store model,store belongsToMany Media

Expected behavior

I should be able to upload files

Steps to reproduce

Update to the latest version Click the curator input Error appears

Reproduction repository

No response

Relevant log output

No response

aeq-dev commented 11 months ago

NB: V3.2.3 works fine

awcodes commented 11 months ago

A reproduction repo would really help. I don't use tenancy and don't have a real way to test. But it sounds like a relationship somewhere isn't tenant aware.

aeq-dev commented 11 months ago

The problem is here : https://github.com/awcodes/filament-curator/commit/141cf06dbcff78795d0d2b614fc1eb20c1cf71a6

public function getFiles(int $page = 0, bool $excludeSelected = false): array
    {
        $files = App::make(Media::class)->query()
            ->when(filament()->hasTenancy(), function ($query) {
                return $query->where(filament()->getTenantOwnershipRelationshipName().'_id',filament()->getTenant()->id);
            })

It only manages bleongsTo relationship where we need to add store_id field into media model. but not belongsToMany

awcodes commented 11 months ago

Wouldn't you still need the store_id regardless of belongsTo or belongsToMany? The whole point of this query is to prevent tenants from modifying other tenant's files.

aeq-dev commented 11 months ago

I chose belongsToMany relation to not add extra field to media model store_id, and also to share files between tenants by just attach ids

awcodes commented 11 months ago

Ok, I'll see what I can do to make it conditional, but that sounds like a security vulnerability if tenants can modify the assets used by other tenants.

aeq-dev commented 11 months ago

Yes but it's up to the admin to share or not, What I'm doing, any file will be auto-attached to the current tenant, and then the admin has the role to share it with any other tenant

awcodes commented 11 months ago

This should be fixed in the next release.

By default the panel will be tenant aware and restricted, but you can override that with ->tenantAware() or globally in the config with is_tenant_aware.

aeq-dev commented 11 months ago

Thanks a lot dear @awcodes

aeq-dev commented 11 months ago

Update: I tried set is_tenant_aware to false but still the error appears, The ->tenantAware() method doesn't exist

awcodes commented 11 months ago

https://github.com/awcodes/filament-curator/blob/9db968c39c1a37b0c274bf85b51bfcafd6abbec6/src/Components/Forms/CuratorPicker.php#L475

aeq-dev commented 11 months ago

Oops my bad, I set it on Panel plugin :D

aeq-dev commented 11 months ago

Okey the same error appearing :/

awcodes commented 11 months ago

Then I will need a reproduction repo to help further.

aeq-dev commented 11 months ago

Hi again, I noticed that the value of tenantAwre method takes always this public bool|Closure $isTenantAware = true; without checking the CuratorPicker method neither the curator config value is_tenant_aware If I set public bool|Closure $isTenantAware = false;, it works fine. Please check here

awcodes commented 11 months ago

I have tested multiple scenarios. The correct value is passed to the picker in all instances.

if you want to disable it on a per field case you need to use ->tenantAware(false) otherwise they will default to the value of the config file.