573173 - Bugzilla::Bug's add_group and remove_group should take group names instead of ids

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Max Kanat-Alexander]

For Bug.update, we're going to want to send group names instead of group ids. That means that Bugzilla needs a bit of refactoring to start using group names for add_group and remove_group instead of group ids. This should be safe (in terms of protecting the names of confidential groups), because you have to be in a group in order to see show_bug.cgi for any bug in that group.

Comment 1

[Max Kanat-Alexander]

I am going to very slightly change our security policy in terms of group names, for this bug: If you try to specify a group name that doesn't exist, Bugzilla will explicitly tell you that it doesn't exist. (So in other words, if they guess the name, they will get a different error message, confirming that the name exists but that they cannot use it.) This is because we're going to be exposing this as an API, and having a group *silently* not get added because of a typo isn't really an acceptable API. I think it's more of a security risk to silently ignore invalid groups than to explicitly deny them.

Comment 2

[Max Kanat-Alexander]

Attachment: Work In Progress

This is a work in progress for the UI side. I was trying to simplify the template code, but I might just go back to a form of $bug->groups.

Comment 3

[Max Kanat-Alexander]

Attachment: v1

Okay, I decided to go simpler and not refactor the code *around* the area that I was modifying, even though some of it really needs it.

Comment 4

[Frédéric Buclin]

(In reply to comment #1)
> If you try to specify a group name that doesn't exist, Bugzilla
> will explicitly tell you that it doesn't exist.

The error message could be "This group doesn't exist, or you are not allowed to restrict bugs to this group". This way, we don't leak any information. No need for two distinct messages.

Comment 5

[Max Kanat-Alexander]

(In reply to comment #4)
> The error message could be "This group doesn't exist, or you are not allowed to
> restrict bugs to this group". This way, we don't leak any information. No need
> for two distinct messages.

  True, but due to the way the current logic works in add_group and remove_group, that would be difficult. Not impossible, but there are currently two different error messages that they throw for groups that *do* exist but can't be set on this bug. Trying to troubleshoot a single "either this group doesn't exist or you can't set it on this bug" error message would be a lot more difficult for admins (and for the people they come to ask for support) than troubleshooting the explicit messages.

Comment 6

[Frédéric Buclin]

(In reply to comment #5)
> Trying to troubleshoot a single "either this group
> doesn't exist or you can't set it on this bug" error message would be a lot
> more difficult for admins (and for the people they come to ask for support)
> than troubleshooting the explicit messages.

That's a pretty weak reason. Then you could apply the same reasoning for product names and users.

Comment 7

[Max Kanat-Alexander]

(In reply to comment #6)
> That's a pretty weak reason. Then you could apply the same reasoning for
> product names and users.

  Well, not really, because there's a single, centralized way of saying, "Oh, User A can or cannot see Product B" but there's no reasonable way (that we'd want to check on every call to Bugzilla::Group->check or similar functions) of saying, "User A cannot see Group A under any circumstances", particularly because people can be CC'ed on a bug or be the reporter of a bug, and see a group that way that they normally couldn't see.

  Also, group controls are very complex and products are much more straightforward.

Comment 8

[Max Kanat-Alexander]

  Also, for what it's worth, we're already exposing the existence of groups in add_group and remove_group, because we throw a special error for groups that exist but can't be set.

Comment 9

[David Lawrence [:dkl]]

Comment on attachment 452434 [details] [diff] [review]
v1

Looks good and works as expected in all of the relevant places. I do have one nit that you can fix on checkin though. It still uses the group id in the error message thrown when trying to place the bug in a group not enabled for a product. This occurs on single edit and multi edit pages.

"You tried to restrict bug 1 to to group id 14, but bugs in the
'TestProduct' product can not be restricted to that group."

Otherwise r=dkl

Comment 10

[Max Kanat-Alexander]

Attachment: Fix Error Messages, v1

Ah, okay. Here's a patch to fix the error messages. It was a little big to just do as a checkin fix.

Comment 11

[David Lawrence [:dkl]]

Comment on attachment 453518 [details] [diff] [review]
Fix Error Messages, v1

>=== modified file 'Bugzilla/Bug.pm'
>             ThrowUserError('group_change_denied',
>-                           { bug => $self, group_id => $group->id });
>+                           { bug => $self, group => $group });

>                 ThrowUserError('group_change_denied',
>-                               { bug => $self, group_id => $group->id });
>+                               { bug => $self, group => $group });

>=== modified file 'template/en/default/global/user-error.html.tmpl'
>   [% ELSIF error == "group_change_denied" %]
>     [% title = "Cannot Add/Remove That Group" %]
>-    You tried to add or remove group id [% group_id FILTER html %]
>+    You tried to add or remove the '[% group_id FILTER html %]' group
>     from [% terms.bug %] [%+ bug.id FILTER html %], but you do not
>     have permissions to do so.

Still using group_id instead of group.name in the template.

Dave

Comment 12

[Max Kanat-Alexander]

Attachment: Fix Error Messages, v2

Well, I'm glad I had you review it! :-)

Comment 13

[David Lawrence [:dkl]]

Comment on attachment 453977 [details] [diff] [review]
Fix Error Messages, v2

Looks good and works as expected. r=dkl

Comment 14

[Max Kanat-Alexander]

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/
modified process_bug.cgi
modified Bugzilla/Bug.pm
modified template/en/default/bug/edit.html.tmpl
modified template/en/default/bug/process/verify-new-product.html.tmpl
modified template/en/default/global/user-error.html.tmpl
modified template/en/default/list/edit-multiple.html.tmpl
Committed revision 7250.