filefield_edit_access() and filefield_view_access() should use content_access() instead of checking content_permissions specific permissions. Attached is a patch that makes this change.

content_access() invokes hook_field_access() which is how the content_permissions module does its magic. I enabled content_permissions and did a bunch of tests to make sure its permissions are still enforced with my new patch. Everything worked fine! So, no one who used content_permissions should notice any changes.

However, if (like in our application) you use a custom hook_field_access() to control access to a field, it will now start working with filefield.

Of course, since this is security related, it would be really good if others could test this a bunch too in case I missed something.

Thanks!
David.

Comments

Heh, I tested content_permissions but I forgot to properly test our application. ;-) We are depending on $node being passed (the 4th argument of content_access()) and, of course, in this patch I am not passing it. So, the current patch isn't the final word on this! I'll need some more time to try and get $node into there, because that will required going deeper into the filefield source.

StatusFileSize
new1.61 KB

Ok, so here is a new patch, which fixes the problem for our purposes.

It adds an additional and optional $node argument to filefield_view_access(), which is used in filefield_file_download() at the same time that its doing the node_access() check.

As I mentioned above, this works for our purposes because we only care about 'view' and we only care about the file download. So, this is no longer an elegant general solution. Yes, its a drop in replacement for directly referencing content_permissions specific permissions, but it may not help in all possible uses of hook_field_access(). For example, if you cared about "edit" and needed $node there, or were trying to do something that affected the AHAH callback and not just filefield_file_download() -- it wouldn't work for you.

It also raises the question: If we are going to do filefield_view_access() on line 183, do we really need to do it earlier on line 165? I didn't remove the code on line 165 because it could be argued that when using only content_permissions this would be faster. But ideally we shouldn't be doing the same check multiple times.

So, now this needs some discussion! Nothing is ever as simple as you think it will be. ;-)

Thank you,
David.

+1. I am working on a module that provides fine-grained access control to fields based on TAC, and filefield's current implementation of access control prevents my module from restricting access to filefield downloads.

I'll test the patch within the next day or so.

Patch works perfectly, thanks.

Status:Needs review» Reviewed & tested by the community

Marking RTBC.

Status:Reviewed & tested by the community» Fixed
StatusFileSize
new2.3 KB

Thanks very much xjm and dsnopek. I think you are both 100% correct in these changes. I also agree with dsnopek's comment in #2 that doing the same check twice is unnecessary. In most cases you're going to be working with a single file and a single check, so doing the same check multiple times for the same file is unnecessary. There are a few situations where the lines could make things slightly more efficient, but I think generally they'll cause the opposite. So this patch is the same as #2 with the duplicate check removed. Please reopen if you find any problems with this change. I've committed it to CVS. Thanks again!

Status:Fixed» Closed (fixed)

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

Version:6.x-3.2» 6.x-3.3
Status:Closed (fixed)» Active
StatusFileSize
new1000 bytes

I've had issues with this fix for fields that appear in more than one content type. The array returned by content_fields($field_name), that's then sent to content_access() on line 479 of filefield.module returns data only relevant to one of those content types which can cause issues in your own implementation of hook_field_access.

For example, you have field_myfile in both the page and story content type. The field data sent to content_access() will have a type_name set to only one of these content types regardless of which content type you're actually uploading the file through.

My workaround was to pass the content type name to content_fields()

Status:Active» Needs review

Thanks, this indeed looks like a problem. I'll give this a test next time I'm working on FileField.

Since the latest release of filefield I had problems on a website uploading images with imagefield. Before it worked fine.
Others then the admin are not allowed anymore to upload images in imagefields using AHAH and they got a message

An HTTP error 0 occurred.
/filefield/ahah/story/field_image/0

my workaround was to change in filefield.module line 478:

function filefield_edit_access($field_name) {
  if (!content_access('edit', content_fields($field_name))) {
    return FALSE;
  }
  // No content permissions to check, so let's fall back to a more general permission.
  return user_access('access content');
}

temporary to

function filefield_edit_access($field_name) {
  return TRUE;
}

So, in my case, it has to do something with the permissions (or checking the permissions in AHAH)
Because the above hack worked, I think this has something to do with this patch.

I found out what happened in my specific case:
- the filefield module version 3.3 checks since this release exclusively for 'access content' permissions, I gave my client permission to 'administer nodes', but not specific for 'node access' permissions.
Why? Administer nodes normally gives permission to access content, see comments in node.module:
* In determining access rights for a node, node_access() first checks
* whether the user has the "administer nodes" permission. Such users have
* unrestricted access to all nodes. Then the node module's hook_access()
* is called, and a TRUE or FALSE return value will grant or deny access.
.

Therefore, I propose the folllowing patch to make filefield more compatible with the node,module permissions hierarchy:

function filefield_edit_access($field_name) {
  if (!content_access('edit', content_fields($field_name))) {
    return FALSE;
  }
  // No content permissions to check, so let's fall back to a more general permission.
-  return (user_access('access content'));
+  return (user_access('access content')||user_access('administer nodes'));
}

That sounds good Hanno, though it should probably be combined with the patch in #8, since that's also a problem.

I have combined them. @Quicksketch I never contributed patches, so I'm not sure if the patch will work. Could you have a look?

Index: filefield.module
===================================================================
--- filefield.module
+++ filefield.module
@@ -36,7 +36,7 @@
     'page callback' => 'filefield_js',
     'page arguments' => array(2, 3, 4),
     'access callback' => 'filefield_edit_access',
-    'access arguments' => array(3),
+    'access arguments' => array(2, 3),
     'type' => MENU_CALLBACK,
   );
   $items['filefield/progress'] = array(
@@ -475,10 +475,10 @@
  * The content_permissions module provides nice fine-grained permissions for
  * us to check, so we can make sure that the user may actually edit the file.
  */
-function filefield_edit_access($field_name) {
-  if (!content_access('edit', content_fields($field_name))) {
+function filefield_edit_access($type_name, $field_name) {
+  if (!content_access('edit', content_fields($field_name, $type_name))) {
     return FALSE;
  }
  // No content permissions to check, so let's fall back to a more general permission.
-  return (user_access('access content'));
+  return (user_access('access content')||user_access('administer nodes'));
}

Status:Needs review» Fixed
StatusFileSize
new1.86 KB

Thanks Hanno. That serves as a good start, we need to do the same thing for filefield_view_access(). I've committed this patch which wraps together #8, #11, plus similar changes to the filefield_view_access() function.

Status:Fixed» Closed (fixed)

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

Sorry to reply to this already closed ticket, but this seems like a knowledgeable group and the other thread (HTTP error 0) is super long.

We've recently run into the HTTP error 0 issue as well, but narrowed it down to a problem somehow related to Organic Groups. When using Organic groups, users get assigned a role but only within the context of the group. So group users don't have site-wide permissions, just within the group. This appears to cause a problem with the AHAH javascript when using the Upload button. A user without site-wide edit access to the file field in question is thrown the HTTP error 0 when pressing the Upload button, but can successfully submit the whole edit form using the Preview or Save buttons. A user WITH site-wide edit access to the file field in question does NOT get the error.

When I give the "Authenticated User" role site-wide edit access to the file field in question the AHAH error goes away for all users. So there seems to be some disconnect with the permission check within the group context. I wonder if this isn't related to the content permission specific checks in the associated patch here.

Thoughts? Should I open another ticket or report this to another project?

Thanks,
-Tim

I'd say open a separate issue if you have a specific, unique problem, or reply to the existing one on that topic, unless you are confident your error is related to the patch in this issue. Doing otherwise only makes it harder for others to troubleshoot and find the info they need. (It is helpful to add references to related issues on both ends, though: #297035: HTTP error 0 .)

xjm,
Thanks. I'll reply in #297035.

-Tim