8000 GuardAuthenticator implementation for Symfony 2.8 and later by niels-nijens · Pull Request #75 · auth0/symfony · GitHub
[go: up one dir, main page]
Skip to content

GuardAuthenticator implementation for Symfony 2.8 and later #75

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Sep 26, 2019
Merged

GuardAuthenticator implementation for Symfony 2.8 and later #75

merged 10 commits into from
Sep 26, 2019

Conversation

niels-nijens
Copy link
Contributor
@niels-nijens niels-nijens commented Jul 17, 2019 •
edited
Loading

The current SimplePreAuthenticator implementation is deprecated since Symfony 4.2. The guard authentication system provides more flexibility to eg. more verbose responses concerning authentication. And it makes this bundle (better) forward-compatible with Symfony 5 and later.

This PR adds the following:

  • A JwtGuardAuthenticator class with service definition to use in the security.y(a)ml.
  • A JwtUserProvider which adheres to the JWTUserProviderInterface. It offers a basic user provider implementation by reading the information from the token and transforming it to a Symfony User.
  • A docblock fix for JWTUserProviderInterface::loadUserByJWT.

At the moment the JwtGuardAuthenticator returns JSON responses, but I guess not everyone would want that.

If you'd like to provide more flexibility, I'm willing to extract that part from the guard authenticator.

References

Testing

Instead of following chapter "Set up the SecurityProvider" of the Symfony API: Authorization guide add the following configuration to the security.y(a)ml:

security:
    providers:
        a0:
            id: jwt_auth.security.user.jwt_user_provider

    firewalls:
        secured_area:
            pattern: ^/api
            stateless: true
            provider: 'a0'
            guard:
                authenticators:
                    - jwt_auth.security.guard.jwt_guard_authenticator

    access_control:
        - { path: ^/api/private-scoped, roles: ROLE_JWT_SCOPE_READ_MESSAGES }
        - { path: ^/api/private, roles: ROLE_JWT_AUTHENTICATED }
        - { path: ^/api/public, roles: IS_AUTHENTICATED_ANONYMOUSLY }

Optionally, chapter "Set up the User and UserProvider" can also be skipped by configuring the JwtUserProvider.

  • This change adds test coverage
  • This change has been tested on Symfony 4.2
  • This change has been tested on the latest version of Symfony

Checklist

@niels-nijens niels-nijens requested a review from a team July 17, 2019 13:57
@joshcanhelp joshcanhelp requested review from joshcanhelp and removed request for a team July 17, 2019 19:48
@niels-nijens
Copy link
Contributor Author

Hey @joshcanhelp, when do you expect to be able to take a look at this PR? 🙂

@joshcanhelp
Copy link
Contributor

@niels-nijens - At first glance, this looks great! Just to clarify ... this is adding another way to authenticate? Do you have some sample code for how this would be used?

Also, thank you for the tests, much appreciated!

Let me know on the above and I'll pull this down and take a look. As long as there aren't any breaking changes (doesn't look like it) and this is Symfony 3 compatible (looks like it is based on CI), then we should be fine to merge this in. I'll look into the Snyk CI check failing on my end.

@A-Marwan
Copy link

any help needed to get this PR merged?

@joshcanhelp
Copy link
Contributor

@A-Marwan - I'd like to know a little more about what this is actually adding or improving.

@darthf1
Copy link
Contributor
darthf1 commented Aug 14, 2019 •
edited
Loading

@joshcanhelp This PR adds a Guard for authentication. Using the SimplePreAuthenticator is deprecated since SF 4.2 and will be removed altogether in SF 5 which will release in november.

The PR (seems to) adds the neccesary Guard code, which needs to be configured
using the .yml example in OP. The .yml code should be included in the docs, as users needs to implement this in their SF config. As far as I can see, nothing is removed so SimplePreAuthenticator should still be working for SF < 4.2 (or as a deprecated feature for SF 4.2 > 4.4).

Would be great if you can merge this and release a new version, together with the other PR's you've merged today.

@darthf1
Copy link
Contributor
darthf1 commented Aug 21, 2019

hi @joshcanhelp , anything else you need? :)

@joshcanhelp
Copy link
Contributor

@darthf1 - Thanks for the run-down, I appreciate it. I'll take some time next week to walk through it.

@joshcanhelp joshcanhelp self-assigned this Sep 6, 2019
@joshcanhelp joshcanhelp removed their request for review September 6, 2019 17:10
joshcanhelp
joshcanhelp suggested changes Sep 16, 2019
View reviewed changes
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.

Great work here! A few minor things that I didn't want to comment over and over:

  • Let's use the short array syntax [] instead of array()
  • Let's switch anything that's authZero to auth0

But my main concern is ... how is this guard meant to be used. There are a lot of User mentions throughout but, if you're protesting an API, this is a client acting on behalf of a user, not a user themself proving authentication. This makes it seem like an ID token is being used to call an API which, in general, is not a good idea.

Can you give an example of how this would be used? The main thing I would look for is the API audience being passed in so it can be validated and, then, we're sure that the token is meant for the API using this authenticator. The tests mock the service so I can't see that happening.

Thanks again!

@darthf1
Copy link
Contributor
darthf1 commented Sep 18, 2019 •
edited
Loading

Hopefully @niels-nijens can help with your comments on the code. I can make some of the changes but I'm not that skilled in PHP testing.

@joshcanhelp Regarding your concerns; the reason there's a lot of references to 'User', is because of assumptions made a long time ago by Symfony regarding security, that a User is always a real user. The eco-system has changed and this assumption is now incorrect. They're reworking everything related to security and your concern is specifically mentioned on the first bullet point. So the reference in code is to a User, but it doesn't necessarily have to be a real user (like in this case).

The implementation of this merge request is actually really easy, you only need to change (in relation to the current implementation) the firewalls config in security.yaml:

firewalls:
  secured_area:
    pattern: ^/api
    stateless: true
    simple_preauth:
      authenticator: jwt_auth.jwt_authenticator
      provider: a0

Becomes:

firewalls:
  secured_area:
    pattern: ^/api
      stateless: true
      provider: 'a0'
      guard:
        authenticators:
          - jwt_auth.security.guard.jwt_guard_authenticator

Now Symfony's Guard authenticator is leveraged, instead of the deprecated simple_preauth.

@niels-nijens
Copy link
Contributor Author

Sorry, due to vacation I wasn't able to respond earlier. I'll make the changes based on your feedback today, @joshcanhelp.

@niels-nijens
Copy link
Contributor Author

@darthf1 Thanks for answering some of @joshcanhelp's questions. 👍

@joshcanhelp I think I addressed all the feedback you had.

@joshcanhelp
Copy link
Contributor

@darthf1 - Thanks from me as well! Great explanation.

@niels-nijens - This looks great. I'll give it one more pass this week and get the CI passing this week.

@joshcanhelp
Copy link
Contributor
  • CodeCov API reports test coverage with this PR merged is 74.24%
  • Ran snyk test in CI SSH and it reported Tested 33 dependencies for known issues, no vulnerable paths found.

joshcanhelp
joshcanhelp approved these changes Sep 26, 2019
View reviewed changes
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.

Looks great @niels-nijens! Thank you so much for your hard work on this.

@joshcanhelp joshcanhelp added this to the 3.2.0 milestone Sep 26, 2019
@joshcanhelp joshcanhelp merged commit a4f273b into auth0:master Sep 26, 2019
@darthf1 darthf1 mentioned this pull request Sep 27, 2019
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@joshcanhelp joshcanhelp joshcanhelp approved these changes

Assignees

@joshcanhelp joshcanhelp

Labels
None yet
Projects
None yet
Milestone
3.2.0
Development

Successfully merging this pull request may close these issues.
4 participants
@niels-nijens @joshcanhelp @A-Marwan @darthf1
0