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

Implement auth0 guard #166

Merged
merged 9 commits into from
Apr 3, 2020
Merged

Implement auth0 guard #166

merged 9 commits into from
Apr 3, 2020

Conversation

Tamrael
Copy link
Contributor

@Tamrael Tamrael commented Mar 3, 2020

Changes

  • new guard driver "auth0" to allow for easy authentication without custom middleware and allow for testing with actingAs
  • bind Auth0UserRepository to it's contract on plugin boot to allow for customization from inside laravel apps while using the guard
  • guard is inactive as long as the config values are not set to use this guard as driver in laravel auth config. See readme for example on how to set them

References

Implementation through closure request guard

Implemented to fix testing problems mentioned in #161

Testing

  • This change has been tested on the latest version Laravel

Checklist

@Tamrael Tamrael requested a review from a team March 3, 2020 14:13
Copy link
Contributor

@joshcanhelp joshcanhelp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Tamrael - Really appreciate the well-thought-out PR here! A few questions for you:

  • Is this primary or only for API authentication or does this handle login as well? I believe it's just the former.
  • Should this replace our current guidance (web application login or API protection) on implementing this module or should it exist alongside?
  • Is this compatible with Laravel 5.7? Judging by the documentation, it looks like it is but just want to make sure.

@Tamrael
Copy link
Contributor Author

Tamrael commented Mar 12, 2020

@joshcanhelp let me try and address your questions to the best of my knowledge

  • the guard only replaces the Middleware as the point of authorization for requests with a jwt bearer token. Login/logout still need to be handled as well as any Userprovider changes and such
  • I would suggest replacing the part about the middleware with guidance of the guard because it's a lot closer to the laravel way of doing authentication. For the sake of people that already implemented middleware I would leave it in as footnote or alternative way of handling API authentication.
  • the guard helper this is using was added in 5.6 and the API never changed, only concerns is the "bearerToken" function that's not well documented. Can't find when this was added atm but I'm almost sure it's a helper that existed a long time.

@joshcanhelp
Copy link
Contributor

@Tamrael - Thanks again!

Let me get this running locally and if everything looks good, we'll get this in.

@joshcanhelp joshcanhelp mentioned this pull request Mar 13, 2020
@Tamrael
Copy link
Contributor Author

Tamrael commented Mar 15, 2020

@joshcanhelp just had the time to check. the bearerToken function is part of the "InteractsWithInput" trait and is available since 5.4 so everything about this guard is 5.4 and above. if you need anything else I'd be glad to help.

@joshcanhelp
Copy link
Contributor

joshcanhelp commented Mar 27, 2020

@Tamrael - Apologies for the delay here.

I appreciate all your explanation here and can see that this would be useful to add. I pulled the branch down and followed the documentation you added. Everything came together as expected but when I hit the route, I get a redirect to the Auth0 login page. If I understand how this works, it's looking for a JWT in the headers with $request->bearerToken(). Since a user would not explicitly add that to the headers that their browser sends, I assumed that this guard is used on API-type routes where a 302 to a login page would be difficult for the API caller to handle. I would have expected this to fail on the route with a 401 or similar.

Am I misunderstanding how this should be used? I followed your setup instructions with the following in routes/api.php:

Route::middleware('auth:auth0')->get('/auth0-guard', function (Request $request) {
    return $request->user();
});

@joshcanhelp joshcanhelp removed the request for review from a team March 27, 2020 15:07
@Tamrael
Copy link
Contributor Author

Tamrael commented Mar 28, 2020

@joshcanhelp sorry didn't think to mention that. the request to the api must expect json as a return or be ajax (not pjax) and accept any content type. the handling for the unauthenticated message is completely done by laravel.
see Exceptions/Handler.php#L216 for the implementation

@joshcanhelp joshcanhelp changed the base branch from master to 7.0.0-dev April 1, 2020 03:57
@joshcanhelp joshcanhelp changed the base branch from 7.0.0-dev to master April 1, 2020 03:58
@joshcanhelp joshcanhelp changed the base branch from master to 7.0.0-dev April 3, 2020 04:28
@joshcanhelp joshcanhelp changed the base branch from 7.0.0-dev to master April 3, 2020 04:28
@joshcanhelp
Copy link
Contributor

@Tamrael - Thank you for the suggestion, that worked as expected.

I'm sorry to keep coming back with questions/changes but I think we should get this in. The next version, however, is a major so this should be merged into that branch. When I switch the branch here to the dev branch 7.0.0-dev, I get a few conflicts. Would you mind switching the base branch and making those changes?

Thank you once again for sticking with this! I'm glad we'll be able to get this in.

Tamrael added 4 commits April 3, 2020 10:01
# Conflicts:
#	composer.json
#	src/Auth0/Login/LoginServiceProvider.php
#	tests/Auth0ServiceTest.php
@Tamrael Tamrael changed the base branch from master to 7.0.0-dev April 3, 2020 08:06
@Tamrael
Copy link
Contributor Author

Tamrael commented Apr 3, 2020

@joshcanhelp i rebased this onto 7.0. tests ran in laravel 5, 6 and 7. can't tests this further atm but I'm pretty sure this works.

I removed the tests/bootstrap.php because it's not needed when setting the ini in the phpunit.xml, hope you don't mind.
Had to fix the function declarations for the setUp of the tests because phpunit > 7 was required for laravel 7 compatibility

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@joshcanhelp joshcanhelp added this to the 6.0.0 milestone Apr 3, 2020
@joshcanhelp joshcanhelp merged commit 5a29f98 into auth0:7.0.0-dev Apr 3, 2020
@joshcanhelp
Copy link
Contributor

@Tamrael - We appreciate your work on this! Thanks for the great communication as well!

@joshcanhelp joshcanhelp changed the title implement auth0 guard Implement auth0 guard Apr 9, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants