• User A created a node with reference to node IDs 1, 2, 3
  • User B edits node, but doesn't have access to nid 3
  • Upon save reference to nid 3 is lost

With patch, we check if there are items user doesn't have access to, and re-add them on validate handler.

If this seems ok, I can adapt the OG tests and move the responsibility of it to ER ;)

CommentFileSizeAuthor
#2 add-hidden-ids-1.patch3.55 KBamitaibu
add-hidden-ids.patch3.55 KBamitaibu
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, add-hidden-ids.patch, failed testing.

amitaibu’s picture

Status: Needs work » Needs review
FileSize
3.55 KB

Correct patch.

Damien Tournoud’s picture

Title: Re-add hidden IDs » The widget should properly handle references to entities the user do not have access to

That's indeed a big oversight, but I don't think the solution here is going to be enough for every widget.

- Will this solution work for all the widgets we provide?
- Do we ever show to the end-users IDs or label of entities it should not see?
- Could we lost ordering in some cases?

Let's start by sketching out some tests about this.

Status: Needs review » Needs work

The last submitted patch, add-hidden-ids-1.patch, failed testing.

amitaibu’s picture

Lets try to figure out how to solve this:

> - Will this solution work for all the widgets we provide?

Maybe we can try hook into field_presave() and get the inaccessible entities from the $entity->original->{$field_name}, like this we don't need to deal with the widgets directly. However, the problem is that we might hit a field cardinality issue - so upon form validation we need to make sure not to exceed the field allowed values.

> - Do we ever show to the end-users IDs or label of entities it should not see?

We shouldn't, and I think we don't by adding the {entity_type}_access tag to the query.

> - Could we lost ordering in some cases?

I don't think we can maintain the order, as it makes better UX to hide those label from the widget, rather than renaming them to "private" or somethink like this.

amitaibu’s picture

I've pushed a first stab, untested, far from working, WIP branch 1352690.

Encarte’s picture

andypost’s picture

I think field should validate values and this should not depend on widget!

The logic here is slightly depends on node_access used because in some cases you can't access a title of referenced entity so there's no way to disaplay referenced entity in any widget. In this case field should store this disabled IDs in form_state and reuse them when saving data

JonAnthony’s picture

Yikes - I just ran into this and it was a bit scary. In essence users with less permissions were undoing the the references created in nodes by admins.
Now I see it I have written my own handlers for my specific use case, but it falls apart in more general scenarios.
The way you are discussing doing it here is more elegant.
Did you ever decide on the best route - i.e. not handle this at widget level but add the values in.
How would you handle the scenario that is was a single value entry, and the lesser permissioned user changes it to another value without being aware that the higher permissioned user had already set another value.
J

In case it helps anyone - as work around (in a separate custom module) I am not allowing users to edit nodes when they contain references with values they do not have permission to

function svalidation_field_widget_form_alter(&$element, &$form_state, $context)
{
  $field = $context['field'];
  $instance = $context['instance'];
  $items = $context['items'];
  if (empty($items))
  {
    return;
  }
  if ($field['type'] != 'entityreference')
  {
    return;
  }
  $target_type = $field['settings']['target_type'];
  foreach ($items as $item)
  {
    $wrapper = entity_metadata_wrapper($target_type, $item['target_id']);
    if (!$wrapper->access('view'))
    {
      // User does not have access to the item, give them an error message and bounce them off the edit page and back to the view page
      drupal_set_message('You do not have permission to view certain items on the form you are trying to edit. Contact your administrator, you may need extra permissions to be granted.', 'error');
      drupal_goto('/node/' . $context['form']['nid']['#value']);
    }
  }
  //
}
DuaelFr’s picture

This issue is really interesting.

I would add a case I had to deal with a few days ago :

  • Entity A references entities B using a View that shows targets having a given field/state/taxonomy/whatever,
  • Create an entity A referencing an entity B given by the view,
  • Edit the entity B to change its fields/state/... as it becomes not referenced by the view anymore,
  • Edit the entity A and the reference is lost.

I suppose the case I described here is not the most common case but it exists.

Maybe we could add a setting to the instance to let the administrator choose its behavior when the referenced data is not accessible (because of permissions or because of any other reason) :

  • drop the reference (actual behavior) ;
  • keep it hidden (alert the user on the fact that the references order will be lost, reinject old references during field validation) ;
  • keep it visible (to be developped, show the unaccessible element in the form and allow to reorder it but not to remove it).

My 2 cts.