Feature/refresh token flow #116
Conversation
…_token in token generation response
… for different token type
feat(refresh token): add hook to retrieve the refresh token from a custom filter
…efinition removing dependency on request variable
…shing refresh token
…stom logger logic to be used
|
Hello @marchrius, Very interested by your PR, i'm facing the same issue Is there anything missing from this MR to send it in for review? Do you need help? |
|
Hi @rtorrente, the missing part is the README.md update with docs on new error codes and how to use the flow hook. |
…ed readme.txt as well
There was a problem hiding this comment.
Thanks a lot for contributing this enhancement! ❤️
From what I can see, the changes seem to be backwards-compatible with the current code in 3.x/master. Therefore, I believe we do not need to block the 3.0 release on this change.
However, before going into further details – there are a lot of coding style changes in this PR, which make it really hard to review the functional changes. So I'd like to ask you whether you can move all of the coding style changes into a separate PR (which we could merge fairly quickly), so that we can focus solely on the functional changes in this PR here?
This would be important, because we are touching security related functionality with this PR, and it would be embarrassing to introduce a security vulnerability just because a tiny but substantial change was hidden in a lot of code formatting changes.
|
@marchrius are you able to have a look at @sun 's requested changes? It would be good to get this in as a few people are asking. |
|
Hi @sun and @dominic-ks, I'll be happy to do that, can you give me some pointers on what might be the best way to split the two things? I'm using PHPStorm as IDE and I have the option If you have some default rules I can use them for future commits and style changes. |
|
For the moment it would be best to disable all automated formattings and then revert all changes in this PR that are not functional changes. In an issue separate from this here we can look into correcting the formatting – although I think we’ll rather look at automations like prettier and phpcs to do so, so that it doesn’t depend on anyone’s editor settings. |
fix(refresh_token): revert logger integration (will be present in a different PR)
|
Hi @sun, I've removed all non-necessary integrations and style modifications. |
There was a problem hiding this comment.
Thanks a lot – this at least allowed me to do a first round of review of the actual code changes. 👍
We should try to make this change in a backwards compatible way, so that existing production sites can update without headaches. We should also minimize the additional code introduced here by reusing the existing code. I added some insights on how we can achieve that, which should not require much effort.
And there are still a bunch of unnecessary code style changes in the surrounding files - if you can revert those, we can see and review the full picture with more certainty.
| public function validate_refresh_token( $refresh_token_cookie, $device ) { | ||
| $parts = explode( '.', $refresh_token_cookie ); | ||
| if ( count( $parts ) !== 2 || empty( intval( $parts[0] ) ) || empty( $parts[1] ) ) { | ||
| public function validate_refresh_token( $refresh_token, $return_response = true ) { |
There was a problem hiding this comment.
- It seems you have changed the content of the refresh token - this means that all existing users will be logged-out when deploying the update. We can avoid that by implementing a backwards compatibility for the previous refresh tokens, so they are migrated individually to the new value when they are silently refreshed or the user re-authenticates. We can then safely remove this B/C layer in an upcoming release.
- Is this a copy of
validate_token()now? We can avoid duplicate code by passing a parameter$typetovalidate_token()that contains either"refresh"or"access"(by default). Mainly seems to be necessary for hook names and error codes. However, let's keep the separate method for the temporary B/C layer and the backend/device validation.
As a result of these two suggestions, the amount of changes in this PR should reduce further.
| * | ||
| * @return string | ||
| */ | ||
| public function generate_refresh_token( \WP_User $user, string $device = '' ): string { |
There was a problem hiding this comment.
Ditto about generate_token() here (DRY)
marchrius
force-pushed
the
feature/refresh_token_flow
branch
4 times, most recently
from
169f867
to
6934c57
Compare
marchrius
force-pushed
the
feature/refresh_token_flow
branch
from
6934c57
to
25363ee
Compare
Supports to REST API flow (in body or parameter) instead of cookie.
The flow can be changed by hook 'jwt_auth_flow'.