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

Introduce PHP 8.0 support and update to latest Auth0 PHP SDK version #108

Merged
merged 43 commits into from
Mar 12, 2021

Conversation

olix21
Copy link

@olix21 olix21 commented Jan 18, 2021

This pull request includes changes necessary to support PHP 8.0, 7.6 of the Auth0 PHP SDK, and modern versions of Symfony, and makes some quality of life improvements along the way.

Functional changes:

  • Auth0Service::decodeJWT updated; supports new Auth0 PHP SDK interface.
  • JwtGuardAuthenticator and JWTAuthenticator interfaces now behave identically and predictably; JwtGuardAuthenticator will now correctly flag a user as IS_AUTHENTICATED_ANONYMOUSLY. Choose the flavor appropriate for your needs with Symfony.
  • Cache support updated; now supports any cache that uses the PSR-6 or PSR-16 standards. Can now rely on Symfony's own caching components. This cache is handed off to the Auth0 PHP SDK for use in JWK fetching. A Auth0Psr16Adapter helper bridge was added to support either caching standard, as Symfony uses PSR-6 but the Auth0 PHP SDK requires PSR-16 presently.
  • Added opt-in JWT validation checks around nonce, azp, and aud claims, and support for max_age and leeway checks; this is handled by the new JwtValidations helper. See the README for configuration.
  • Simplifies the configuration scheme, as outline in the updated README.
  • Simplifies our Symfony services definition.
  • Enforces strict typing and introduces type hinting where possible.

Testing changes:

  • Upgrades to PHPUnit 9, which supports PHP 8.0.
  • Updates our unit tests to support syntax changes for PHPUnit 9.
  • Adds unit tests for new Auth0Psr16Adapter and JwtValidations helper classes.
  • Adds phpcs; helps enforce coding standards and identifying breaking changes against supported PHP releases.
  • Adds phpstan static code analysis; aids in catching code weaknesses.
  • Updates our CircleCI configuration to test against PHP 8.0, fixes CodeCov, and adds Snyk.

Documentation changes:

  • Ensures all parameters, functions, classes, and methods are documented in-code.
  • Updates README with new configuration options.
  • Updates LICENSE year and link.

@olix21
Copy link
Author

olix21 commented Jan 18, 2021

@evansims This one could close #103 && #107

Or could be reused to make you gain some time.

I'm still not sure how to replace the JWTVerifier.
I did replace the service defined in jwt_auth.cache.file_system by the Auth0\SDK\Helpers\Cache\NoCacheHandler as it seems there is no more cache provided by the bundle.

I don't know if you plan to totally remove the service or replace it

@olix21 olix21 force-pushed the php8 branch 2 times, most recently from a4b8c8b to 0df0770 Compare January 18, 2021 17:49
@mkilmanas
Copy link
Contributor

While running vimeo/psalm I've discovered this issue: https://psalm.dev/docs/running_psalm/issues/ParamNameMismatch/ on JwtGuardAuthenticator::start(). The Symfony interface that this is implementing calls the second parameter $authException and in this bundle it's renamed to $authenticationException.

It seems like a very minor thing and the code works as-is right now. But the Psalm issue description gives a very good point - in PHP8 one can use named-parameters when calling the method, and one would expect the parameter name to be the same as in the interface definition (since you would want to depend on the interface and not a specific implementation).

I suppose it could be solved under this PR as it seems most relevant to this

@evansims
Copy link
Member

evansims commented Feb 2, 2021

Hey @olix21 👋 Thanks so much for your contribution on this, I really appreciate your efforts! I'm on holiday right now but I'll dig into this as soon as I return! 🧐

@mkilmanas Definitely a good catch, this seems like a fine opportunity to slip that fix in here too. I can do that when I'm back from holiday as well. Thanks!

@evansims evansims changed the title Php 8 support PHP 8.0 support Feb 18, 2021
@evansims evansims closed this Feb 18, 2021
@evansims evansims reopened this Feb 18, 2021
@evansims evansims marked this pull request as draft February 18, 2021 23:37
@evansims
Copy link
Member

(Seems some of our CI test hooks on this repo have broken, may need to close/repo this PR a few times as things are fixed to resolve the checks, apologies in advance there.)

Good progress and thanks for your contributions so far. Starting to look into what can be done to address the remaining issues here

@evansims
Copy link
Member

Alright, status checks fixed, sorry for the commit spam. Codecov might report some strange results that will require us to force merge this PR but it's all good at any rate.

@olix21
Copy link
Author

olix21 commented Mar 9, 2021

I'll test it today or tomorrow on my project to see if everything is okay 👍

@evansims
Copy link
Member

evansims commented Mar 9, 2021

Thanks @olix21!

I've added new configuration options for optional JWT validators to help with token checks:

jwt_auth:
  # Token validations to run during JWT decoding:
  validations:
    # Validate AUD claim against a value, such as an API identifier. Set to false to skip. Defaults to jwt_auth.audience.
    aud: "%env(AUTH0_API_AUDIENCE)%"
    # Validate the AZP claim against a value, such as a client ID. Set to false to skip. Defaults to false.
    azp: "%env(AUTH0_CLIENT_ID)%"
    # Maximum age (in seconds) since the auth_time of the token. Set to false to skip. Defaults to false.
    max_age: 3600
    # Clock tolerance (in seconds) for token expiration checks. Requires an integer value. Defaults to 60 seconds.
    leeway: 60

These are applied during Auth0Service->decodeJWT, which is now using a new JwtValidations Helper I've added.

@evansims
Copy link
Member

Hey @olix21 just wanted to see if you'd had a chance to try these changes out yet. No worries if not, but I wanted to move ahead with getting this approved through our internal review process if you felt alright with things.

@olix21
Copy link
Author

olix21 commented Mar 12, 2021

Hello @evansims I just managed to fix some issues with vendor dependencies compatibility.
Now I see that the Authenticator no longer instantiates with an api_identifier but with a client_id and a client_secret.
As I don't manage our Auth0 app myself, I'm waiting for the credentials to test it in our app.

I hope it will be done today

EDIT: I did receive the information I needed, I tested your latest implementation and it's working well on my app!

Thank you for your work!

@evansims
Copy link
Member

@olix21 Thrilled to hear that, thanks for checking on it for me! And thank YOU for your efforts here, we really appreciate it!

Just a heads up, I'm going to update your original post here to outline the changes made, just to ease future reference and aid in our review process.

@evansims evansims changed the title PHP 8.0 support Introduce PHP 8.0 support and update to latest Auth0 PHP SDK version Mar 12, 2021
@evansims evansims self-assigned this Mar 12, 2021
@evansims
Copy link
Member

Pinging @jimmyjames 👷 As discussed in our call, now that I have confirmed this works for the community, this is ready for you to take a look at when you have the time! I updated the original post to break down the changes. Please feel free to ping me on Slack with questions!

Copy link
Contributor

@jimmyjames jimmyjames left a comment

Choose a reason for hiding this comment

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

👍 great to see this collaboration here!

@evansims
Copy link
Member

evansims commented Mar 12, 2021

Thanks for taking a look at this, @jimmyjames!

@olix21, again, thanks so much for your help on this! Going to merge this in for the next release.

Next, we'll get an internal security review going on these changes so we can officially cut the release, hopefully within the next week-ish. I'll update you with the progress.

@evansims evansims merged commit 00769c7 into auth0:master Mar 12, 2021
@darthf1
Copy link
Contributor

darthf1 commented Mar 13, 2021

@evansims I would like to test these changes but when I request dev-master in my composer.json I'm getting an outdated branch:

{
  "require": {
    "auth0/jwt-auth-bundle": "dev-master",
  }
}
  - Locking auth0/auth0-php (5.7.0)
  - Locking auth0/jwt-auth-bundle (dev-master cff682d)

Which is this commit: cff682d. Specifying the latest comit (#00769c76fe264071746e86be6a422d4ac076e520) seems to give me an outdated folder as well.

Just to make sure; do we need to do anything on our end to test the latest branch or is this just an issue on my side?


Edit:
Not sure why but if I add a repository its fetching the latest version.

{
    "repositories": [
        {
            "type": "vcs",
            "url": "https://github.com/auth0/jwt-auth-bundle"
        }
    ],
}
  - Upgrading auth0/auth0-php (5.7.0 => 7.6.2)
  - Upgrading auth0/jwt-auth-bundle (3.4.0 => dev-master 00769c7)

@evansims
Copy link
Member

Sorry about that @darthf1, it looks like our webhook for auto-updating Packagist wasn't working right, so it was using stale data. I've kicked it manually for now, so requiring dev-master should reflect the appropriate state of things now.

@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.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants