256135 - Parameter 'movers' is handled differently in Bug.pm and buglist.cgi

Status: RESOLVED FIXED

(Bugzilla :: Bug Import/Export & Moving, defect)

Description

[Egil Kvaleberg]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040626 Firefox/0.9.1
Build Identifier: 

Presumably 'movers' is intended to be a list of logins allowed the 'Move' privilege.

In the bug form, 'movers' only works when it consists of a single login.

In the bug list form, 'movers' only works if it is a list (a single login does
in fact not work).

Reproducible: Always
Steps to Reproduce:
1. Enable move mechanism and define 'movers'
2. Edit bug or many bugs


Actual Results:  
The MOVE button does not appear as expected. See 'Details'


Patch attached.

A Perl person should verify that it is sane.

Comment 1

[Egil Kvaleberg]

Attachment: Patch to fix the problem

Comment 2

[Dave Miller [:justdave]]

Reassigning bugs that I'm not actively working on to the default component owner
in order to try to make some sanity out of my personal buglist.  This doesn't
mean the bug isn't being dealt with, just that I'm not the one doing it.  If you
are dealing with this bug, please assign it to yourself.

Comment 3

[Dave Miller [:justdave]]

*** This bug has been marked as a duplicate of 106918 ***

Comment 4

[Dave Miller [:justdave]]

actually, this isn't...  buglist wasn't fixed on the other bug, so we had it
different in three different places.

This patch appears to have other stuff mixed in with it though, and bug_form.pl
no longer exists...

but the bug is valid.

Comment 5

[Dave Miller [:justdave]]

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

Comment 6

[Frédéric Buclin]

Attachment: patch, v1

I have copied this part of the code from process_bug.cgi (previously move.pl
before the merge of the two files).

Comment 7

[Frédéric Buclin]

Comment on attachment 193527 [details] [diff] [review]
patch, v1

oops, I forgot to check whether Param('move-enabled') is turned on.

Comment 8

[Frédéric Buclin]

Attachment: patch, v2

Param('move-enabled') is now checked. :)

Comment 9

[Frédéric Buclin]

The patch applies cleanly on 2.18.3, 2.20rc2 and tip.

Comment 10

[Dave Miller [:justdave]]

Comment on attachment 193528 [details] [diff] [review]
patch, v2

You're setting yourself up for a code injection here by using an unescaped
variable in a regexp.  How about if we do it the same way Bug.pm does?

Comment 11

[Frédéric Buclin]

Attachment: patch for HEAD, v3

This patch applies cleanly only for 2.21, because move.pl still exists in 2.20
and 2.18.

Comment 12

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

Comment on attachment 193685 [details] [diff] [review]
patch for HEAD, v3

>Index: buglist.cgi

>+$vars->{'ismover'} = Bugzilla->user->is_mover;

Nit: since the template has access to the Bugzilla->user object, it should be
able to check the value of is_mover itself without that value being separately
reflected into $vars.


> if ($action eq Param('move-button-text')) {
>     Param('move-enabled') || ThrowUserError("move_bugs_disabled");
> 
>-    my $exporter = $user->login;
>-    my $movers = Param('movers');
>-    $movers =~ s/\s?,\s?/|/g;
>-    $movers =~ s/@/\@/g;
>-    if ($exporter !~ /($movers)/) {
>+    unless ($user->is_mover) {
>         ThrowUserError("auth_failure", {action => 'move',
>                                         object => 'bugs'});

Nit: it would be better if the syntax of this check was consistent with the
syntax of the "move-enabled" check above it, i.e.:

$user->is_mover || ThrowUserError(...);


>@@ -615,7 +611,7 @@
>         $comment = $cgi->param('comment') . "\n\n";
>     }
>     $comment .= "Bug moved to " . Param('move-to-url') . ".\n\n";
>-    $comment .= "If the move succeeded, $exporter will receive a mail\n";
>+    $comment .= "If the move succeeded, " . $user->login . " will receive a mail\n";

Urk, this should really be templatized at some point.


>Index: Bugzilla/Bug.pm

>-    my @movers = map { trim $_ } split(",", Param("movers"));
>-    my $canmove = Param("move-enabled") && Bugzilla->user->id && 
>-                  (lsearch(\@movers, Bugzilla->user->login) != -1);
>+    my $user = Bugzilla->user;
>+    my $canmove = $user->is_mover;

For consistency with how it's done in process_bug.cgi and so that
$user->is_mover can mean just what it sounds like it means, this should be:

my $canmove = Param('move-enabled') && $user->is_mover;


>+sub is_mover {
>+    my $self = shift;
>+
>+    return $self->{'is_mover'} if defined $self->{'is_mover'};
>+
>+    $self->{'is_mover'} = 0;
>+    if (Param('move-enabled')) {
>+        my @movers = map { trim($_) } split(',', Param('movers'));
>+        $self->{'is_mover'} = ($self->id
>+                               && lsearch(\@movers, $self->login) != -1);
>+    }
>+    return $self->{'is_mover'};
>+}

This should only check whether or not the user is a mover, not whether moving
itself is enabled.  Otherwise it conflates two different concepts that are
treated separately elsewhere (f.e. in the params, where there are separate
"move-enabled" and "movers" parameters), and it will confuse future coders who
expect it to mean only what it says.

Otherwise this looks good and is a valuable refactor!

Comment 13

[Frédéric Buclin]

Attachment: patch for HEAD, v4

Comment 14

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

Comment on attachment 195209 [details] [diff] [review]
patch for HEAD, v4


>     return $self->{'user'} if exists $self->{'user'};
>     return {} if $self->{'error'};
> 
>-    my @movers = map { trim $_ } split(",", Param("movers"));
>-    my $canmove = Param("move-enabled") && Bugzilla->user->id && 
>-                  (lsearch(\@movers, Bugzilla->user->login) != -1);
>+    my $user = Bugzilla->user;
>+    my $canmove = Param('move-enabled') && $user->is_mover;

Everything this Bug->user function does seems extraneous.  Why return a special
hash instead of just returning the user object?  For that matter, why have a
special Bugzilla::Bug->user accessor for the current user when that object is
already generally accessible via Bugzilla->user?

None of this is the fault of your patch, of course, but this function is ripe
for refactoring (anything in it which isn't already available from
Bugzilla->user should be made available).  In the meantime, r=myk, but nit:
$canmove could be removed from this function entirely if you just change the
test in knob.html.tmpl from "bug.user.canmove" to "Param('move-enabled') &&
user->is_mover" (the same as the test in edit-multiple.html.tmpl).

Comment 15

[Frédéric Buclin]

bug.user.canedit and bug.user.isreporter should be determined on a per bug basis.
bug.user.canmove and bug.user.canconfirm do not depend on bugs.

Comment 16

[Frédéric Buclin]

And note that Bug::user() is only used in bug/knob.html.tmpl.

Comment 17

[Frédéric Buclin]

Attachment: backport for 2.20, v1

I had to do some cleanup in move.pl in order to have an acceptable UI. The
actual one is a shame (no CSS and/or no header/footer)!

Comment 18

[Frédéric Buclin]

Attachment: backport for 2.18, v1

Bug::user has to make sure the user exists before calling User::is_mover,
because User->new doesn't return an empty user object in 2.18 (so
$user->is_mover would fail). That's the reason of $::userid in this routine.

I also made the UI more acceptable when moving bugs.

Comment 19

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

> bug.user.canedit and bug.user.isreporter should be determined on a per bug 
> basis.

Then these should be either bug.canedit($user) or user.canedit($bug).

Comment 20

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

Comment on attachment 195251 [details] [diff] [review]
backport for 2.20, v1

>+my $user = Bugzilla->login(LOGIN_REQUIRED);
>+my $cgi = Bugzilla->cgi;
>+
> unless ( Param("move-enabled") ) {
>+  print $cgi->header();
>+  PutHeader("Move Bugs");
>   print "\n<P>Sorry. Bug moving is not enabled here. ";
>   print "If you need to move a bug, contact " . Param("maintainer");
>+  PutFooter();
>   exit;
> }
> 
>-Bugzilla->login(LOGIN_REQUIRED);
>-
>-my $cgi = Bugzilla->cgi;

$cgi instantiation can move up before the move-enabled check, but the login
check should stay after it so that users who aren't logged in can see that
moving is disabled without having to log in first.

Other than that, this looks good.  r=myk with that change made.

Comment 21

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

Comment on attachment 195255 [details] [diff] [review]
backport for 2.18, v1

Looks good except for the same issue noted in the 2.20 backport.  r=myk with
that change made

Comment 22

[Frédéric Buclin]

Attachment: backport for 2.20, v2

r=myk per IRC

(00:58:58) myk: LpSolit: yes please submit an updated patch, but you can mark
it r+

Comment 23

[Frédéric Buclin]

Attachment: backport for 2.18, v2

Comment 24

[Frédéric Buclin]

tip:

Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.310; previous revision: 1.309
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.284; previous revision: 1.283
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.95; previous revision: 1.94
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.81; previous revision: 1.80
done
Checking in template/en/default/list/edit-multiple.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/edit-multiple.html.tmpl,v
 <--  edit-multiple.html.tmpl
new revision: 1.30; previous revision: 1.29
done


2.20rc2:

Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.299.2.2; previous revision: 1.299.2.1
done
Checking in move.pl;
/cvsroot/mozilla/webtools/bugzilla/Attic/move.pl,v  <--  move.pl
new revision: 1.31.4.2; previous revision: 1.31.4.1
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.263.2.3; previous revision: 1.263.2.2
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.81.2.3; previous revision: 1.81.2.2
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.61.2.7; previous revision: 1.61.2.6
done
Checking in template/en/default/list/edit-multiple.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/edit-multiple.html.tmpl,v
 <--  edit-multiple.html.tmpl
new revision: 1.26.2.2; previous revision: 1.26.2.1
done


2.18.3:

Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.255.2.12; previous revision: 1.255.2.11
done
Checking in move.pl;
/cvsroot/mozilla/webtools/bugzilla/Attic/move.pl,v  <--  move.pl
new revision: 1.26.2.4; previous revision: 1.26.2.3
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.205.2.23; previous revision: 1.205.2.22
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.37.2.6; previous revision: 1.37.2.5
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.20.2.3; previous revision: 1.20.2.2
done
Checking in template/en/default/list/edit-multiple.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/edit-multiple.html.tmpl,v
 <--  edit-multiple.html.tmpl
new revision: 1.17.2.5; previous revision: 1.17.2.4
done