Honor entity and field access control in REST Services

Sounds like a rather important (security?) bug to me, not just a normal task?

REST access should follow the same rules as normal access to an entity.

I updated the issue summary.

The pluggable authentication/user problem is one that services.module in 7.x has too.

Every permission/access thing right now in Drupal is tied to users. (Be it user_access() or entity access, node grant system, ...). I don't see that changing in 8.x, so we might just have to live with it and assume that other authentication mechanisms are going to populate a user object into the context too (global currently, but at least that might change in 8.x).

However, if you want to expose the web API to untrusted consumers then the need to limit access is unavoidable. We have an entity access API that goes down to the field level, but unfortunately it is also tied to the Drupal user account system, i.e. global $user. This is fine for on-site javascript clients that use cookie-based authentication to interact with the web API, but for external clients that want to use a different authentication provider like OAuth from contrib it gets painful. Should an OAuth contrib module create fake sub-user accounts to make this work?

This is by design. Not sure why it is considered a problem here? Access control is only valid in the presence of a user. The access control system does *not* require cookie-based authentication or a session, but it does require a user.

Currently a user requires a cookie-based authentication system. There's work to change that, but that's still in progress and we may not be able to do so fully.

Currently a user requires a cookie-based authentication system.

@Crell: this has never been the case. Any module can set the global $user, which is set by, but not bound to, the session system. They have been alternative authentication schemes in Drupal since basically forever.

The problem is that the user entity is also the account object. There is no concept of abstract accounts, an account will always show up as human user entity on administration screens for example. So OAuth module asks the user if she wants to allow node read access to a third party application. As soon as that is confirmed OAuth could just use her user object for access checks when third party requests come in. But Oauth module does not want to do that because the human user account has probably a lot more permissions than the third party application should have. So it has to create a fake sub-user account that belongs to the human user account with limited permissions. Then it has to alter a lot of admin views, so that the fake account does not show up, the account has to be disabled on the login form and it has to compensate for 1000 other assumptions about user accounts in Drupal. You can imagine me now tearing my hair out and screaming if I would have to implement something like that.

Anyway, we will most likely be stuck with this for Drupal 8.

I propose an iterative approach to tackle this: first we mark all entity REST permissions as "restrict access" => TRUE, so that we reflect the current situation correctly. Then we solve entity access per operation, remove the restriction and see how far we can get until the Drupal 8 release. If we fail we have admin permissions in place and are safe, if we succeed we have a working entity access system at least.

Patch attached. Marks all REST entity permissions as administrative.

#8: rest-access-1866908-8.patch queued for re-testing.

+++ b/core/modules/rest/lib/Drupal/rest/Plugin/rest/resource/EntityResource.php
@@ -111,4 +111,18 @@ public function delete($id) {
+    // Mark all items as administrative permissions for now.
+    // @todo Remove this restriction until proper entity access control is
+    // implemented. See http://drupal.org/node/1866908

The comment is wrong. Remove the restriction until ACL is implemented? Don't you mean "once ACL is implemented?"

issue summary template

Of course. I'm blunt and go straight back to RTBC if you have no other concerns :-)

Committed to 8.x. Changing this to 'active' so we can figure out the proper solution (per DamZ). Meanwhile, this is a good improvement.

So my idea is to simply check access() on every field of the entity and unset() it if necessary before serializing it for the GET/read use case (see the patch).

While implementing I realized that field level access is a TODO in the code, so I filed #1883152: Field level access for EntityNG. Before we can continue here that must be in place.

Note that #1883152: Field level access for EntityNG is pretty ready, so I have incorporated it here. Please review it to help push it through.

This new patch adds full support for EntityNG field level access restrictions. I have implemented it for GET, PUT and POST including test cases.

TODO:
* Implement entity level access (example: node access)
* Remove the 'restrict access' => TRUE flag from the REST entity permissions

Glad to see this moving forward, but the PUT case seems incomplete:

+    foreach ($entity as $field_name => $field) {
+      if (isset($entiy->{$field_name})) {
+        if ($entiy->{$field_name} != $original_entity->{$field_name} && !$original_entity->{$field_name}->access('update')) {
+          throw new AccessDeniedHttpException(t('Access denied on updating field @field.', array('@field' => $field_name)));
+        }
+      }
+      else {
+        if (isset($original_entity->{$field_name}) && !$original_entity->{$field_name}->access('delete')) {
+          throw new AccessDeniedHttpException(t('Access denied on deleting field @field.', array('@field' => $field_name)));
+        }
+      }
+    }

Other then the typo ($entiy instead of $entity), this is missing checks on whether the user can view the field.

We remove the fields that the user is not allowed to see from the representation on GET:

+      foreach ($entity as $field_name => $field) {
+        if (!$field->access('view')) {
+          unset($entity->{$field_name});
+        }
+      }

As a consequence, those fields are not part of the representation that the user is using in PUT either. We should not touch those fields at all (ie. they need to keep their value from $original_entity).

Implemented the entity level access control. Unfortunately I had to remove the test cases for deleting nodes and users because their access controllers do not exist yet.

Also fixed the variable typo.

Not sure about changing how PUT works now, need to think about it a bit more.

Not sure in which respect? How, Why, When?

Simple reroll, not other changes.

@Grayside: Thinking about changing PUT the way Damien suggested in #17. Nut sure I want to check view access when I'm actually updating a field, that's why we have the separate update access operation key. PUT does not specify anywhere that the representation structure retrieved in a GET request has to be the same as in a subsequent PUT request.

So we need to think about use cases where the client can update/write to a field while it can never read that field. Or more precisely: we need to figure out if that is even a valid use case or not.

PUT does not specify anywhere that the representation structure retrieved in a GET request has to be the same as in a subsequent PUT request.

It doesn't. And that's not my point.

On GET we generate a representation that is tailored to the current user. It doesn't have a proper MIME type, but it could. You can think about it as:

Content-Type: application/vnd.drupal.ld+json; user=damz

On PUT, what the user submits is (again, implicitly, but it could be explicit) a representation of this resource using the same content type. Only the fields that are part of this representation should ever be taken into account when replacing the entity.

Or more precisely: we need to figure out if that is even a valid use case or not.

Perhaps the "password" field of a user entity? #23 seems like a good argument, and if what it implies is that updating such fields needs to be done via PATCH, POST, or some custom API, I think that's ok. Though if someone wants to present a counter argument to #23, please do.

Ok, implemented the proposed changes for PUT.

What about PATCH and POST? Do we allow the creation of fields that cannot be viewed? Do we allow patching in field values that cannot be viewed?

Rerolled because #1883152: Field level access for EntityNG changed.

A change in the field access hook got lost in the merge, restored.

Rerolled now that #1883152: Field level access for EntityNG got committed.

Patch rerolled, no other changes.

So we have the PUT request changes in an acceptable form here, but don't spend much time on reviewing those because we are removing PUT support from REST module anyway. See #1945794: Remove entity PUT support from REST module.

Rerolled now that PUT support has been removed, no other changes.

#33: field-access-1866908-33.patch queued for re-testing.

Committed to 8.x. Thanks!

Fallout from this: #2664780: Remove REST's resource- and verb-specific permissions for EntityResource, but provide BC and document why it's necessary for other resources.