1541155 - Bug bounty attachments attached by people that are no longer members of the bounty group are not considered to be bounty attachments

Status: VERIFIED FIXED

(bugzilla.mozilla.org :: Extensions, defect, P2)

Description

[Al Billings [:abillings - ex-MoCo]]

We have a custom bug bounty attachment form created in bug 1062775. Per bug 1103837, when a user clicks on a form that is entered, it should edit the attachment as a bug bounty form item. I expect that it should also not have a "Add Bounty Tracking Attachment" item under "Attachments" if there is already a bounty attachment.

This seems to be broken for some older bugs. I noticed today when looking at bug 1317023 from 2018 and bug 1318911 from 2017 today, which had attachments added in 2018 and 2017, that the attachments are not being recognized as bug bounty attachments. Both the "Add a Bounty Tracking Attachment" item is listed and if you click on the existing attachment, it doesn't bring up the bug bounty tracking form and populate it, making it impossible to easily view or edit old bounty data.

For the same reporter, I added an attachment this week for bug 1440079 and it worked fine so this is some kind of issue with older data but I don't know how far back it goes. Since we have bounty attachments going to 2014 and I'm seeing this on a 2018 bug, I expect this issue is affecting many of our bounty bugs.

Comment 1

[Daniel Veditz [:dveditz]]

The mime types and filenames match current bounty attachments. How are these known to be bounty forms?

Both of the bugs mentioned in comment 0 had attachments from a currently disabled account (a @mozilla.com address for someone who left the company). Is that part of it?

Comment 2

[Dylan Hardison [:dylan] (he/him)]

sub _attachment_is_bounty_attachment {
  my ($attachment) = @_;

  return 0 unless $attachment->filename eq 'bugbounty.data';
  return 0 unless $attachment->contenttype eq 'text/plain';
  return 0 unless $attachment->isprivate;
  return 0 unless $attachment->attacher->in_group('bounty-team');

  return $attachment->description =~ /^(?:[^,]*,)+[^,]*$/;
}

So probably want to remove the group check.

Comment 3

[Daniel Veditz [:dveditz]]

I put raymond's disabled account back into the bounty-team group and the attachments magically turned back into bounty forms. That's OK in this case because it's a disabled account so even if he's turned evil he can't abuse it :-) but in the bigger picture that means lots of dataloss when someone leaves the company :-(

Comment 4

[Daniel Veditz [:dveditz]]

Yeah, I think we need to remove the group check and can rely on the other three things -- only members of the insidergroup can set that and we have to trust those folks already. We can edit the attachment details if we have to should someone abuse it (as I did in invalid bug 1349150)

Comment 5

[Daniel Veditz [:dveditz]]

We also won't be looking for or relying on these attachments unless the sec-bounty flag is set, and for that one the bounty-team membership is checked at the time the data is changed, not retrospectively based on the current group membership of the account which set it. That should work fine.

Comment 6

[Daniel Veditz [:dveditz]]

Now that abillings left the company and was removed from the bounty-team group this has become a much bigger problem: he has created 80-90% of these over the past several years. Since his BMO account is still active the hack I used in comment 3 is not appropriate.

Emma: as the triage owner can we assign this to someone more likely to fix it than Dylan at this point? The comment 2 suggestion of just removing the group check seems a reasonable place to start.

Comment 7

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

:dveditz, can you give this bug a priority? Do we need to resolve it now?

Comment 8

[Daniel Veditz [:dveditz]]

We have nearly 1400 of these, and most of them are going to be wrong like bug 1524246 instead of correctly formatted like bug 1574265 (most people won't be able to see those attachments, but you should). We don't often need to edit these after the fact so mucking with the description text manually can be done (carefully, so as not to break the syntax). I'm more concerned that the UI will now allow us to add a second bounty attachment for these bugs and that will mess up reporting.

It's not an emergency, but we should fix it soon. "P2"?

From comment 2 it should just be a matter of deleting return 0 unless $attachment->attacher->in_group('bounty-team'); by someone who's already fixing something in BMO (Israel? Glob?).

Comment 9

[:glob ✱]

Attachment: GitHub Pull Request

Comment 10

[David Lawrence [:dkl]]

Merged to master.

Comment 11

[Daniel Veditz [:dveditz]]

Thanks, works great!