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

Scopes and Permissions #8

Open
dshanske opened this issue Nov 26, 2019 · 10 comments
Open

Scopes and Permissions #8

dshanske opened this issue Nov 26, 2019 · 10 comments

Comments

@dshanske
Copy link

The way WordPress is designed, permissions are mapped to users and their roles. OAuth maps permissions to scopes.

Unless we want to give applications the same permissions as the users they are associated with, we need to map out a system to grant limited permissions to tokens, as well as all the administrative functionality around that.

@aerych
Copy link
Member

aerych commented Dec 9, 2019

Would it be reasonable for an initial implementation to assume the same permissions as the user and allow for more granular scoping in a later iteration?

Just thinking about the use case for the WordPress and WooCommerce mobile apps where its largely assumed the app can do whatever the user can do. We're keen to switch to the core REST API and move away from XML-RPC. I'd anticipate it would take much longer to implement the limited permissions tied to tokens as described above.

@aaronpk
Copy link

aaronpk commented Dec 9, 2019

OAuth scopes are meant to limit what an access token can do within a user's existing permissions. OAuth scopes are definitely not a good way to build out the actual internal permissions system of the API.

A shortcut implementation is to completely ignore OAuth scopes, granting every access token the full permissions of what a user can do.

@dshanske
Copy link
Author

dshanske commented Dec 9, 2019

@aerych I added this issue because I think we shouldn't take that shortcut if we can help it. It will be more painful later to change, especially with WordPress having a backward compatible philosophy. I would rather we build something that can be expanded on than start with allowing everything.

@dshanske
Copy link
Author

dshanske commented Dec 9, 2019

@aaronpk We have a capabilities system, we just map capabilities to roles, and roles to users. It is just conceptualizing how to connect the pieces

@dshanske
Copy link
Author

dshanske commented Dec 9, 2019

So, the basic proposal is this...scopes are essentially designed similar to roles, a role being a collection of capabilities. Scope would be a collection of capabilities, but adding the checks that they can't exceed the capabilities of the role of the user that is associated with the token.

@dshanske
Copy link
Author

dshanske commented Dec 9, 2019

Then ensuring that when a token is used for authentication, that the current_user_can function returns the capabilities of the token not the user.

@jkmassel
Copy link

jkmassel commented Apr 5, 2020

One thing to consider – what happens if a user's role changes?

In a situation in which a user authenticated as an editor and in the future sends a request as an administrator, would we continue to use the capabilities of the token instead of the user? That would be odd from a user's point of view, because I don't think it would be obvious that they need to reauthenticate in order to take advantage of their new capabilities.

In the reverse situation, in which a user authenticated as an editor and was later changed to a subscriber(perhaps by an administrator), it'd be important not to allow them the capabilities associated with their token.

One other consideration – currently, if a WordPress user changes the role in the UI for another user, it's expected that the changes will take effect immediately – a token overriding this breaks the mental model for user access, which seems like it'd be considered backward-incompatible.


One solution to resolve this might be to have current_user_can only return true for the common capabilities between the token and the user (probably using an early filter on user_has_cap to retrieve and calculate the common set of capabilities). This could potentially have implications for plugins (I'm thinking about pre-existing permissions management in particular, or anything scheduled by wp_cron, which would inherit a different set of permissions when run), but I don't know enough about these to be sure – just that we should be sure to consider them! :-)

@dshanske
Copy link
Author

dshanske commented Apr 5, 2020

The scope should be a filter on the current user capabilities...so if the user loses them, the token would too. I implemented a version of this where it does an array comparison between the token capabilities and the user capabilities and what is the same on both is what is allowed...the alternative is invalidating tokens on user role change.

@dshanske
Copy link
Author

dshanske commented Apr 5, 2020

map_meta_cap might work better than the user_has_cap filter.

@TimothyBJacobs
Copy link
Member

Yeah, the scopes would only be able to limit existing capabilities. Probably using user_has_cap.

Cron shouldn't be an issue since the actual cron processing would be evaluated in a different request.

I think any general background tasks that might get triggered wouldn't be reliant on the current user's permissions. But if there was a cap check on a user that happened to be the current user, that could be an issue.

It probably wouldn't be a great idea for people to evaluate their background tasks during a REST API request anyways, since performance is obviously key.

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

No branches or pull requests

5 participants