Rietveld schedules builds on buildbucket

Rietveld schedules builds on buildbucket

Rietveld deprecated UI now schedules builds through buildbucket instead of
/edit_flags endpoint. It uses Google API JavaScript Client (gapi) to
authenticate the user to Google APIs and call buildbucke.put() on their behalf. 
API authentication happens the first time the user tries to schedule builds: a
popup windows opens asking for user's consent to share their email address with
Rietveld. Then the access token is saved to cookies. This consent screen happens
because API authentication and GAE authentication are separate.

If user logs out and logs in with a different rietveld account, API will be
authenticated with a different user by sending "login_hint" query string
parameter during authentication.

BuildBucket buckets have own ACLs. Users in chromium-project-tryjob-access 
group [1] have SCHEDULER role on master.tryserver.chromium.* buckets.

The client id used to call APIs must be put into the "OwnClientId" entity 
with key "web". It is created on the first attempt to load any page. The client id is
included in the rendered in base.html template. Then script.js uses it to call
authenticate to APIs. If client id is not set, nothing will blow up, but users
won't be able to schedule builds.

script.js now has global "rietveld" variable and keeps global state there. The
rationale is to avoid polluting global namespace.

Two dependencies added:
* Google API JavaScript API
* ES6 Promise polyfill

Instructions for deployment:
* Create a new client id in Permissions page of the Developer Console
* Whitelist the client id at
  https://chrome-infra-auth.appspot.com/auth/oauth_config
* Upload a new version
* Load any page
* Go to Developers Console, find an existing OwnClientId entity and set
  its client_id property
* Verify that build scheduling works

[1]: https://chrome-infra-auth.appspot.com/auth/groups#project-chromium-tryjob-access

R=jrobbins@chromium.org
CC=esprehn@chromium.org, sheyang@chromium.org
BUG=461620
COMMIT=false (CQ-BuildBucket will be verified first. Then M_scheduleBuild will be updated if needed)

[nodir, 2015-04-11 07:59:47]

Patchset #1 (id:1) has been deleted

[nodir, 2015-04-11 08:08:24]

Patchset #1 (id:20001) has been deleted

[nodir, 2015-04-11 08:39:32]

Patchset #1 (id:40001) has been deleted

[nodir, 2015-04-11 08:44:08]

Patchset #1 (id:60001) has been deleted

[nodir, 2015-04-13 16:53:50]

Patchset #1 (id:60002) has been deleted

[nodir, 2015-04-13 16:56:48]

Jason, PTAL.

I will not land this CL until CQ successfully processes >=15% of all issues
through BuildBucket. After that M_scheduleBuild will be updated if needed.
Please verify the rest in the meantime.

https://codereview.chromium.org/1058893004/diff/90001/appengine/chromium_riet...
File appengine/chromium_rietveld/static/script.js (right):

https://codereview.chromium.org/1058893004/diff/90001/appengine/chromium_riet...
appengine/chromium_rietveld/static/script.js:3717: var callPut =
gapi.client.buildbucket.put({
This call has to be compatible with existing recipes used on tryservers.
CQ->BuildBucket will be verified before landing this CL. Then this code will be
updated if needd

[esprehn, 2015-04-14 01:00:10]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org

[esprehn, 2015-04-14 01:00:11]

We need to be able to schedule jobs in a single http request, not make one per
build. That would mean scheduling 10 bots makes 10 requests.

https://codereview.chromium.org/1058893004/diff/90001/appengine/chromium_riet...
File appengine/chromium_rietveld/static/script.js (right):

https://codereview.chromium.org/1058893004/diff/90001/appengine/chromium_riet...
appengine/chromium_rietveld/static/script.js:705: var buildPromise =
M_scheduleBuild(
Is this making one http call per builder?

https://codereview.chromium.org/1058893004/diff/90001/appengine/chromium_riet...
appengine/chromium_rietveld/static/script.js:3703: * Schedules a build on
buildbucket.
This makes N http requests now when scheduling a bunch of bots, it shouldn't
need to do that.

https://codereview.chromium.org/1058893004/diff/90001/appengine/chromium_riet...
File appengine/chromium_rietveld/templates/base.html (right):

https://codereview.chromium.org/1058893004/diff/90001/appengine/chromium_riet...
appengine/chromium_rietveld/templates/base.html:65: rietveld.preferredDomainName
= "{{preferred_domain_name}}";
This needs the |jsescape filter. (all of these do)

[nodir, 2015-04-14 18:45:13]

Adding put_batch endpoint https://codereview.chromium.org/1082303002/

[jrobbins, 2015-04-16 18:09:27]

What's forcing this change?  I thought we had settled on the JS talking to
rietveld and rietveld talking to buildbucket.

I'm concerned that having an extra auth window pop up is a bad user experience. 
Also, it adds complexity and I am concerned that if there are ever problems with
it in the future that it will be hard to debug.

https://codereview.chromium.org/1058893004/diff/90001/appengine/chromium_riet...
File appengine/chromium_rietveld/static/es6-promise/es6-promise-2.0.1.min.js
(right):

https://codereview.chromium.org/1058893004/diff/90001/appengine/chromium_riet...
appengine/chromium_rietveld/static/es6-promise/es6-promise-2.0.1.min.js:3: *
@copyright Copyright (c) 2014 Yehuda Katz, Tom Dale, Stefan Penner and
contributors (Conversion to ES6 API by Jake Archibald)
I was going to say that this needs to be under third_party, but after looking at
other libraries used in the deprecated UI, I think we can keep what you have and
we will eventually resolve it when we remove all the deprecated UI
implementation.

https://codereview.chromium.org/1058893004/diff/90001/appengine/chromium_riet...
File appengine/chromium_rietveld/static/script.js (right):

https://codereview.chromium.org/1058893004/diff/90001/appengine/chromium_riet...
appengine/chromium_rietveld/static/script.js:18: buildbucketHostname:
"cr-buildbucket-test.appspot.com",
It looks like you hardcoded a test value here.

[nodir, 2015-04-16 18:15:11]

FWIU, even if rietveld server talks to buildbucket, you still need user consent
at least in the very beginning.

I tried to get your feedback on this design in
https://code.google.com/p/chromium/issues/detail?id=461620

Please describe how do you see this should work. In particular, how server
acquires an access token.

https://codereview.chromium.org/1058893004/diff/90001/appengine/chromium_riet...
File appengine/chromium_rietveld/static/es6-promise/es6-promise-2.0.1.min.js
(right):

https://codereview.chromium.org/1058893004/diff/90001/appengine/chromium_riet...
appengine/chromium_rietveld/static/es6-promise/es6-promise-2.0.1.min.js:3: *
@copyright Copyright (c) 2014 Yehuda Katz, Tom Dale, Stefan Penner and
contributors (Conversion to ES6 API by Jake Archibald)
On 2015/04/16 18:09:27, Jason Robbins wrote:
> I was going to say that this needs to be under third_party, but after looking
at
> other libraries used in the deprecated UI, I think we can keep what you have
and
> we will eventually resolve it when we remove all the deprecated UI
> implementation.

Acknowledged.

https://codereview.chromium.org/1058893004/diff/90001/appengine/chromium_riet...
File appengine/chromium_rietveld/static/script.js (right):

https://codereview.chromium.org/1058893004/diff/90001/appengine/chromium_riet...
appengine/chromium_rietveld/static/script.js:18: buildbucketHostname:
"cr-buildbucket-test.appspot.com",
On 2015/04/16 18:09:27, Jason Robbins wrote:
> It looks like you hardcoded a test value here.

it is overwritten in base.html

[esprehn, 2015-04-16 18:15:18]

I too would like to avoid this, I thought it was required because of some
authentication quirk with GAE and Apiary? It would be nice if we could
resolve that some other way.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[nodir, 2015-04-16 18:50:53]

On 2015/04/16 18:15:18, esprehn wrote:
> I too would like to avoid this, I thought it was required because of some
> authentication quirk with GAE and Apiary? 

Yes, FWIU, API and GAE authentications are different mechanisms. I am not sure
it is avoidable in deprecated ui. FWIU, Polymer UI will face this problem too if
it uses GAE authentication to get Rietveld data.

If the Polymer UI will ever be used for seamless Rietveld->Gerrit transition, it
will need to use gapi auth to talk to Gerrit too. Gerrit will not act as a proxy
for buildbucket, so Polymer UI should talk to buildbucket directly.

+vadimsh, our auth expert

[esprehn, 2015-04-16 18:56:15]

On 2015/04/16 at 18:50:53, nodir wrote:
> On 2015/04/16 18:15:18, esprehn wrote:
> > I too would like to avoid this, I thought it was required because of some
> > authentication quirk with GAE and Apiary? 
> 
> Yes, FWIU, API and GAE authentications are different mechanisms. I am not sure
it is avoidable in deprecated ui. FWIU, Polymer UI will face this problem too if
it uses GAE authentication to get Rietveld data.
> 
> If the Polymer UI will ever be used for seamless Rietveld->Gerrit transition,
it will need to use gapi auth to talk to Gerrit too. Gerrit will not act as a
proxy for buildbucket, so Polymer UI should talk to buildbucket directly.
> 
> +vadimsh, our auth expert

Making you login twice is not acceptable long term, we need to find some way to
have a single login system.

[nodir, 2015-04-16 19:00:40]

The long term solution is to use OAuth2 everywhere, which means
authenticate to Rietveld from ui with oauth2

On Thu, Apr 16, 2015, 11:56 AM  <esprehn@chromium.org> wrote:

> On 2015/04/16 at 18:50:53, nodir wrote:
> > On 2015/04/16 18:15:18, esprehn wrote:
> > > I too would like to avoid this, I thought it was required because of
> > some
> > > authentication quirk with GAE and Apiary?
>
> > Yes, FWIU, API and GAE authentications are different mechanisms. I am not
> > sure
> it is avoidable in deprecated ui. FWIU, Polymer UI will face this problem
> too if
> it uses GAE authentication to get Rietveld data.
>
> > If the Polymer UI will ever be used for seamless Rietveld->Gerrit
> > transition,
> it will need to use gapi auth to talk to Gerrit too. Gerrit will not act as
> a
> proxy for buildbucket, so Polymer UI should talk to buildbucket directly.
>
> > +vadimsh, our auth expert
>
> Making you login twice is not acceptable long term, we need to find some
> way to
> have a single login system.
>
> https://codereview.chromium.org/1058893004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[nodir, 2015-04-16 19:08:56]

Actually it is questionable whether is is unacceptable. If Buildbucket had its
own auth scope, the consent screen would ask whether Rietveld is allowed to
access Buildbucket on user's behalf. It wouldn't be perceived as second login.
Maybe we should get Chrome Infra scope

-Vadim, +vadimsh

[jrobbins, 2015-04-16 19:11:35]

I'm sorry to have not responded to issue 461620 sooner.  In fact, at one point I
also thought that we should go with direct polymer UI -> BuildBucket
communication.  However, now that I see what that would actually be like, I am
not sure that it is worth the downside of the added complexity of code and user
experience.  

In the past, I have found auth issues hard to debug when users report problems,
so I favor limiting ourselves to just the amount of auth logic that is already
in Rietveld.   And, as a Googler user of several internal tools, I feel that I
face daily distractions of having to re-authenticate and refresh tokens and
whatnot.  Even though you have a solution that shouldn't be to much of a burden
on users, I fear that it will end up being a hassle at the worst times.  So I
think we have to tread very lightly on our users' patience in that area.

Regarding Polymer UI and Gerrit integration, we will need some changes in Gerrit
to make that work, so I don't think it is out of the question to need one more
change to make Gerrit proxy requests to BuildBucket the same way that Rietveld
is doing.  If I am wrong about that, then we might come right back to this
solution, but I'd be happier if we can hang on to it until then.

[nodir, 2015-04-16 19:25:44]

Let's accept Rietveld server being a proxy for Buildbucket for a moment. How
would it authenticate to Buildbucket on behalf of the user? It would need user's
consent because Buildbucket supports auth2 only.

If Rietveld authenticates with its own credentials, Buildbucket will lose info
who (human) created a build. This might be okay becuase cq does that.

The alternative is to implement impersonation in our auth server: Rietveld
authenticates with it own credentials and states that it acts on behalf of
someone else. It will probably take a lot of time.

[jrobbins, 2015-04-16 19:28:39]

On 2015/04/16 at 19:00:40, nodir wrote:
> The long term solution is to use OAuth2 everywhere, which means
> authenticate to Rietveld from ui with oauth2

I think that you may be right, but I would need to learn more about it before I
could even fully review this CL, much less support it or debug it under
pressure.
So, I'd like to defer this for now so that we can focus on other features needed
to the polymer UI ready to fully replace the deprecated UI.

[nodir, 2015-04-16 19:28:50]

On 2015/04/16 19:25:44, nodir wrote:

> If Rietveld authenticates with its own credentials, Buildbucket will lose info
> who (human) created a build. This might be okay becuase cq does that.
> 

This effectively disables trying job access list check

[jrobbins, 2015-04-16 19:34:50]

> If Rietveld authenticates with its own credentials, Buildbucket will lose info
who (human) created a build. This might be okay becuase cq does that.

This is what I was assuming.

[nodir, 2015-04-16 19:37:07]

On 2015/04/16 19:34:50, Jason Robbins wrote:
> > If Rietveld authenticates with its own credentials, Buildbucket will lose
info
> who (human) created a build. This might be okay becuase cq does that.
> 
> This is what I was assuming.

It works for CQ because it does comitter check. Rietveld doesn't and not
supposed to. Buildbucket has ACLs implemented, only cq and people with tryjob
access can schedule builds

[Vadim Sh., 2015-04-16 19:50:26]

On 2015/04/16 19:37:07, nodir wrote:
> On 2015/04/16 19:34:50, Jason Robbins wrote:
> > > If Rietveld authenticates with its own credentials, Buildbucket will lose
> info
> > who (human) created a build. This might be okay becuase cq does that.
> > 
> > This is what I was assuming.
> 
> It works for CQ because it does comitter check. Rietveld doesn't and not
> supposed to. Buildbucket has ACLs implemented, only cq and people with tryjob
> access can schedule builds

I'll can quickly (~today) put together some simple "unconditional trust"
impersonation scheme for users of components.auth (e.g. buildbucket). Currently
I'm thinking of:
1) Rietveld authenticated a user however it likes.
2) When making a request to buildbucket (as GAE app to another GAE app),
Rietveld uses its own credentials (e.g. app_identity.get_access_token()), but
puts ID of authenticated user into "X-Auth-Impersonate" header.
3) On buildbucket side, components.auth looks at X-Auth-Impersonate header, and
if request sender is in "impersonators" group (or whatever), components.auth
"pretends" request came from impersonated account.

That way we can start properly propagate end-user IDs now (without code changes
to any of the services), and add a better security for impersonation later when
we have a clearer idea whether it is needed. (I'm personally not concerned about
possibility of Rietveld internal GAE service account being abused... I have more
concerns about CQ credentials though, it's relatively easy to steal them).

[jrobbins, 2015-04-16 19:53:42]

On 2015/04/16 at 19:37:07, nodir wrote:
> On 2015/04/16 19:34:50, Jason Robbins wrote:
> > > If Rietveld authenticates with its own credentials, Buildbucket will lose
info
> > who (human) created a build. This might be okay becuase cq does that.
> > 
> > This is what I was assuming.
> 
> It works for CQ because it does comitter check. Rietveld doesn't and not
supposed to. Buildbucket has ACLs implemented, only cq and people with tryjob
access can schedule builds

OK, then let's try the impersonation approach.   When you say "impersonation" it
sounds like you are substituting the authenticated user with another
authenticated user as a very special piece of information in the BuildBucket
implementation.  I was thinking of the email address of the person who requested
the build as just one field in a big JSON dict of info about the request, so
more like "annotation".  Does it have to be more hard-wired than that?

[nodir, 2015-04-16 21:01:37]

On 2015/04/16 19:53:42, Jason Robbins wrote:
> On 2015/04/16 at 19:37:07, nodir wrote:
> > On 2015/04/16 19:34:50, Jason Robbins wrote:
> > > > If Rietveld authenticates with its own credentials, Buildbucket will
lose
> info
> > > who (human) created a build. This might be okay becuase cq does that.
> > > 
> > > This is what I was assuming.
> > 
> > It works for CQ because it does comitter check. Rietveld doesn't and not
> supposed to. Buildbucket has ACLs implemented, only cq and people with tryjob
> access can schedule builds
> 
> OK, then let's try the impersonation approach.   When you say "impersonation"
it
> sounds like you are substituting the authenticated user with another
> authenticated user as a very special piece of information in the BuildBucket
> implementation.  I was thinking of the email address of the person who
requested
> the build as just one field in a big JSON dict of info about the request, so
> more like "annotation".  Does it have to be more hard-wired than that?

If it is just an string field with plaintext email address, how can we verify
that a request came from that person indeed (I might be missing something very
obvious).
I am not an auth expert, but FWIU an access token embdes list of scopes (list of
permissions), where "email" scope allows reading the email address from a
trusted google service.

[jrobbins, 2015-04-16 23:10:14]

> If it is just an string field with plaintext email address, how can we verify
that a request came from that person indeed (I might be missing something very
obvious).

I thought Vadim outlined a solution that would work well.

[nodir, 2015-04-16 23:43:00]

On 2015/04/16 23:10:14, Jason Robbins wrote:
> > If it is just an string field with plaintext email address, how can we
verify
> that a request came from that person indeed (I might be missing something very
> obvious).
> 
> I thought Vadim outlined a solution that would work well.

Yes, impersonation will work, I've just clarified that it cannot be plaintext
email address.
I will update this CL with server-side scheduling.

[Adrian Kuegel, 2015-05-12 14:30:20]

On 2015/04/16 23:43:00, nodir wrote:
> On 2015/04/16 23:10:14, Jason Robbins wrote:
> > > If it is just an string field with plaintext email address, how can we
> verify
> > that a request came from that person indeed (I might be missing something
very
> > obvious).
> > 
> > I thought Vadim outlined a solution that would work well.
> 
> Yes, impersonation will work, I've just clarified that it cannot be plaintext
> email address.
> I will update this CL with server-side scheduling.

What is the status with this CL? Now that CQ uses buildbucket, I guess the
blocker regarding verification CQ->Buildbucket is gone.

[nodir, 2015-05-12 14:50:02]

Yes, I'll continue working on this one today

On Tue, May 12, 2015, 7:30 AM  <akuegel@chromium.org> wrote:

> On 2015/04/16 23:43:00, nodir wrote:
> > On 2015/04/16 23:10:14, Jason Robbins wrote:
> > > > If it is just an string field with plaintext email address, how can
> we
> > verify
> > > that a request came from that person indeed (I might be missing
> > something
> very
> > > obvious).
> > >
> > > I thought Vadim outlined a solution that would work well.
>
> > Yes, impersonation will work, I've just clarified that it cannot be
> > plaintext
> > email address.
> > I will update this CL with server-side scheduling.
>
> What is the status with this CL? Now that CQ uses buildbucket, I guess the
> blocker regarding verification CQ->Buildbucket is gone.
>
> https://codereview.chromium.org/1058893004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[nodir, 2015-05-14 03:31:33]

This is blocked on chrome-infra-auth's impersonation implementation.

Adrian, is this urgent for you?

[nodir, 2015-09-17 17:40:43]

Closing this because scheduling is implemented on the server now, with
impersonation https://codereview.chromium.org/1344253002/