When strict_isolation is set, don't allow inappropriate users on new bugs

RESOLVED FIXED in Bugzilla 2.22

Status

()

Bugzilla
Creating/Changing Bugs
--
minor
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Joel Peshkin, Assigned: Joel Peshkin)

Tracking

2.21
Bugzilla 2.22
Bug Flags:
approval +
blocking2.22 +

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

12 years ago
Strict isolation enforces rules on who can be assigned or cc'd on existing bugs.  post_bug.cgi must do the same.
(Assignee)

Comment 1

12 years ago
Created attachment 200570 [details] [diff] [review]
The same check as is done by process_bug
Attachment #200570 - Flags: review?
(Assignee)

Updated

12 years ago
Attachment #200570 - Flags: review?(LpSolit)

Comment 2

12 years ago
Comment on attachment 200570 [details] [diff] [review]
The same check as is done by process_bug

>Index: post_bug.cgi

>+if (Param("strict_isolation")) {
>+    my @roles = ('assigned_to');
>+    if (Param("useqacontact")) {
>+        push @roles, 'qa_contact';
>+    }
>+    foreach my $role (@roles) {
>+        my $role_user = Bugzilla::User->new($cgi->param($role));
>+        if (!$role_user->can_edit_product($product_id)) {
>+            ThrowUserError("invalid_user_group",

If you enter a new bug in a component which has no default QA contact, you get an error! You should only consider the QA contact if both Param('useqacontact') is on *and* a QA contact has been given.


>+        if (Param("strict_isolation")) {
>+            my $cc_user = Bugzilla::User->new($ccid);
>+            if (!$cc_user->can_edit_product($product_id)) {
>+                push (@blocked_cc, $person);

Nit: I prefer $cc_user->login than $person.


>+    my $blocked_cc = join(", ", @blocked_cc);
>+    ThrowUserError("invalid_user_group",
>+        {'user' => $blocked_cc});

The error message doesn't expect to receive a list of user:

>     User '[% user FILTER html %]' is not able to edit the
Attachment #200570 - Flags: review?(LpSolit)
Attachment #200570 - Flags: review?
Attachment #200570 - Flags: review-

Updated

12 years ago
Severity: enhancement → minor

Comment 3

12 years ago
Looks like this bug needs to be fixed for 2.21.2. This is the last piece of the strict_isolation stuff.
This should be part of the strict_isolation package when it is released for the first time, I'd say.
Flags: blocking2.22?

Comment 5

12 years ago
Yeah, we can't release with this bug in existence.
Flags: blocking2.22? → blocking2.22+
(Assignee)

Comment 6

12 years ago
Created attachment 206264 [details] [diff] [review]
Fixed and clean version
Attachment #200570 - Attachment is obsolete: true
Comment on attachment 206264 [details] [diff] [review]
Fixed and clean version

>Index: post_bug.cgi
>===================================================================
>+    $related_users{$cgi->param('qa_contact')} = 1 if Param('useqacontact');

This still doesn't work when no QA contact is specified for the product. A blank user is added to the hash and later checked, which naturally ends up in an error message stating "User '' is not able to edit the 'TestProduct' Product .".

>Index: template/en/default/global/user-error.html.tmpl
>===================================================================
>     [%+ field_descs.product FILTER html %]

Nit: This adds a word product with capital P. True, your patch didn't add this line but this makes the error look awfull. :(

>+    [% ELSIF new %]
>+      .

Nit: This leaves a blank before the dot.
Attachment #206264 - Flags: review-

Comment 8

12 years ago
joel, have you time to update your patch this week?
Status: NEW → ASSIGNED
(Assignee)

Comment 9

12 years ago
Created attachment 207211 [details] [diff] [review]
v3
Attachment #206264 - Attachment is obsolete: true
(Assignee)

Updated

12 years ago
Attachment #207211 - Flags: review?(wicked)

Comment 10

12 years ago
Comment on attachment 207211 [details] [diff] [review]
v3

>+    if (Param('useqacontact') && (trim($cgi->param('qa_contact')) ne '')) {
>+        $related_users{$cgi->param('qa_contact')} = 1;

$cgi->param('qa_contact') is an integer (it has been converted from a login name to a user ID earlier), so trim() doesn't make sense and comparing it to '' even less. You should only check that $cgi->param('qa_contact') is given:

if (Param('useqacontact') && $cgi->param('qa_contact'))


>+        if (!$related_user->can_edit_product($cgi->param('product_id'))) {

Nit: you could write $product_id instead of $cgi->param('product_id').


Fix at least the first point on checkin. And why not the second one too. ;) Works fine. r=LpSolit
Attachment #207211 - Flags: review?(wicked) → review+

Updated

12 years ago
Flags: approval?
Flags: approval? → approval+
(Assignee)

Comment 11

12 years ago
Checked in with LpSolit's nits fixed

Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.134; previous revision: 1.133
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v  <--  user-error.html.tmpl
new revision: 1.145; previous revision: 1.144
done
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.