Closed Bug 573173 Opened 14 years ago Closed 14 years ago

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

Categories

(Bugzilla :: Creating/Changing Bugs, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 4.0

People

(Reporter: mkanat, Assigned: mkanat)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
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.
Attached patch Work In Progress (obsolete) — Splinter Review
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
Attached patch v1Splinter Review
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)
(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.
(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.
(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.
(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.
  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.
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
Flags: approval?
Flags: approval? → approval+
Attached patch Fix Error Messages, v1 (obsolete) — Splinter Review
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-
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+
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
Closed: 14 years ago
Resolution: --- → FIXED
Blocks: 581663
Blocks: 581668
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: