Last Comment Bug 385453 - email_in.pl resets group restrictions
: email_in.pl resets group restrictions
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: Incoming Email (show other bugs)
: 3.0
: All All
: -- major (vote)
: Bugzilla 3.0
Assigned To: Trent Lillehaugen
: default-qa
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-06-22 04:49 PDT by Bernd Kuemmerlen
Modified: 2008-01-19 11:17 PST (History)
3 users (show)
mkanat: approval+
LpSolit: approval3.0+
LpSolit: blocking3.0.1+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Possible Workaround (2.08 KB, patch)
2007-08-06 16:06 PDT, Trent Lillehaugen
LpSolit: review-
Details | Diff | Splinter Review
Patch for 3.0.1, v2 (1.24 KB, patch)
2007-08-08 08:54 PDT, Trent Lillehaugen
LpSolit: review+
Details | Diff | Splinter Review
Patch for 3.1 - v1 (1.68 KB, patch)
2007-08-08 15:13 PDT, Trent Lillehaugen
LpSolit: review-
Details | Diff | Splinter Review
patch for 3.1, v2 (868 bytes, patch)
2007-08-16 09:34 PDT, Frédéric Buclin
mkanat: review+
Details | Diff | Splinter Review

Description Bernd Kuemmerlen 2007-06-22 04:49:09 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.8.1.4) Gecko/20070515 Firefox/2.0.0.4
Build Identifier: 

When using the inbound mail interfact email_in.pl, group permissions for Bugs are all cleared.

Reproducible: Always

Steps to Reproduce:
1. Make sure you have bug groups enabled
2. Add a bug to a group by checking the box for the group
3. Modify the Bug through email_in.pl
Actual Results:  
=> The bug is removed from the group, the checkbox is not set, the bug activity shows that the "Group" value of the Bug has changed to empty.

Expected Results:  
All groups which are set for a bug should be kept.

I think this is a security issue: If you use bug groups to control bug access, a bug will be visible by all after it has been modified through the e-mail interface. This is a potential security risk.
Comment 1 Bernd Kuemmerlen 2007-06-22 04:50:11 PDT
I forgot: This is similar to Bug 365229, but it seems that the fix for this is not quite that easy.
Comment 2 Max Kanat-Alexander 2007-06-23 00:24:40 PDT
I think we've had a few reports of this on the support list, but we need to investigate and confirm it here. 
Comment 3 Trent Lillehaugen 2007-08-06 11:09:20 PDT
I have noticed this same problem.
Comment 4 Trent Lillehaugen 2007-08-06 16:06:04 PDT
Created attachment 275500 [details] [diff] [review]
Possible Workaround

Maybe I'm missing something but it looks like there is a pretty simple fix for this problem.  I've attached a patch which I think resolves the issue.  Essentially I've wrapped the code in process_bug.cgi that checks if the group flags have been changed with:
unless (Bugzilla->usage_mode == USAGE_MODE_EMAIL) {
...
}

It seems to be working fine for me.
Comment 5 Frédéric Buclin 2007-08-06 16:28:45 PDT
Comment on attachment 275500 [details] [diff] [review]
Possible Workaround

>+# We don't want to change the group settings if this came from
>+# email_in.pl
>+
>+unless (Bugzilla->usage_mode == USAGE_MODE_EMAIL) {

This is a workaround, not a real fix. We should still be allowed to edit group settings using email_in.pl.
Comment 6 Trent Lillehaugen 2007-08-06 17:11:51 PDT
(In reply to comment #5)
> (From update of attachment 275500 [details] [diff] [review])
> >+# We don't want to change the group settings if this came from
> >+# email_in.pl
> >+
> >+unless (Bugzilla->usage_mode == USAGE_MODE_EMAIL) {
> 
> This is a workaround, not a real fix. We should still be allowed to edit group
> settings using email_in.pl.
> 

I agree that there should possibly be a way to set/clear group settings using email_in.pl.  That being said, I do believe that this is a fix for the originally reported problem:

> When using the inbound mail interfact email_in.pl, group permissions for Bugs
> are all cleared.

This problem, aside from just being annoying, is also a security risk as Bernd originally pointed out.  Because of that I think that it is OK to deal with it independently from adding new features.
Comment 7 Max Kanat-Alexander 2007-08-06 17:31:26 PDT
Comment on attachment 275500 [details] [diff] [review]
Possible Workaround

Yeah, this actually looks like a pretty good idea.

Although I prefer "if !=" instead of unless.
Comment 8 Max Kanat-Alexander 2007-08-06 17:32:58 PDT
This is almost certainly a real bug, so I'm confirming it based on the number of reports.

I think we should take this as a fix for 3.0 and we can add the ability to modify group settings for 3.2 if we want.
Comment 9 Bernd Kuemmerlen 2007-08-06 22:51:31 PDT
(In reply to comment #6)
> This problem, aside from just being annoying, is also a security risk as Bernd
> originally pointed out.

Thanks a lot for your patch, I will integrate it into our installation.

Comment 10 Frédéric Buclin 2007-08-07 16:08:25 PDT
Comment on attachment 275500 [details] [diff] [review]
Possible Workaround

>+        if (defined $cgi->param('id') || defined $cgi->param("bit-$b")) {

The reason why email_in.pl deletes groups when editing a bug is because process_bug.cgi assumes that all groups you are allowed to add/remove the bug to/from are displayed in the UI, and the absence of bit-X (X being the ID of a group) is interpreted as the checkbox of the group being unckecked, i.e. that the user doesn't want to restrict the bug to this group, and so it "logically" removes the bug from this group.

You have two ways to prevent that:

1) The easiest one, and which could be taken for 3.0.1 (with the promise of a better fix for 3.0.2 or 3.0.3) is to change the line above to

 if ((defined $cgi->param('id') && Bugzilla->usage_mode != USAGE_MODE_EMAIL)
     || defined $cgi->param("bit-$b"))

This way, if you explicitly pass bit-X in the email, it will be interpreted correctly: bit-X = 1 means restrict the bug to this group, bit-X = 0 means remove the bug from this group, and bit-X = undef (i.e. not given) means ignore this group. There is no risk to bypass security checks as process_bug.cgi only looks at groups you are allowed to edit.

2) Together with the fix above, email_in.pl could automatically generate the bit-X params based on data passed per email. For instance, if I write:

 @add_groups = security, webtools
 @remove_groups = management

then the corresponding bit-X params would be generated and set to either 0 or 1. Those not reported here are ignored. This syntax is only a proof of concept; mkanat may have another opinion on this.
Comment 11 Frédéric Buclin 2007-08-07 16:14:17 PDT
Now that we know the exact reason of the problem, it clearly blocks 3.0.1 as using email_in.pl basically means "remove the bug from all non-mandatory" groups, which is pretty critical on all installations using email_in.pl.
Comment 12 Frédéric Buclin 2007-08-07 16:16:38 PDT
And while we are on it, we really have to report this bug in the release notes for 3.0.1 as all maintainers should know about it before enabling it on their installations.
Comment 13 Trent Lillehaugen 2007-08-07 17:15:23 PDT
(In reply to comment #5)
> This is a workaround, not a real fix. We should still be allowed to edit group
> settings using email_in.pl.

I didn't realize that it was possible to modify the group settings via email using lines such as:
@bit-X = 1

If that's the case, and people actually do that, then I agree with you - my first patch is a crude workaround that disables the group settings completely from email_in.pl.

I will put together a better patch and if someone blesses the syntax I can certainly try to include Frédéric's feature:

(from comment #10)
>2) Together with the fix above, email_in.pl could automatically generate the
>bit-X params based on data passed per email. For instance, if I write:
>
> @add_groups = security, webtools
> @remove_groups = management

Comment 14 Frédéric Buclin 2007-08-07 17:35:01 PDT
(In reply to comment #13)
> I will put together a better patch and if someone blesses the syntax I can
> certainly try to include Frédéric's feature:

I suggest to attach the trivial fix for 3.0.1 as we plan to release it in the coming days. We can implement 2) later. Also make sure that @bit-X = 1 and @bit-X = 0 both work as expected. I have no way to test email_in.pl on my local installation and so I wouldn't feel confident approving 2) so close from the 3.0.1 release.
Comment 15 Trent Lillehaugen 2007-08-08 08:54:25 PDT
Created attachment 275782 [details] [diff] [review]
Patch for 3.0.1, v2

(In reply to comment #14)
> (In reply to comment #13)
> > I will put together a better patch and if someone blesses the syntax I can
> > certainly try to include Frédéric's feature:

> I suggest to attach the trivial fix for 3.0.1 as we plan to release it in 
> the coming days. We can implement 2) later. Also make sure that @bit-X = 1 
> and @bit-X = 0 both work as expected. I have no way to test email_in.pl on
> my local installation and so I wouldn't feel confident approving 2) so close
> from the 3.0.1 release.

This patch is better than the original one I provided (attachment #275500 [details] [diff] [review]) as it still allows explicit changes to the group settings in the email body via lines such as:
@bit-XX = 1 
@bit-XX = 0

It is the code suggested by Frédéric in comment #10.  I tested it on my installation here and it works as expected.
Comment 16 Frédéric Buclin 2007-08-08 08:59:52 PDT
Comment on attachment 275782 [details] [diff] [review]
Patch for 3.0.1, v2

>+    # state of the specified group (by including lines such as "@bit-XX
>+    # = 1 or @bit-XX = 0 in the body of the email).

Nit: = 1 should be on the previous line, for readability. This can easily be fixed on checkin. r=LpSolit
Comment 17 Frédéric Buclin 2007-08-08 09:01:36 PDT
This patch doesn't apply on the 3.1 code. Could you attach one for it too?
Comment 18 Trent Lillehaugen 2007-08-08 15:13:28 PDT
Created attachment 275857 [details] [diff] [review]
Patch for 3.1 - v1

I took a slightly different approach for the fix in 3.1.

I made a change in email_in.pl so that for each group the bug currently belongs to, if the bit-XX field is not set or cleared explicitly in the email body we will set it to 1.

The group logic in process_bug.cgi is more complex now - and by making this fix in email_in.pl it keeps process_bug.cgi sane.  It made more sense for me to have email_in.cgi give process_bug.cgi what it was looking for rather than complicating the logic in process_bug.cgi
Comment 19 Frédéric Buclin 2007-08-08 17:51:20 PDT
Comment on attachment 275857 [details] [diff] [review]
Patch for 3.1 - v1

>+    foreach my $gid (keys %original_groups) {
>+        if(not defined $fields{"bit-$gid"}) {
>+            $fields{"bit-$gid"} = 1;
>+        }
>+    }

You mostly have no way to know the group ID. email_in.pl should accept group names and convert them to their ID itself. It's like filing a new bug and email_in.pl would take the product ID instead of the product name. This would be a nightmare.

Also, you don't need to rewrite the SQL query from process_bug.cgi. You can simply use $bug->groups_in as $bug is already defined.
Comment 20 Frédéric Buclin 2007-08-09 05:46:54 PDT
I'm going to check in the patch for 3.0.1. For 3.1, the syntax should be of the form:

@group = foo
@group = bar
@group_remove = baz

I think bit-XX should be ignored to force the use of the syntax above.
Comment 21 Frédéric Buclin 2007-08-09 05:56:34 PDT
3.0:

Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.351.2.6; previous revision: 1.351.2.5
done
Comment 22 Trent Lillehaugen 2007-08-09 08:53:58 PDT
(In reply to comment #20)
> For 3.1, the syntax should be of the
> form:
> 
> @group = foo
> @group = bar
> @group_remove = baz
> 
> I think bit-XX should be ignored to force the use of the syntax above.
> 

I think we should fix this bug in 3.1 first (with a modified version of attachment #275857 [details] [diff] [review]; were I remove the SQL query as you pointed out in comment #19) then add this new feature - preferably as a new feature enhancement bug.  

Although the two tasks are related - they are mutually exclusive.  Adding the automatic name to bit-xx translation does not address the original problem where all the group flags get cleared.
Comment 23 Frédéric Buclin 2007-08-09 09:13:19 PDT
OK, I will try to be clearer:

The first step is to fix process_bug.cgi to behave the same way as you did for 3.0.1. In 3.1, you have to fix line 1207 of process_bug.cgi:

        elsif (($user->in_group_id($gid) || $product_change)
               && (defined $cgi->param('id') || defined $cgi->param("bit-$gid")))

You probably recognize |defined $cgi->param('id')|. ;) That's what we will take for 3.1.1 as we are going to release it at the same time as 3.0.1.

Then the second step about the group name -> id conversion which will generate required bit-XX fields can indeed be implemented in a separate bug, but should be done before we release Bugzilla 3.2, i.e. you have at least 2 months to do it. Is this roadmap ok for you?
Comment 24 Max Kanat-Alexander 2007-08-09 13:43:28 PDT
Could you also check in that fix on 3.1 for now so that we don't release 3.1.1 with such a problem, and open another bug for the "right" fix?
Comment 25 Frédéric Buclin 2007-08-09 13:47:30 PDT
(In reply to comment #24)
> Could you also check in that fix on 3.1 for now so that we don't release 3.1.1
> with such a problem, and open another bug for the "right" fix?

Isn't it what I said in my previous comment? :)

> You probably recognize |defined $cgi->param('id')|. ;) That's what we will take
> for 3.1.1 as we are going to release it at the same time as 3.0.1.
> 
> Then the second step about the group name -> id conversion which will generate
> required bit-XX fields can indeed be implemented in a separate bug
Comment 26 Max Kanat-Alexander 2007-08-09 14:07:45 PDT
(In reply to comment #25)
> (In reply to comment #24)
> > Could you also check in that fix on 3.1 for now so that we don't release 3.1.1
> > with such a problem, and open another bug for the "right" fix?
> 
> Isn't it what I said in my previous comment? :)

  Oh, yep! :-) Just didn't realize that was exactly what you meant.
Comment 27 Trent Lillehaugen 2007-08-09 16:56:13 PDT
Just to re-iterate what I said in comment #18: attachment #275857 [details] [diff] [review] does implement the fix you want for 3.1.1 just in a different manner.  As I said, I think it makes more sense to put the work-around in email_in.pl rather than process_bug.pl.  I tested this patch and it works correctly.  The only improvement I could see making would be to get rid of the extra SQL query as Frédéric suggested.

We can certainly fix it by modifying process_bug.pl again - but I won't be able to try that out and test it until later tomorrow.

BTW, doesn't it seem like there have been a LOT of comments for such a small patch? :)
Comment 28 Frédéric Buclin 2007-08-09 16:59:59 PDT
(In reply to comment #27)
> implement the fix you want for 3.1.1 just in a different manner.  As I said, I
> think it makes more sense to put the work-around in email_in.pl rather than
> process_bug.pl.

No, this won't do it on product change. All this logic is in process_bug.cgi itself. The fix in process_bug.cgi is a trivial one-liner while fixing email_in.pl is much more invasive.
Comment 29 Max Kanat-Alexander 2007-08-09 19:16:45 PDT
(In reply to comment #27)
> BTW, doesn't it seem like there have been a LOT of comments for such a small
> patch? :)

  Oh, I think development is 90% talking and 10% coding. :-)
Comment 30 Trent Lillehaugen 2007-08-10 10:38:53 PDT
(In reply to comment #28)
> No, this won't do it on product change. All this logic is in process_bug.cgi
> itself. The fix in process_bug.cgi is a trivial one-liner while fixing
> email_in.pl is much more invasive.

It looks to me like my patch will work even during a product change.  By making the fix in email_in.pl I am presenting process_bug.pl with the same set of data it would receive when a change is done through the web interface.  There doesn't need to be additional logic in process_bug.pl to determine where the request came from - the burden is on email_in.pl to look the same as a "normal" request.  I still believe this is a cleaner solution even though it is not a one line fix.

Regardless, I tried to test it (and the one-liner fix) and ran into a problem.  I sent this email to email_in.pl to try and change the product of an existing bug:

From: xxx@xxx.xxx
Subject: [Bug 1]

@product = TestProduct 
@version = unspecified
@component = Default

Even though the version and component values are valid for the new product it sends me the "Verify Version, Component" page.  Which says:

"You are moving the bug(s) to the product TestProduct, and the version and component fields are no longer correct. Please set the correct version and component now: ..."

Is it possible to change the product of a bug via email?  I am not able to do it and would like to know how before I can fully test either of the patches.
Comment 31 Frédéric Buclin 2007-08-12 06:14:48 PDT
(In reply to comment #30)
> Is it possible to change the product of a bug via email?

Max?
Comment 32 Max Kanat-Alexander 2007-08-12 07:06:47 PDT
(In reply to comment #31)
> (In reply to comment #30)
> > Is it possible to change the product of a bug via email?

  It's supposed to be, yes.
Comment 33 Frédéric Buclin 2007-08-15 04:00:37 PDT
Any progress on this bug for 3.1.1? Else I will take the same fix we used in 3.0.1 as a bandaid, and make it better for 3.1.2.
Comment 34 Frédéric Buclin 2007-08-16 09:34:14 PDT
Created attachment 276965 [details] [diff] [review]
patch for 3.1, v2

This is the same fix as for 3.0.1. This will work for now. We can back this out in a later release if we manage to get email_in.pl behave nicely with groups.
Comment 35 Max Kanat-Alexander 2007-08-16 10:14:45 PDT
Comment on attachment 276965 [details] [diff] [review]
patch for 3.1, v2

Great. This is what I wanted to be done in this bug anyway.
Comment 36 Frédéric Buclin 2007-08-16 10:33:01 PDT
File another bug if you want email_in.pl to manage groups in a better way.

tip:

Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.383; previous revision: 1.382
done
Comment 37 Frédéric Buclin 2008-01-04 06:19:37 PST
Has been relnoted in 3.0.1.

Note You need to log in before you can comment on or make changes to this bug.