File attachments in gradebook bypass normal file upload access control
| Project: | Gradebook |
| Version: | 6.x-2.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | MGN |
| Status: | postponed (maintainer needs more info) |
Jump to:
If I upload a file attached to a particular assignment grade for a student, that file is viewable by anyone who knows the URL, even if Drupal's file system is set to "private" and user permissions are set such that Anonymous users cannot access File Uploads. In other words, if I create a file with comments about a student's grade and upload it as a file attachment, anyone in the world can see this comments (which might mean I'm violating FERPA, incidentally).
I'd really like to use this, since I create annotated PDFs of student papers as I grade them, and I'd really like to avoid emailing all these, but presently it's not viable because these files are not stored privately.
So, can this be fixed? Or, am I misunderstanding how this feature is meant to be used?
#1
Thanks for reporting this. I'll try to look into this today.
#2
I've been able to verify that this is a problem, and I am working up a solution for it.
We need to check file ownership whether the site is configured for public or private downloads. This can be done by implementing hook_menu for any request for a file in the gradebook_attachments directory, and then hook_file_download() to check that the user requesting the file matches either the student's or the teacher's user id. This will probably require a database modification as well.
#3
Great! I look forward to seeing what you come up with.
Just so I understand though (that is, in case I have time to hack up my own patch), why would you need to modify the database? Wouldn't hook_file_download() be able to include the necessary checking?
#4
Yes. You are right there is no need to modify the database. All the information (which user attached the file, and which user the attachment is intended for) is already there.
The idea is to make the gradebook_attachments folder private by placing an .htaccess file in that folder to redirect requests for attachments to the gradebook_attachments module (via gradebook_attachments_menu).
Here is the .htaccess file
#
# Apache/PHP/Drupal settings:
#
<IfModule mod_rewrite.c>
RewriteEngine on
RewriteBase /html/system/files/gradebook_attachments
RewriteRule ^(.*)$ $1 [L,R=301]
</IfModule>
# $Id$
(The rewrite base path might need to be adjusted depending on the site configuration - this works on my test site).
<?php
/**
* Implementation of hook_menu().
*/
function gradebook_attachments_menu() {
$items = array();
$items['system/files/gradebook_attachments'] = array(
'access arguments' => array('access gradebook attachments folder'),
'page callback' => 'file_download',
'page arguments' => array('gradebook_attachments'),
'type' => MENU_CALLBACK,
);
return $items;
}
?>
This way it doesn't matter if the site has public or private downloads - the module gets the final word on who gets to see the file. I've also added another permission so access to the drupal path can be restricted by role (i.e. limit it to teachers and students).
<?php/**
* Implementation of hook_perm().
*/
function gradebook_attachments_perm() {
return array('attach files to gradebook', 'access gradebook attachments folder');
}
?>
gradebook_attachments_file_download then checks to see if logged in user is either the creator of the file, the student for whom the attachment is intended, or the student's teacher. Only then is the file download permitted.
<?php/**
* Implementation of hook_file_download().
*/
function gradebook_attachments_file_download($path) {
global $user;
$filepath = file_create_path($path);
$file = db_fetch_object(db_query("SELECT fid, uid FROM {files} WHERE filepath = '%s'", $filepath));
$attached = db_fetch_object(db_query('SELECT nid, uid FROM {gradebookapi_files} WHERE fid = %d', $file->fid));
// Get gradebook from the nid.
$node = node_load($attached->nid);
foreach (gradebookapi_assignment_terms($node) as $term) {
$gradebook = gradebookapi_get_tid_gradebook($term->tid);
$teacher = gradebookapi_is_teacher($gradebook);
if ($teacher) break;
}
// Allow file access only if file belongs to the logged in user, or is attached to the logged in users' grade.
// Teachers of the logged in user can also access the file.
if (($user->uid == $file->uid) || ($user->uid == $attached->uid) || $teacher) {
return array('Content-type: '. file_get_mimetype($path));
}
else {
return -1;
}
}
?>
I have this working on a test site now. Tomorrow I will test it on a live site (running organic groups, og_gradebook, etc.) to make sure this works before I post an official patch against the latest dev.
#5
Thanks! This looks awesome.
I've got the code in place, but (and you may have anticipated this) I think I'm running into trouble with .htaccess. I can't tell how exactly the handoff you describe is supposed to work, so I'm not sure how to adapt it to my set up. I need this to run under a multisite (which probably complicates things), so my original file location (running on zachwhalen.net) would look like this:
http://test.zachwhalen.net/sites/test.zachwhalen.net/files/gradebook_att... [not a real file location]
Or in the file system:
/[someotherstuff]/public_html/sites/test.zachwhalen.net/files/gradebook_attachments/testfile.pdf
So is the /html/system/ in your example .htaccess pointing to the document root in your site?
Of course, this could just be my failure to grasp the hook_menu system. Any suggestions?
#6
OK, I got it. I was getting ahead of myself when I first tried this, but I took it a little slower and it makes sense now. :)
All I had to do was drop the '/html/' off of my RewriteRule to point it to the right URL (which I see now is picked up by the hook_menu).
Thanks for figuring this out so quickly!
#7
Thanks for testing. The path to use in the rewrite rule will depend on how the server is configured. Pretty much all of my sites are running in subdirectories, so that html piece just reflects my own setup. I'll write up a readme.txt file for setting up the .htaccess and leave the rewrite base at /system/files/gradebook_attachments with an appropriate caveat.
Please report back if it doesn't behave as you would expect it to. I've been running it on a live site for the last day and everything seems fine so far.
#8
I had my students try it out today, and most reported no problems. In a couple of cases, however, their accounts were somehow being logged out before they could load the file -- so they'd get "Access Denied."
I couldn't re-create the problem, but in at least one case what I saw was a student viewing her grade info on an assignment but with the site presenting itself to her as though she were an anonymous user.
I have no idea how that could happen, but she logged back in and everything was fine. Moreover, trying to access that grade info URL as anonymous yielded (correctly) Access Denied.
I know that's not very helpful, but I mention it in case you've seen something similar or can make sense of how that might happen.
In any case, file attachment access is now working fine as far as I can tell.
#9
I haven't seen any unintended logouts yet, but I'll look for anything like this.
Thanks again for testing. These changes have now been committed to the 6.x-2.x-dev version.
#10
Sorry to reopen this, but I may have found a new bug or conflict. I don't have time to really isolate this right now, but there seems to be a conflict with the way ImageCache (or maybe imagefield) handles files.
In my case, when I use the image version of filefield insert, and then resize it so that imagecache's image resize filter is invoked, it looks for a path like this:
mysite.com/system/files/resize/whateverimage-360-250.png
With the additions to gradebook_attachment.module, I get an Access Denied for that path. I switched off the access part of your function by setting the "deny access" part from
return -1toreturn 1, just to see if that's what was denying access. And indeed, the result was that I got a "Page Not Found" error. Of course, that indicates there could be other problems, but I thought I'd post it here until I have time to look closer, in case that rings any bells.#11
@zwhalen, I am not familar with these modules so I'll need some more info. Do you know which module is defining a menu callback on system/files/resize ? I've been browsing ImageCache and imagefield in CVS and they do not appear to use this path. ImageCache uses 'system/files/imagecache'
If there is no menu callback at system/files/resize, the closest match might be system/files/gradebook_attachments . That would trigger the access check that you are seeing. Otherwise, I am not sure why gradebook_attachments would be intercepting another module's menu callback.
Please let me know if the path that you indicated is correct (turn off gradebook_attachments and verify that you can access the image file as expected), and what module is defining this callback.