show bugs should take into account product group permissions

RESOLVED DUPLICATE of bug 95923

Status

()

Bugzilla
Creating/Changing Bugs
RESOLVED DUPLICATE of bug 95923
14 years ago
12 years ago

People

(Reporter: Albert Ting, Assigned: myk)

Tracking

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

14 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5) Gecko/20031021 Firebird/0.7
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5) Gecko/20031021 Firebird/0.7

showbugs.cgi should take into account product group permissions, rather than
wait until process_bug calls CanEditProductId.  I believe a small patch to
Bug.pm does the trick.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.
(Reporter)

Comment 1

14 years ago
Created attachment 133964 [details] [diff] [review]
patch to Bug.pm
what exactly is this supposed to accomplish?  The description isn't very good,
and the patch looks like it's removing a permission check that I'm pretty sure
was there on purpose...
OS: Linux → All
Hardware: PC → All

Comment 3

14 years ago
justdave: the patch appears to be reversed. IOW, he wants to -add- the check 
you noted.
(Reporter)

Comment 4

14 years ago
Suppose I define a product group and assign these settings:

   Entry = NO
   Canedit = YES
   MemberControl = mandatory
   OtherControl = mandatory

Any user not assigned to this group can not edit bugs in this product.  Bugzilla
behaves correctly by reporting an error message when the user tries to edit and
hit "submit".  

But it would be better if he didn't even have he option to edit, to only see 
the bug in "read-only" mode.  

This patch seems to solve this problem, although of course I did limited testing.
OS: All → Linux
Hardware: All → PC
Aha...   ok, looking at it as reversed, this then boils down to a question of
whether or not the editbugs priv is supposed to be able to override the
"canedit" property of a product...
OS: Linux → All
Hardware: PC → All
(Reporter)

Comment 6

14 years ago
The current low level behavior is what I would expect (and want!).  The product 
group allows me to fine tune the global "canedit" user property.

What I'm trying to do is clean up the UI to better fit the current behavior.
OS: All → Linux
Hardware: All → PC

Comment 7

14 years ago
Comment on attachment 133964 [details] [diff] [review]
patch to Bug.pm

If I understand the request correctly, he does not want the user to be given an
editable form just to be told later that he should not have tried to edit that
bug.  

This patch would break security by letting a user who is permitted to make
chnages to any bugs make changes to bugs regardless of product permissions.
Attachment #133964 - Flags: review-
(Reporter)

Comment 8

14 years ago
Created attachment 134378 [details] [diff] [review]
updated patch

Ok, here's the patch in the correct order.

I'm trying to add additional restrictions to what a person can see in the
show_bugs form.  Unless I'm mistaken, I don't think I'm breaking security.
Attachment #133964 - Attachment is obsolete: true
(Reporter)

Comment 9

14 years ago
Could somebody explain to me the concern of this patch?  I believe the patch 
is making the UI more restrictive, not less restrictive.  It's also an
improvement to the flow.  That is, without the patch, you will be issued a
"not allowed" message after the process_bug call.
The new patch appears to make it so that you have to have both canedit and
editbugs in order to edit a bug...  canedit without editbugs would be useless.
(Reporter)

Comment 11

14 years ago
Created attachment 151707 [details] [diff] [review]
updated patch

Ah, good point.  Per comment #10, I've updated the patch and removed the 
redundancy.  It now just calls CanEditProductId().
(Reporter)

Updated

14 years ago
Attachment #134378 - Attachment is obsolete: true
Attachment #151707 - Flags: review?(bugreport)

Comment 12

14 years ago
WRT comment 10, I suspect that is what we want.  Upgraded sites would still be
using editbugs alone to conrol this.

Comment 13

14 years ago
Comment on attachment 151707 [details] [diff] [review]
updated patch

see comment 12
Attachment #151707 - Flags: review?(bugreport) → review-

Comment 14

14 years ago
The key distinction beteen the "editbugs" permission and the "canedit"
associated with a product.

If a product requires any group of which you are not a member in for canedit,
then bugs in that product are 100% read-only to you regardless of other permissions.

The editbugs group distinguishes between non-members who can only comment on
bugs (unless they reported them or are assigned to them) and members who can
change the state, priority, whiteboard, summary, etc....

For the purposes of this feature, both are really required.  However, check the
logic in process_bug to make sure that any cases where some of these activities
are allowed anyway are covered so you don't remove the UI from some actions that
are supposed to be allowed.

For example, you can confirm a bug if you have EITHER canconfirm or editbugs
(unless CanEditProductId said you cannot)

The more I look at this, the more I think that you want to first check
CanEditProductID() and then make a series of calls to CheckCanChangeField to
ask, hypothetically, if the user would be permitted to make a change.  That will
also help in the case where someone customized their bugzilla within
CheckCanChangeField which is where we keep telling people they must make this
sort of change.



Comment 15

14 years ago
Actually, since this starts to involv CheckCanChangeField, we should sync up
with Gerv as well.
This bug is somewhat confusing to follow.

Why do you need to hypothetically call CheckCanChangeField()? Why not just call
it - it'll throw an error if the user can't change the field, but that's what
you would want anyway, isn't it?

Gerv

Comment 17

14 years ago
What Albert is trying to do is to keep the UI from enticing users to attempt to
edit things they are not permitted to change.

Joel: how does checking the editbugs permission and seeing whether the user
"canedit" the bug's product not do what he wants? OK, there are some edge cases,
but that's life.

Gerv

Comment 19

13 years ago
If you want to alter the way Bug.pm works, then also take into account that
process_bug.cgi calls CanEditProductId(). The stupid part of it is that all
fields are checked through CheckCanChangeField() and the CanEditProductId() is
called to make sure the user is allowed to alter the bug. I would do the
opposite: first check that the user is allowed to alter the bug based on its
product, *then* check all fields one by one.
OS: Linux → All
Hardware: PC → All

Updated

12 years ago
QA Contact: mattyt-bugzilla → default-qa

Comment 20

12 years ago

*** This bug has been marked as a duplicate of 95923 ***
Status: UNCONFIRMED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.