Apply file permissions to attachment table [#315061]

This is the bad case:
Setting "view webfm attachments" active e.g. for the role guest, all attachments to a node are shown and downloadable!
I expected that only those were shown that have the per file setting public download.

The better case:
Setting "view webfm attachments" inactive for the role guest makes the per file setting public download work.
but then I have no attachments to the node at all!

I believe the function webfm_get_attachments should consider wether the webfm_file is set to public downlad or not.
this query (2078)

$file_result = db_query('SELECT * FROM {webfm_file} f INNER JOIN {webfm_attach} a ON f.fid = a.fid WHERE a.nid = %d ORDER BY a.weight', $nid);

seems not to reflect on that.

on the other hand the call of that function (550)

if ((variable_get("wfm_attach_$node->type", 1) == 1) &&
          user_access('view webfm attachments')) {
        $output['webfm_files'] = webfm_get_attachments($node->nid);
      }

is suppressed without that permission 'view webfm attachments'.

In the end it looks like being impossible to use that nice feature to set a single file to public download.
Perhaps one should alter the permission
'view webfm attachments' to
'view public webfm attachments'

or say
'view all webfm attachments' and
'view public webfm attachments'

I hope this explanation is not too confused. Please ask, if it is not clear. Or there is a way to handle my problem.
I Like webfm very much but for this problem I cannot use it in an actual project :(

Comment	File	Size	Author
#3	webfm.module.txt	110.29 KB	AxelBernhardt

Comments

Comment #1

AxelBernhardt commented 29 September 2008 at 23:11

my try looks like this:

the call (550):

    case 'load':
      if ((variable_get("wfm_attach_$node->type", 1) == 1)) { // && user_access('view webfm attachments')) {
        $output['webfm_files'] = webfm_get_attachments($node->nid);
      }
      return $output;
      break;

line 2075:

function webfm_get_attachments($nid) {
  $files = array();
  if(user_access('view webfm attachments')) {
    $file_result = db_query('SELECT * FROM {webfm_file} f INNER JOIN {webfm_attach} a ON f.fid = a.fid WHERE a.nid = %d ORDER BY a.weight', $nid);
  }else {
    $file_result = db_query('
      SELECT * FROM
      {webfm_file} f
      INNER JOIN {webfm_attach} a ON f.fid = a.fid
      WHERE
        a.nid = %d
        AND f.perm = 1
      ORDER BY a.weight', $nid);
  }
  while($file_record = db_fetch_object($file_result)) {
    $_file = new webfm_fdesc($file_record);
    if($_file->result == TRUE) {
      $files[] = $_file;
    }
  }
  return $files;
}

Here I checked the file for a perm=1.
So if the user has no permission 'view webfm attachments' [all attachments...], he can see those are set to public download per file.

checking the access dropped down into webfm_get_attachments ...

With this I got what I wanted to have.
Please check and give some feedback.

PS:
perm=2 is still missing (maybe):
=>WEBFM_FILE_ACCESS_ROLE_VIEW, role can view/download file (owner/admin can always view)
Perhaps i did not understand that concept ...

Comment #2

Frank Steiner commented 30 September 2008 at 09:45

Title:	Having "view webfm attachments" for guest role, all attachments are public	» Apply file permissions to attachment table
Category:	bug	» feature

Detail first: I guess f.perm = 1 is wrong because perms can be added, i.e. f.perm=3 would mean "public download" and "role view/download".

But in general I think that the permission settings are only for browsing the webfm tree, i.e. when you go to "http://yoururl/webfm". There you can define which files users can see and download even without "view webfm attachments" permissions.
With that permission, you can always see everything.

You want the distinction in the attachment table. webfm comments about this:

  // Files that have been attached are alway considered public to whoever can
  // access that node/comment (nodeaccess/commentaccess security).

so files you can see in the attachment table in a node have the same rights as the node itself. Who can view the node, can view and download the attachment. This makes sense to me. If one would change that, I guess then the whole permission table must be considered in webfm_get_attachments. There are some access helper functions in webfm.modulet that might help writing a complete patch for this, but this would be a little more work I guess.

An easier solution might be this:
If you don't attach the file to the node but put the link in the node yourself (you might want to give this patch a try http://drupal.org/node/310119), then everyone can see this link but if you revoke the "public download" permission, a user without "view webfm attachments" will get "File not found" when clicking the link.

Would that help?

Comment #3

AxelBernhardt commented 30 September 2008 at 11:14

Status	File	Size
new	webfm.module.txt	110.29 KB

sorry,
to me problem is/was that one could guess a web_send link counting the numbers up/down.

a user/role with "view webfm attachments" permissions should only see and get a file when there is role/view permission active for that file.
by now this user will get any file vie webfm_send.

f.perm = 1 is wrong

you are right, of course.

my actual webfm_get_attachments (2075):
new code:
(f.perm > 0 OR f.uid = %d )
...
AND f.perm & 1

function webfm_get_attachments($nid) {
  global $user;
  $uid = $user->uid;
  $files = array();
  if(user_access('view webfm attachments')) {
    $file_result = db_query('
      SELECT *
      FROM {webfm_file} f
      INNER JOIN {webfm_attach} a ON f.fid = a.fid
      WHERE
        a.nid = %d
      AND
        (f.perm > 0 OR f.uid = %d )
      ORDER BY a.weight', array( $nid, $uid));
  }else {
    $file_result = db_query('
      SELECT * FROM
      {webfm_file} f
      INNER JOIN {webfm_attach} a ON f.fid = a.fid
      WHERE
        a.nid = %d
        AND f.perm & 1
      ORDER BY a.weight', $nid);
  }
  while($file_record = db_fetch_object($file_result)) {
    $_file = new webfm_fdesc($file_record);
    if($_file->result == TRUE) {
      $files[] = $_file;
    }
  }
  return $files;
}

but in webfm_send_file I also needed to try a change (2247):

if($match == FALSE && $webfm_perm != WEBFM_ADMIN) {
    if($f->perm & WEBFM_FILE_ACCESS_PUBLIC_VIEW) {
      $match = TRUE;
//    } else if($webfm_perm == WEBFM_USER || $webfm_perm == WEBFM_ATTACH_VIEW){
//      //Check if the file is attached to a node
//      $query = 'SELECT nid FROM {webfm_attach} WHERE fid = %d';
//      $result = db_query($query, $f->fid);
//      if($result !== FALSE) {
//        while ($dbfid = db_fetch_array($result)) {
//          $node = node_load($dbfid['nid']);
//          if (node_access('view', $node)) {
//            $match = TRUE;
//            break;
//          }
//        }
//      }
    }
  }

the reason: WEBFM_ATTACH_VIEW lets a user with "view webfm attachments" receive any file he wants.
(perhaps I am wrong. but in my actual env it works well so far...)

now I set these per file permissions "public download" / "role view"
and the user only receives those with propper permission.

i hope someone copes with me that this is an anhancement ... or am I wrong?

Comment #4

Frank Steiner commented 30 September 2008 at 13:28

>the reason: WEBFM_ATTACH_VIEW lets a user with "view webfm attachments" receive any file he wants.

This is not true. Look at the code for the webfm_send function. If you have a user with "view webfm attachments" permissions, and a file which is not attached to a node which this user can see. Then try to type the webfm_send-download URL in the browser. It will result in "File not found" if this file has no public download permission. I just tried this.

Of course, if the file is attached to *any* node which the user can see, then he can download the file.

Also, the more I think about it, I'm not sure if the feature you request would make sense. If you give a user the "view webfm attachments" permissions you want him to see the attachment tables in the nodes. No one would expect that the user than can only see part of it. That would be confusing...

Are there really so many situations where this makes sense and couldn't be worked around with the type or linking without attaching that I showed above?

Comment #5

AxelBernhardt commented 30 September 2008 at 14:07

Frank,
first of all thanks for your replies...

my case is this:
I have some node types in my project wich show products.
each product can have attached pdf files, some are public and others only for specific roles.

the trick to attach a pdf-node-type to this product is a bit too complicated for my customer.

it would be the best to set the visibility of each file for it self.
pdf-public would be set to 'view public'
pdf-groupA should only be visible to users in groupA.

all these files shall be attached dierctly.

now in my test env i have attached 3 files with file based permissions:
a) Public download
b) Role View/Download
c) none of both

my test user has the right 'view attachment'
and he can see all three files.

I would expect that he can only a+b.
c should not be send out IMHO.

a guest user with no right 'view attachment' can see none, not even a).
also webfm_send does not send this file a). its not only the attachment table.

when I set the right 'view attachment' active, guest can see all 3 files.

can you reproduce this problem?
to me this is still an issue.

Comment #6

Frank Steiner commented 30 September 2008 at 15:33

I see your problem, but this is not a bug in webfm, its just beyond its scope. What you could try:
1. Extend your patch so that it covers all file permission cases, i.e. public_downloads as well as role view etc. I think the snippet above just cares about public_download.

2. Make it a configurable option, i.e. a checkbox in the webfm settings like "Apply file permissions to attachment table". Otherwise I guess there is no chance getting this feature in because it would change the old behaviour.

3. If the option is checked, apply your additional checks to both, the attachment table and webfm_send.
So a user must have the view attachments permission to see anything at all. Only then you can refine the permission by your configurable option.
To get it clean, I would recommend to write your own access check function outside webfm_send and webfm_get_attachments. Because if you do it the way I proposed, you must only further restrict access already given. So let webfm_send and webfm_get_attachments get their files and do their checks, and then at one place inside those functions call your function and pass the fid or whatever and check for the file permissions. If they don't match, remove this file from the list or deny the send.
This should be a clean and easy-to-understand structure.

4. Put your patch into a real patch format (diff -Nur or sth.), otherwise no one is going to test it.

Comment #7

robmilne commented 18 October 2008 at 15:48

The attachment viewing permissions are very simple and currently outside the scope of "file permissions" within the module. My rationale was that attached files are public in the same way they are with the upload module (at least from my understanding of the upload module from my drupal 4.6/4.7 days when I was actively programming drupal sites). If you can see a node you can see the attachments.

Your statement...

the reason: WEBFM_ATTACH_VIEW lets a user with "view webfm attachments" receive any file he wants.
(perhaps I am wrong. but in my actual env it works well so far...)

isn't true. A user with "view webfm attachments" could randomly punch fids into webfm_send addresses, but only files that are attached to viewable nodes would be accessible. If you can show it otherwise then indeed there is a security hole.

That said, I sympathize with your desire for fine grained attachment controls. The permissions used for file browsing is the natural place to look. My problem with the finer controls is complicating the interface. Can I expect users to understand additional conditions that determine attachment visibility? Would those conditions be set with an cascading context menu? What if the file is not marked as public by the owner of the file, but the person making the attachment has "Role attach" rights and he/she wants it publicly viewable? What if the owner changes the permissions after the attacher sets attachment visibility? Complexity is starting to rear its head here and I'm not in the mood to tame it. The current system may not be perfect for all users but I believe it works well enough for the majority.

Where webfm_send_file can condition file access in a more fine grained manner is via webfm_send links (ie: inside node content areas). If the file permissions has "Role attach" unchecked, and indeed the file remains unattached to any node, then access to the file is governed by the permissions only. The shortcoming of this scheme is that the link itself is still visible to users without view rights.

I won't work on this. I will reconsider the issue if a system can be demonstrated that satisfies my concerns for usability. In other words, a very fine patch.

Comment #8

Andrew Schulman commented 1 December 2008 at 00:57

I have some node types in my project wich show products.
each product can have attached pdf files, some are public and others only for specific roles.

pdf-public would be set to 'view public'
pdf-groupA should only be visible to users in groupA.

all these files shall be attached dierctly.

now in my test env i have attached 3 files with file based permissions:
a) Public download
b) Role View/Download
c) none of both

my test user has the right 'view attachment'
and he can see all three files.

I would expect that he can only a+b.
c should not be send out IMHO.

FWIW, this is exactly my case. I want to attach files to nodes, and have the user only see (and be able to download, even if he knows or guesses the URLs) those files for which he has permissions.

It's really a very basic requirement, and I'm amazed that Drupal doesn't have a solution for it. WebFM is the most sophisticated file manager for Drupal, and it doesn't do it.

Comment #9

Frank Steiner commented 1 December 2008 at 11:14

This is sth. for D7 I guess.
In D6, access control is all based on nodes. In D7, files will be first-class objects with their own file module and hook_file etc. I'm pretty sure that it will be easy to build access control modules for files then, specifying which role should be able to do what with a file (similar to the node-privacy-by-role module). If we had sth. like a file-privacy-by-role module it would be easy for webfm to reuse its forms to specify file permissions and let the other file-privacy-by-role module do all the hooking stuff etc. instead of implenting permission stuff on its own.

So this might be a very simple task with D7, provided there will be a nice file permission managing module.

Comment #10

Andrew Schulman commented 1 December 2008 at 15:15

Thank you, that is good to know. I understand that this is outside of WebFM's current scope and will hold out for D7. Andrew.

Comment #11

robmilne commented 6 December 2008 at 15:35

Status:

Active

» Closed (won't fix)

Apply file permissions to attachment table