Adds logout function to Gateway guard so we can make multiple API requests with different tokens in unit tests.

DoSomething / gateway

:lock_with_ink_pen: An opinionated PHP REST API client.

MIT License

2 stars 0 forks source link

Adds logout function to Gateway guard so we can make multiple API requests with different tokens in unit tests. #116

Closed chloealee closed 6 years ago

chloealee commented 6 years ago

What's this PR do?

Adds logout function to Gateway guard so we can make multiple API requests with different tokens/users in unit tests (needed in this Rogue PR!).

How should this be reviewed?

👀

Checklist

[ ] Tests added for new features/bug fixes.
[ ] Is this a breaking change?

DFurnes commented 6 years ago

So that we don't have to think about this in the future, let's try calling this function in withAccessToken, maybe right above here?

// If a user is already authenticated, reset the guard.
auth('api')->logout();

chloealee commented 6 years ago

ah yes thanks @DFurnes ! Added :)

DFurnes commented 6 years ago

Looks great! If you haven't already, just give a quick test to make sure this fixes your issue on DoSomething/rogue#697 by updating that app's composer.json to point to dev-logout & running composer update. If tests pass, merge away!

chloealee commented 6 years ago

Cool - I didn't know you could do that @DFurnes ! Good to know for the future! I ran the tests against these changes and a lot of Rogue tests were getting a 401 Unauthorized :-/

DFurnes commented 6 years ago

Oh no! That's a bummer. 😟 Want to dig into this together tomorrow?

chloealee commented 6 years ago

that'd be great, @DFurnes !

DFurnes commented 6 years ago

We looked into this together and ended up doing a little more refactoring!

With the changes in 70fe130, we'll always create a new Token instance and parse the JWT when calling Auth::id(). This prevents issues where we have a cached token/key in memory (from calling Auth::id() too early, like in a test). This change should have a negligible performance impact, and make it a lot easier to reason about what's going on in the future.

chloealee commented 6 years ago

Thanks for adding that context, @DFurnes ! I ran the phpunit tests and getting a lot of failures :-/

When testing manually and phpunit, here are the errors I'm seeing:

DELETE /v3/signups/:signup_id, DELETE /v3/posts/:post_id, POST api/v3/posts/:post_id/reviews, POST api/v3/posts/:post_id/tags
- 500 "message": "Trying to get property 'role' of non-object"

In addition, it looks like using $post->tag('Hide In Gallery'); isn't working in the tests as expected.

chloealee commented 6 years ago

@DFurnes update - the latest commit now gets all endpoints in Rogue working and $post->tag('Hide In Gallery'); works in Rogue tests as expected! But... breaking gateway tests now 👎

chloealee commented 6 years ago

@DFurnes false alarm! The changes I made in RequireRole were actually not needed and were causing errors. I changed them back in the last commit and now everything seems to be working!

Just for a peace of mind, would you mind giving one more 👀 before I merge?