Security: Different user permissions per Service method

This could get a bit messy with a bunch of methods, but I suppose the same can be said for having lots of modules. Maybe someday the user access admin page will provide or even use the same grouping that modules now use on the modules admin page. That would be nice. I think per method permissions are necessary though. I'm with you.

Scott

I'd appreciate some further discussion about how Services implements security:

Currently, Services authenticates an application with an API key, and allows any authenticated application to call any service method.

For the most part, the service methods are wrappers directly to Drupal API functions (node.save calls a function that calls node_save without using node_access; node.delete calls node_delete directly!)

So, Services security depends on (1) the API key being accessible only by valid applications, and (2) the valid applications implementing their own security for each service method.

One solution is Rob's idea: to provide permissions for each service method, so the administrator could decide which roles have access to node.save, etc.

Another solution is to add the proper permissions code to each services method, so that the services methods depend on the same permissions for a module's functions as exist on the Drupal site.

Are there good reasons for service methods to have different permissions from the methods that are invoked?

One hallmark of Drupal is how people use it in ways previously not thought of. Proper Drupal permission controls for each service – while messy – would seem to support that same concept of flexibility. Leaving it up to the valid application to protect the API key is not always reasonable. Speaking from a Flash/Flex perspective, those can often be reverse engineered to glean data such as the API key out.

I am also concerned about the user access page getting unwieldy, however I am still in favor of this. I think its a good addition, even if it isn't perfect. One thing that would be nice is if "access services" was first, and then all the service-specific permissions followed. Not sure if you can force this though, it appears to force them into alpha order.

I did some testing with this today and found two problems

- a missing space when checking permissions caused all checks to fail ('access'. $method_name as opposed to 'access '. $method_name)
- I noticed the check was being done before the sess_id was passed to the session handler, which would in theory cause authenticated sessions to always fail. Unfortunately it appears that there is another problem with session handling preventing authenticated users from working. I'm filing another bug on that.

With this patch anonymous works when checked, and fails when unchecked, which is a start.

Detour,
+1 for your second solutions which seems more "integrated". Like you said, i think it is a better solution to rely on drupal's user permissions rather than provide permissions for each method. After all the Services Module is a somekind of translater/presentation layer enabler and (i think) shouldn't get too much involved in deciding on security.

PS:i'm quite a newb, bear with me if my statements are inaccurate...

I'm not a php developer (aaaarrgh) but i'm able to read php code and i'd like to make a suggestion :
For example for node service :
Why not simply call node_access (http://api.drupal.org/api/function/node_access/6) before calling node_load, in node_service.module line 68 ? and make it look like this

/**
 * Returns a specified node.
 */
function node_service_load($nid, $fields = array()) {
  
  $node = node_load(array('nid' => $nid));

  if (!$node) {
    return services_error(t("Could not find the node."));
  }

  // check if user can view node
  if (!node_access('view', $node)) {
    return services_error(t("You do not have permission."));
  }

  $node = services_node_load(node_load(array('nid' => $nid)), $fields);
  
  return $node;
}

Of course we can implement this for all operations (view/update/delete/create)

My code might be messy but i hope my point is clear. May be you can find some inspiration from the blogapi module http://api.drupal.org/api/function/blogapi_blogger_edit_post/5

Attched to this comment is node_service.module with some modificatoins :
- node_service_load as i sent it on my previous comment,
- an additional (helper) service method node_service_get_permissions that lists the view/update/delete/create permissions for a node for the current user...

PS: I hope i am not braking any rules by posting this code (in addition i don't no how to write patches)

The idea in #12 would allow each of the core services to then use the same accesss checks as a regular user on drupal.

+1

Access control in services is definitely an issue that needs some creative thought. A given site may wish to offer multiple web services, some of which are wide open and others of which have highly restricted access.

The current access restriction system seems to be "all or nothing". Restrictions can be introduced through IP filtering and key-based authentication, but, once in, all clients have full access to all services.

Perhaps our way forward is to start with some principles. Here's a set of suggestions:

• Web services should be limited by user-level access restrictions. A web service user should never have access that s/he would not have as a regular site user.
• Where there is an exact match with an existing Drupal access permission, specific services should be restricted by that permission.
• Where services introduce extra exposure, they need to define and implement appropriate access control mechanisms (perhaps permissions), possibly in combination with existing mechanisms.
• Ideally, access restrictions should be implemented outside of (prior to) the service methods themselves. That is, a user without access to a service should not be able to call the service method.
• Access restrictions may be conditional on data associated with the method call. For example, evaluating access to a node save operation might require a test referring to the particular node being saved.

This is starting to sound a lot like menu access....

Let's take two of the methods in node_service.module as examples.

node.save
here we seem to have a close match with an existing access control mechanism: node_access. Yet it's not an exact match, since node access needs an argument distinguishing between 'create' and 'update' operations. We would need to pass the argument through something like this:

function node_service_save_access($node) {
  if (isset($node['nid'])) {
    return node_access('update', $node);
  }
  return node_access('create', $node['type']);
}

node.load
We could easily mistake this for a match with node_access's 'view' op, but clearly it's not. Unless a set of fields is specified (and with the exception of running body through filtering), services_node_load() returns the full raw data returned by node_load(). Viewing a rendered node on a typical themed Drupal site, users with view access to a node would never get that much data. So our existing access control mechanism is of some use, but won't always be enough.

So we might need a new permission here, e.g. 'load raw node data'. But we'll still want the existing node view permissions to count. So we would want something like the following:

function node_service_load_access($node) {
  return node_access('view', $node) && user_access('load raw node data');
}

So far we have (if I've counted right) three suggested approaches to implementing access control in services:

1. Introducing a permission for each service.
2. Testing for access in the service callbacks.
3. Introducing and implementing new metadata in the service definition, e.g., an #access property

How well does each meet the criteria I suggested above?

Introducing a permission for each service doesn't help much. It ignores our current access restrictions.

Testing for access in service callbacks would be an improvement, but is after the fact.

New metadata properties seems the most promising approach. Rather than just an access property, we would need an access callback. And the ability to pass arguments to that callback.

    '#access callback' => 'node_service_save_access',
    '#access arguments' => array([node]),

So, indeed, this is sounding a lot like the 6.x menu system.

Maybe it is in fact the menu system? Should we be introducing a menu item for each service and relying on that for access control? Tempting. One issue would be passing e.g. the node object. In the menu system, the relevant data are read from an ID in the path, e.g., node/%node. Another would be the need to determine also the server we're hitting--it's in fact the servers that we hit, rather than services directly.

So, perhaps, our best approach is to model on the D6 menu system, introducing two new metadata parameters for services, '#access callback' and '#access arguments'.

Or, I'm thinking now, maybe, we don't need the latter. We have an '#access callback' and simply feed it the arguments that come with the request. In the case of node.save, this is the node being saved.

Adding meta data makes sense to the service defintition makes sense.

This is similar to the idea me and Rob discussed in Boston but we never considered using a full callback system for access we were simply looking using pure user permissions. Having full callback functionality would certainly allow for improved flexibility.

The question then also starts to emerge whether we should have some sort of service registry which holds descriptive information about the call.

With this however access system it generally relies on a user object (or at least the functions utilised do) and hence a sign in must occur. On certain sites there will be a requirement to not require testing therefore the system should allow for a simple way to allow for a the return of access = true. I realise that it doesn't affect the fundmental structure proposed and could be as simple as an admin option of allow access to all users but it something we need to consider while designing the solution.

The arguments may not be required but a design which allows for additional parameters code result in less head aches in the future.

I definitely thing nedjo's got it right. Rob and Marc, will either of you be available some time soon, maybe even tomorrow to have a go at this, the three of us? We need to get this in ASAP as security team has done a review and expressed their concern. I think I'm gonna try to get a patch together in the morning. I'll hop on IRC.

Scott

Here's an early and rough patch.

What's been done:

* 2 more metadata params for methods '#access callback' and '#access arguments'
* '#access callback' and '#access arguments' defaults to 'user_access' and 'access services'
* '#access arguments' can be set without '#access callback' to provide 'user_access' arguments
* if '#access callback' is set without '#access arguments', the request arguments are passed to the access callback
* only node_service has been given custom callbacks for load, save and delete

Some things I noticed:
* As nedjo mentioned, in node.load for example, we don't get a full node object as an argument, just nid, so we have to do a node_load before passing it to node_access in the custom access callback function, resulting in 2 node_loads. You can still do access checking within the service method itself though, maybe that's the solution to this problem.

Re-rolled to support all core services. Still rough and needs input, but its a complete 1st draft.

I'll have a look at this tomorrow and see where improvements can be made.

Have reviewed the patch and it looks good. I agree with rob that the access services should just return true (or indeed not be tested at all).

Attatched is a drupal 6 patch rolled against head

Noob question here, but I'm trying to help out by testing it out. I've never installed a patch before, so I may have done something wrong but:

.../sites/all/modules/services$ patch < method_permissions_1.patch
::answered the questions::

And nothing appears to have changed. There are no additional service entries on the "access control" page.

Am I missing something?

Actually, it looks like it did patch, because now my xmlrpc() test script returns "Access denied" when I attempt a node.load. So it looks like it was decided to lock down all methods so that they can only be accessed by people who have logged in? How does one do this? I tried doing a user.login (which was successful) but then node.load still doesn't work. Here's my code:

$ret= xmlrpc("http://www.example.com:8080/drupal5sandbox/xmlrpc.php", 'user.login', 'admin', 'XXXXXX');
if (xmlrpc_error()) {
  $error_num = xmlrpc_errno();
  $error = xmlrpc_error();
  print_r ($error);
}

$ret= xmlrpc("http://www.example.com:8080/drupal5sandbox/xmlrpc.php", 'node.load', 1, '');
if (xmlrpc_error()) {
  $error_num = xmlrpc_errno();
  $error = xmlrpc_error();
  print_r ($error);
}
print_r($ret);

I know I have to somehow "continue the session" after the first call and it looks like the 2nd xmlrpc() call doesn't automagically do this...

This patch users standard drupal permissions ie if an anonymous user can create a node then after this patch has been applied they will be able to save a node. If an anonymous user can not do that then they will get an accessed denied message.

At the moment you need to ensure that an anonymous user can access webservices otherwise you won't be able to access any services anyway. Removing this check has been proposed by myself and Rob - as the new checks make this overall check redundant or so it appears.

Hi Marc,

I have it enabled for anonymous access. It works because I'm able to "login". I just don't know how to use that authenticated login for subsequent xmlrpc() commands. Do you know how to do this?

That is one of snelson

Hey Scott (or anyone),

Can you give me some advice as to how to get this working? 'user.login' works fine, but 'node.load' gives "permission denied". I'm not sure how to continue the session from one xmlrpc() request in order to gain access to the node data.

Ah, thanks. That did it!

I'm happy to mark it as RTBC

Committed to drupal 5 and drupal 6

That sounds good.

Not sure how this works, if alpha / beta comes before 0.* releases or visa versa, but definitely don't want to do a 1.0 release yet. It needs to be super solid, tried and true before I'm comfortable doing a 1.0 release. I'm thinking 0.* comes first, so yes go ahead and make a 0.8 release.

Thanks,
Scott