Re: Phabricator and confidential reviews

2017-09-02 Thread Randell Jesup
>> 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

2017-08-28 Thread Mark Côté
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

2017-08-25 Thread Randell Jesup
>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

2017-08-10 Thread Frederik Braun
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

2017-08-09 Thread Daniel Veditz
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

2017-08-09 Thread Mark Côté
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

2017-08-09 Thread Daniel Veditz
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

2017-08-09 Thread Daniel Veditz
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

2017-08-09 Thread Daniel Veditz
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

2017-08-09 Thread Ehsan Akhgari

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

2017-08-09 Thread Gijs Kruitbosch

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

2017-08-09 Thread Axel Hecht

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 one-way mirroring, from BMO to Differential, sufficient?

Re: Phabricator and confidential reviews

2017-08-09 Thread Nicolas B. Pierron

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

2017-08-08 Thread 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 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