Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Black list #62

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Black list #62

wants to merge 3 commits into from

Conversation

cedricDevWP
Copy link
Contributor

Add blacklist feature

Update whitelist for authorize all request

cedche99 added 3 commits December 3, 2021 16:11
@cedricDevWP cedricDevWP mentioned this pull request Mar 17, 2022
@sun
Copy link
Collaborator

sun commented Mar 17, 2022

Thanks for taking the time to create a PR!

Personally, I do not think we should make the functionality even more complex than it already is. There are dedicated plugins that go into that direction; e.g., https://wordpress.org/plugins/disable-rest-api-and-require-jwt-oauth-authentication/

As mentioned in #60 (comment) in my opinion the whole whitelist/blacklist approach is fundamentally flawed as it duplicates the regular permission layer of the REST API.

@mikmikmik
Copy link

As mentioned in #60 (comment) white/blacklist/permission_callback is necessary when you use Wordpress as a Headless CMS (no is_user_logged_in). Also even in a more general context you wouldn't be able to decide which endpoints are public or Authorized by JWT only (which is the aim of that plugin I think).

@vyarmolenko
Copy link

I support this idea.
We recently ran into an issue with a similar request. We need to allow all routes except for one. We must obtain all routes from the WP_REST_Server instance with the current implementation and then filter them. The thing is that we have a 3rd-party plugin that triggers current_user_can before the REST initialization and we have an empty list of routes/namespaces in jwt_auth_whitelist and jwt_auth_default_whitelist filters. This leads to the inability to filter REST routes and results in the target route not being blacklisted.

@niklasdahlheimer
Copy link

FYI: I also needed a blacklist feature and implemented it before I saw this pull request.
It looks like a blacklist feature is not wanted by the maintainers, but I link my fork here for anyone interested in it.

A small "case study" why I still think a blacklist is a good idea:
In my case switching to other plugin or using the default WP auth mechanism was not an option:

  1. I needed a way to authenticate my app against a Wordpress E-Learning website, so only authenticated users have access to certain assets.
  2. I used JWT Auth -> all works perfectly
  3. The website is growing, other PlugIns are needed which are heavily depending on rest routes -> I started whitelisting these endpoints
  4. The endpoints used by other plugins become too many and sometimes are not well documented. So I always have to keep in mind: "If a plugin is not working as expected, maybe JWT is blocking a certain endpoint". This makes it hard to maintain the site. So what a needed was JWT auth to protect ONLY my specific endpoints and ignore ALL others.

The approach is simple: If you specify a blacklist by the filter, only these endpoints are checked. If not, default JWT auth behaviour is applied.

https://github.com/niklasdahlheimer/jwt-auth/tree/feature/blacklist

@dominic-ks
Copy link
Collaborator

Hey @niklasdahlheimer, not sure if you've had a look at this discussion - #60 - but there were some good points about why this plugin shouldn't have a whitelist / blacklist feature, the main point being that REST routes should determine who can access the route via the permissions callback leaving this plugin just to authenticate users. i.e. if there's a route that should be protected for logged-in users only, the permissions callback of the route should take care of that and users can potentially authenticate via any method.

@niklasdahlheimer
Copy link

niklasdahlheimer commented Nov 27, 2022

Hey @dominic-ks , I get the point, that route permissions should be handled by their permission callbacks and would be happy to do it this way BUT, if I see this correctly, the current version of JWT intercepts/blocks all calls on any routes if they are not whitelisted. So, right now I have this options:

  1. list all routes of all my plugins in the JWT whitelist
  2. stop using JWT auth

which both is not what I want :).

So, yes, if you

  1. remove the whitelist completely
  2. remove the interception of all endpoints
  3. only handle authenticating users by JWT

in JWT Auth, I could use the permission callbacks for my custom routes to only allow authenticated users and let JWT handle the authentication.
But right now the only solution to use JWT for my custom routes AND avoid listing all plugin routes in the JWT whitelist is to use my blacklist-mod

Or do I miss something?

@dominic-ks
Copy link
Collaborator

  1. remove the whitelist completely

You are right, and that PR is proposing to remove the whitelist all together. I think we're all pretty much agreed on that now, there are just a couple of things to do to make sure existing users are aware of the upcoming changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants