Plan for authenticating the user to RB as part of the push

[Ehsan Akhgari]

We need to have a way to associate a user with their bugzilla auth pair in order to be able to create the review request under that user name, as bugzilla will be the authentication provider to RB.

We discussed how is going to work, and this is the plan going forward:

* We will make the ssh server accept any public key (but of course won't give out shell access to people!).  This means that we will accept pushes from everybody as far as ssh is concerned.

* We will prompt on the client side for your Bugzilla user name and password, use the Bugzilla REST API <http://www.bugzilla.org/docs/tip/en/html/api/Bugzilla/WebService/Server/REST.html#AUTHENTICATION> to perform the authentication, and will store the auth token locally, and send it over to the server as part of the push.  The token can then be verified on the server side as part of the hook.

The work on the client side will be part of the ./mach review command.

Cheers,

--
Ehsan
<http://ehsanakhgari.org/>

[Gregory Szorc]

On 4/3/14, 10:23 AM, Ehsan Akhgari wrote:
> We need to have a way to associate a user with their bugzilla auth pair
> in order to be able to create the review request under that user name,
> as bugzilla will be the authentication provider to RB.
>
> We discussed how is going to work, and this is the plan going forward:
>
> * We will make the ssh server accept any public key (but of course won't
> give out shell access to people!). This means that we will accept
> pushes from everybody as far as ssh is concerned.
>
> * We will prompt on the client side for your Bugzilla user name and
> password, use the Bugzilla REST API
> <http://www.bugzilla.org/docs/tip/en/html/api/Bugzilla/WebService/Server/REST.html#AUTHENTICATION>
> to perform the authentication, and will store the auth token locally,
> and send it over to the server as part of the push. The token can then
> be verified on the server side as part of the hook.

We can go one step further.

The bzexport Mercurial extension [1] and the python-mozautomation
project [2] have Python modules to extract Bugzilla cookies from your
Firefox profile. If a user has a Firefox profile with an active Bugzilla
session, we should be able to silently piggyback on top of those
credentials. I suppose we could go one step further and obtain
credentials from Firefox's password manager.

That being said, I think there is a huge security problem here.

Correct me if I'm wrong, but I believe the Bugzilla auth token is as
good as your login credentials. The only differences are that it has
undergone a one-way transformation and expires after a shorter duration.
What you are proposing is effectively giving the code review service the
keys to *your* kingdom. I have doubts that will pass security review. If
it did, I'm guessing we'd have to bring the code review tool into the
same security scope as Bugzilla itself. And I thought we were trying to
avoid that (at least initially).

I think we'll have to settle for:

a) Having the client perform all interaction with Bugzilla itself (this
sucks because the server may want to track that state and distributed
"transactions" are hard).
b) Chained authentication with delegated Bugzilla keys. Client proves
its identity with code review server. Code review server communicates
with Bugzilla using some special "admin" account that can post as others.
c) Something like OAuth with service access keys, signed requests, etc.

Distributed service authentication is hard. But it's how you play the
game while staying secure. It's best to ask someone in the security
group what our next steps should be. Perhaps someone from BMO can
comment on Bugzilla's delegated authentication story?

[1]
https://hg.mozilla.org/hgcustom/version-control-tools/file/b4d8563b207e/hgext/bzexport/bzauth.py
[2]
https://bitbucket.org/indygreg/python-mozautomation/src/935dfb1d466f965afcbc3df524e227327c121b1f/mozautomation/bugzilla.py?at=default

[Ehsan Akhgari]

On Thu, Apr 3, 2014 at 4:54 PM, Gregory Szorc <g...@mozilla.com> wrote:

On 4/3/14, 10:23 AM, Ehsan Akhgari wrote:

We need to have a way to associate a user with their bugzilla auth pair
in order to be able to create the review request under that user name,
as bugzilla will be the authentication provider to RB.

We discussed how is going to work, and this is the plan going forward:

* We will make the ssh server accept any public key (but of course won't
give out shell access to people!).  This means that we will accept
pushes from everybody as far as ssh is concerned.

* We will prompt on the client side for your Bugzilla user name and
password, use the Bugzilla REST API
<http://www.bugzilla.org/docs/tip/en/html/api/Bugzilla/WebService/Server/REST.html#AUTHENTICATION>
to perform the authentication, and will store the auth token locally,
and send it over to the server as part of the push.  The token can then
be verified on the server side as part of the hook.

We can go one step further.

The bzexport Mercurial extension [1] and the python-mozautomation project [2] have Python modules to extract Bugzilla cookies from your Firefox profile. If a user has a Firefox profile with an active Bugzilla session, we should be able to silently piggyback on top of those credentials. I suppose we could go one step further and obtain credentials from Firefox's password manager.

Yeah, sorry I didn't mention this.  This was also discussed but this method is fragile in a lot of edge cases (some of them are the kinds of things that developers can hit regularly, such as having a Firefox profile as the default where you have not logged in to Bugzilla.)

 

That being said, I think there is a huge security problem here.

Correct me if I'm wrong, but I believe the Bugzilla auth token is as good as your login credentials. The only differences are that it has undergone a one-way transformation and expires after a shorter duration. What you are proposing is effectively giving the code review service the keys to *your* kingdom. I have doubts that will pass security review. If it did, I'm guessing we'd have to bring the code review tool into the same security scope as Bugzilla itself. And I thought we were trying to avoid that (at least initially).

That's a great point.  Is that something that we can ask the security team about?  Taras, can you please take that on?

 

I think we'll have to settle for:

a) Having the client perform all interaction with Bugzilla itself (this sucks because the server may want to track that state and distributed "transactions" are hard).
b) Chained authentication with delegated Bugzilla keys. Client proves its identity with code review server. Code review server communicates with Bugzilla using some special "admin" account that can post as others.
c) Something like OAuth with service access keys, signed requests, etc.

Option (c) has been discussed for a long time, I think, but it won't happen in time for us to be able to use it.  Also, OAuth still wouldn't give us the ideal UI-less interaction that we're striving to achieve.

I don't really know what option (b) would look like, I'm not an expert at this stuff.

Option (a) would be really unfortunate as we can all agree.  :-)

 

Distributed service authentication is hard. But it's how you play the game while staying secure. It's best to ask someone in the security group what our next steps should be. Perhaps someone from BMO can comment on Bugzilla's delegated authentication story?

[1] https://hg.mozilla.org/hgcustom/version-control-tools/file/b4d8563b207e/hgext/bzexport/bzauth.py
[2] https://bitbucket.org/indygreg/python-mozautomation/src/935dfb1d466f965afcbc3df524e227327c121b1f/mozautomation/bugzilla.py?at=default

AFAIK there is no better delegated authentication story for Bugzilla right now (mcote attended the meeting.)

I think we should try to get the security team's advice on this.

Cheers,
Ehsan

[Taras Glek]

Ehsan Akhgari

Thursday, April 3, 2014 14:09

On Thu, Apr 3, 2014 at 4:54 PM, Gregory Szorc <g...@mozilla.com> wrote:

On 4/3/14, 10:23 AM, Ehsan Akhgari wrote:

We need to have a way to associate a user with their bugzilla auth pair
in order to be able to create the review request under that user name,
as bugzilla will be the authentication provider to RB.

We discussed how is going to work, and this is the plan going forward:

* We will make the ssh server accept any public key (but of course won't
give out shell access to people!).  This means that we will accept
pushes from everybody as far as ssh is concerned.

* We will prompt on the client side for your Bugzilla user name and
password, use the Bugzilla REST API
<http://www.bugzilla.org/docs/tip/en/html/api/Bugzilla/WebService/Server/REST.html#AUTHENTICATION>
to perform the authentication, and will store the auth token locally,
and send it over to the server as part of the push.  The token can then
be verified on the server side as part of the hook.

We can go one step further.

The bzexport Mercurial extension [1] and the python-mozautomation project [2] have Python modules to extract Bugzilla cookies from your Firefox profile. If a user has a Firefox profile with an active Bugzilla session, we should be able to silently piggyback on top of those credentials. I suppose we could go one step further and obtain credentials from Firefox's password manager.

Yeah, sorry I didn't mention this.  This was also discussed but this method is fragile in a lot of edge cases (some of them are the kinds of things that developers can hit regularly, such as having a Firefox profile as the default where you have not logged in to Bugzilla.)

 

That being said, I think there is a huge security problem here.

Correct me if I'm wrong, but I believe the Bugzilla auth token is as good as your login credentials. The only differences are that it has undergone a one-way transformation and expires after a shorter duration. What you are proposing is effectively giving the code review service the keys to *your* kingdom. I have doubts that will pass security review. If it did, I'm guessing we'd have to bring the code review tool into the same security scope as Bugzilla itself. And I thought we were trying to avoid that (at least initially).

That's a great point.  Is that something that we can ask the security team about?  Taras, can you please take that on?

I could, but lets discuss this with bz folks first

[Gregory Szorc]

On 4/3/14, 2:09 PM, Ehsan Akhgari wrote:
> On Thu, Apr 3, 2014 at 4:54 PM, Gregory Szorc <g...@mozilla.com
> <mailto:g...@mozilla.com>> wrote:
>
> On 4/3/14, 10:23 AM, Ehsan Akhgari wrote:
>
> We need to have a way to associate a user with their bugzilla
> auth pair
> in order to be able to create the review request under that user
> name,
> as bugzilla will be the authentication provider to RB.
>
> We discussed how is going to work, and this is the plan going
> forward:
>
> * We will make the ssh server accept any public key (but of
> course won't
> give out shell access to people!). This means that we will accept
> pushes from everybody as far as ssh is concerned.
>
> * We will prompt on the client side for your Bugzilla user name and
> password, use the Bugzilla REST API

> <http://www.bugzilla.org/docs/__tip/en/html/api/Bugzilla/__WebService/Server/REST.html#__AUTHENTICATION

> <http://www.bugzilla.org/docs/tip/en/html/api/Bugzilla/WebService/Server/REST.html#AUTHENTICATION>>
> to perform the authentication, and will store the auth token
> locally,
> and send it over to the server as part of the push. The token
> can then
> be verified on the server side as part of the hook.
>
>
> We can go one step further.
>
> The bzexport Mercurial extension [1] and the python-mozautomation
> project [2] have Python modules to extract Bugzilla cookies from
> your Firefox profile. If a user has a Firefox profile with an active
> Bugzilla session, we should be able to silently piggyback on top of
> those credentials. I suppose we could go one step further and obtain
> credentials from Firefox's password manager.
>
>
> Yeah, sorry I didn't mention this. This was also discussed but this
> method is fragile in a lot of edge cases (some of them are the kinds of
> things that developers can hit regularly, such as having a Firefox
> profile as the default where you have not logged in to Bugzilla.)

That is why I improved on bzexport's version to scan *all* the Firefox
profiles and sort the cookies by their date :) (There is a problem with
multiple cookies, but you can always fall back to prompting in that
case.) Unfortunately, the python-mozautomation code is out of sync with
bzexport's and therefore lacks some bug fixes. I would like to
eventually consolidate this code. In time.

But the security problem is more important than an optimization.

[George Miroshnykov]

So I’ve played around with different authentication approaches and here’s the outcome so far.

First, let me recap the steps of our flow that requires authentication:

1. hg push via SSH

2. RB API to create and update review requests

3. BZ API to automatically attach the link to RB review request (and possibly keep track of review status etc.)

For the first step, we’ve discussed that we will allow anonymous pushes.

Before that we also discussed that we’re allowing force-pushes for a number of reasons.

AFAIK those two points combined mean than anyone can wipe out the whole review repo and it’s history.

There may be ways to work around this, but I’m not yet sure how reliable they’ll be.

Thoughts?

The second authentication (RB API) was previously implemented via server-side configuration.

The hook was pre-configured with RB credentials (admin), so all review request activity was performed via that user.

What we want instead is to manage review requests using user-specific Bugzilla credentials.

We already have a RB extension that adds BZ authentication backend:

https://github.com/mozilla/rbbz

Unfortunately I couldn’t install it: after I did `pip install rbbz`, RB insisted that "There are no extensions installed.”

I’ll pick this up separately with Mark.

So at this point, the hook doesn’t have to know anything about Bugzilla credentials - it only needs to authenticate with user-specific RB credentials.

This means that RB credentials are no longer configured once on the server, but instead they must be passed along during `hg push` like this:

RT_BUG=31337 RT_USERNAME=gmiroshnykov RT_PASSWORD=secret hg push -f -e 'ssh -o SendEnv=RT_*’

Of course this will be hidden behind `mach`, but for now we can rewrite it for readability:

export RT_BUG=31337

export RT_USERNAME=gmiroshnykov

export RT_PASSWORD=secret

hg push -f -e 'ssh -o SendEnv=RT_*’

(I also renamed BUG to RT_BUG to simplify configuration)

While rbbz is not integrated, you can manually create a RB user and try it out.

If you do, please be sure to give your new user the permissions to create review request (I simply made myself a superuser).

The third authentication (for posting review request links to Bugzilla) is not yet implemented.

I think this kind of functionality can be provided by rbbz too, let’s discuss this.

Thanks,

George

--
You received this message because you are subscribed to the Google Groups "mozilla-code-review" group.
To unsubscribe from this group and stop receiving emails from it, send an email to mozilla-code-re...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[James Graham]

On 04/04/14 16:21, George Miroshnykov wrote:
> So I’ve played around with different authentication approaches and here’s the outcome so far.
>
> First, let me recap the steps of our flow that requires authentication:
>
> 1. hg push via SSH
> 2. RB API to create and update review requests
> 3. BZ API to automatically attach the link to RB review request (and possibly keep track of review status etc.)

I feel like I am pushing back on requirements all the time, and maybe
that's not helpful, but I think a level of integration with bugzilla
that requires the authentication solution to be designed around bugzilla
might not be the best thing. What are the arguments against a system
that only integrates with bugzilla at the edges? If the review tool
takes care of assigning reviewers, storing comments, informing everyone
involved in the review about status updates, and so on, why does
bugzilla need to be involved at all? It might be useful to do simple
things like link to the review request in the bug, but perhaps this
could be done using a single "code review system" user? (if that's not
good enough for e.g. security reasons, how do we plan to solve this for
things like the auto commit bot, that should presumably also have the
ability to comment on a bug to indicate that things have been committed,
but that must run non-interactively).

[George Miroshnykov]

Quick update:

I’ve upgraded Review Board to 2.0 RC2 and rbbz extension works great so far!

Here’s the review request I’ve created using my real b.m.o credentials:

http://review.infinity.com.ua/r/42/

(viewing squashed diff is broken, because I forgot to reapply that RB patch, but the rest should work fine)

Before trying it out with your real credentials, please keep in mind that the service right now is not secure!

Your password can end up in some logs.

Please make sure to change your password after testing.

Thanks,

George

[George Miroshnykov]

Hey James,

I believe the intention is to not copy users over into Review Board and then keeping them in sync with Bugzilla.

Instead we need a way to a) authenticate and b) assign reviewers based on the user accounts from Bugzilla,

because otherwise the maintenance overhead of the review tool will be too high.

But personally I don’t have a preference here.

I’m not familiar with Bugzilla workflow regarding bots, but if we stick to the current auth scheme, we can create separate accounts for them.

Thanks,

George

[James Graham]

On 04/04/14 17:05, George Miroshnykov wrote:
> Hey James,
>
> I believe the intention is to not copy users over into Review Board and then keeping them in sync with Bugzilla.
> Instead we need a way to a) authenticate and b) assign reviewers based on the user accounts from Bugzilla,
> because otherwise the maintenance overhead of the review tool will be too high.

Have we considered using the same authentication system as Try i.e. has
the idea of requiring L1 commit access to post a review been considered
and rejected? It seems like basing the auth around LDAP makes a lot of
sense, and, as you point out, there are consequences to being able to
push to code review which are not dissimilar to those of being able to
push to Try (especially if force pushes are allowed).

[Mark Côté]

Been trying to follow this, but it has evolved over the last few days and I'm not sure where we're at. :) What exactly do you need BMO's opinion/advice on atm, if anything?

Mark

[George Miroshnykov]

AFAIK we have to support contributions from developers without commit access.

--
You received this message because you are subscribed to the Google Groups "mozilla-code-review" group.

To unsubscribe from this group and stop receiving emails from it, send an email to mozilla-code-review+unsub...@googlegroups.com.

[Taras Glek]

Can we do something stupid here to start?

Eg support unauthenticated pushed and then allow them to be claimed via a web ui?

Taras

---

To unsubscribe from this group and stop receiving emails from it, send an email to mozilla-code-re...@googlegroups.com.

[George Miroshnykov]

That would be unfortunate, considering we’ve just dropped the web UI :)

The way it works right now is basically you need a b.m.o account in order to push for review.

Is this good enough or should we look for another solution?

On April 4, 2014 at 19:40:59 , Taras Glek (tg...@mozilla.com) wrote:

Can we do something stupid here to start?

Eg support unauthenticated pushed and then allow them to be claimed via a web ui?

Taras

---

From: "George Miroshnykov" <gmiros...@rebbix.com>
To: "James Graham" <ja...@hoppipolla.co.uk>
Cc: mozilla-c...@googlegroups.com
Sent: Friday, April 4, 2014 9:32:27 AM
Subject: Re: Plan for authenticating the user to RB as part of the push

AFAIK we have to support contributions from developers without commit access.

On Apr 4, 2014 7:09 PM, "James Graham" <ja...@hoppipolla.co.uk> wrote:

On 04/04/14 17:05, George Miroshnykov wrote:

Hey James,

I believe the intention is to not copy users over into Review Board and then keeping them in sync with Bugzilla.
Instead we need a way to a) authenticate and b) assign reviewers based on the user accounts from Bugzilla,
because otherwise the maintenance overhead of the review tool will be too high.

Have we considered using the same authentication system as Try i.e. has the idea of requiring L1 commit access to post a review been considered and rejected? It seems like basing the auth around LDAP makes a lot of sense, and, as you point out, there are consequences to being able to push to code review which are not dissimilar to those of being able to push to Try (especially if force pushes are allowed).

--
You received this message because you are subscribed to the Google Groups "mozilla-code-review" group.

To unsubscribe from this group and stop receiving emails from it, send an email to mozilla-code-re...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[George Miroshnykov]

Hey Mark,

We need to figure out the requirements in RB - BZ integration.

For instance, we probably want to automatically attach a review request URL to a corresponding Bugzilla bug.

But I’m not sure who’s responsibility that should be.

It can be done by my push hook, by rbbz or maybe even by Review Board itself.

Once we have the initial URL, we probably want to track review status in Bugzilla, but to what extent?

Should we add a comment in Bugzilla each time someone leaves a review comment?

Should we add a comment for every new changeset pushed?

Should we show remove a Bugzilla attachment for discarded review requests?

How exactly should we track things like RB’s “ship it” and Bugzilla’s r+es or something like that (pardon me, I’m still new to Bugzilla)?

What about some events taking place in Bugzilla that we want to sync into Review Board, like closed bugs?

We briefly discussed some of those questions before, but AFAIK never came to some specific workflow.

My push hook definitely can’t handle all this, so those features should probably go into rbbz or some other extension(s)?

Thanks,

George

[Taras Glek]

It's good enough :)

George Miroshnykov

Friday, April 4, 2014 12:06

That would be unfortunate, considering we’ve just dropped the web UI :)

The way it works right now is basically you need a b.m.o account in order to push for review.

Is this good enough or should we look for another solution?

On April 4, 2014 at 19:40:59 , Taras Glek (tg...@mozilla.com) wrote:

Taras Glek

Friday, April 4, 2014 9:40

Can we do something stupid here to start?

Eg support unauthenticated pushed and then allow them to be claimed via a web ui?

Taras

---

--
You received this message because you are subscribed to the Google Groups "mozilla-code-review" group.
To unsubscribe from this group and stop receiving emails from it, send an email to mozilla-code-re...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

George Miroshnykov

Friday, April 4, 2014 9:32

AFAIK we have to support contributions from developers without commit access.

--

You received this message because you are subscribed to the Google Groups "mozilla-code-review" group.
To unsubscribe from this group and stop receiving emails from it, send an email to mozilla-code-re...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

James Graham

Friday, April 4, 2014 9:08

Have we considered using the same authentication system as Try i.e. has the idea of requiring L1 commit access to post a review been considered and rejected? It seems like basing the auth around LDAP makes a lot of sense, and, as you point out, there are consequences to being able to push to code review which are not dissimilar to those of being able to push to Try (especially if force pushes are allowed).

George Miroshnykov

Friday, April 4, 2014 9:05

Hey James,

I believe the intention is to not copy users over into Review Board and then keeping them in sync with Bugzilla.

Instead we need a way to a) authenticate and b) assign reviewers based on the user accounts from Bugzilla,

because otherwise the maintenance overhead of the review tool will be too high.

But personally I don’t have a preference here.

I’m not familiar with Bugzilla workflow regarding bots, but if we stick to the current auth scheme, we can create separate accounts for them.

Thanks,

George

On April 4, 2014 at 6:53:06 PM, James Graham (ja...@hoppipolla.co.uk) wrote:

[Ehsan Akhgari]

Hey Mark,

Sorry if this is very confusing.  I think this is currently the most important question:

If we transfer the bugzilla login token to the Mercurial server which is going to accept the pushes which initiate the review process, that means that the Mercurial server essentially has your auth pair.  Is there any way to limit the kind of damage that an attacker could do if they got their hands on this token?  If not, does this mean that the push server would need to have the same security clearances that the bugzilla servers currently do?

Thanks,

--
Ehsan
<http://ehsanakhgari.org/>

[Ehsan Akhgari]

On Fri, Apr 4, 2014 at 12:32 PM, George Miroshnykov <gmiros...@rebbix.com> wrote:

AFAIK we have to support contributions from developers without commit access.

Yes.

 

To unsubscribe from this group and stop receiving emails from it, send an email to mozilla-code-re...@googlegroups.com.

[Mark Côté]

Thanks for putting that succinctly. :) Off-hand, I don't think there is any way to limit damages; as others have said, it's basically as good as a username & password. But I'll ask glob on Monday. If you have any ideas as to how damage might be limited in a perfect world (e.g. with unlimited resources :) please let me know.

Mark

[Ehsan Akhgari]

On Fri, Apr 4, 2014 at 11:21 AM, George Miroshnykov <gmiros...@rebbix.com> wrote:

So I’ve played around with different authentication approaches and here’s the outcome so far.

First, let me recap the steps of our flow that requires authentication:

1. hg push via SSH

2. RB API to create and update review requests

3. BZ API to automatically attach the link to RB review request (and possibly keep track of review status etc.)

For the first step, we’ve discussed that we will allow anonymous pushes.

Before that we also discussed that we’re allowing force-pushes for a number of reasons.

AFAIK those two points combined mean than anyone can wipe out the whole review repo and it’s history.

There may be ways to work around this, but I’m not yet sure how reliable they’ll be.

What you said above is only true in git by default, not in hg (since it will happily allow you to have multiple unnamed heads per named branch.)

For git we can do smart things in the future, such as backing up each ref before it's moved through a push to make sure this won't be an issue.

 

The second authentication (RB API) was previously implemented via server-side configuration.

The hook was pre-configured with RB credentials (admin), so all review request activity was performed via that user.

What we want instead is to manage review requests using user-specific Bugzilla credentials.

We already have a RB extension that adds BZ authentication backend:

https://github.com/mozilla/rbbz

Unfortunately I couldn’t install it: after I did `pip install rbbz`, RB insisted that "There are no extensions installed.”

I’ll pick this up separately with Mark.

So at this point, the hook doesn’t have to know anything about Bugzilla credentials - it only needs to authenticate with user-specific RB credentials.

This means that RB credentials are no longer configured once on the server, but instead they must be passed along during `hg push` like this:

RT_BUG=31337 RT_USERNAME=gmiroshnykov RT_PASSWORD=secret hg push -f -e 'ssh -o SendEnv=RT_*’

Of course this will be hidden behind `mach`, but for now we can rewrite it for readability:

export RT_BUG=31337

export RT_USERNAME=gmiroshnykov

export RT_PASSWORD=secret

hg push -f -e 'ssh -o SendEnv=RT_*’

I assume once the rbbz extension issue is fixed this will be replaced by the bugzilla token right?

If yes that sounds good!

Ehsan

[James Graham]

On 04/04/14 20:36, George Miroshnykov wrote:

> Once we have the initial URL, we probably want to track review status in Bugzilla, but to what extent?
> Should we add a comment in Bugzilla each time someone leaves a review comment?

That would be terrible, I think. Once you have a good code review tool,
people tend to leave more comments than they do with the current
bugzilla setup. Presumably the code review tool will already send out
emails (batched, I hope; github doesn't batch email and its code review
system is intolerable for that reason alone, although it is also flawed
in other ways) for those comments. Getting the same email from bugzilla
would be pure spam.

> Should we add a comment for every new changeset pushed?

From the code review tool, yes. After all reviewers need to know that
they have more work to do in that review. I don't see why we would want
bugzilla to duplicate that.

> Should we show remove a Bugzilla attachment for discarded review requests?

We aren't planning to try and upload a patch to bugzilla for every
review request are we? What would be the point of that?

> How exactly should we track things like RB’s “ship it” and Bugzilla’s r+es or something like that (pardon me, I’m still new to Bugzilla)?

I think that having the review status purely be in the code review tool
is fine. Once we have a good code review system, it should be the
ultimate source of truth for questions related to who reviewed the code,
what comments they made, and so on. Bugzilla should tell us about the
known issues that have to be fixed in the code. The code review tool
should tell us about the current and past proposed changes to the code.
The VCS should tell us about the history of the code itself. The commit
bot should move code from the code review tool to the VCS. Mixing these
roles simply because we have historically used Bugzilla as a
swiss-army-knife type tool is going to prevent us making each tool as
good as it could be, and from optimising our workflow to minimise overheads.

> What about some events taking place in Bugzilla that we want to sync into Review Board, like closed bugs?

I can see the use case for dropping a review request if the
corresponding bug is closed. That's something that I have set up for my
critic instance that's connected to GihHub and it works pretty well.

> We briefly discussed some of those questions before, but AFAIK never came to some specific workflow.
> My push hook definitely can’t handle all this, so those features should probably go into rbbz or some other extension(s)?

So, fwiw my ideal workflow would be like this:

1. Make a commit and push it to the review tool via whatever mechanism
2. Review tool picks up the bug ID (if any) from the commit message or
the bug ID or whatever
3. Review tool links to review in the bug
4. Review tool sends out email to reviewers based on the files touched
by the commit (with an option to override if necessary)
5. Reviewers add comments, I push more commits to fix up, etc.
6. Once review is accepted, I push the "Integrate this Fix button"
7. At this point I get the chance to interactively rebase the commits on
the review branch
8. The review tool rewrites the commit messages to add the review url or
something in the commit message e.g "Bug 123 - fix a foo, r/678"
7. The branch is added to to queue for the commit bot

[Mark Côté]

Ah yes, Ehsan and I talked about this last week. There's an etherpad with some concrete to-do items (starting around line 48) that I still need to file bugs for. https://etherpad.mozilla.org/nFytGATPyJ

I believe all your questions are answered there, and the short answer is that rbbz should probably be doing most, if not all, of them (I have successfully implemented hooks in core Review Board where the existing extension framework was insufficient for rbbz's needs). The amount of work varies quite a bit; we'll definitely be able to implement some of them this quarter, but at the moment I'm not entirely certain which.

Mark

[Ehsan Akhgari]

On Fri, Apr 4, 2014 at 4:56 PM, Mark Côté <mc...@mozilla.com> wrote:

Thanks for putting that succinctly. :) Off-hand, I don't think there is any way to limit damages; as others have said, it's basically as good as a username & password. But I'll ask glob on Monday. If you have any ideas as to how damage might be limited in a perfect world (e.g. with unlimited resources :) please let me know.

On thing which would be interested to know is whether we have a good way to issue specific types of tokens which only have powers to do certain things through the API, not everything that the user has the permission to do.

Thanks!
Ehsan

[Gregory Szorc]

On 4/4/14, 1:55 PM, Ehsan Akhgari wrote:
> On Fri, Apr 4, 2014 at 11:21 AM, George Miroshnykov
> <gmiros...@rebbix.com <mailto:gmiros...@rebbix.com>> wrote:
>
> So I’ve played around with different authentication approaches and
> here’s the outcome so far.
>
> First, let me recap the steps of our flow that requires authentication:
>
> 1. hg push via SSH
> 2. RB API to create and update review requests
> 3. BZ API to automatically attach the link to RB review request (and
> possibly keep track of review status etc.)
>
> For the first step, we’ve discussed that we will allow anonymous pushes.
> Before that we also discussed that we’re allowing force-pushes for a
> number of reasons.
> AFAIK those two points combined mean than anyone can wipe out the
> whole review repo and it’s history.
> There may be ways to work around this, but I’m not yet sure how
> reliable they’ll be.
>
>
> What you said above is only true in git by default, not in hg (since it
> will happily allow you to have multiple unnamed heads per named branch.)
>
> For git we can do smart things in the future, such as backing up each
> ref before it's moved through a push to make sure this won't be an issue.

While Git will let you kill the ref, I thought you could still access
orphaned commits up until the point they are garbage collected?
Presumably we'll disable garbage collection on the code review repo? I
suppose we could even have the code review tool create missing refs as
needed?

[Ehsan Akhgari]

Yeah, I was trying to find this etherpad, thanks, Mark!

George, James, others, can we please discuss other issues in other threads?  I'm not sure how much of the other concerns have already been answered, if there are any unanswered issues left not related to auth, please start a new thread.  It's hard to keep track of several conversations all happening in the same thread.  :-)

Thanks,

--
Ehsan
<http://ehsanakhgari.org/>

[Ehsan Akhgari]

Yes, that's because the entries will be referenced in the reflog, but we need actual named references (like "bugnumber/reviewnumber" so that once you submit a push, it has an eternal name till the end of time.  Then GC won't get in our way any more.

Cheers,
Ehsan

[George Miroshnykov]

On April 4, 2014 at 23:58:07 , James Graham (ja...@hoppipolla.co.uk) wrote:

On 04/04/14 20:36, George Miroshnykov wrote: 

> Once we have the initial URL, we probably want to track review status in Bugzilla, but to what extent? 
> Should we add a comment in Bugzilla each time someone leaves a review comment? 

That would be terrible, I think. Once you have a good code review tool, 
people tend to leave more comments than they do with the current 
bugzilla setup. Presumably the code review tool will already send out 
emails (batched, I hope; github doesn't batch email and its code review 
system is intolerable for that reason alone, although it is also flawed 
in other ways) for those comments. Getting the same email from bugzilla 
would be pure spam. 

> Should we add a comment for every new changeset pushed? 

From the code review tool, yes. After all reviewers need to know that 
they have more work to do in that review. I don't see why we would want 
bugzilla to duplicate that. 

> Should we show remove a Bugzilla attachment for discarded review requests? 

We aren't planning to try and upload a patch to bugzilla for every 
review request are we? What would be the point of that? 

Sorry, that was really unclear on my part.
What I meant was that AFAIK you can add a Review Board link to Bugzilla bug by adding it as an attachment with a special content-type.
If the corresponding review request is closed for some reason, we may wish to discard that attachment containing the link.
I’m not suggesting to attach the actual patches :)

[Ehsan Akhgari]

Guys, please read the etherpad!  :-)  All of this stuff has already been discussed.

--
Ehsan
<http://ehsanakhgari.org/>

[Mark Côté]

Ah, that I can say a categorical "no" too, but it's part of the plan for API keys/OAUTH...which of course doesn't help now. :)

Mark

[George Miroshnykov]

On April 4, 2014 at 11:56:13 PM, Ehsan Akhgari (ehsan....@gmail.com) wrote:

On Fri, Apr 4, 2014 at 11:21 AM, George Miroshnykov <gmiros...@rebbix.com> wrote:

Of course this will be hidden behind `mach`, but for now we can rewrite it for readability:

export RT_BUG=31337

export RT_USERNAME=gmiroshnykov

export RT_PASSWORD=secret

hg push -f -e 'ssh -o SendEnv=RT_*’

I assume once the rbbz extension issue is fixed this will be replaced by the bugzilla token right?

If yes that sounds good!

Ehsan

Good question!

It looks like we need the following auth options from Review Board:

1. Authenticate Review Board user logins with Buzilla username and password - this one already works.
2. Authenticate Review Board Web API calls with Bugzilla tokens - I’m not sure this is possible at the moment, so I’ve implemented it like 1.

Mark, any chance rbbz could provide the second option?

Thanks,

George

[Mark Côté]

I believe it can; djblets provides an overrideable WebAPIAuthBackend class. I'll add it to the to-do list.

Mark

[Mark Côté]

Hm there will be a bit of further yak-shaving, since rbbz uses the XMLRPC API (the REST API wasn't ready when I first started this work last year). I was planning on supporting REST as well, but this will definitely be necessary to implement auth-token support.

Btw I am going to put the integration plan in bug 515210 (the original, old Review Board integration bug) and file dependent bugs for everything. I think I'll have a rbbz component created as well.

Mark