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

[4.x] Fix: GateWatcher shows allowed when it is denied via Response class #1010

Merged

Conversation

ReeceM
Copy link
Contributor

@ReeceM ReeceM commented Feb 8, 2021

Error that happens

While returning and using the Illuminate\Auth\Access\Response class deny and allow methods from policies, I noticed that telescope is recording the Gate results as always allowed

I determined that the $result argument that the GateWatcher class is looking at it as only a bool, but this value also has the possibility of being an instance of Illuminate\Auth\Access\Response in the case of a policy that is returning a response, so when the policy has $this->denied('Reason for denial') the request entry reflects a status code of 403, but it is confusing to see the gate value is allowed.

As the intention of Telescope is to assist in debugging and seeing whats happening, this actually causes confusion in the long run.

Fix

The changes I made was to look if the $result argument is an instance of the Illuminate\Auth\Access\Response class, if not it defaults to the original behaviour.

Tests

The tests all passed locally, but will only know once the repo's ones run if they are correct.

The tests have a few changes for the Gate Watcher, but these are the line numbers and the additions I made. I am not sure if I was supposed to change the line numbers, but they shifted because of my additions I believe.

@driesvints driesvints changed the title Fix: GateWatcher shows allowed when it is denied via Response class [4.x] Fix: GateWatcher shows allowed when it is denied via Response class Feb 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants