#1696660: Add an entity access API for single entity access got commited, but now we need to actually implement the API for all entity types.

  • Implement an entity access controller for terms and vocabularies
  • Convert access checks to use $entity->access().
Files: 
CommentFileSizeAuthor
#67 taxonomy-term-create-access-1862758-67.patch2.49 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 54,473 pass(es).
[ View ]
#67 taxonomy-term-create-access-1862758-67-interdiff.txt1.01 KBBerdir
#47 taxonomy-term-create-access-1862758-47.patch2.3 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 54,450 pass(es).
[ View ]
#45 taxonomy-term-create-access-1862758-45.patch2.36 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 49,907 pass(es).
[ View ]
#43 drupal-1862758-43.patch1.13 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 49,447 pass(es), 0 fail(s), and 434 exception(s).
[ View ]
#36 interdiff.txt1.06 KBdawehner
#36 drupal-1862758-36.patch12.42 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 49,048 pass(es).
[ View ]
#34 drupal-1862758-34.patch11.99 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 49,050 pass(es).
[ View ]
#34 interdiff.txt2.5 KBParisLiakos
#30 drupal-1862758-30.patch12.18 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 49,037 pass(es).
[ View ]
#30 interdiff.txt880 bytesParisLiakos
#24 drupal-1862758-24.patch11.32 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 49,043 pass(es), 22 fail(s), and 10 exception(s).
[ View ]
#24 interdiff.txt587 bytesdawehner
#21 interdiff.txt2.26 KBdawehner
#21 drupal-1862758-21.patch11.26 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 49,282 pass(es).
[ View ]
#20 interdiff.txt4.05 KBdawehner
#20 drupal-1862758-20.patch11.05 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 49,136 pass(es), 92 fail(s), and 105 exception(s).
[ View ]
#17 drupal-1862758-17.patch7.48 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 49,345 pass(es).
[ View ]
#17 interdiff.txt396 bytesdawehner
#11 drupal-1862758-11.patch7.5 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/taxonomy/lib/Drupal/taxonomy/VocabularyAccessController.php.
[ View ]
#8 interdiff.txt1.8 KBdawehner
#8 drupal-1862758-8.patch4.18 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 49,249 pass(es), 115 fail(s), and 8 exception(s).
[ View ]
#6 drupal-1862758-6.patch3.72 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 49,287 pass(es), 110 fail(s), and 5 exception(s).
[ View ]
#3 drupal-1862758-3.patch7.07 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1862758-3.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#1 drupal-1862758-1.patch7.07 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Comments

dawehner’s picture

Status:Active» Needs review
StatusFileSize
new7.07 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Started with using access controller for the taxonomy term.

Not sure about the best way to handle vocabularies as access is currently only done via user_access, so the access controller could override all methods, so there should be discussion, whether we really want to add a new function for vocabulary access.

In general it feels odd to have a createAccess() method which requires an instance of an entity (so basically the same question as the entity operation API).

Status:Needs review» Needs work

The last submitted patch, drupal-1862758-1.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new7.07 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1862758-3.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/Core/Entity/Term.phpundefined
@@ -21,6 +21,7 @@
+ *   access_controller_class => "Drupal\taxonomy\TaxonomyAccessController",

Ups!

ParisLiakos’s picture

#3: drupal-1862758-3.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, drupal-1862758-3.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new3.72 KB
FAILED: [[SimpleTest]]: [MySQL] 49,287 pass(es), 110 fail(s), and 5 exception(s).
[ View ]

Just a rerole against current version of core.

Berdir’s picture

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -394,14 +394,8 @@ function taxonomy_admin_paths() {
+function taxonomy_term_access($op, Term $term) {
+  return $term->access($op);

The user issue added a entity_page_access() access callback that should be able to replace the whole function :)

dawehner’s picture

StatusFileSize
new4.18 KB
FAILED: [[SimpleTest]]: [MySQL] 49,249 pass(es), 115 fail(s), and 8 exception(s).
[ View ]
new1.8 KB

Good point!

Changed that.

ParisLiakos’s picture

Status:Needs review» Needs work
+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -306,7 +306,7 @@ function taxonomy_menu() {
     'access callback' => 'taxonomy_term_access',
-    'access arguments' => array('delete', 2),
+    'access arguments' => array(2, 'delete'),

I guess you need to change access callback here as well

Besides that this seems ready

ParisLiakos’s picture

Also, seems class files are missing from the patch

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new7.5 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/taxonomy/lib/Drupal/taxonomy/VocabularyAccessController.php.
[ View ]

Thanks rootawc, such small things could cause hours.

In general it probably helps to include the actual access controllers as well :)

Status:Needs review» Needs work

The last submitted patch, drupal-1862758-11.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review

This looks good.

I guess most other access checks related to terms and vocabularies use direct user_access() checks as the actual conversion here is extremely short.

I'm not sure if we want to check them too and replace with correct access() method calls. We could defer that to a follow-up but we could end up with an inconsistent situation where not everything goes through the access controller in case that follow-up doesn't get done.

So I'd vote for a quick check for user_access() calls in the module and replace with access() where easily possible in this issue :)

ParisLiakos’s picture

ParisLiakos’s picture

Assigned:Unassigned» ParisLiakos

i am gonna take care of #13

ParisLiakos’s picture

Assigned:ParisLiakos» Unassigned

hrm..really i cant find any user_access call for taxonomy permissions:/
and i think my grep queries had nothing wrong

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new396 bytes
new7.48 KB
PASSED: [[SimpleTest]]: [MySQL] 49,345 pass(es).
[ View ]

Oh great!

ParisLiakos’s picture

Status:Needs review» Reviewed & tested by the community

RTBC if bot agrees:)

Berdir’s picture

Status:Reviewed & tested by the community» Needs work

Hm, well, there are user_access() "calls" in taxonomy_menu()

<?php
  $items
['taxonomy/term/%taxonomy_term'] = array(
   
'title' => 'Taxonomy term',
   
'title callback' => 'taxonomy_term_title',
   
'title arguments' => array(2),
   
'page callback' => 'taxonomy_term_page',
   
'page arguments' => array(2),
   
'access arguments' => array('access content'),
   
'file' => 'taxonomy.pages.inc',
  );
?>

This for example should now use entity_page_access('view'). Same for the feed path below. And there is an edit vocabulary path as well that could use this.

Create is tricky, I'm not sure what to do about that yet. Because we have no entity object that we could invoke access('create') on, so I guess we can leave that for now. But the ones above should be easy to implement.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new11.05 KB
FAILED: [[SimpleTest]]: [MySQL] 49,136 pass(es), 92 fail(s), and 105 exception(s).
[ View ]
new4.05 KB

Let's see whether this works already.

dawehner’s picture

StatusFileSize
new11.26 KB
PASSED: [[SimpleTest]]: [MySQL] 49,282 pass(es).
[ View ]
new2.26 KB

Opened a follow up to improve the EntityTestAccessController: #1900198: Use the access controller in the entity_test

There is no need for taxonomy_add_term_access().

ParisLiakos’s picture

Ah,hmm right, taxonomy_menu..i missed that indeed
not sure about the create here..i would prefer not to add a callback and just keep using user_access in the hook_menu implementation..
but i can live with it ofc

ParisLiakos’s picture

Status:Needs review» Needs work
+++ b/core/includes/entity.incundefined
@@ -749,3 +749,21 @@ function entity_query($entity_type, $conjunction = 'AND') {
+ * This access callback might not be enough depending on the entity type.

This needs an explanation berdir thinks and i agree. Its too vague now, can we add why?

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new587 bytes
new11.32 KB
FAILED: [[SimpleTest]]: [MySQL] 49,043 pass(es), 22 fail(s), and 10 exception(s).
[ View ]

What about that?

Johnny vd Laar’s picture

In the issue that I was working on mentioned in comment #14 I also added a number of tests and usability improvements perhaps we can merge those into this patch?

Berdir’s picture

Status:Needs review» Reviewed & tested by the community

This issue does not introduce any new features. It's just an internal refactoring of how the permissions are checked.

So your issue will stay open but will have to be re-rolled after this is in to build on top of this.

Comments looks fine to me. I personally try to avoid using "you" and write it indirectly but that's more related to translatable strings I think, there are already a lot of you's in core, so I don't think that's a problem.

fago’s picture

+++ b/core/includes/entity.inc
@@ -749,3 +749,22 @@ function entity_query($entity_type, $conjunction = 'AND') {
+function entity_page_create_access($entity_type) {
+  $entity = drupal_container()->get('plugin.manager.entity')
+    ->getStorageController($entity_type)

I think we should find a way to create that object via the menu system, such that we can use the same object for access checks and for the page callback.

However, that's another clean-up which should be handled in its own issue - for now I think this is a good way to proceed.

Patch looks RTBC to me as well.

ParisLiakos’s picture

Can we get this committed please?
#1848686: Split "Administer vocabularies and terms" permission: Access the taxonomy overview page and Add terms in vocabulary name is kinda blocked on this one and we are running out of time:(

#24: drupal-1862758-24.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, drupal-1862758-24.patch, failed testing.

ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new880 bytes
new12.18 KB
PASSED: [[SimpleTest]]: [MySQL] 49,037 pass(es).
[ View ]

some more taxonomy_term_access callbacks sneaked in term translation UI test

Status:Needs review» Needs work

The last submitted patch, drupal-1862758-30.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review

#30: drupal-1862758-30.patch queued for re-testing.

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

It has been RTBC before. The last fail seemed to be yet another random test failure.

ParisLiakos’s picture

StatusFileSize
new2.5 KB
new11.99 KB
PASSED: [[SimpleTest]]: [MySQL] 49,050 pass(es).
[ View ]

Just noticed that term access controller is named TaxonomyAccessController
I guess it should be TermAccessController

dawehner’s picture

Oh right that's way better

dawehner’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new12.42 KB
PASSED: [[SimpleTest]]: [MySQL] 49,048 pass(es).
[ View ]
new1.06 KB

We can simplify the term translation controller based on that.

ParisLiakos’s picture

Status:Needs review» Reviewed & tested by the community

Bot agrees, lets get this in

catch’s picture

Status:Reviewed & tested by the community» Fixed

Looks great. Committed/pushed to 8.x.

swentel’s picture

Status:Fixed» Active
+++ b/core/includes/entity.incundefined
@@ -742,3 +742,22 @@ function entity_query($entity_type, $conjunction = 'AND') {
+  $entity = drupal_container()->get('plugin.manager.entity')
+    ->getStorageController($entity_type)
+    ->create(array());

So this line bothers me, especially the call to create(). If you go to admin/structure/taxonomy/tags, this will trigger hook_entity_create(), which triggers field_entity_create(), which triggers $entity->getTranslationLanguages() - however, there's no bundle on the $entity, so field_info_instances will return all fields. Now, all is ok when you have no fields on the tags vocabulary. But it you add one, instances will be found, however, no bundle, so this code:

<?php
     
foreach (field_info_instances($this->entityType, $this->bundle()) as $field_name => $instance) {
       
$field = field_info_field($field_name);
        if (
field_is_translatable($this->entityType, $field) && isset($this->$field_name)) {
?>

is not getting the $field_name, but the actual bundle as the first parameter. field_info_field() will fail and field_is_translatable() will receive an empty field.

Now, field_is_translatable() is simply this:

<?php
 
return $field['translatable'] && field_has_translation_handler($entity_type);
?>

I don't even understand why this doesn't trigger notices when field is NULL, must be something I'm learning again in PHP. However, while converting field api to cmi, $field['translatable'] becomes $field->translatable which /does/ trigger notices.

Anyway, something is funky here anyway because of that whole chain. A simply fix could be to add a check on $field, but that seems really dumb to me.

dawehner’s picture

Thanks for pointing out the problem with that approach. It feels like we need some kind of way to disable hooks in places like that, but yeah this is no actual good solution. Some other idea could be to introduce another step before create, called rawCreate or anything else.

fago’s picture

however, there's no bundle on the $entity, so field_info_instances will return all fields.

entity_create() should always receive the bundle (see its docs), thus if that's not happening that's the bug. We might want to pro-actively check for that and through an exception if it's missing but that's another issue.

Berdir’s picture

+++ b/core/includes/entity.incundefined
@@ -742,3 +742,22 @@ function entity_query($entity_type, $conjunction = 'AND') {
+function entity_page_create_access($entity_type) {
+  $entity = drupal_container()->get('plugin.manager.entity')
+    ->getStorageController($entity_type)
+    ->create(array());
+  return $entity->access('create');

Right, which means this helper function is wrong and we need to add an argument for the bundle, check the bundle key and pass it correctly to create()

dawehner’s picture

Status:Active» Needs review
StatusFileSize
new1.13 KB
FAILED: [[SimpleTest]]: [MySQL] 49,447 pass(es), 0 fail(s), and 434 exception(s).
[ View ]

Yeah this was wrong.

Status:Needs review» Needs work

The last submitted patch, drupal-1862758-43.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new2.36 KB
PASSED: [[SimpleTest]]: [MySQL] 49,907 pass(es).
[ View ]

Ok, made the bundle optional (entities are not required to have bundles, which means that there is single bundle named after the entity type) and added a helper function to translate $vocabulary to the bundle name. Maybe we can standardize that later on but this is only temporary as we will need to solve id differently for the new routing system anyway.

This passed all taxonomy tests locall.

dawehner’s picture

+++ b/core/includes/entity.incundefined
@@ -752,13 +752,24 @@ function entity_page_access(EntityInterface $entity, $operation = 'view') {
+  if (isset($definition['entity_keys']['bundle']))

oh the { seems to be missing.

+++ b/core/includes/entity.incundefined
@@ -752,13 +752,24 @@ function entity_page_access(EntityInterface $entity, $operation = 'view') {
   return $entity->access('create');

Should we return FALSE, explicit else?

Berdir’s picture

StatusFileSize
new2.3 KB
PASSED: [[SimpleTest]]: [MySQL] 54,450 pass(es).
[ View ]

Forgot about this, that if actually shouldn't exist at all...

ParisLiakos’s picture

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -369,6 +369,19 @@ function taxonomy_menu() {
+function taxonomy_term_create_access(Vocabulary $vocabulary) {
+  return entity_page_create_access('taxonomy_term', $vocabulary->id());
+}

i may be missing something, but after looking this after a while..why do we want this funtion, if it only passes the ->id() method? Since we pretty much know that all entities have id() method..why dont we pass the $entity_type (or subtype now) and then call ->id() inside entity_page_create_access() ?

eg sth like

<?php
function entity_page_create_access($entity, $entity_type = NULL) {
 
$bundle = $entity_type ? $entity_type->id() : FALSE;
 
// Do stuff
}
?>
dawehner’s picture

@rootatwc
This feels really hacky, but if we do this here, we should make it really clear that we have an bundle_entity in that case.

Berdir’s picture

There is currently no requirement for the bundle types to be an entity themself. vocabularies are and node types will be soon but I'd say we should keep it as a string for now. It's a temporary thing anyway, access checks move to the container anyway for symfony routes.

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

I agree

Johnny vd Laar’s picture

I have used the patch from #47 to create a new "Create terms in vocabulary" permission in this issue:
http://drupal.org/node/1848686#comment-7208060

xjm’s picture

Status:Reviewed & tested by the community» Needs work

The last submitted patch, taxonomy-term-create-access-1862758-47.patch, failed testing.

ParisLiakos’s picture

Status:Needs work» Needs review

Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].

#47: taxonomy-term-create-access-1862758-47.patch queued for re-testing.

ParisLiakos’s picture

Status:Needs review» Reviewed & tested by the community

back to rtbc

xjm’s picture

We might want to pro-actively check for that and through an exception if it's missing but that's another issue.

Can we get a followup for that?

Also, it'd be good to update the summary with the current issue status, including why we're making this change. And next time, please file a followup instead of reopening the original issue. It's really confusing for reviewers. :) It stopped this followup patch from maybe getting committed last week, in fact.

Berdir’s picture

I'm adding such an exception in #1818560: Convert taxonomy entities to the new Entity Field API because I needed it to help with debugging cases where it was missing.

Johnny vd Laar’s picture

Perhaps I'm misunderstanding this but here the permission is called "update terms in":

<?php
 
/**
   * Implements \Drupal\Core\Entity\EntityAccessControllerInterface::updateAccess().
   */
 
public function updateAccess(EntityInterface $entity, $langcode = LANGUAGE_DEFAULT, User $account = NULL) {
    return
user_access("update terms in {$entity->bundle()}", $account) || user_access('administer taxonomy', $account);
  }
?>

and here it's called "edit terms in":

<?php
    $permissions
+= array(
     
'edit terms in ' . $vocabulary->id() => array(
       
'title' => t('Edit terms in %vocabulary', array('%vocabulary' => $vocabulary->name)),
      ),
    );
?>

shouldn't this be both update?

Or should I create a new issue for this?

Berdir’s picture

@Johnny vd Laar: Hm, strange. Let's open a new issue for that, the patch in here already got derailed for weeks because of being a follow-up patch in an already commited issue that looks scary :) Sounds like we will also have to improve our test coverage if it's not failing at the moment.

Johnny vd Laar’s picture

I'm adding better test coverage here:
http://drupal.org/node/1848686#comment-7261878

I'll open a new issue. (http://drupal.org/node/1966614)

webchick’s picture

Assigned:Unassigned» xjm

Jess said she wanted to take a crack at this before commit.

xjm’s picture

Title:Implement entity access API for terms and vocabularies» [Followup] Implement entity access API for terms and vocabularies
Assigned:xjm» Unassigned

Don't hold this on me; we misidentified what this issue was (again, apparently, based on my comment in #57) because of the misleading title and reuse of the original issue for a followup. (A good time to get my input would have been back before the first commit in #38, but we have plenty of time before code freeze if something is incomplete.) :)

webchick’s picture

Berdir’s picture

Priority:Normal» Major

I'm not sure why nobody wants to commit this, this was RTBC since over a month and it's blocking #1818560: Convert taxonomy entities to the new Entity Field API, so setting to major.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work

Small nit...

+++ b/core/includes/entity.incundefined
@@ -774,13 +774,23 @@ function entity_page_access(EntityInterface $entity, $operation = 'view') {
+function entity_page_create_access($entity_type, $bundle = NULL) {
+  $definition = drupal_container()->get('plugin.manager.entity')->getDefinition($entity_type);

Old way of accessing the container

Berdir’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new1.01 KB
new2.49 KB
PASSED: [[SimpleTest]]: [MySQL] 54,473 pass(es).
[ View ]

Re-roll, setting back to RTBC if that's ok with you :) If not, bump it back.

Also removed a now bogus line in the docblock of that function as we now do support bundles, this was pointed out by @WimLeers in the taxonomy term NG review.

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed 17f4d14 and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.