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

RESOLVED FIXED in Bugzilla 4.0

Status

()

--
enhancement
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: mkanat, Assigned: mkanat)

Tracking

Bugzilla 4.0
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

9 years ago
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.
(Assignee)

Comment 1

9 years ago
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.
(Assignee)

Comment 2

9 years ago
Created attachment 452408 [details] [diff] [review]
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.
Assignee: create-and-change → mkanat
Status: NEW → ASSIGNED
(Assignee)

Comment 3

9 years ago
Created attachment 452434 [details] [diff] [review]
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.
Attachment #452408 - Attachment is obsolete: true
Attachment #452434 - Flags: review?(dkl)

Comment 4

9 years ago
(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.
(Assignee)

Comment 5

9 years ago
(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

9 years ago
(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.
(Assignee)

Comment 7

9 years ago
(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.
(Assignee)

Comment 8

9 years ago
  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.

Updated

8 years ago
Attachment #452434 - Flags: review?(dkl) → review+
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

Updated

8 years ago
Flags: approval?
(Assignee)

Updated

8 years ago
Flags: approval? → approval+
(Assignee)

Comment 10

8 years ago
Created attachment 453518 [details] [diff] [review]
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.
Attachment #453518 - Flags: review?(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
Attachment #453518 - Flags: review?(dkl) → review-
(Assignee)

Comment 12

8 years ago
Created attachment 453977 [details] [diff] [review]
Fix Error Messages, v2

Well, I'm glad I had you review it! :-)
Attachment #453518 - Attachment is obsolete: true
Attachment #453977 - Flags: review?(dkl)
Comment on attachment 453977 [details] [diff] [review]
Fix Error Messages, v2

Looks good and works as expected. r=dkl
Attachment #453977 - Flags: review?(dkl) → review+
(Assignee)

Comment 14

8 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Updated

8 years ago
Blocks: 581663

Updated

8 years ago
Blocks: 581668
You need to log in before you can comment on or make changes to this bug.