Frédéric Buclin Assignee
	Description • 20 years ago

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'));

	Dave Miller [:justdave]
	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.

	Frédéric Buclin Assignee
	Comment 2 • 20 years ago

please mark this bug as a dupe if you think it is one...

	Frédéric Buclin Assignee
	Comment 3 • 20 years ago

This patch also solve the problem with chart.cgi, see bug 249930.

	Frédéric Buclin 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.

	GavinS
	Comment 5 • 20 years ago

I think you need to filter the 'group' template parameter in that patch

	Frédéric Buclin Assignee
	Comment 6 • 20 years ago

Filtering the group parameter as suggested by GavinS + adding a missing
condition to add/remove users from groups.

	Frédéric Buclin Assignee
	Comment 7 • 20 years ago

Oops, I removed the $editall variable which is used later...

	Frédéric Buclin 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.

	Vlad Dascalu
	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.

	Vlad Dascalu
	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)

	Frédéric Buclin Assignee
	Comment 11 • 20 years ago

Using the 3 params approach, as discussed with vladd on IRC.

	Frédéric Buclin 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.

	Vlad Dascalu
	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.

	Frédéric Buclin Assignee
	Comment 14 • 20 years ago

... as requested by vladd.

	Frédéric Buclin 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! :)

	Frédéric Buclin 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.

	Frédéric Buclin Assignee
	Comment 17 • 20 years ago

I keep editusers.cgi in this patch, even if vladd would prefer to see it out.

	Frédéric Buclin 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.

	Frédéric Buclin 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.

	Dave Miller [:justdave]
	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)

	Vlad Dascalu
	Comment 21 • 20 years ago

Targeting bug to 2.20 since the 2.20 feature freeze was canceled.

	Vlad Dascalu
	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