Closed Bug 265898 Opened 15 years ago Closed 15 years ago

edit*.cgi files should all use ThrowUserError()

Categories

(Bugzilla :: Administration, task)

2.19
task
Not set

Tracking

()

RESOLVED FIXED
Bugzilla 2.20

People

(Reporter: LpSolit, Assigned: LpSolit)

References

Details

Attachments

(1 file, 6 obsolete files)

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'));
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.
please mark this bug as a dupe if you think it is one...
No longer blocks: 249930
This patch also solve the problem with chart.cgi, see bug 249930.
Assignee: justdave → LpSolit
Status: NEW → ASSIGNED
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
Filtering the group parameter as suggested by GavinS + adding a missing
condition to add/remove users from groups.
Attachment #165083 - Attachment is obsolete: true
Oops, I removed the $editall variable which is used later...
Attachment #165115 - Attachment is obsolete: true
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)
Attachment #165083 - Flags: review?(vladd)
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-
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)
Using the 3 params approach, as discussed with vladd on IRC.
Attachment #165116 - Attachment is obsolete: true
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 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-
... as requested by vladd.
Attachment #165804 - Attachment is obsolete: true
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)
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-
Attachment #165807 - Flags: review-
Attached patch New attempt, v5 (obsolete) — Splinter Review
I keep editusers.cgi in this patch, even if vladd would prefer to see it out.
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
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)
Attachment #165980 - Flags: review?(vladd) → review+
Flags: approval?
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)
No longer depends on: 46296, 119485, 190196, 190226
Target Milestone: --- → Bugzilla 2.22
Targeting bug to 2.20 since the 2.20 feature freeze was canceled.
Target Milestone: Bugzilla 2.22 → Bugzilla 2.20
Flags: approval? → approval+
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: 15 years ago
Resolution: --- → FIXED
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.