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.
There was a problem hiding this comment.
Did you look into these points? Let me know if I can support.
There was a problem hiding this comment.
- Do you think the effort to create a backward code is worth it? If yes, how could be painless the backward implementation?
- No, the method for verification of refresh token is not exposed as API Rest. A common method that sorts the functionality and the different error code keys can be done. IMHO this will not reduce the the amount of changes but will increase it.
There was a problem hiding this comment.
Please consider that the plugin is used in end-user / customer facing scenarios. Changing the refresh token value means that all users will get logged out 10 minutes after deploying the updated plugin, including users who just happened to log in afresh, leaving a negative brand and user experience. On our platform this would affect roughly ten to fifteen thousand users. Such incidents often go along with a spike of questions and complaints hitting the customer service, inducing further costs. And this is just talking about one site using the plugin.
It seems we can avoid this domino effect fairly easily by just simply supporting the previous refresh token value in addition to the new one; e.g., until the second to next release. In concrete terms, this would mean: check whether the flow is cookie and whether the value matches the previous format {userId}.{hash-64chars}, and if so, validate it.
| * | ||
| * @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)
There was a problem hiding this comment.
This can became a single function that generates a valid token without type in it and let the caller to set a specific type on it.
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
|
Hello. My team and I are following this PR with great interest because this change is vital to unblock our mobile app to interact with a WP server. Are there any blockers we can help? I'm not into WP or PHP, but if there is anything I can do to help, please tell me and I will do it. I guess a lot of mobile devs are really interested in this very important PR (please leave a comment guys), we cannot use the plugin otherwise. |
Great interest too for the same reason of a mobile app. Glad to help if there is anything possible ! |
I'm also very interested in this improvement otherwise we can't go ahead with our mobile app and our Wordpress based project. Thanks in advance to all guys that will solve this issue ì. |
|
Thanks for your works. |
I'm very interested in this too |
|
A new version 3.0.3 will be highly welcome to address the issue with "obsolete token" when re-send credentials and to have a JSON based token handling instead for cookie. Is there anything we can help to ensure a fast updated version? |
|
This is still blocked on backwards compatibility; see #116 (comment) |
|
Has there been any update on this? |
|
@napcoder @EleonoraCarrano @tizianopitisci Just to clarify, implementing this in a mobile app works fine even with the current setup, where refresh_token is sent as a cookie. We've successfully done it ourselves without needing any modifications or a new release. In the end, a cookie is just a header — you can capture it from the response, store it securely on the device, and include it manually in the request headers when refreshing the token. No need for it to be in the body. |
|
As there has been no activity on this PR in some time, and I'm unclear on the way forward, I have created a newer one with fewer changes if anyone would care to review and provide feedback: |
11 participants
Supports to REST API flow (in body or parameter) instead of cookie.
The flow can be changed by hook 'jwt_auth_flow'.