problem with checking filesize during uploading if user belongs into more than one role [#54913]

If an user belongs into more than one role validation of uploading file doesn't work properly.
I think there is a problematic piece of code starting at line 307 in upload.module

if (count($error)) {
 unset($node->files[$fid], $_SESSION['file_previews'][$fid]);
  file_delete($file->filepath);
}

as for instance there could be a validation issue for one role but it could pass for a different role. But count($error) is set and hence uploaded file is deleted
I think a solution is to iterate over the $error array instead and check all items whether there are equal to number of user's roles, something like that:

foreach ($error as $name => $value) {
    if ($value == count($user->roles)) {
        unset($node->files[$fid], $_SESSION['file_previews'][$fid]);
        file_delete($file->filepath);
       break;
    }
}

Comment	File	Size	Author
#12	54913_5.patch	2.06 KB	rkerr
#12
#7	54913_4.patch	2.01 KB	dopry
#7
#6	54913_3.patch	2.01 KB	dopry
#6
#5	54913_2.patch	1.1 KB	dopry
#5
#4	54913_1.patch	1.1 KB	dopry
#4
#2	54913_0.patch	1.11 KB	dopry
#2
#1	54913.patch	1.11 KB	dopry
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

dopry CreditAttribution: dopry commented 20 March 2006 at 17:48

File	Size
54913.patch	1.11 KB

Right you are sir, good catch.

Here's the patch.

Comment #2

dopry CreditAttribution: dopry commented 20 March 2006 at 17:48

Assigned:	Unassigned	» dopry
Status:	Active	» Needs review

File	Size
54913_0.patch	1.11 KB

Right you are sir, good catch.

Here's the patch.

Comment #3

nedjo

he/him/his

English

CreditAttribution: nedjo commented 22 March 2006 at 17:05

Looks good in theory (didn't test).

$name => in the patch is superfluous, since not used.

Comment #4

dopry CreditAttribution: dopry commented 23 March 2006 at 05:13

File	Size
54913_1.patch	1.1 KB

always has to be something small...
well here you go I dropped the $name =>...

I'd still prefer to see it testedthan just looked at.

Comment #5

dopry CreditAttribution: dopry commented 23 March 2006 at 05:15

File	Size
54913_2.patch	1.1 KB

What I meant was pretty please test this one...

  wget http://drupal.org/files/issues/54913_2.patch
  patch -p0 < 54913_2.patch

Comment #6

dopry CreditAttribution: dopry commented 23 March 2006 at 05:24

File	Size
54913_3.patch	2.01 KB

And if you're really adventurous try out this one which drops a loop and all those calls to count..
in favor of a boolean, and count variable.

Comment #7

dopry CreditAttribution: dopry commented 23 March 2006 at 05:26

File	Size
54913_4.patch	2.01 KB

Proof that I'm sleep deprived...
I initialized $valid = FALSE, instead of TRUE in that last patch.

Comment #8

rkerr CreditAttribution: rkerr commented 24 March 2006 at 01:43

Well, the patch applies fine. But my upload aren't working with or without the patch, so unfortunately I can't test what sort of difference it makes. :(

Looks about right to me, though.

Comment #9

rkerr CreditAttribution: rkerr commented 24 March 2006 at 02:19

Just tried again and it works!

(Don't seem to be able to upload anything over 2 megs on my test site, but that's another issue).

So, yeah ... good patch :)

Comment #10

killes@www.drop.org CreditAttribution: killes@www.drop.org commented 24 March 2006 at 11:00

Version:	4.7.0-beta6	» x.y.z
Status:	Needs review	» Needs work

The patch removes the special handlinbgof uid 1. While i agree that one could argue for this to be removed, I'd not want to change it this late in the release cycle.

Comment #11

bonobo CreditAttribution: bonobo commented 24 March 2006 at 14:06

This patch worked for me -- I set up three roles, three users, and uploaded a series of files. The least restrictive permission worked --

Can't speak to the UID1 issue Killes raised, however.

Cheers,

Bill

Comment #12

rkerr CreditAttribution: rkerr commented 24 March 2006 at 15:04

Status:

Needs work

» Needs review

File	Size
54913_5.patch	2.06 KB

Attaching updated patch, re-adding uid 1 special case handling.

Comment #13

dopry CreditAttribution: dopry commented 24 March 2006 at 15:35

The uid=1 check I added was redundant cruft...
this code block is already wrapped in a uid 1 check.

Comment #14

dopry CreditAttribution: dopry commented 24 March 2006 at 15:39

Status:

Needs review

» Reviewed & tested by the community

Just fyi... the primary uid =1 check is around line 259 of upload.module...
so I'm going to maintain my +1 for http://drupal.org/files/issues/54913_4.patch
in comment #7.

I'm setting it RTBC.