223439 - show bugs should take into account product group permissions

Status: RESOLVED DUPLICATE of bug 95923

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Albert Ting]

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.

Comment 1

[Albert Ting]

Attachment: patch to Bug.pm

Comment 2

[Dave Miller [:justdave]]

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

Comment 3

[chaduv]

justdave: the patch appears to be reversed. IOW, he wants to -add- the check 
you noted.

Comment 4

[Albert Ting]

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.

Comment 5

[Dave Miller [:justdave]]

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

Comment 6

[Albert Ting]

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.

Comment 7

[Joel Peshkin]

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.

Comment 8

[Albert Ting]

Attachment: 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.

Comment 9

[Albert Ting]

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.

Comment 10

[Dave Miller [:justdave]]

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.

Comment 11

[Albert Ting]

Attachment: updated patch

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

Comment 12

[Joel Peshkin]

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

Comment 13

[Joel Peshkin]

Comment on attachment 151707 [details] [diff] [review]
updated patch

see comment 12

Comment 14

[Joel Peshkin]

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

[Joel Peshkin]

Actually, since this starts to involv CheckCanChangeField, we should sync up
with Gerv as well.

Comment 16

[Gervase Markham [:gerv]]

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

[Joel Peshkin]

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.

Comment 18

[Gervase Markham [:gerv]]

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

[Frédéric Buclin]

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.

Comment 20

[Frédéric Buclin]

*** This bug has been marked as a duplicate of 95923 ***