Re: Phabricator and confidential reviews
>> Bite the bullet and at least make all CC'd people able to see all >> patches, always. It's needed. > >Yeah, that's the direction I think we should take. Good, thanks. >For now, we will implement exact syncing of the CC list + reporter as the >revision's subscriber list. This means that anyone being added as a >subscriber who isn't CCed will unfortunately be quickly removed (we may >even prevent manual additions to the subscriber list). The reasoning here >is that I believe it's important that anyone looking at the bug knows >exactly who can see the patch. If the two lists are kept in sync, it will >be obvious. However, if we let people add subscribers directly, then it >may turn out that a bug with only, say, 10 CCed people might have a patch >that 100 people can view, and that's probably something you don't want to >hide. > >We'll also investigate what bidirectional syncing would take. Due to race >conditions and circular references, though, it won't be straightforward, >and I suspect our time would be best spent elsewhere. Don't bother with bidirectional syncing - your point about being able to look at the bug and know who can see it is important, but I don't see any advantage in subscribe mirroring into cc. I would (as you suggest) block subscribe on sec issues, to avoid confusion (if you can, tell people to use cc, but that's just gravy). -- Randell Jesup, Mozilla Corp remove "news" for personal email ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Phabricator and confidential reviews
On Saturday, 26 August 2017 00:40:08 UTC-4, Randell Jesup wrote: > >And don't forget reporter and assignees. Occasionally a reporter not in the > >security group will notice that a patch is insufficient which is nicer to > >find before the patch is committed than after the commit link is added to > >the bug. Sometimes someone other than the official assignee adds a patch > >for an alternate approach to a fix and asks the assignee for feedback, > >though that's uncommon and the assignee could just be manually subscribed > >to the patch at that point. > > In fact it's quite common for people who are cc'd other than the patch > writer and reviewer(s) to look at the bug and comment on it, or to > submit an alternate or modified version of a patch previously uploaded. > And as Dan points out, frequently the reporter (when an external > 'researcher') will weigh in on the patches being posted or use them to > verify if they fix the problem they found (since many times only they > can reliably reproduce the problem - private test setups, drivers, HW, etc). Right, it's easy to add the reporter since that person won't change. The assignee will also automatically have access if they are the patch author. I also got some data from BMO that strongly suggests that, indeed, many people who aren't the author or reviewer look at sec-bug patches, and most of those aren't in the sec group; they have access by being CCed in. That's surprising to me, but it definitely suggests the need for syncing the bug's CC list to the revision's subscriber list. > >We can wait and see how common the "please subscribe me to the patch" > >requests are, but I suspect they'll be common enough. > > Bite the bullet and at least make all CC'd people able to see all > patches, always. It's needed. Yeah, that's the direction I think we should take. For now, we will implement exact syncing of the CC list + reporter as the revision's subscriber list. This means that anyone being added as a subscriber who isn't CCed will unfortunately be quickly removed (we may even prevent manual additions to the subscriber list). The reasoning here is that I believe it's important that anyone looking at the bug knows exactly who can see the patch. If the two lists are kept in sync, it will be obvious. However, if we let people add subscribers directly, then it may turn out that a bug with only, say, 10 CCed people might have a patch that 100 people can view, and that's probably something you don't want to hide. We'll also investigate what bidirectional syncing would take. Due to race conditions and circular references, though, it won't be straightforward, and I suspect our time would be best spent elsewhere. We can and will revisit this after Phabricator goes live for everyone. > Another aspect of all this that needs to be thought out and verified is > what happens when a non-sec bug becomes a sec bug (and vice-versa, > though I'm far less concerned about that). When a bug becomes a sec > bug, all patches associated with that bug must become confidential. And > when a bug moves to be open (not sec-restricted), the patches should > also become open. Of course. This is already implemented. Mark ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Phabricator and confidential reviews
>On Wed, Aug 9, 2017 at 11:32 AM, Mark Côté wrote: > >> I actually like Gijs's proposal, to mirror *from* Phabricator *to* BMO. >> That way, if you're looking at the bug and want to pull someone in, you CC >> them; if you're looking at the fix and want to involve someone, you add >> them as a subscriber which then incidentally lets them view the bug, for >> background and updates and such. > > >What if there's not "the" fix but multiple patches? This is quite common >for security bugs where a testcase that reveals the vulnerability is done >in a separate patch so it can be checked in after we release the fix. Or >occasionally bugs like https://bugzilla.mozilla.org/show_bug.cgi?id=1371259 >that had 9 separate patches. Would the same people have to be separately >subscribed to each? I've seen landings (for non-security bugs) that involved up to 100 patches on one bug... or even more. While that's probably never happened for a sec bug, multiple patches is not unusual (and in fact common where there's a test added). >And don't forget reporter and assignees. Occasionally a reporter not in the >security group will notice that a patch is insufficient which is nicer to >find before the patch is committed than after the commit link is added to >the bug. Sometimes someone other than the official assignee adds a patch >for an alternate approach to a fix and asks the assignee for feedback, >though that's uncommon and the assignee could just be manually subscribed >to the patch at that point. In fact it's quite common for people who are cc'd other than the patch writer and reviewer(s) to look at the bug and comment on it, or to submit an alternate or modified version of a patch previously uploaded. And as Dan points out, frequently the reporter (when an external 'researcher') will weigh in on the patches being posted or use them to verify if they fix the problem they found (since many times only they can reliably reproduce the problem - private test setups, drivers, HW, etc). >We can wait and see how common the "please subscribe me to the patch" >requests are, but I suspect they'll be common enough. Bite the bullet and at least make all CC'd people able to see all patches, always. It's needed. Another aspect of all this that needs to be thought out and verified is what happens when a non-sec bug becomes a sec bug (and vice-versa, though I'm far less concerned about that). When a bug becomes a sec bug, all patches associated with that bug must become confidential. And when a bug moves to be open (not sec-restricted), the patches should also become open. -- Randell Jesup, Mozilla Corp remove "news" for personal email ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Phabricator and confidential reviews
Having both reported, fixed and reviewed security bugs, I feel an uni-directional sync from Phabricator to BMO is not going to cut it. I think it will be unexpected for most users and might just lead to additional "why can I not see the patch" bug comments. I understand that it's more work, but I really think we should aim for "perfect" rather than "good enough", when it comes to developer experience. I don't see a strong "security reason" not to include people copied in the bug to see the patch. Quite contrary: It's also what distinguishes our bug bounty program from others. We're more open, we have outside reporters work with us on the patch and see all the comments and iterations that lead to its perfection. This encourages early feedback and improves the outcome. On 10.08.2017 00:57, Daniel Veditz wrote: > On Wed, Aug 9, 2017 at 11:32 AM, Mark Côté wrote: > >> I actually like Gijs's proposal, to mirror *from* Phabricator *to* BMO. >> That way, if you're looking at the bug and want to pull someone in, you CC >> them; if you're looking at the fix and want to involve someone, you add >> them as a subscriber which then incidentally lets them view the bug, for >> background and updates and such. > > > What if there's not "the" fix but multiple patches? This is quite common > for security bugs where a testcase that reveals the vulnerability is done > in a separate patch so it can be checked in after we release the fix. Or > occasionally bugs like https://bugzilla.mozilla.org/show_bug.cgi?id=1371259 > that had 9 separate patches. Would the same people have to be separately > subscribed to each? > > There's a case to be made for adding in both directions. How much work > would it be to (add all CCs to subscriber list) when attaching a new patch, > and (subscribe person to all existing patches) when someone new is CC'd to > a bug? This was my attempt to match your "one time mirroring" approach to > going the other way, which makes sense to me. > > And don't forget reporter and assignees. Occasionally a reporter not in the > security group will notice that a patch is insufficient which is nicer to > find before the patch is committed than after the commit link is added to > the bug. Sometimes someone other than the official assignee adds a patch > for an alternate approach to a fix and asks the assignee for feedback, > though that's uncommon and the assignee could just be manually subscribed > to the patch at that point. > > We can wait and see how common the "please subscribe me to the patch" > requests are, but I suspect they'll be common enough. > > - > Dan Veditz > ___ > dev-platform mailing list > dev-platform@lists.mozilla.org > https://lists.mozilla.org/listinfo/dev-platform > ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Phabricator and confidential reviews
On Wed, Aug 9, 2017 at 11:32 AM, Mark Côté wrote: > I actually like Gijs's proposal, to mirror *from* Phabricator *to* BMO. > That way, if you're looking at the bug and want to pull someone in, you CC > them; if you're looking at the fix and want to involve someone, you add > them as a subscriber which then incidentally lets them view the bug, for > background and updates and such. What if there's not "the" fix but multiple patches? This is quite common for security bugs where a testcase that reveals the vulnerability is done in a separate patch so it can be checked in after we release the fix. Or occasionally bugs like https://bugzilla.mozilla.org/show_bug.cgi?id=1371259 that had 9 separate patches. Would the same people have to be separately subscribed to each? There's a case to be made for adding in both directions. How much work would it be to (add all CCs to subscriber list) when attaching a new patch, and (subscribe person to all existing patches) when someone new is CC'd to a bug? This was my attempt to match your "one time mirroring" approach to going the other way, which makes sense to me. And don't forget reporter and assignees. Occasionally a reporter not in the security group will notice that a patch is insufficient which is nicer to find before the patch is committed than after the commit link is added to the bug. Sometimes someone other than the official assignee adds a patch for an alternate approach to a fix and asks the assignee for feedback, though that's uncommon and the assignee could just be manually subscribed to the patch at that point. We can wait and see how common the "please subscribe me to the patch" requests are, but I suspect they'll be common enough. - Dan Veditz ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Phabricator and confidential reviews
For brevity and clarity I'm just replying to Dan here, but I am attempting to address other points raised so far in this thread. On Wednesday, 9 August 2017 13:07:08 UTC-4, Daniel Veditz wrote: > On Tue, Aug 8, 2017 at 5:30 PM, Mark Côté wrote: > > > I am not sure how often CCed users are involved with confidential bugs' > > patches > > [ > > ] Anecdotally I have been told that a lot of the time users are CCed > > just to be informed of the problem, e.g. a manager might want to be aware > > of a vulnerability. > > > > People are CC'd for both reasons quite often. Couldn't tell you if it's > 50-50 or 30-70. I think Gijs's post is quite interesting. It furthers my idea that there are two main reasons you get involved in a confidential bug: either someone figures you should know about the issue, or you are pulled in to review the patch. (Although there are some outliers, like Ehsan, who fall under both.) I actually like Gijs's proposal, to mirror *from* Phabricator *to* BMO. That way, if you're looking at the bug and want to pull someone in, you CC them; if you're looking at the fix and want to involve someone, you add them as a subscriber which then incidentally lets them view the bug, for background and updates and such. We can also add a clear comment ("user X has been CCed to this bug via Differential revision Y" or such). I think we'd want to simplify here, though, and make such mirroing "one time", meaning you get CCed when added as a subscriber, but you wouldn't be removed from the CC list of the bug if removed as a subscriber, since people may be being directly CCed to the bug as well. > > Given that adding subscribers to a revision is just as easy as CCing a > > user on a bug, if it is infrequent that outsiders need to be involved in > > reviewing confidential patches, I lean towards taking the simple route of > > making this manual. > > > > Can someone who is the assignee of a confidential bug, but is not in the > security group, create a confidential Phabricator patch for that bug? That > case happens quite often as some simpler security bugs are taken by the > reporter or assigned to newer team members. If so then I guess we could try > the manual approach as a MVP until it annoys people too much. Yes. You won't be able to submit a revision associated with a bug you don't have access to, but as long as you can attach a file to the bug, you can create an associated revision. > The second question that would come up is whether this synchronization > > should apply to all revisions or just confidential ones. > > > For a confidential bug, why would some revisions be confidential and some > not? It's not uncommon for a bug to be marked confidential after work has > started on it and security ramifications become clear. We'd want that > confidentiality to apply to the patches that already exist because they may > provide outsiders the same hints about the problem they gave us. Sorry, I was not clear here. I meant whether we'd want to do any mirroring of CC/subscriber lists for public bugs. (This was rephrased as question 3 in my summary). Seems that the answer continues to be "no" here. > 1. Is mirroring a confidential bug's CC list to association Differential > > revisions' subscriber lists actually useful? That is, does the utility > > justify the cost of implementation and maintenance? > > > > How much work is it? Hard to know how much work reducing the annoyance and > friction is worth. Yeah that's going to depend on what solution we come up with. :) But my proposed solution above wouldn't be too difficult, as we are already doing some revision->bug updates anyway. > 2. If yes, is one-way mirroring, from BMO to Differential, sufficient? > > > > Probably not, especially if you don't block subscribing people for > confidential bugs. People will be blindsided if they've subscribed people > and then it gets wiped out. Being told you have to go to BMO for that bug > will be an annoyance, but at least you won't think you've given someone > access when you really haven't and then wonder why they never review your > patch. I think my above proposal addresses this, particularly with comments indicating when someone has been automatically added to the CC list. Thanks everyone for your input so far. Mark ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Phabricator and confidential reviews
On Tue, Aug 8, 2017 at 5:30 PM, Mark Côté wrote: > I am not sure how often CCed users are involved with confidential bugs' > patches > [ > ] Anecdotally I have been told that a lot of the time users are CCed > just to be informed of the problem, e.g. a manager might want to be aware > of a vulnerability. > People are CC'd for both reasons quite often. Couldn't tell you if it's 50-50 or 30-70. > Given that adding subscribers to a revision is just as easy as CCing a > user on a bug, if it is infrequent that outsiders need to be involved in > reviewing confidential patches, I lean towards taking the simple route of > making this manual. > Can someone who is the assignee of a confidential bug, but is not in the security group, create a confidential Phabricator patch for that bug? That case happens quite often as some simpler security bugs are taken by the reporter or assigned to newer team members. If so then I guess we could try the manual approach as a MVP until it annoys people too much. The second question that would come up is whether this synchronization > should apply to all revisions or just confidential ones. For a confidential bug, why would some revisions be confidential and some not? It's not uncommon for a bug to be marked confidential after work has started on it and security ramifications become clear. We'd want that confidentiality to apply to the patches that already exist because they may provide outsiders the same hints about the problem they gave us. 1. Is mirroring a confidential bug's CC list to association Differential > revisions' subscriber lists actually useful? That is, does the utility > justify the cost of implementation and maintenance? > How much work is it? Hard to know how much work reducing the annoyance and friction is worth. 2. If yes, is one-way mirroring, from BMO to Differential, sufficient? > Probably not, especially if you don't block subscribing people for confidential bugs. People will be blindsided if they've subscribed people and then it gets wiped out. Being told you have to go to BMO for that bug will be an annoyance, but at least you won't think you've given someone access when you really haven't and then wonder why they never review your patch. 3. Again if #1 is yes, should such mirroring be limited to confidential > bugs, given that Phabricator's notification system is more fine-grained, > and thus more useful, than BMO's product- and component-watching feature? > I'm on the fence about #1 (I lean toward 'yes'), but definitely only needed for confidential bugs. - Dan Veditz ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Phabricator and confidential reviews
On Wed, Aug 9, 2017 at 12:20 AM, Axel Hecht wrote: > I think we should strive to have as few people as possible with general > access to security bugs. We do. We've reduced the number of people with access, and split the "client" security group into ~10 sub groups so that any given person has access to a smaller subset of security bugs that are more likely to be relevant to them. So, in that sense, I think we should make this a general assumption, that > the folks writing patches and doing reviews are not in group of the bug. > We would get little done if we couldn't CC people to give them access. Many of those people are crucial to the production of the patch itself (as the author or reviewer) and will need access to the Phabricator version. - Dan Veditz ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Phabricator and confidential reviews
On Tue, Aug 8, 2017 at 11:38 PM, Nicolas B. Pierron < nicolas.b.pier...@mozilla.com> wrote: > However, users outside of the security group(s) can see confidential bugs >> if they are involved with them in some way. Frequently the CC field is >> used as a way to include outsiders in a bug. > > > Note that Bugzilla warns us against adding people who do not have s-s > access to s-s bug. (This is an awesome feature by the way) > It really shouldn't. Unless we expand the group of people who can see security bugs to thousands of people (everyone with an NDA? even that might not be enough) there will always be people who need to see a bug who weren't able to see it by default. Since we have the CC'ing mechanism we can keep the "default" group small and then freely CC people as needed. I only know of two such warnings. 1) when you needinfo? someone who can't see a bug. That's warning you that they won't ever see your request, not that you shouldn't add them to the bug. If it were the latter we'd also be warning every time you CC someone on a hidden bug. Since a named request is obviously inviting that person into the bug we should just automatically CC that person at the same time. 2) when duping a bug. Normally when you dupe a bug the reporter of the dupe is silently CC'd to the active bug. For security bugs the warning makes this a conscious choice. Most of the time I'd say "sure, go ahead": the reporter already knows about the issue, they might as well continue to be involved in the solution. There are cases, though, where that's not true so it's good to have people make a conscious choice. We might not want to CC the dupe reporter if the active bug is not an identical dupe but is instead a broader issue, or if the dupe target has a more damaging example that the dupe reporter hadn't thought of yet. Sometimes people dupe bugs to one that will fix it, but isn't the same kind of testcase. When it's a security bug I usually prefer that we mark those as "Depends on" the other bug and leave them open so we can verify the fix later. As a bonus, then the CC'ing issue doesn't come up. - Dan Veditz ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Phabricator and confidential reviews
On 08/08/2017 08:30 PM, Mark Côté wrote: First I want to double check that this is truly useful. I am not sure how often CCed users are involved with confidential bugs' patches (I might be able to ballpark this with some Bugzilla searches, but I don't think it would be easy to get a straight answer). No, it wouldn't be. Right now we're living in a world where the discussions about patches happen in the bug, which means parts of the patch often get quoted on the bug, which may make people want to look at the patch while they're on the bug page, whereas if they were following the same conversation on Phabricator perhaps they may find it sufficient to follow code related discussions there. Or perhaps not, maybe old culture turns out to be hard to change and code related conversations still find their way back into bugs, which would make people reading the bug naturally want to look at the code from the bug. It's impossible for me to tell which way it will work. Anecdotally I have been told that a lot of the time users are CCed just to be informed of the problem, e.g. a manager might want to be aware of a vulnerability. Given that adding subscribers to a revision is just as easy as CCing a user on a bug, if it is infrequent that outsiders need to be involved in reviewing confidential patches, I lean towards taking the simple route of making this manua My anecdotal experience is that I almost always look at the patch no matter what the security bug is about in order to try to see if the patch reveals a pattern that we can build a static analysis around in order to prevent further occurrences of the bug, so the proposal of requiring an extra step of finding someone to CC you on the review in order to see the patch will introduce a hurdle for me when I look at bugs in the components where I'm not in the security group for (aka, most components.) That being said, I don't do this every day, and it's not clear what the cost of implementing the syncing proposal is, so I'm not sure what the right trade-off should be here. (BTW, do we have data about this from Bugzilla server logs, for example about how often patch attachments on security sensitive bugs are viewed by people who are recently CCed?) Cheers, Ehsan ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Phabricator and confidential reviews
On 09/08/2017 01:30, Mark Côté wrote: If you have any thoughts on this, please reply. I'll answer any questions and summarize the feedback with a decision in a few days. Note that we can, of course, try a simple approach to start, and add in more complex functionality after an evaluation period. To sum up, there are three questions: 1. Is mirroring a confidential bug's CC list to association Differential revisions' subscriber lists actually useful? That is, does the utility justify the cost of implementation and maintenance? Probably not as you're describing here, but read on. 2. If yes, is one-way mirroring, from BMO to Differential, sufficient? I hit the case of "I need info / a review from person X on this security bug that they can't currently see", and the associated modal warning that :nbp mentioned, so often that I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1211377 (please read esp. the bottom of comment #0). So yes, people get CCd on sec bugs if/when required for reviews/needinfo, and because we keep security groups relatively small (for the reasons Axel stated), that means I end up using CCs heavily in order to get work done on those bugs. I think generally, what I would really want, rather than mirroring the entire CC list, is making it painless to tell bugzilla/mozreview/differential/whatever "here's a patch for confidential bug X, request review from Y", and have whatever tool is doing that not make me jump through 42 hoops to allow Y to see/do the review. A warning would be acceptable and maybe useful, but I want to be able to say "yes, I know X is a sec bug, and I know Y can't currently see it, please do whatever is necessary to make Y see it anyway". Oddly, that might mean that I actually want mirroring from differential to BMO, but NOT the other way. That is, if I post a patch for a bug and ask someone for review, they should be able to see the bug. :-) I can always add people to the differential thing manually if there are non-reviewers and non-sec-group-members who really need to see the patch, right? (Seems much less likely!) 3. Again if #1 is yes, should such mirroring be limited to confidential bugs, given that Phabricator's notification system is more fine-grained, and thus more useful, than BMO's product- and component-watching feature? Yes. ~ Gijs ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Phabricator and confidential reviews
To answer the question not asked ;-) I think we should strive to have as few people as possible with general access to security bugs. The concerns the folks have when crossing borders is awful. And just from a general risk profile. Saying that as someone that neither has nor wants access to security bugs in general. So, in that sense, I think we should make this a general assumption, that the folks writing patches and doing reviews are not in group of the bug. As to mirroring the information, I think it'd be good if there couldn't be people on phabricator that are not on the bug. That way, the folks on the bug that manage the security risk of it have one way of tracking the visibility of the security issue. I could see how not everybody involved in the bug automatically wants all the bugmail. I personally wouldn't mind if I could opt-in to that, but I'd be annoyed if I had to ask folks to manually add me again, after I asked them to CC me to the bug. More a question if it's technically feasible, could folks ask phabricator for access by following the link, and it would go back and check bugzilla on-demand if it should do that? Depends a bit on the additional private-attachment thing that Nicolas mentioned. Axel Am 09.08.17 um 02:30 schrieb Mark Côté: (Cross-posted to mozilla.tools) Hi, I have an update and a request for comments regarding Phabricator and confidential reviews. We've completed the functionality around limiting access to Differential revisions (i.e. code reviews) that are tied to confidential bugs. To recap the original plan, various security groups in BMO are mirrored to Phabricator as "projects", containing the same set of users. When a bug has such a security group added to it, e.g. dom-core-security, thus restricting its visibility largely to members of that group, a Phabricator "policy" is similarly set on any associated revisions, restricting their visibility to the same group of people (plus the author of the revision, if they are not in the project already). However, users outside of the security group(s) can see confidential bugs if they are involved with them in some way. Frequently the CC field is used as a way to include outsiders in a bug. Phabricator has a similar feature, called "subscribers", which, as with CCs, both grants visibility to confidential revisions and also sends email updates when the revision changes. It was suggested that we attempt to synchronize CC and subscriber lists. First I want to double check that this is truly useful. I am not sure how often CCed users are involved with confidential bugs' patches (I might be able to ballpark this with some Bugzilla searches, but I don't think it would be easy to get a straight answer). Anecdotally I have been told that a lot of the time users are CCed just to be informed of the problem, e.g. a manager might want to be aware of a vulnerability. Given that adding subscribers to a revision is just as easy as CCing a user on a bug, if it is infrequent that outsiders need to be involved in reviewing confidential patches, I lean towards taking the simple route of making this manual. However if this is more common than I suspect, then we must decide how to synchronize the lists. The most straightforward approach is one-way synchronization from BMO, that is, anyone CCed on the bug will automatically be added as a subscriber to any associated revisions, but anyone manually added to the subscribers list who is not CCed on the bug would be automatically removed by the BMO-Phabricator synchronization routine. The alternative is to keep track of who was manually added and who was automatically synchronized, which gets complicated rather quickly, both in terms of implementation and usability. The second question that would come up is whether this synchronization should apply to all revisions or just confidential ones. Given the dual nature of CCs/subscribers, for both visibility and notifications, I lean towards only doing this synchronization for confidential revisions, where it is more important. A further justification for limiting the mirroring is that Phabricator has a much more powerful and fine-grained notification system (Herald) than BMO's product- and component-watching feature. Automatic mirroring everywhere would reduce the utility of the former. If you have any thoughts on this, please reply. I'll answer any questions and summarize the feedback with a decision in a few days. Note that we can, of course, try a simple approach to start, and add in more complex functionality after an evaluation period. To sum up, there are three questions: 1. Is mirroring a confidential bug's CC list to association Differential revisions' subscriber lists actually useful? That is, does the utility justify the cost of implementation and maintenance? 2. If yes, is on
Re: Phabricator and confidential reviews
On 08/09/2017 12:30 AM, Mark Côté wrote: Hi, I have an update and a request for comments regarding Phabricator and confidential reviews. First of all, thanks for considering confidential bugs as part of this process. This was my main reason for not using moz-review. We've completed the functionality around limiting access to Differential revisions (i.e. code reviews) that are tied to confidential bugs. […] However, users outside of the security group(s) can see confidential bugs if they are involved with them in some way. Frequently the CC field is used as a way to include outsiders in a bug. Note that Bugzilla warns us against adding people who do not have s-s access to s-s bug. (This is an awesome feature by the way) […] First I want to double check that this is truly useful. […] I did that multiple time in the past. The main reason for doing it was to CC the person who contributed the patch, such that at best they can contribute a fix as well, and in the worst case they can contribute insight for fixing the issue. So, not only the CC-ed persons are asked to review, I might ask them to even submit patches to these security bugs. This is a way to gradually empower contributors, from my point of view. Also, some users can open s-s bugs, and contribute patches too. We should at least accept people from the CC list / reporters to be able to submit new patches. The second question that would come up is whether this synchronization should apply to all revisions or just confidential ones. […] Currently Bugzilla has a "private" flag on attachments, and adding anybody without s-s flags in the CC list of the bug should not have access to the private attachments, but should have access to any non-private attachments. A similar "private" flag could be used to prevent the synchronization of the CC list / reporter which are out-side the s-s group. -- Nicolas B. Pierron ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Phabricator and confidential reviews
(Cross-posted to mozilla.tools) Hi, I have an update and a request for comments regarding Phabricator and confidential reviews. We've completed the functionality around limiting access to Differential revisions (i.e. code reviews) that are tied to confidential bugs. To recap the original plan, various security groups in BMO are mirrored to Phabricator as "projects", containing the same set of users. When a bug has such a security group added to it, e.g. dom-core-security, thus restricting its visibility largely to members of that group, a Phabricator "policy" is similarly set on any associated revisions, restricting their visibility to the same group of people (plus the author of the revision, if they are not in the project already). However, users outside of the security group(s) can see confidential bugs if they are involved with them in some way. Frequently the CC field is used as a way to include outsiders in a bug. Phabricator has a similar feature, called "subscribers", which, as with CCs, both grants visibility to confidential revisions and also sends email updates when the revision changes. It was suggested that we attempt to synchronize CC and subscriber lists. First I want to double check that this is truly useful. I am not sure how often CCed users are involved with confidential bugs' patches (I might be able to ballpark this with some Bugzilla searches, but I don't think it would be easy to get a straight answer). Anecdotally I have been told that a lot of the time users are CCed just to be informed of the problem, e.g. a manager might want to be aware of a vulnerability. Given that adding subscribers to a revision is just as easy as CCing a user on a bug, if it is infrequent that outsiders need to be involved in reviewing confidential patches, I lean towards taking the simple route of making this manual. However if this is more common than I suspect, then we must decide how to synchronize the lists. The most straightforward approach is one-way synchronization from BMO, that is, anyone CCed on the bug will automatically be added as a subscriber to any associated revisions, but anyone manually added to the subscribers list who is not CCed on the bug would be automatically removed by the BMO-Phabricator synchronization routine. The alternative is to keep track of who was manually added and who was automatically synchronized, which gets complicated rather quickly, both in terms of implementation and usability. The second question that would come up is whether this synchronization should apply to all revisions or just confidential ones. Given the dual nature of CCs/subscribers, for both visibility and notifications, I lean towards only doing this synchronization for confidential revisions, where it is more important. A further justification for limiting the mirroring is that Phabricator has a much more powerful and fine-grained notification system (Herald) than BMO's product- and component-watching feature. Automatic mirroring everywhere would reduce the utility of the former. If you have any thoughts on this, please reply. I'll answer any questions and summarize the feedback with a decision in a few days. Note that we can, of course, try a simple approach to start, and add in more complex functionality after an evaluation period. To sum up, there are three questions: 1. Is mirroring a confidential bug's CC list to association Differential revisions' subscriber lists actually useful? That is, does the utility justify the cost of implementation and maintenance? 2. If yes, is one-way mirroring, from BMO to Differential, sufficient? 3. Again if #1 is yes, should such mirroring be limited to confidential bugs, given that Phabricator's notification system is more fine-grained, and thus more useful, than BMO's product- and component-watching feature? Thanks, Mark ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform