Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug when using exists rule on multiple select and modifyQueryUsing #12152

Closed
ryanmortier opened this issue Apr 2, 2024 · 2 comments · Fixed by #12800
Closed

Bug when using exists rule on multiple select and modifyQueryUsing #12152

ryanmortier opened this issue Apr 2, 2024 · 2 comments · Fixed by #12800
Labels
bug Something isn't working low priority unconfirmed
Milestone

Comments

@ryanmortier
Copy link

ryanmortier commented Apr 2, 2024

Package

filament/filament

Package Version

v3.2.62

Laravel Version

v11.1.1

Livewire Version

v3.4.9

PHP Version

PHP 8.2.16

Problem description

I have the following form in an AccountResource table action (take note of the modified relationship query and added constraint to the exists rule).

Forms\Components\CheckboxList::make('id')
    ->label('Collections')
    ->searchable()
    ->relationship(
        'collections',
        'name',
        fn (Builder $query): Builder => $query->ownedByCurrentUser()
    )
    ->exists(
        Collection::class,
        'id',
        fn (Exists $rule): Exists => $rule->where(
            'user_id',
            Auth::user()->id
        )
    ),

The relationships: An Account model has a many-to-many relationship to a Collection model. A collection belongs to a single user.

The collection model has a scope ownedByCurrentUser() which simply applies a ->where('user_id', Auth::user()->id) constraint.

It looks like this on the front end:

User 1:

CleanShot 2024-04-02 at 12 54 37

User 2:

CleanShot 2024-04-02 at 12 55 35

As you can see, each user see's their own collections.

The problem occurs when one user adds an account to one of their collections, and then another user tries to add the same account to one of their collections. The second user will fail with validation errors.

If you view the query that is sent to the database I believe it's because the exist rule is trying to validate against all collections that the account belongs to regardless of the scoped query and since this could be another user's collections, it fails.

For example, in the first screenshot above, User 1 had added this account to two collections. In the second screenshot, User 2 tries to add that same account to one of their collections and they get the following:

CleanShot 2024-04-02 at 13 03 47

The query that is sent to the database looks like this:

select
  count(distinct "id") as aggregate
from
  "collections"
where
  "id" in ('3', '5', '10')
  and "user_id" = '2'

When I believe it should look like this:

select
  count(distinct "id") as aggregate
from
  "collections"
where
  "id" in ('10')
  and "user_id" = '2'

The reason I have this exists rule constraint is because I don't want a user to be able to modify another user's collections like if they modified the ajax request somehow maliciously.

Expected behavior

I would expect that the only data that is validated is the data that is constrained by the modified query.

Steps to reproduce

  1. Clone reproduction repo, install and migrate (sqlite database).
  2. Navigate to /admin
  3. Login as test1@example.com / password.
  4. Click the accounts resource.
  5. Click the collections action on an account.
  6. Assign at least one collection to the account.
  7. Login as test2@example.com / password.
  8. Click the collections action on the same account in step 4.
  9. Try to assign at least one collection to the account and observe the validation error.

Reproduction repository

https://github.com/ryanmortier/filament-issue-exists-rule/

Relevant log output

No response

Donate 💰 to fund this issue

  • You can donate funding to this issue. We receive the money once the issue is completed & confirmed by you.
  • 100% of the funding will be distributed between the Filament core team to run all aspects of the project.
  • Thank you in advance for helping us make maintenance sustainable!
Fund with Polar
@ryanmortier ryanmortier added bug Something isn't working low priority unconfirmed labels Apr 2, 2024
@ryanmortier
Copy link
Author

ryanmortier commented Apr 2, 2024

I believe the issue is because the loadStateFromRelationshipsUsing() is not applying the modified query constraint. This is likely because the saveRelationshipsUsing() is using sync which requires the entire array rather than only the validated data.

Ref:

$this->loadStateFromRelationshipsUsing(static function (CheckboxList $component, ?array $state): void {
$relationship = $component->getRelationship();
/** @var Collection $relatedModels */
$relatedModels = $relationship->getResults();
$component->state(
// Cast the related keys to a string, otherwise Livewire does not
// know how to handle deselection.
//
// https://github.com/filamentphp/filament/issues/1111
$relatedModels
->pluck($relationship->getRelatedKeyName())
->map(static fn ($key): string => strval($key))
->toArray(),
);
});
$this->saveRelationshipsUsing(static function (CheckboxList $component, ?array $state) {
$pivotData = $component->getPivotData();
if ($pivotData === []) {
$component->getRelationship()->sync($state ?? []);
return;
}
$component->getRelationship()->syncWithPivotValues($state ?? [], $pivotData);
});

@danharrin
Copy link
Member

Should be fixed by #12800

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working low priority unconfirmed
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants