usefulteam / jwt-auth

WordPress JSON Web Token Authentication
https://wordpress.org/plugins/jwt-auth/
116 stars 49 forks source link

Do not validate authentication headers if a valid user was determined already. #75

Closed bradmkjr closed 1 year ago

bradmkjr commented 1 year ago

Allow for Basic Auth, by not attempting to validate Authentication Headers if a valid user has already been determined.

dominic-ks commented 1 year ago

Hey @bradmkjr @sun

This looks OK but is it actually needed? I tried using basic auth with the jwt-auth plugin active at the same time and it seemed to work OK for me.

Steps I took:

Unless you're trying to do something else with basic auth that isn't working?

Cheers,

sun commented 1 year ago

Hm. You're probably right.

However, the change proposed here still makes sense to me. If any other mechanism successfully authenticated a user already, then there is not really a point in trying to authenticate a user once more.

bradmkjr commented 1 year ago

Hey @bradmkjr @sun

This looks OK but is it actually needed? I tried using basic auth with the jwt-auth plugin active at the same time and it seemed to work OK for me.

Steps I took:

  • Installed latest master of jwt-auth plugin from GitHub
  • Installed REST API Basic Auth plugin from here
  • Called GET /wp-json/wp/v2/users/me from Postman with Basic Auth and get a successful response

Unless you're trying to do something else with basic auth that isn't working?

Cheers,

@dominic-ks

I looked over my code, and have slightly modified

add_filter( 'determine_current_user', 'json_basic_auth_handler', 20 );

in my use case it's set to

add_filter( 'determine_current_user', array( $this, 'json_basic_auth_handler' ), 10 );

With JWT running AFTER basic auth, it negates my validation done with basic auth. I believe this is why I opened this pull request. I agree with @sun that exiting early if authentication has already been done it won't hurt anything and improve compatibility in the future.

Thanks, Brad

sun commented 1 year ago

Still looks good to me. We need a second review/approval to move forward. 😉