Bug 454251: Implement Bugzilla::Attachment->create() and $attachment->update() -…

Bugzilla/Constants.pm

@@ -148,6 +148,7 @@ use File::Basename;
     MAX_LOGINCOOKIE_AGE
 
     SAFE_PROTOCOLS
+    LEGAL_CONTENT_TYPES
 
     MIN_SMALLINT
     MAX_SMALLINT
@@ -236,18 +237,6 @@ use constant LOGOUT_ALL => 0;
 use constant LOGOUT_CURRENT => 1;
 use constant LOGOUT_KEEP_CURRENT => 2;
 
-use constant contenttypes =>
-  {
-   "html"=> "text/html" , 
-   "rdf" => "application/rdf+xml" , 
-   "atom"=> "application/atom+xml" ,
-   "xml" => "application/xml" , 
-   "js"  => "application/x-javascript" , 
-   "csv" => "text/csv" ,
-   "png" => "image/png" ,
-   "ics" => "text/calendar" ,
-  };
-
 use constant GRANT_DIRECT => 0;
 use constant GRANT_REGEXP => 2;
 
@@ -377,6 +366,22 @@ use constant SAFE_PROTOCOLS => ('afs', 'cid', 'ftp', 'gopher', 'http', 'https',
                                 'irc', 'mid', 'news', 'nntp', 'prospero', 'telnet',
                                 'view-source', 'wais');
 
+# Valid MIME types for attachments.
+use constant LEGAL_CONTENT_TYPES => ('application', 'audio', 'image', 'message',
+                                     'model', 'multipart', 'text', 'video');
+
+use constant contenttypes =>
+  {
+   "html"=> "text/html" ,
+   "rdf" => "application/rdf+xml" ,
+   "atom"=> "application/atom+xml" ,
+   "xml" => "application/xml" ,
+   "js"  => "application/x-javascript" ,
+   "csv" => "text/csv" ,
+   "png" => "image/png" ,
+   "ics" => "text/calendar" ,
+  };
+
 # Usage modes. Default USAGE_MODE_BROWSER. Use with Bugzilla->usage_mode.
 use constant USAGE_MODE_BROWSER    => 0;
 use constant USAGE_MODE_CMDLINE    => 1;

Bugzilla/Flag.pm

@@ -1057,6 +1057,46 @@ sub FormToNewFlags {
     return \@flags;
 }
 
+# This is a helper to set flags on a new bug or attachment.
+# For existing bugs and attachments, errors must be reported.
+sub set_flags {
+    my ($class, $bug, $attachment, $timestamp, $vars) = @_;
+    my $cgi = Bugzilla->cgi;
+
+    # The order of these function calls is important, as Flag::validate
+    # assumes User::match_field has ensured that the
+    # values in the requestee fields are legitimate user email addresses.
+    my $match_status = Bugzilla::User::match_field($cgi, {
+        '^requestee(_type)?-(\d+)$' => { 'type' => 'multi' },
+    }, MATCH_SKIP_CONFIRM);
+
+    $vars->{'match_field'} = 'requestee';
+    if ($match_status == USER_MATCH_FAILED) {
+        $vars->{'message'} = 'user_match_failed';
+    }
+    elsif ($match_status == USER_MATCH_MULTIPLE) {
+        $vars->{'message'} = 'user_match_multiple';
+    }
+
+    # 1. Add flags, if any. To avoid dying if something goes wrong
+    # while processing flags, we will eval() flag validation.
+    #
+    # 2. Flag::validate() should not detect any reference to existing flags
+    # when creating a new attachment. Setting the third param to -1 will
+    # force this function to check this point.
+    my $error_mode_cache = Bugzilla->error_mode;
+    Bugzilla->error_mode(ERROR_MODE_DIE);
+    eval {
+        validate($bug->bug_id, $attachment ? -1 : undef, SKIP_REQUESTEE_ON_ERROR);
+        $class->process($bug, $attachment, $timestamp, $vars);
+    };
+    Bugzilla->error_mode($error_mode_cache);
+    if ($@) {
+        $vars->{'message'} = 'flag_creation_failed';
+        $vars->{'flag_creation_error'} = $@;
+    }
+}
+
 =pod
 
 =over

post_bug.cgi

@@ -85,12 +85,10 @@ if ($token) {
 }    
 
 # do a match on the fields if applicable
-
-&Bugzilla::User::match_field ($cgi, {
+Bugzilla::User::match_field ($cgi, {
     'cc'            => { 'type' => 'multi'  },
     'assigned_to'   => { 'type' => 'single' },
     'qa_contact'    => { 'type' => 'single' },
-    '^requestee_type-(\d+)$' => { 'type' => 'multi' },
 });
 
 if (defined $cgi->param('maketemplate')) {
@@ -194,10 +192,35 @@ if (defined $cgi->param('version')) {
 # Add an attachment if requested.
 if (defined($cgi->upload('data')) || $cgi->param('attachurl')) {
     $cgi->param('isprivate', $cgi->param('commentprivacy'));
-    my $attachment = Bugzilla::Attachment->create(!THROW_ERROR,
-                                                  $bug, $user, $timestamp, $vars);
+
+    # Must be called before create() as it may alter $cgi->param('ispatch').
+    my $content_type = Bugzilla::Attachment::get_content_type();
+    my $attachment;
+
+    # If the attachment cannot be successfully added to the bug,
+    # we notify the user, but we don't interrupt the bug creation process.
+    my $error_mode_cache = Bugzilla->error_mode;
+    Bugzilla->error_mode(ERROR_MODE_DIE);
+    eval {
+        $attachment = Bugzilla::Attachment->create(
+            {bug           => $bug,
+             creation_ts   => $timestamp,
+             data          => scalar $cgi->param('attachurl') || $cgi->upload('data'),
+             description   => scalar $cgi->param('description'),
+             filename      => $cgi->param('attachurl') ? '' : scalar $cgi->upload('data'),
+             ispatch       => scalar $cgi->param('ispatch'),
+             isprivate     => scalar $cgi->param('isprivate'),
+             isurl         => scalar $cgi->param('attachurl'),
+             mimetype      => $content_type,
+             store_in_file => scalar $cgi->param('bigfile'),
+            });
+    };
+    Bugzilla->error_mode($error_mode_cache);
 
     if ($attachment) {
+        # Set attachment flags.
+        Bugzilla::Flag->set_flags($bug, $attachment, $timestamp, $vars);
+
         # Update the comment to include the new attachment ID.
         # This string is hardcoded here because Template::quoteUrls()
         # expects to find this exact string.
@@ -222,22 +245,8 @@ if (defined($cgi->upload('data')) || $cgi->param('attachurl')) {
     };
 }
 
-# Add flags, if any. To avoid dying if something goes wrong
-# while processing flags, we will eval() flag validation.
-# This requires errors to die().
-# XXX: this can go away as soon as flag validation is able to
-#      fail without dying.
-my $error_mode_cache = Bugzilla->error_mode;
-Bugzilla->error_mode(ERROR_MODE_DIE);
-eval {
-    Bugzilla::Flag::validate($id, undef, SKIP_REQUESTEE_ON_ERROR);
-    Bugzilla::Flag->process($bug, undef, $timestamp, $vars);
-};
-Bugzilla->error_mode($error_mode_cache);
-if ($@) {
-    $vars->{'message'} = 'flag_creation_failed';
-    $vars->{'flag_creation_error'} = $@;
-}
+# Set bug flags.
+Bugzilla::Flag->set_flags($bug, undef, $timestamp, $vars);
 
 # Email everyone the details of the new bug 
 $vars->{'mailrecipients'} = {'changer' => $user->login};

template/en/default/global/code-error.html.tmpl

@@ -47,6 +47,14 @@
     Attachment #[% attach_id FILTER html %] ([% description FILTER html %]) 
     is already obsolete.
 
+  [% ELSIF error == "attachment_local_storage_disabled" %]
+    [% title = "Local Storage Disabled" %]
+    You cannot store attachments locally. This feature is disabled.
+
+  [% ELSIF error == "attachment_url_disabled" %]
+    [% title = "Attachment URL Disabled" %]
+    You cannot attach a URL. This feature is currently disabled.
+
   [% ELSIF error == "auth_invalid_email" %]
     [% title = "Invalid Email Address" %]
     We received an email address (<b>[% addr FILTER html %]</b>)

template/en/default/global/user-error.html.tmpl

@@ -211,6 +211,11 @@
     [% title = "Attachment Deletion Disabled" %]
     Attachment deletion is disabled on this installation.
 
+  [% ELSIF error == "attachment_illegal_url" %]
+    [% title = "Illegal Attachment URL" %]
+    <em>[% url FILTER html %]</em> is not a legal URL for attachments.
+    It must start either with http://, https:// or ftp://.
+
   [% ELSIF error == "attachment_removed" %]
     [% title = "Attachment Removed" %]
     The attachment you are attempting to access has been removed.
@@ -875,8 +880,7 @@
     [% title = "Invalid Content-Type" %]
     The content type <em>[% contenttype FILTER html %]</em> is invalid.
     Valid types must be of the form <em>foo/bar</em> where <em>foo</em>
-    is either <em>application, audio, image, message, model, multipart,
-    text,</em> or <em>video</em>.
+    is one of <em>[% constants.LEGAL_CONTENT_TYPES.join(', ') FILTER html %]</em>.
     
   [% ELSIF error == "invalid_context" %]
     [% title = "Invalid Context" %]
@@ -1615,6 +1619,11 @@
     <tt>[% name FILTER html %]</tt> does not exist or you are not allowed 
     to see that user.
 
+  [% ELSIF error == "user_not_insider" %]
+    [% title = "User Not In Insidergroup" %]
+    Sorry, but you are not allowed to (un)mark comments or attachments
+    as private.
+
   [% ELSIF error == "votes_must_be_nonnegative" %]
     [% title = "Votes Must Be Non-negative" %]
     [% admindocslinks = {'voting.html' => 'Setting up the voting feature'} %]