305136 - (bz-postbugpm) Make Bug.pm able to create bugs--move most post_bug.cgi code into Bug.pm

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement, P2)

Description

[André Batosti]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.10) Gecko/20050725 Firefox/1.0.6 (Ubuntu package 1.0.6)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.10) Gecko/20050725 Firefox/1.0.6 (Ubuntu package 1.0.6)

move the code of the post_bug.cgi to Bugzilla::Bug.pm to clean the post bug and
in the future combine it on process_bug.cgi. this bug must be a blocker of bug
174988.

Reproducible: Always

Comment 1

[André Batosti]

Attachment: batosti_v1

moving the code and changing the older SendSQL functions to dbh functions.

Comment 2

[Max Kanat-Alexander]

Comment on attachment 194381 [details] [diff] [review]
batosti_v1

Before I review this -- could you explain why it's OK to change the order of
things in post_bug? That is, you remove code in one place, and add back similar
code in another place.

But in general, just looking at the patch, this patch looks great! :-)

Comment 3

[André Batosti]

it's a part of bug 174988 to join bugs functions in Bug.pm and clean up post_bug
and process_bug and then use the this functions in the future.

Comment 4

[Max Kanat-Alexander]

(In reply to comment #3)
> it's a part of bug 174988 to join bugs functions in Bug.pm and clean up post_bug
> and process_bug and then use the this functions in the future.

  Yes, I know that. My question is:

  Why did you change the *order* of the code?

Comment 5

[André Batosti]

(In reply to comment #4)
>   Why did you change the *order* of the code?

some new methods of Bug.pm only can be called if you have a bug object and i
change the code to be called after the object is created.

Comment 6

[Frédéric Buclin]

Comment on attachment 194381 [details] [diff] [review]
batosti_v1

>Index: post_bug.cgi

> # Add the bug report to the DB.
> $dbh->bz_lock_tables('bugs WRITE', 'bug_group_map WRITE', 'longdescs WRITE',
>                      'cc WRITE', 'keywords WRITE', 'dependencies WRITE',

>-# Add the group restrictions
>-foreach my $grouptoadd (@groupstoadd) {
>-    SendSQL("INSERT INTO bug_group_map (bug_id, group_id)
>-             VALUES ($id, $grouptoadd)");
>-}

> # All fields related to the newly created bug are set.
> # The bug can now be made accessible.

>+# Groups
>+my @groupstoadd = ();
>+foreach my $b (grep(/^bit-\d*$/, $cgi->param())) {
>+    if ($cgi->param($b)) {
>+        my $v = substr($b, 4);
>+        push @groupstoadd, $v;
>+    }
>+}
>+
>+$bug->add_groups(@groupstoadd);


The order of the different checks/validations/insertions into the DB in
post_bug.cgi is important and I ask you to respect it. Two important things
about your patch:

1) *all* validations must have been done *before* inserting the bug into the
DB. You moved such validations after the insertion of the bug for at least
groups, dependencies and users on the CC list, which all contain a
ThrowUserError(), meaning that the implementation of the bug into the DB may be
interrupted before being complete.

2) the part of your patch which I show here has security implications.
Actually, groups are validated *before* the bug is being inserted into the DB
and required group restrictions are inserted into the bug_group_map table
*before* the bug is being *unlocked*, see the security bug 292544! Here, you
move these security checks after the bug is being released, making the bug
publicly accessible!

Comment 7

[André Batosti]

Attachment: batosti_v1_fix

i splited the functions of add groups and keywords in two. one for checking
before put bug in DB and the add function after have a bug.

Comment 8

[Joel Peshkin]

Comment on attachment 194682 [details] [diff] [review]
batosti_v1_fix


I'm just reviewing the Bug.pm additions for now, but there are a couple of
fixes needed.

All of these routines need POD documents




>+    my $dbh = Bugzilla->dbh;
>+
>+    foreach my $person (@add_cc) {
>+        if (lsearch($self->cc, $person)<0) {
>+            my $ccid = &::DBNameToIdAndCheck($person);

There is a user_to_id function in User.pm for this.

>+
>+sub check_groups {
>+    my ($user, $product_id, $cgi) = (@_);
>+
>+    my $dbh = Bugzilla->dbh;
>+
>+    my @groups = ();
>+    foreach my $b (grep(/^bit-\d*$/, $cgi->param())) {
>+        if ($cgi->param($b)) {
>+            my $v = substr($b, 4);
>+            push @groups, $v;
>+        }

Let's not pass cgi objects to Bug.pm or expect form names to be part of the
interface.  How about passing @groups to check_groups instead?

Comment 9

[Frédéric Buclin]

Comment on attachment 194682 [details] [diff] [review]
batosti_v1_fix

I'll wait for an updated patch

Comment 10

[André Batosti]

Attachment: batosti_v2

no $cgi to the Bug functions and with POD docs.

Comment 11

[Max Kanat-Alexander]

Comment on attachment 194939 [details] [diff] [review]
batosti_v2

  OK, I don't know enough about post_bug to review that part, but I have some comments on the Bug.pm part.

>Index: Bugzilla/Bug.pm
>@@ -687,6 +689,84 @@ sub choices {
>     return $self->{'choices'};
> }
> 
>+sub add_cc_list {
>+    my $self = shift;
>+    my (@add_cc) = @_;

  I think the functions should take a *reference* to an array instead of taking the array directly. That way we can add more params later if we want. That goes for all the functions.

>+    return if !@add_cc;

  This isn't really necessary, because the function doesn't do anything if @add_cc is empty...

>+    my $dbh = Bugzilla->dbh;
>+
>+    foreach my $person (@add_cc) {
>+        if (lsearch($self->cc, $person)<0) {

  How is $self->cc going to work when the bug doesn't even exist yet? Or does the bug somehow already exist?

>+            my $ccid = login_to_id($person);
>+            if ($ccid) {

  That looks like it'll just hide errors -- any $person passed into this function should always be a valid $person, since we will have already done the user-matching, right?

>+        $dbh->do("INSERT INTO bug_group_map (bug_id, group_id)
>+                 VALUES (?, ?)", undef, $self->{'bug_id'}, $group);

  Isn't there a $self->bug_id instead of {'bug_id'}? Or $self->id or something?

>+sub add_keywords {
>+    my $self = shift;
>+    my @keywords = (@_); 
>+
>+    my $dbh = Bugzilla->dbh;
>+
>+    if (@keywords) {
>+        foreach my $keyword (@keywords) {
>+            $dbh->do("INSERT INTO keywords (bug_id, keywordid) 
>+                     VALUES ( ?, ?)", undef, $self->{'bug_id'}, 
>+                     $keyword);
>+        }

  This is going to throw taint errors all over the place. That's probably true of all these functions, actually.

>+sub check_groups {
>+    my ($user, $product_id, $groups) = (@_);
>+
>+    my $dbh = Bugzilla->dbh;
>+
>+    my $sth = $dbh->prepare("SELECT DISTINCT groups.id, 
>+                                             groups.name,
>+                                             membercontrol, 
>+                                             othercontrol, 
>+                                             description
>+                            FROM groups 
>+                            LEFT JOIN group_control_map 
>+                            ON group_id = id AND 
>+                               product_id = ?
>+                            WHERE isbuggroup != 0 AND 
>+                                  isactive != 0 
>+                            ORDER BY description");

  Isn't there some Group.pm function that will do that for you??

>+    $sth->execute($product_id);
>+    while (my ($id, $groupname, $membercontrol, $othercontrol) =
>+           $sth->fetchrow_array) {
>+        $membercontrol ||= 0;
>+        $othercontrol ||= 0;
>+        # Add groups required
>+        if (($membercontrol == CONTROLMAPMANDATORY)
>+           || (($othercontrol == CONTROLMAPMANDATORY) 
>+                && (!UserInGroup($groupname)))) {
>+            # User had no option, bug needs to be in this group.
>+            push(@$groups, $id)
>+        }
>+    }

  This whole section needs some comments, I have almost no idea what's going on.


  Otherwise, I have no general architectural problems with this. I think LpSolit would probably have better comments on it than I do, since he probably knows more about post_bug.

  Your POD docs need to describe *why* the check_ functions return arrays.

Comment 12

[André Batosti]

Attachment: batosti_v3

Comment 13

[Max Kanat-Alexander]

This bug needs to be split into multiple bugs, one for each function that we're moving. It's too overwhelming to review the whole patch at once.

Comment 14

[Max Kanat-Alexander]

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

Comment 15

[Max Kanat-Alexander]

Comment on attachment 203533 [details] [diff] [review]
batosti_v3

Okay, this needs to be split into the bugs that are now blocking this bug.

Also, that check_groups function does too many things. Some of its functions should be in Bugzilla::Group, and it shouldn't have to take an $user argument -- the Bugzilla::Bug already has a built-in $self->user.

Comment 16

[Max Kanat-Alexander]

I'm actually going to take this over. I have some ideas as to how to do it, and I want it to get in for Bugzilla 3.0.

Comment 17

[Max Kanat-Alexander]

Okay, here's how we're going to do this, actually:

1) We're going to use Bugzilla::Object->create.

2) We're going to implement the validators one at a time, in the blocking
   bugs, by moving code from post_bug into Bugzilla::Bug.

3) Once all the validators are implemented, we'll change all the DB-updating 
   code from post_bug into a call to Bugzilla::Bug->create.

Comment 18

[Frédéric Buclin]

Done! The next step is process_bug.cgi. :)