Closed sun closed 2 years ago
Added the following final changes:
Therefore, the endpoints changed as follows:
/token
generates a new access token. Either by passing username/password combo (as before) or a valid refresh token in a HttpOnly cookie./token/validate
validates whether a token is still valid. Token can be an access token or refresh token./token/refresh
generates a new refresh token as HttpOnly cookie. Requires a valid refresh token sent as cookie.Hey @sun , would you mind if I add you as one of the maintainer here? So that you guys don't have to wait for me for updates in GitHub.
The WordPress plugin repository account for this plugin is still on me. I'd be happy to push an update there.
Thanks a lot for this awesome PR.
@sun this is awesome, @contactjavas can you please review and test and then merge, this is a super important feature, that I think everyone trying to build a headless WP implementation would appreciate.
Thanks
Thanks for your feedback! Meanwhile I studied more tutorials and client libraries and server implementations from various services. I think the security needs hardening. I added a list of todos to the PR summary:
You can try and help test this branch on your non-public development sites already. These additional measures are only going to invalidate refresh tokens sooner or more often.
Note that I will be in holidays for the next two weeks. It is unlikely that I'll be able to implement these points before I leave. But I will come back to this directly afterwards – one of our customers needs this to move forward.
Wow, thanks for your awesome effort @sun ! I'll try to learn and test it. This is awesome man!
Made some major progress. This PR now also introduces REST API tests using the PHPUnit framework. The tests confirm that all requests and responses are meeting the expectations. Especially important for the refresh token rotation, where we expect a past refresh token to no longer be valid.
Run the tests with:
$ composer install
$ composer test
> ./vendor/bin/phpunit
PHPUnit 9.5.13 by Sebastian Bergmann and contributors.
....F.......F 13 / 13 (100%)
Time: 00:20.465, Memory: 6.00 MB
There were 2 failures:
1) UsefulTeam\Tests\JwtAuth\AccessTokenTest::testTokenWithInvalidRefreshToken
Failed asserting that 200 matches expected 401.
/wp-content/plugins/jwt-auth/tests/src/AccessTokenTest.php:117
2) UsefulTeam\Tests\JwtAuth\RefreshTokenTest::testTokenWithRotatedRefreshToken
Failed asserting that 401 matches expected 200.
/wp-content/plugins/jwt-auth/tests/src/RefreshTokenTest.php:200
FAILURES!
Tests: 13, Assertions: 71, Failures: 2.
Script ./vendor/bin/phpunit handling the test event returned with error code 1
This is the actual current test result output, so there are indeed two failures that need closer inspection.
We will be able to automatically run the tests for every PR using GitHub Actions in the future. However, I'll probably defer that to a separate PR.
The release packaging script needs to be updated to use the --no-dev
flag for composer install
.
@contactjavas How do you build the releases currently? I did not see a script in the repo, or did miss it?
Hi @sun, this is awesome ... ❤️
I'd like to try this PR on my end. Yes, currently there's neither build script nor GitHub hook provided in this repo. Composer package is there, but hasn't been taken any care. I haven't even tried if the composer package even works/not. About WordPress build, it's still manual.
If you need to, or if you would like to do something about the composer or WP SVN, I've added you:
I'm sorry, I wasn't active here for long time. I'll be back to this repo sometime after Kirki 4 is released. Thanks a lot for your great work!
Ah, great, thanks! 👍
The composer.json works fine. The composer.lock file should also be committed though; otherwise every new invocation of composer install
will install a different version of the JWT library. We can address that in a separate PR.
I'm using https://github.com/sun/wordpress-git-svn-release to build and publish releases on WordPress.org. Here is an actual example config file: https://github.com/netzstrategen/wordpress-gallerya/blob/master/.wp-release.conf – There are also GitHub actions for releasing, but I haven't used them yet. Maybe we can also look into this in a separate PR.
Resolved another todo from the list: Refresh tokens are no longer JWTs now. Various tutorials are also recommending this.
This makes all tests pass now:
$ composer test
> ./vendor/bin/phpunit
PHPUnit 9.5.13 by Sebastian Bergmann and contributors.
............ 12 / 12 (100%)
Time: 00:18.611, Memory: 6.00 MB
OK (12 tests, 92 assertions)
I will try to resolve the remaining todos mentioned in the PR summary in the next days.
After a long period of testing with iOS, Android, and web app clients, we are currently preparing the launch of this in our project (finally). Therefore, I am going to resolve the remaining todos and would then look into merging this PR, ideally already tomorrow.
I added some commits already. The refresh token rotation by device was not correctly working when a client sends the POST parameters as JSON data instead of form data. This is now fixed for the refresh tokens and I added a test to ensure this remains to work in the future. (Yay to more tests! 🎉 )
I created a separate issue for the Devices class, which still accesses the $_POST
array directly instead of using the REST Request object parameters: #58
Created a separate follow-up issue for #59
All remaining todos have been resolved. Everything is still the same, just cleaned up now.
The major version is bumped to 3.0.0. The changelog explains the new and possibly breaking changes. All tests still pass.
Unless someone raises concerns, I will proceed with merging this tomorrow.
Resolves #1
Problem
Goal
Proposed solution
/token
generates a new access token./token/validate
validates whether a token is still valid. Token can be an access token or refresh token.~/token/refresh
generates a new refresh token and sends it in a HttpOnly cookie.Resources
To clarify
refreshToken
in the/token
response require a major version bump?If yes, only the plugin version or also the REST API version?Missing data point: How many actual implementations of this plugin in production environments can there realistically be in the wild without a refresh token concept? And assuming all of those are very custom anyway, is a plugin major version bump (without REST API version bump) sufficient?#19 attempted/suggested a REST version bump, but the changes in there are by far incomplete. A REST version bump would have to ensure that v1 remains to work as in the past, in parallel to a new v2.Remaining todos
From https://github.com/usefulteam/jwt-auth/issues/1#issuecomment-867050042: The general idea is that the client should not store the username/password credentials themselves. It should only store the refresh token. The common flow looks like this:
Authenticate with actual user login credentials to retrieve refresh token
Response:
Authenticate with refresh token to create new access token
Response:
Refresh the refresh token
Response:
Note: The
/token/refresh
response only supplies a new refresh token. It does not generate a new access token.