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.

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

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

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.

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

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

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.

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.

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

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

Comment on attachment 151707 [details] [diff] [review] updated patch see comment 12

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.

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

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

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.

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