87404 - Attachments don't work if you need to use user matching

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect, P1)

Description

[Matthew Tuck [:CodeMachine]]

I have an IP that rotates between page loads.  For most things, it's just a
hassle, but when I tried to upload an attachment the other day, the following
happened:

(a) I was logged on, I uploaded the attachment.
(b) I then got a log in screen after I did the upload.
(c) It then complained I had uploaded an empty attachment.

Apparently the attachment got lost.  I retried and it worked OK the second time
(I was lucky with my IP).  Apparently this is difficult or impossible to fix. 
But it indicates that cookieless operation doesn't work at all.

If that's the case, we should either fix it (Dave indicated there was
alternative ways we could do this since we could detect cookieless operation out
front), or detect it and tell them they can't upload attachments.

Comment 1

[Dave Miller [:justdave]]

Just to put it in here before I forget, the alternative I had in mind was to 
detect if they had to log in to view that page...  if they did, 90% probability 
they have cookies off.  Add a password field to the page, and embed their user_id 
in the HTML.  Make them enter their password again on the same screen with 
uploading the attachment.  This is similar to the trick we pulled with the change 
password screen in the user prefs recently.  Unfortunately this wouldn't have 
solved Matty's problem, since his IP changed in the middle of it, and he was 
already logged in prior, so we would not have detected he was running without 
cookies.

Another alternative would be to special-case the login screen if the user has to 
log in in the middle of an attachment submission, and have the file selection 
input on the login screen.  They'd wind up having to pick the file twice, but it 
would work the second time this way.

Yet another is if they have to log in to get to the attachments screen, get the 
login/password and attachment info all at the same time instead of showing the 
normal login screen.  This would get around the problem mentioned above of having 
to select the file twice.

Comment 2

[Zach Lipton [:zach]]

moving

Comment 3

[Dave Miller [:justdave]]

correcting version field lost in product move

Comment 4

[Matthew Tuck [:CodeMachine]]

*** Bug 99590 has been marked as a duplicate of this bug. ***

Comment 5

[Dave Miller [:justdave]]

no patch, not a blocker, 2.16 is now it freeze mode.

-> 2.18

Comment 6

[timeless]

i vote for the last option

Comment 7

[Bradley Baetz (:bbaetz)]

Yeah, I've been bitten by this recently too. I think all of the proposed
solutions are ugly. We should:

a) Apply my non-ip specific patch. This will reduce the impact of this problem
(since you have to confirm_login before you get to the page asking for teh file
in the first place)

b) Change the code which pops up the login to give some sort of error, rather
than prompting, in the case where we will lose some data. Doing this for all
POST is bad - maybe we can check the POST mimetype, or something? This may need
CGI.pm to do properly.

Comment 8

[Dave Miller [:justdave]]

*** Bug 175283 has been marked as a duplicate of this bug. ***

Comment 9

[Bradley Baetz (:bbaetz)]

*** Bug 177246 has been marked as a duplicate of this bug. ***

Comment 10

[Dave Miller [:justdave]]

*** Bug 179298 has been marked as a duplicate of this bug. ***

Comment 11

[Joel Peshkin]

*** Bug 189061 has been marked as a duplicate of this bug. ***

Comment 12

[Bradley Baetz (:bbaetz)]

*** Bug 192599 has been marked as a duplicate of this bug. ***

Comment 13

[Dave Miller [:justdave]]

*** Bug 218356 has been marked as a duplicate of this bug. ***

Comment 14

[Dave Miller [:justdave]]

All 2.18 bugs that haven't been touched in over 60 days and aren't flagged as
blockers are getting pushed out to 2.20

Comment 15

[joey afortunado]

testing

Comment 16

[Dave Miller [:justdave]]

*** Bug 259696 has been marked as a duplicate of this bug. ***

Comment 17

[Dave Miller [:justdave]]

Another thought I had which will probably work way better than anything else
listed here so far, is to just stash the file somewhere when we get it, and
issue a token to identify it with.  Stick that token into the login form or the
user matching form, and then when the form is submitted again, use the token to
retrieve the file from where we stashed it and put it where it really needs to go.

It probably needs to be a table in the database...  the obvious thing is a
temporary folder on disk, but that would end up requiring the resubmit go back
to the same webserver, which might not happen in a load balancing situation with
two Bugzillas on the same database.

Comment 18

[Dave Miller [:justdave]]

OK, Kiko and I just chatted up some ideas on this on IRC...

When the attachment form is initially uploaded, we need to snag the attachment
and stash it prior to checking if the user is logged in.  The idea we're
thinking of is to ensure that the uploader column in the attachments table
allows nulls, and insert the attachment with a null uploader id.  The nullness
indicates it's an incomplete upload.  We then create a unique token that
identifies it (it shouldn't be the attachment id to prevent attachment stealing
- use the existing Token mechanism and create one with the attachment id in the
event data) and stick that token into the form data before checking if the user
is logged in.  When we check if they're logged in, they'll get the login form if
they aren't.  Once they're logged in, usermatching on the request field is
checked.  that form field with the token is still there.  Once we know who they
are, and usermatching is complete, then we can add the user's ID to the uploader
id field to mark it as a completed upload, and dispose of the token.  We should
at some point check for old tokens and unowned attachments and dispose of them
(old = more than an hour?  more than a day?) in case of someone not submitting
the next form.  If someone submits a form that contains a file token that we've
expired, inform them that they took too long to submit the form, and please try
again.  Note that anywhere else in the codebase that deals with attachments
(including queries, and the show_bug page, etc) we need to make sure we're
excluding attachments with a null owner as if they don't exist.

Comment 19

[Christian Reis]

Question: can we store the token usen the hidden-fields template and have them
passed on as necessary? I'm not familiar with how that works.

Comment 20

[Alexandre Michetti Manduca]

Attachment: first try

I think this works but there are many things to improve in this patch.
1 - Do I realy need to use all this trick_taint? (I don't know for sure what is
this trick_taint for).
2 - I'd like to know what in this code should became a function.
3 - I don't know when I need to clean the tokens and the attachments with a
NULL submitter_id. Maybe in user login?

Comment 21

[Myk Melez [:myk] [@mykmelez]]

The proposal in this bug seems like a lot of work and added code complexity for
the benefits it brings.

We could solve the authentication problem by putting the login token into a form
field in the page.  Then, when the user submits the form, they submit the login
token along with it, even if they don't submit a cookie containing the login token.

Putting the token in the form is about as secure as putting it into a cookie. 
It travels with the request and can live on the user's hard drive (in the cache
or cookies.txt) in both cases.  The form field approach could reveal the token
if the user sends the page to someone, but that's unlikely for the "create
attachment" form, and it's not much of a loss even then, since tokens are
IP-address-specific.

We could fix user matching by doing on-the-spot data validation via a request
that doesn't unload the current page.  It could pop up a dialog box of matches
or (even better) populate a drop-down menu in the form with them.  This approach
has the added benefit of being friendlier to users than an intermediate page and
can be used to good effect for other kinds of data validation.

Comment 22

[Christian Reis]

Dave, what's your take here? Should we move in the direction Myk suggests or
just finish reviewing this patch which Already Works?

Comment 23

[Dave Miller [:justdave]]

(In reply to comment #22)
> Dave, what's your take here? Should we move in the direction Myk suggests or
> just finish reviewing this patch which Already Works?

I personally don't see any reason we can't do both.  As much as popular opinion
is starting to head towards dropping support for non-graphical browsers, I still
like the idea of having Bugzilla usable in Lynx, and Lynx doesn't do javascript.
 Having a more intuitive popup window come up instead of an intermediate page
would be cool when javascript is available, but I still think it shouldn't be in
place of a backup system (which as you pointed out, we already have working)
that is usable in Lynx.

Comment 24

[Myk Melez [:myk] [@mykmelez]]

>I still like the idea of having Bugzilla usable in Lynx, and Lynx doesn't
>do javascript.

I didn't propose making Bugzilla unusable in Lynx, I only suggested that the
user matching code not be available to Lynx users.  Lynx usage comprises a very
small percentage of total Bugzilla usage (even though some users, including me,
occasionally use it), and while I agree that Bugzilla should be usable to them,
I don't think that means every feature should be equally usable.

We should rather design the application to degrade gracefully on clients that
don't support JavaScript, just as CSSification is designed to degrade gracefully
on browsers that don't support CSS, and in this case I think having Lynx users
enter exact matches is sufficient support for the "create attachment" form.

In fact, I think not supporting user matching at the moment is better than the
added complexity of a system for storing pending attachments that mucks with the
attachments table data model and requires every query referencing that table to
conditionally exclude these attachments.  I think it makes the code overcomplex,
making development more difficult, hurting performance, and leaving us more
vulnerable to security holes.

Thus, if you must implement a pending attachments feature regardless of what I
think, then please create a separate "pending_attachments" table with an
identical schema to the "attachments" table (except for the userid field, which
"pending_attachments" doesn't need) and put pending attachments into that table
instead.  Then you won't need to touch any existing attachment queries to
exclude the pending attachments, and moving a confirmed attachment into the
permanent attachments table can be as easy as an ANSI standard "INSERT INTO ...
SELECT ..." statement.

And instead of special temporary tokens, please embed the login token into the
form per the second paragraph in comment 21.

Comment 25

[Christian Reis]

(In reply to comment #21)
> We could fix user matching by doing on-the-spot data validation via a request
> that doesn't unload the current page.  It could pop up a dialog box of matches
> or (even better) populate a drop-down menu in the form with them.  This approach
> has the added benefit of being friendlier to users than an intermediate page and
> can be used to good effect for other kinds of data validation.

I suspect there are problems with page history that can occur when we do this
sort of JS processing of form fields (Justdave probably knows better than I
having hacked this for the Ubuntu Bugzilla). If so, I don't think this is a
route we should take. 

Dave, could you take a look at the issue and make a statement as to what should
be done here? We're halted work on this one.

Comment 26

[Dave Miller [:justdave]]

*** Bug 274405 has been marked as a duplicate of this bug. ***

Comment 27

[Dave Miller [:justdave]]

(In reply to comment #25)

> Dave, could you take a look at the issue and make a statement as to what should
> be done here? We're halted work on this one.

I think we should go ahead and do this, but Myk has a real good point in comment
24 about all the queries that would have to change with it like this...  using a
separate table for the temporary attachment storage would be much cleaner, and
it is relatively painless to copy a row between two tables with identical schemas.

Comment 28

[Dave Miller [:justdave]]

*** Bug 274606 has been marked as a duplicate of this bug. ***

Comment 29

[Dave Miller [:justdave]]

Comment on attachment 159646 [details] [diff] [review]
first try

per comment 25, let's do this, but let's use a separate table to store the
"pending" attachments in so we don't have to change the queries all over the
rest of the codebase to ignore them.

Comment 30

[Dave Miller [:justdave]]

Thoughts on implementation:  We can kill two birds with one stone here...  The
pending attachments table should be made as a generic file storage mechanism,
without needing any of the other metadata fields from the attachments table
except perhaps the mime type and the filename (the stuff that's transmitted as
part of the header in the file part of the form upload).  It doesn't need a
description (that'll be preserved by the hidden fields passthrough on the form
re-submit), and it doesn't need an attachment_id.  The primary key for the table
can be the token.  The existing token expiration hooks can deal with deleting
the file from the table when the token expires (or is used to move the
attachment into the attachments table).

What I'm thinking of is also a generic CGI which retrieves arbitrary files out
of this table (token required) for delivery to the end-user.  Think dependency
graphs.  Webdot, local dot, whatever. :)  No more webdot directory on the
filesystem to have to screw with permissions for (one less place apache needs
arbitrary write access to).  This would also quite effectively allow the
dependency graphs to work on a load-balanced setup with multiple application
servers serving Bugzilla (without having to guarantee that the user hits the
same physical machine again when they come back looking for the image file after
downloading the HTML).  We're probably a ways off from needing Bugzilla to be
THAT scalable, but it never hurts to be prepared.  All of this of course doesn't
need to be implimented with this patch, I'm just saying to keep that in mind in
the design of this one. :)

Comment 31

[Frédéric Buclin]

*** Bug 275031 has been marked as a duplicate of this bug. ***

Comment 32

[Dave Miller [:justdave]]

we need to do something with this ASAP for 2.20, either finish this patch up and
get it in, or we'll need to disable user matching on the createattachment screen
prior to release if this isn't ready to go.

Comment 33

[Shane H. W. Travis]

*** Bug 277151 has been marked as a duplicate of this bug. ***

Comment 34

[Boris Zbarsky [:bzbarsky]]

Well, neither option from comment 32 happened, and this bug severely reduces the
usefulness of the "request review" fields on the attachment page...  Since you
never know whether they'll actually work, the default fallback is to simply not
use them.

Comment 35

[Max Kanat-Alexander]

I'm willing to take this. I don't think it will make it for 2.20, though.

So, basically, what I think we'd *like* to do is make a filestorage table, just
two fields: token and contents. Then we should really move the "thedata" field
entirely out of the "attachments" table and replace it with a "token" field.
This could take a *REALLY* long time in checksetup on a larger installation
(moving many GB of data around in the DB), but it will only have to happen once.
We'd have to relnote it.

For security reasons, I suppose the token should be a crypted token based on the
time (ideally to the microsecond), so arbitrary users can't "validate" an
attachment that they didn't upload by guessing a random integer.

I'm willing to change all necessary code to accomplish this entirely.

Comment 36

[Dave Miller [:justdave]]

(In reply to comment #35)
> I'm willing to take this. I don't think it will make it for 2.20, though.

OK, two options then, to get this off the 2.20 blocker list:

a) we back out the patch from bug 254371 (requests on the create attachment
screen) and reopen it, and make this bug block it going back in, or

b) continue to allow requests on the create attachment screen, but don't allow
usermatching for the requestee fields (and say so on the create attachment screen).

(a) will probably be easier to do, but I suspect some folks might fight for it...

Anyone have a preference?

Comment 37

[Max Kanat-Alexander]

I vote for (b).

Comment 38

[Max Kanat-Alexander]

I've filed bug 277838. I'd like to request that we clear blocking2.20+ on this bug.

Comment 39

[Dave Miller [:justdave]]

This is no longer blocking the 2.20 release in deference to bug 277838.

Comment 40

[Dave Miller [:justdave]]

in light of our changed roadmap, this is now blocking 2.20 again.

Comment 41

[Eyal Rozenberg]

For the sake of making this bug easier to find by word search: after the user
matching page, you get a page saying: "You did not specify a file to attach" and
whose title is "no file specified".

Comment 42

[Max Kanat-Alexander]

I'm going to try to solve it this way:

If we match only one user for all the fields, everything is good. If we match
more than one user, we throw an error and tell them to go back.

Comment 43

[Myk Melez [:myk] [@mykmelez]]

"If it's not a regression from 2.18 and it's not a critical problem with
something that's already landed, let's push it off." - Dave

Comment 44

[Max Kanat-Alexander]

OK, here's how I think I'm going to solve this for 2.20:

If we fail a match, or the match matches more than one user, we're going to
simply skip the flag change entirely, and put a message on the process_bug page.
I think that's at least much better than what happens now.

We can't just fail the upload, because somebody could have wasted a significant
amount of time trying to upload the file, which they would then have to do again.

I'll also put a brief explanation on the review page that user-matching doesn't
really work, and shouldn't be used.

Comment 45

[timeless]

*** Bug 293349 has been marked as a duplicate of this bug. ***

Comment 46

[Frédéric Buclin]

(In reply to comment #44)
> If we fail a match, or the match matches more than one user, we're going to
> simply skip the flag change entirely, and put a message on the process_bug page.

Don't skip the flag change entirely, just skip the requestee and make the
request "from the wind". It's a much better solution. Moreover, I don't think a
message in show_bug is helpful. I would do it silently.

This partial fix is straightforward! ;)

Comment 47

[Dave Miller [:justdave]]

<justdave> how about doing the usermatching *after* we add the attachment?
<justdave> i.e. insert the attachment, put up the attachment submitted, then put
the usermatching for the request underneath it
<justdave> and just process the request separately (submit to process_bug or
something) if it didn't match the first time through
<LpSolit> what happens if the submitter stop the process at this level? are
emails sent?
<justdave> email about attachment added would be, but not about the requests.
<justdave> since it would still be two separate transactions.
<justdave> defeats the purpose of combining emails, but the user's intent
succeeds at least

Comment 48

[Max Kanat-Alexander]

Attachment: Skip Confirmation Screen, v1

This patch just skips the confirmation screen if a requestee doesn't match
anyone, or if the requestee matches more than one user. Instead, it will just
be a blank request with no user, and a message will be displayed to the user
telling them what we've done.

I've tested it, and it works on landfill.

My only thought is that perhaps the message displayed could be better. Any
ideas?

Comment 49

[Frédéric Buclin]

Comment on attachment 187264 [details] [diff] [review]
Skip Confirmation Screen, v1

>+    my $match_success = Bugzilla::User::match_field($cgi, {
>+        '^requestee(_type)?-(\d+)$' => { 'type' => 'single' },
>+    }, "skip confirmation");
>+    $vars->{'message'} = 'user_match_failed' if !$match_success;

You don't like magic numbers and I don't like magic strings. I'd prefer a
SKIP_CONFIRMATION constant set to 1. Who knows, maybe one day some additional
values would be required (I could imagine a RETURN_FIRST_BY_DEFAULT).

Moreover, match_field() should be more specific about the result, i.e. make the
distinction between no match found and more than one match found. So I suggest
returning '-1' when no match is found, '0' when exactly one match is found, and
'1' when more than one match are found and renaming your param as $match_status
or something equivalent. You now need a second message: 'user_match_failed' and
'too_many_matches'.


>+# You also have the choice of *never* displaying the confirmation screen.
>+# In this case, match_field will return 1 if the match was successful,
>+# and 0 if it wasn't. To make match_field behave this way, pass a "true"
>+# value as the *third* parameter.

This comment has to be updated accordingly.


> sub match_field {
>     my $cgi          = shift;   # CGI object to look up fields in
>     my $fields       = shift;   # arguments as a hash
>+    my $skip_confirm = shift;

Add a short comment here too.


>-    return 1 unless $need_confirm; # skip confirmation if not needed.
>+    # We never need confirmation if skip_confirm is true.
>+    $need_confirm = 0 if $skip_confirm;
>+
>+    # skip confirmation if not needed.
>+    return $matchsuccess unless $need_confirm;

This is a hack. I won't let you altering $need_confirm. ;) Write:
return $matchsuccess if ($skip_confirm || !$need_confirm);


>+  [% ELSIF message_tag == "user_match_failed" %]
>+    You entered a username that did not match any known 
>+    [% terms.Bugzilla %] users, so we have instead left
>+    that field blank.

A second message relative to too many matches has to be added here as well.


I did not test your patch yet, but it looks great, really! :)

Comment 50

[Max Kanat-Alexander]

> You don't like magic numbers and I don't like magic strings. I'd prefer a
> SKIP_CONFIRMATION constant set to 1. Who knows, maybe one day some additional
> values would be required (I could imagine a RETURN_FIRST_BY_DEFAULT).

  OK, agreed. :-)

> Moreover, match_field() should be more specific about the result, i.e. make 
> the distinction between no match found and more than one match found. So I 
> suggest returning '-1' when no match is found, '0' when exactly one match is 
> found, and '1' when more than one match are found and renaming your param as 
> $match_status or something equivalent. You now need a second message: 
> 'user_match_failed' and 'too_many_matches'.

  match_field doesn't work on one field at a time. match_field parses *every*
requestee entry on the page, no matter how many there are. I thought about
modifying the function to return info about how the match failed (a hash), but
that would have been a complex, invasive change to the function.

> return $matchsuccess if ($skip_confirm || !$need_confirm);

  OK. I think the way I wrote it is *clearer*, but this is less hacky.

> I did not test your patch yet, but it looks great, really! :)

  Thank you! :-)

Comment 51

[Frédéric Buclin]

(In reply to comment #50)
>   match_field doesn't work on one field at a time. match_field parses *every*
> requestee entry on the page, no matter how many there are. I thought about
> modifying the function to return info about how the match failed (a hash), but
> that would have been a complex, invasive change to the function.

You are right. But based on $matchsuccess and $need_confirm, you can already get
interesting results which would be fine to report to the user:

1) $need_confirm = 0 => everything is fine;
2) $need_confirm = 1 && $matchsuccess = 0 => at least one field did not match
anything;
3) $need_confirm = 1 && $matchsuccess = 1 => all fields match, but some of them
with more than one user.

Then the two messages I suggested, 'user_match_failed' and 'too_many_matches',
would be based on cases 2) and 3) respectively. No need for a hash here.

An easy rule based on cases 1) - 3) above and my comment 49 is:

return (2 * $matchsuccess - $need_confirm) if (!$need_confirm ||
$skip_confirmation);

Returned values are 0, -1 and 1 respectively, as desired.

The next step would be to mention which fields are affected, but 1) this would
be part of another bug, and 2) I'm not sure it worths doing it.

Comment 52

[Max Kanat-Alexander]

Attachment: v2

OK, I've modifed it as you suggested. I agree, it really is better this way.
:-)

Comment 53

[Frédéric Buclin]

Comment on attachment 187848 [details] [diff] [review]
v2

>+    if ($match_multiple) {
>+        $retval = USER_MATCH_MULTIPLE;
>+    }
>+    elsif (!$matchsuccess) {
>+        $retval = USER_MATCH_FAILED;
>+    }
>+    else {
>+        $retval = USER_MATCH_SUCCESS;
>+    }

USER_MATCH_FAILED should take precendence over USER_MATCH_MULTIPLE.


>+=item C<USER_MATCH_MULTIPLE>
>+
>+Returned by C<match_field()> when at least one field matched more than 
>+one user, but no matches failed.
>+
>+=item C<USER_MATCH_FAILED>
>+
>+Returned by C<match_field()> when at least one field failed to match 
>+anything.

The actual code does not reflect what you write in the POD docs.


Check (!$matchsuccess) first have you have my r+!

Comment 54

[Max Kanat-Alexander]

Attachment: v3

OK, FAILED is first, and I fixed the undef error LpSolit pointed out on IRC.

Comment 55

[Frédéric Buclin]

Comment on attachment 187871 [details] [diff] [review]
v3

r=LpSolit

Comment 56

[Dave Miller [:justdave]]

OK, so what we have here is a workaround for the second of the two issues listed
in the summary of this bug, which doesn't solve the first issue at all, and
nothing in this patch precludes or conflicts with the potential to be able to
fix the two issues outright.  So in theory, this really ought to be on another
bug and this bug should stay open, since the issues this bug covers aren't
really getting solved...

Comment 57

[Frédéric Buclin]

OK, what we could do to fix the second issue as well is to create a token when
the user access the "Create a New Attachment" page. When he authenticate, a
token is created and saved in the 'tokens' table, using
Token::IssueSessionToken(). When the user submits his new attachment, we first
check his identity using cookies. If the user has cookies turned off, we fall
back to the token passed as a hidden field and we check its validity using
Token::GetTokenData(). If this token exists and is not "too" old (I suggest a
lifetime of 30-60 minutes), we accept the submission and use the userID returned
by Token::GetTokenData(), else an error message is displayed. When the
submission is complete, we remove the token using Token::DeleteToken().

Comment 58

[Max Kanat-Alexander]

I've filed bug 299405 for the other half of this. I think it's a more minor
issue, since there are fewer users who need to log in on every page, or who lose
their login before submitting the attachment.

Comment 59

[Max Kanat-Alexander]

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.88; previous revision: 1.87
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.58; previous revision: 1.57
done
Checking in template/en/default/global/messages.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/messages.html.tmpl,v
 <--  messages.html.tmpl
new revision: 1.30; previous revision: 1.29
done