Improve entity_metadata_taxonomy_access to cover existing permissions

Looks like the right approach, but needs cleaning up for coding standards and there are a few code errors too.

1. +++ b/modules/callbacks.inc
   @@ -795,14 +795,31 @@ function entity_metadata_comment_properties_access($op, $property, $entity = NUL
   +  if (user_access('administer taxonomy', $account)) return TRUE;
   

   if statements should always have a block, even if a single line.

2. +++ b/modules/callbacks.inc
   @@ -795,14 +795,31 @@ function entity_metadata_comment_properties_access($op, $property, $entity = NUL
   +        return taxonomy_term_edit_access($entity);
   

   There's a security hole here if $account is set and is not the same as the global $user, as taxonomy_term_edit_access() ALWAYS returns access for the current user.

3. +++ b/modules/callbacks.inc
   @@ -795,14 +795,31 @@ function entity_metadata_comment_properties_access($op, $property, $entity = NUL
   +      break;
   

   You don't need to break if you've returned.

4. +++ b/modules/callbacks.inc
   @@ -795,14 +795,31 @@ function entity_metadata_comment_properties_access($op, $property, $entity = NUL
   +        //Check for taxonomy_access_fix which adds permissions to create new terms in a given vocab
   

   Comments need to be wrapped to 80 chars. Also, they need to end with a full stop and they need a space after the //.

5. +++ b/modules/callbacks.inc
   @@ -795,14 +795,31 @@ function entity_metadata_comment_properties_access($op, $property, $entity = NUL
   +        if(function_exists('taxonomy_access_fix_access')) {
   

   if takes a space before the ().

6. +++ b/modules/callbacks.inc
   @@ -795,14 +795,31 @@ function entity_metadata_comment_properties_access($op, $property, $entity = NUL
   +        } else {
   

   elses should not be coddled.

7. +++ b/modules/callbacks.inc
   @@ -795,14 +795,31 @@ function entity_metadata_comment_properties_access($op, $property, $entity = NUL
   +        return user_access("delete terms in $entity->vid");
   

   $account should be passed in to any calls to user_access().

Getting there! Still a few more things to fix:

1. +++ b/modules/callbacks.inc
   @@ -795,14 +795,34 @@ function entity_metadata_comment_properties_access($op, $property, $entity = NUL
   +  if (user_access('administer taxonomy', $account)) return TRUE;
   

   This still needs to be a block.

   Worth adding a comment too, to the effect that this is letting the admin through for everything.

2. +++ b/modules/callbacks.inc
   @@ -795,14 +795,34 @@ function entity_metadata_comment_properties_access($op, $property, $entity = NUL
   +        return user_access("edit terms in $entity->vid", $account) || user_access('administer taxonomy', $account);
   

   You don't need that here; if the account has 'admin taxo' permission, they've already been let in at the very top.

3. +++ b/modules/callbacks.inc
   @@ -795,14 +795,34 @@ function entity_metadata_comment_properties_access($op, $property, $entity = NUL
   +        //Check for taxonomy_access_fix contrib module which adds additional
   +        //permissions to create new terms in a given vocabulary.
   

   You still need to add a space after the // here.

4. +++ b/modules/callbacks.inc
   @@ -795,14 +795,34 @@ function entity_metadata_comment_properties_access($op, $property, $entity = NUL
   +        } ¶
   

   Stray whitespace here.

5. +++ b/modules/callbacks.inc
   @@ -795,14 +795,34 @@ function entity_metadata_comment_properties_access($op, $property, $entity = NUL
   +        else {
   +          return user_access('administer taxonomy', $account);
   +        }
   

   Same as earlier -- it's redundant to check that permission here. We know if we get this far, the user doesn't have that permission.

This seems to be affecting Drupal.org, as reported at #2344265: Excessive access control on field collection entities.

I don't see anything specifically testing this in entity.test. Since this is access-checking code, good test coverage is especially important. I recommend writing tests.

Rerolled for 7.x-1.6

applies cleanly to 7.x-1.x-dev #10 unable to apply must have been that it was not applicable on the version in the meta data of the issue 1.5. I will send it for a retest.

Patch makes sense, please add test coverage.

Added test coverage

Patch #17 worked for me.
Thanks!

I reviewed the patch and it seems fine + tested it on a site.

Committed, thanks!