Closed
Bug 313547
Opened 19 years ago
Closed 19 years ago
When strict_isolation is set, don't allow inappropriate users on new bugs
Categories
(Bugzilla :: Creating/Changing Bugs, defect)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.22
People
(Reporter: bugreport, Assigned: bugreport)
References
Details
Attachments
(1 file, 2 obsolete files)
1.93 KB,
patch
|
LpSolit
:
review+
|
Details | Diff | Splinter Review |
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•19 years ago
|
||
Attachment #200570 -
Flags: review?
Assignee | ||
Updated•19 years ago
|
Attachment #200570 -
Flags: review?(LpSolit)
Comment 2•19 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•19 years ago
|
Severity: enhancement → minor
Comment 3•19 years ago
|
||
Looks like this bug needs to be fixed for 2.21.2. This is the last piece of the strict_isolation stuff.
Comment 4•19 years ago
|
||
This should be part of the strict_isolation package when it is released for the first time, I'd say.
Updated•19 years ago
|
Flags: blocking2.22?
Comment 5•19 years ago
|
||
Yeah, we can't release with this bug in existence.
Flags: blocking2.22? → blocking2.22+
Assignee | ||
Comment 6•19 years ago
|
||
Attachment #200570 -
Attachment is obsolete: true
Comment 7•19 years ago
|
||
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-
Assignee | ||
Comment 9•19 years ago
|
||
Attachment #206264 -
Attachment is obsolete: true
Assignee | ||
Updated•19 years ago
|
Attachment #207211 -
Flags: review?(wicked)
Comment 10•19 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•19 years ago
|
Flags: approval?
Updated•19 years ago
|
Flags: approval? → approval+
Assignee | ||
Comment 11•19 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
Closed: 19 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•