Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

BelongsToMany relation not fully respect policy rules in attach #6353

Closed
freebuu opened this issue Apr 19, 2024 · 5 comments
Closed

BelongsToMany relation not fully respect policy rules in attach #6353

freebuu opened this issue Apr 19, 2024 · 5 comments
Labels
pending Issues that are pending triage

Comments

@freebuu
Copy link

freebuu commented Apr 19, 2024

  • Laravel Version: 10.39.0
  • Nova Version: 4.32.12
  • PHP Version: 8.1.2
  • Database Driver & Version: Postgres 16
  • Operating System and Version: MacOS
  • Browser type and version: Safari 17.3.1

Description:

I found a (possible) bug in the way BelongsToMany works. In the policy I created a condition like

    public function attachAnyGame(User $user, Model $model): bool
    {
        if ($model->games()->count() > 6) {
            return false;
        }

        return true;
    }

It works, the Attach button disappears if it is exceeded limit of 6.

However, if you use the Attach & Attach Another functionality, then the policy does not work and you can add it endlessly.

Detailed steps to reproduce the issue on a fresh Nova installation:

  • Add BelongsToMany relation field
  • Create policy for attaching this relation (like upper example)
  • Try to attach relations with Attach & Attach Another

Expected Behavior

When clicking Attach & Attach Another - the policy checks the conditions and returns 403 if policy return false

@crynobone crynobone added the pending Issues that are pending triage label Apr 19, 2024
@freebuu
Copy link
Author

freebuu commented May 9, 2024

@crynobone any news? Maybe you need more info?

@crynobone
Copy link
Member

This feel a chicken vs egg scenario. Policy is not able to predict that adding 2 new records is not allowed. It can only check if we attempting to create a record.

@freebuu
Copy link
Author

freebuu commented May 9, 2024

But why policy don't check conditions AFTER i press the Attach button? In my opinion, this is two separate scenarios:

  • check policy for show/hide buttons
  • check policy when button is pressed

For now only first scenario works.

@crynobone
Copy link
Member

As I said earlier, during button press it can only get the policy for current process and cannot predict if it can add another. Showing 403 as suggested above is not the ideal solution (possibly making the UX worst)

@freebuu
Copy link
Author

freebuu commented May 9, 2024

Understand. But current behavior is not ideal too - you have policy with some restrictions, but if user open attach screen - it completely not working.

My workaround now - use model creating callback, but this is bit ugly. Hope you can do something with this in your way!

@laravel laravel locked and limited conversation to collaborators May 14, 2024
@crynobone crynobone converted this issue into discussion #6384 May 14, 2024

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
pending Issues that are pending triage
Projects
None yet
Development

No branches or pull requests

2 participants