373660 - Move dependency checking from process_bug into Bugzilla::Bug

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Max Kanat-Alexander]

The next step of the process_bug re-write is to move the code that validates the dependson and blocks fields into Bugzilla::Bug. This should be fairly easy, since the validation code for those is already there.

Comment 1

[Max Kanat-Alexander]

Attachment: v1

Okay, here we go. I've tested this on landfill with some basic tests, and it seems to work. (I didn't test the dumb strict_isolation stuff, though.)

Comment 2

[Frédéric Buclin]

Comment on attachment 258335 [details] [diff] [review]
v1

>Index: Bugzilla/Bug.pm

>+        # For existing bugs, the unchanged bug_ids are only checked for
>+        # existence

Nit: I don't think you should care about deleted bugs here, especially because this should not happen. Let sanitycheck.cgi manages this.


>+            my $old = $invocant->$type;
>+            my ($removed, $added) = diff_arrays($old, \@bug_ids);

This won't work if @bug_ids contains aliases. process_bug.cgi first calls ValidateBugID() before comparing arrays because ValidateBugID() converts aliases to ID.


>+            # Figure out which bug_ids haven't been changed.
>+            # Here's an example that explains why this works:
>+            # If "1, 2, 3, 4" becomes "1, 3, 4, 5"
>+            # then removed is 2 and added is 5.
>+            # We see that 1, 3, and 4 didn't change.
>+            # if we compare "1, 2, 3, 4" to "2, 5", removed is "1, 3, 4"
>+            my ($unchanged) = diff_arrays($old, $changed);

You don't need all this at all, see below.


>+                # XXX I think this is kind of dumb, but this is what
>+                # process_bug used to do, so we're doing it here, still.

Please remove this comment. If you are not allowed to edit bugs being in a given product, then this also involves dependent bugs. That's definitely what is expected here. Being able to view the bug has nothing to do here.


>+                # Under strict isolation, you can't modify a bug ever,
>+                # even if you can see it.

This should be reworded to say you cannot modify it if you cannot *edit* the bug.


>+                # We only actually add added bugs, though.
>+                if (grep($_ eq $modified_id, @$added)) {
>+                    push(@result, $modified_id);
>+                }

You only care about $added defined above. If you called ValidateBugID on time, you would simply write @result = (@unchanged, @$added).


>+            foreach my $unchanged_id (@$unchanged) {
>+                # Make sure this bug still exists. If it doesn't, then
>+                # silently discard it.
>+                my $bug = new Bugzilla::Bug($unchanged_id) || next;

Nit: I don't think you should do it here, even if it doesn't hurt.


>+                # We do this so the result array contains only ids, never
>+                # aliases.
>+                push(@result, $bug->id);

No, it's impossible to have aliases here, see the TYPE of columns in the dependencies table. So we already know we have valid integers as they come from the DB.

Comment 3

[Max Kanat-Alexander]

Attachment: v2

Okay, I read all your comments, and I understand them.

You're right about the fact that unchanged bug_ids can never be aliases, so I simplified that code.

Apparently nowadays "strict_isolation" means WHEVER WE DECIDE RANDOMLY IN OUR TWISTED MINDS, instead of what the parameter description says. That's why I had that XXX there. :-) But I did remove it, and I modified the comment like you said. That is, I agree that it makes sense that you can't edit those things, but why should that be restricted to strict_isolation, which has to do with bug visibility?


The reason I made all of those other changes is that I don't really like using the "field" parameter of ValidateBugID at all, particularly not to skip certain parts of the code. I much prefer to have the security-control code be obviously in _check_dependencies. That is, I prefer the way the code is that I've written. There won't be any speed difference, it's just a bit more code.

Comment 4

[Max Kanat-Alexander]

Oh, also, if somebody adds an alias to the field that's the same as a bug id already there, and they don't have access to that alias id...then we can throw an error, that's fine.

Oh, but maybe we should make sure that all of the ids in @result are unique.

Comment 5

[Teemu Mannermaa (:wicked)]

(In reply to comment #3)
> Apparently nowadays "strict_isolation" means WHEVER WE DECIDE RANDOMLY IN OUR
> TWISTED MINDS, instead of what the parameter description says. That's why I 
...
> but why should that be restricted to strict_isolation, which has to do with
> bug visibility?

No, strict_isolation is about separating products. Just like it's description says:
 "Don't allow users to be assigned to, be qa-contacts on, be added to CC list, or make or remove dependencies involving any bug that is in a product on which that user is forbidden to edit."
AFAIU, it's an extension to group security so that users can't work around group restrictions. That way users can't be given access to bugs in products that logically belongs to some other group of people, department or company.

Are you perhaps thinking usevisibilitygroups or useentrygroupdefault params? Although, first one is about making sure a user only sees other users belonging to same group and second one is about deciding what products are available in enter_bug.

Comment 6

[Max Kanat-Alexander]

(In reply to comment #5)
> (In reply to comment #3)
> or make or remove dependencies involving any bug that is in a product on which

  Ah, okay. :-) You're right! Never mind, then.

Comment 7

[Frédéric Buclin]

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

>Index: Bugzilla/Bug.pm

>+        # Eliminate nulls.
>+        @bug_ids = grep {$_} @bug_ids;


>+            my ($removed, $added) = diff_arrays($old, \@bug_ids);

>+                # We only actually add added bugs, though.
>+                if (grep($_ eq $modified_id, @$added)) {
>+                    push(@result, $modified_id);
>+                }

This doesn't work. Now you can no longer use bug aliases because $modified_id has been updated by ValidateBugID() (which converts bug aliases to their ID), but $added still refers to the "not yet converted" list.


>+        else {
>+            ValidateBugID($_) foreach (@{$deps_in{$type}});
>+        }

You have to use @bug_ids defined above as it has nulls being removed. Else you get errors like:

Can't use string (",,,,,,,hwad,,,,,,,   ") as an ARRAY ref while "strict refs" in use at Bugzilla/Bug.pm line 771.

Moreover, entering a valid bug ID doesn't work either:

Can't use string ("1") as an ARRAY ref while "strict refs" in use at Bugzilla/Bug.pm line 771.


Nit: I really dislike the use of $unchanged. I hope there is a nicer way to do it.

Comment 8

[Max Kanat-Alexander]

Attachment: v3

Okay, I made it work more like the old process_bug.cgi code.

Comment 9

[Frédéric Buclin]

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

Works fine. r=LpSolit

Comment 10

[Frédéric Buclin]

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

>Index: Bugzilla/Bug.pm

>+        foreach my $modified_id (@check_access) {

>+                my $user      = Bugzilla->user;

Nit: move the $user definition out of the foreach loop.

Comment 11

[Max Kanat-Alexander]

Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.361; previous revision: 1.360
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.179; previous revision: 1.178
done