Skip to content
This repository was archived by the owner on Jan 29, 2021. It is now read-only.

Bug hunt #1165

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Bug hunt #1165

wants to merge 5 commits into from

Conversation

aaronschachter
Copy link
Contributor

What's this PR do?

This pull request looked promising to fix the bug reported in DoSomethingArchive/chompy#198 -- the debugging in Chompy displays the posts details in the API response.

However, when my local Chompy then attempts to execute a PATCH /posts/:id request to update the post status, it receives an error with status 403: This action is unauthorized.

Per the 2nd commit, I thought that might be the issue where Gambit doesn't see why_participated, but I can see it breaks a ton of our tests (the 1st commit for PostPolicy does not).

How should this be reviewed?

...

Any background context you want to provide?

...

Relevant tickets

References Pivotal #.

Checklist

  • This PR has been added to the relevant Pivotal card.
  • Documentation added for new features/changed endpoints.
  • Added appropriate feature/unit tests.

@aaronschachter
Copy link
Contributor Author

@DFurnes Per out chat today, I tried both using the full path to the Gate facade within the PostTransfomer, and also using kebab case view-all instead of the camelCase viewAll when calling the Gate method and didn't have luck with either.

Looking at the Northstar policy methods, I saw that the Rogue policy methods were missing the nullable type and thinking that could be a clue... but wondering if this potentially sheds any new light on the bug?

@DFurnes
Copy link
Contributor

DFurnes commented Jan 27, 2021

Hmm, I think you're onto something! By default, Laravel disallows anonymous users from being authorized to do things unless you use those nullable types to add "guest user" support. These requests aren't really anonymous.... but they also come from machines (Gambit & Chompy) and not a traditional logged-in user!

If you add those nullable types, does the logger from within viewAll run?

@aaronschachter
Copy link
Contributor Author

Yes, just committed adding a logger into the Post Policy's viewAll and confirmed it logs (which wasn't happening in the branch that I initially was working on during our call -- I created a new branch once it seemed like this nullable / machine-user issue was the culprit).

@aaronschachter
Copy link
Contributor Author

Running the signup tests and can see this error -- where the test is passing a DoSomething\Gateway\Server\RemoteUser, causing a failure...

[2021-01-27 22:42:32] testing.ERROR: Argument 1 passed to App\Policies\SignupPolicy::viewAll() must be an instance of App\Policies\Authenticatable or null, instance of DoSomething\Gateway\Server\RemoteUser given, called in /home/vagrant/Code/rogue/vendor/laravel/framework/src/Illuminate/Auth/Access/Gate.php on line 706 {"exception":"[object] (Symfony\\Component\\Debug\\Exception\\FatalThrowableError(code: 0): Argument 1 passed to App\\Policies\\SignupPolicy::viewAll() must be an instance of App\\Policies\\Authenticatable or null, instance of DoSomething\\Gateway\\Server\\RemoteUser given, called in /home/vagrant/Code/rogue/vendor/laravel/framework/src/Illuminate/Auth/Access/Gate.php on line 706 at /home/vagrant/Code/rogue/app/Policies/SignupPolicy.php:19)
[stacktrace]

@aaronschachter
Copy link
Contributor Author

Tried working around defining the class in 3a281de by setting a null default, which gets tests to pass locally -- but our linter won't allow it. Would we always expect the viewAll to be called with a nullable DoSomething\Gateway\Server\RemoteUser ? (which I'd imagine won't be the case anymore once app is merged into Northstar...)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants