Tmeister / wp-api-jwt-auth

A simple plugin to add JSON Web Token (JWT) Authentication to WP REST API
GNU General Public License v2.0
546 stars 160 forks source link

Fix the condition where it checks if the request is a REST Request #256

Closed marioshtika closed 10 months ago

marioshtika commented 1 year ago

Related to issue #257

I used the below variables to make it easy to read

$isRestRequestConstantDefined = defined( 'REST_REQUEST' ) && REST_REQUEST;
$isRestRequest = $isRestRequestConstantDefined || strpos( $requested_url, $rest_api_slug );
if ( $isRestRequest && $user ) {
    return $user;
}

If you want it a single line you can use

if ( ( ( defined( 'REST_REQUEST' ) && REST_REQUEST ) || strpos( $requested_url, $rest_api_slug ) ) && $user ) {
    return $user;
}
marioshtika commented 1 year ago

Hello @Tmeister,

any update on this pull request?

marioshtika commented 1 year ago

@Tmeister any update on this, this is also happening on the default posts API (more and more often), on different websites.

mam4dali commented 1 year ago

thank you This fixes the problems of the last three versions @Tmeister Please merge if possible

ckubitza commented 1 year ago

I also think the current condition is wrong (as of v1.3.2), because we noticed that since v1.3.1 authorized users did not receive non-public data through API anymore. But your fix changes the condition completely, I think a smaller change would be sufficient. The original condition checks for the constant REST_REQUEST and at least in our tests, this constant is defined after determine_current_user() is called. So the condition is always true.

So I propose to change the condition from

if ( ! defined( 'REST_REQUEST' ) || ! REST_REQUEST || strpos( $requested_url, $rest_api_slug ) === false || $user ) {

to

if ( (defined( 'REST_REQUEST' ) && ! REST_REQUEST) || strpos( $requested_url, $rest_api_slug ) === false || $user ) {

Or maybe the check for this constant can be dropped completely.

marioshtika commented 1 year ago

Hello @ckubitza. Did you test my pull request, is that working for you? The code you are suggesting, is that in comparison with the core plugin code or my pull request code? Thank you.

ckubitza commented 1 year ago

Hey @marioshtika, yes, I tried it, and it is indeed working. But as I don't know all edge cases the original condition is pointing at, I have tried to change it only as little as possible to make it work. And my suggestion is compared to the original code of the plugin.

align-systems commented 1 year ago

Thanks for the fix @marioshtika and @ckubitza. I got errors when upgrading to 1.3.2 a little while back. 404 errors updating posts, so I rolled back due to time constraints but upgrading a site today another plugin (Pods) had a clash with the Firebase\JWT namespace so ran the update again which solved that problem but this problem still existed and after searching found this PR so I've patched for now but @Tmeister it would be really good if you could merge a fix for this. Cheers, Vic

marioshtika commented 1 year ago

@Tmeister any update on this PR? Thank you in advance.

sashagra commented 1 year ago

@marioshtika thank you. it solve my problem.

@Tmeister can u merge it please

marioshtika commented 1 year ago

@Tmeister any update on this pull request. Is it ok to merge it? Thank you in advance.

GForceWeb commented 1 year ago

@Tmeister Would be great if you could merge this. This fix has also worked for me

sashagra commented 1 year ago

it is not working anymore what can i do?

GForceWeb commented 10 months ago

@Tmeister Pinging again on this. Please respond