Closed
Bug 265898
Opened 20 years ago
Closed 20 years ago
edit*.cgi files should all use ThrowUserError()
Categories
(Bugzilla :: Administration, task)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.20
People
(Reporter: LpSolit, Assigned: LpSolit)
References
Details
Attachments
(1 file, 6 obsolete files)
19.37 KB,
patch
|
goobix
:
review+
|
Details | Diff | Splinter Review |
Authorization Failures should all have a common display, i.e. the one defined by ThrowUserError() and using error messages from template/en/default/global/user-error.html.tmpl. Among the 11 edit*.cgi files, 4 of them do not use ThrowUserError(). Here is a summary of what we have for the trunk: editclassifications.cgi -> OK ThrowUserError("auth_cant_edit_classifications") ThrowUserError("auth_classification_not_enabled") editcomponents.cgi -> OK ThrowUserError('auth_cant_edit_components') editflagtypes.cgi -> OK ThrowUserError("authorization_failure", { action => "administer flag types" }) editgroups.cgi -> OK ThrowUserError("auth_cant_edit_groups") editkeywords.cgi -> OK ThrowUserError("keyword_access_denied") editmilestones.cgi -> OK ThrowUserError('auth_cant_edit_milestones') editparams.cgi -> FAIL print Bugzilla->cgi->header(); if (!UserInGroup("tweakparams")) { print "<H1>Sorry, you aren't a member of the 'tweakparams' group.</H1>\n"; print "And so, you aren't allowed to edit the parameters.\n"; PutFooter(); exit; } editproducts.cgi -> FAIL print Bugzilla->cgi->header(); unless (UserInGroup("editcomponents")) { PutHeader("Not allowed"); print "Sorry, you aren't a member of the 'editcomponents' group.\n"; print "And so, you aren't allowed to add, modify or delete products.\n"; PutTrailer(); exit; } editusers.cgi -> FAIL print Bugzilla->cgi->header(); $editall = UserInGroup("editusers"); if (!$editall) { if (!Bugzilla->user->can_bless) { PutHeader("Not allowed"); print "Sorry, you aren't a member of the 'editusers' group, and you\n"; print "don't have permissions to put people in or out of any group.\n"; print "And so, you aren't allowed to add, modify or delete users.\n"; PutTrailer(); exit; } } editversions.cgi -> FAIL print Bugzilla->cgi->header(); unless (UserInGroup("editcomponents")) { PutHeader("Not allowed"); print "Sorry, you aren't a member of the 'editcomponents' group.\n"; print "And so, you aren't allowed to add, modify or delete versions.\n"; PutTrailer(); exit; } editwhines.cgi -> OK ThrowUserError('whine_access_denied') We see that editparams.cgi, editproducts.cgi, editusers.cgi and editversions.cgi use print ""; exit; to display a warning. This is moreover a problem for internationalization. I suggest all edit*.cgi should use the form ThrowUserError('xxxxxxx') unless (UserInGroup('yyyyyyyyy'));
Comment 1•20 years ago
|
||
This looks like all the edit*.cgi that haven't been templatized yet. I'm willing to bet this gets fixed automatically when those files get templatized.
Assignee | ||
Comment 2•20 years ago
|
||
please mark this bug as a dupe if you think it is one...
Blocks: bz-admintemplates
Assignee | ||
Comment 3•20 years ago
|
||
This patch also solve the problem with chart.cgi, see bug 249930.
Assignee: justdave → LpSolit
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•20 years ago
|
||
Comment on attachment 165083 [details] [diff] [review] Common error messages for administrative pages vladd, as discussed on IRC, here is what I suggest. I already include not yet templatized pages, so that they could already be translated. This also solve the problem mentionned in bug 249930.
Attachment #165083 -
Flags: review?(vladd)
I think you need to filter the 'group' template parameter in that patch
Assignee | ||
Comment 6•20 years ago
|
||
Filtering the group parameter as suggested by GavinS + adding a missing condition to add/remove users from groups.
Attachment #165083 -
Attachment is obsolete: true
Assignee | ||
Comment 7•20 years ago
|
||
Oops, I removed the $editall variable which is used later...
Attachment #165115 -
Attachment is obsolete: true
Assignee | ||
Comment 8•20 years ago
|
||
Comment on attachment 165116 [details] [diff] [review] Common error messages for administrative pages, v2.1 Sounds better now. Again asking vladd for review.
Attachment #165116 -
Flags: review?(vladd)
Assignee | ||
Updated•20 years ago
|
Attachment #165083 -
Flags: review?(vladd)
Comment 9•20 years ago
|
||
Comment on attachment 165116 [details] [diff] [review] Common error messages for administrative pages, v2.1 I don't like the "group" and "category" names. Besides, there is an obvious overlap between those two (most of the time the value of "category" would indicate that it could be deducted based on the "groups" parameter). Trying to produce some design structure, here's what I came up with: The auth_failure message will always be displayed because the user will not belong to a group. He will try to perform an action on some objects, and he will be rejected. So we have three elements: -> the group in which he doesn't belong. -> the action that he tries to do: ---> add, modify or delete ---> edit (for parameters) ---> use (for the new charts feature) -> the object(s) upon he tries to do the action. So we have 2 params, "group" and "action". For example "group" could be "parameters" and "action" could be "edit", or group could be "bz_canusewhine" and "action" could be "schedule". The issue is whether we should have a third parameter that specifies the objects upon which the action is desired to be performed, or this can always be deducted based on the "group" field. Due to our modular and orthogonal design we make groups pretty independent, so practically not having parameters group-access for example would mean that you can't perform actions on parameters. So in my opinion two parameters structured in the way mentioned above would do the trick. The template should look in the lines of: Auth-failure yadda yadda, you aren't in the <group> group, and therefore yadda yadda you can't <action> <objects>. <action> and <objects> would have to be conditional IF/ELSIF/END, the first one based on the "action" parameter and the second one based on the "group" parameter.
Attachment #165116 -
Flags: review?(vladd) → review-
Comment 10•20 years ago
|
||
It seems we have several objects for the "editcomponents" group, so the plan with 2 params falls, and we should use 3 instead. We could have: group="editcomponents", action="add", object="milestone" group="editcomponents", action="delete", object="versions" (of course, probably action would be "add-modify-delete" since the code threats that unitary from what I see)
Assignee | ||
Comment 11•20 years ago
|
||
Using the 3 params approach, as discussed with vladd on IRC.
Attachment #165116 -
Attachment is obsolete: true
Assignee | ||
Comment 12•20 years ago
|
||
Comment on attachment 165804 [details] [diff] [review] Common error messages for all administrative pages, v3 vladd, I think it is enough to let action empty for special cases as this can easily be considered in user-error.html.tmpl knowing the object.
Attachment #165804 -
Flags: review?(vladd)
Comment 13•20 years ago
|
||
Comment on attachment 165804 [details] [diff] [review] Common error messages for all administrative pages, v3 I'd prefer some structure in the template code itself; in this way, we don't mix action with object, that's the whole purpose of having 3 distinct fields. So: + [% IF object == "chartgroup" %] + use the "New Charts" feature seems to assert that action="edit" when object = "chartgroup". By the way, the object is no group at all, so I would choose "chart" or "new-chart" probably. + [% ELSIF object == "parameters" %] + edit the parameters This also seems to imply that action="edit" if object="parameters", which is kind of wrong as well. We should have something in the lines of: [% IF action == "add-modify-delete" %] add, modify or delete [% ELSIF action == "use" %] use [% ELSIF action == "edit" %] edit [% END %] and then take the conditional ifs with the object, without mixing it with the action.
Attachment #165804 -
Flags: review?(vladd) → review-
Assignee | ||
Comment 14•20 years ago
|
||
... as requested by vladd.
Attachment #165804 -
Attachment is obsolete: true
Assignee | ||
Comment 15•20 years ago
|
||
Comment on attachment 165807 [details] [diff] [review] Completely separate groups, actions and objects, v4 Hope this to be the last one! :)
Attachment #165807 -
Flags: review?(vladd)
Assignee | ||
Comment 16•20 years ago
|
||
Comment on attachment 165807 [details] [diff] [review] Completely separate groups, actions and objects, v4 I deny it myself, vladd! There is an error in editusers.cgi, as discussed on IRC.
Attachment #165807 -
Attachment is obsolete: true
Attachment #165807 -
Flags: review?(vladd) → review-
Updated•20 years ago
|
Attachment #165807 -
Flags: review-
Assignee | ||
Comment 17•20 years ago
|
||
I keep editusers.cgi in this patch, even if vladd would prefer to see it out.
Assignee | ||
Comment 18•20 years ago
|
||
All edit*.cgi files are considered, as well as chart.cgi due to localization problems mentioned in bug 249930.
Attachment #165846 -
Attachment is obsolete: true
Assignee | ||
Comment 19•20 years ago
|
||
Comment on attachment 165980 [details] [diff] [review] Complete (?) patch, v6 vladd, here is the "perfect" :) solution we discussed this week-end. Please review (I tested it successfully)! Note that authorization failures may also occur in some other .cgi files such as attachment.cgi when the insidergroup param is used. Such files are not included in this patch as I only talk about edit*.cgi files (+ chart.cgi because of localization problems, see my previous comment). We may later add another patch for such files.
Attachment #165980 -
Flags: review?(vladd)
Updated•20 years ago
|
Attachment #165980 -
Flags: review?(vladd) → review+
Updated•20 years ago
|
Flags: approval?
Comment 20•20 years ago
|
||
because of the way this ended up being done, most of the dependencies are now backwards, because most of that stuff now depends on this :) (this will make those be less work) holding approval for the 2.20 branch to be created (this will go on the trunk immediately after we branch)
Comment 21•20 years ago
|
||
Targeting bug to 2.20 since the 2.20 feature freeze was canceled.
Target Milestone: Bugzilla 2.22 → Bugzilla 2.20
Updated•20 years ago
|
Flags: approval? → approval+
Comment 22•20 years ago
|
||
Frederic: editversions.cgi templatization just landed, and this created: [% ELSIF error == "auth_cant_edit_versions" %] in user-error.html.tmpl, which probably should be converted to your generic handler as well. If you have the time open a bug about it and try to see if that's fixable. Thanks :-) Checking in chart.cgi; /cvsroot/mozilla/webtools/bugzilla/chart.cgi,v <-- chart.cgi new revision: 1.9; previous revision: 1.8 done Checking in editclassifications.cgi; /cvsroot/mozilla/webtools/bugzilla/editclassifications.cgi,v <-- editclassifications.cgi new revision: 1.2; previous revision: 1.1 done Checking in editcomponents.cgi; /cvsroot/mozilla/webtools/bugzilla/editcomponents.cgi,v <-- editcomponents.cgi new revision: 1.45; previous revision: 1.44 done Checking in editflagtypes.cgi; /cvsroot/mozilla/webtools/bugzilla/editflagtypes.cgi,v <-- editflagtypes.cgi new revision: 1.13; previous revision: 1.12 done Checking in editgroups.cgi; /cvsroot/mozilla/webtools/bugzilla/editgroups.cgi,v <-- editgroups.cgi new revision: 1.44; previous revision: 1.43 done Checking in editkeywords.cgi; /cvsroot/mozilla/webtools/bugzilla/editkeywords.cgi,v <-- editkeywords.cgi new revision: 1.23; previous revision: 1.22 done Checking in editmilestones.cgi; /cvsroot/mozilla/webtools/bugzilla/editmilestones.cgi,v <-- editmilestones.cgi new revision: 1.26; previous revision: 1.25 done Checking in editparams.cgi; /cvsroot/mozilla/webtools/bugzilla/editparams.cgi,v <-- editparams.cgi new revision: 1.23; previous revision: 1.22 done Checking in editproducts.cgi; /cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v <-- editproducts.cgi new revision: 1.63; previous revision: 1.62 done Checking in editusers.cgi; /cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v <-- editusers.cgi new revision: 1.71; previous revision: 1.70 done Checking in editwhines.cgi; /cvsroot/mozilla/webtools/bugzilla/editwhines.cgi,v <-- editwhines.cgi new revision: 1.2; previous revision: 1.1 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.83; previous revision: 1.82 done
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
QA Contact: matty_is_a_geek → default-qa
You need to log in
before you can comment on or make changes to this bug.
Description
•