Run Rietveld using the AppEngine SDK

I suspect that upload.py will still fail to authenticate - but other than that it seems to be working.

[Wladimir Palant, 2015-06-02 21:19:18]

Note that this is still work in progress, even though OAuth works now. OAuth
credentials have to be configured rather than being hardcoded in the script, we
also need to configure admin users (fixtures won't work, this code needs to be
replaced). Finally, I didn't test upload.py yet - I suspect that it won't be
able to authenticate, it uses a different authentication approach.

[Sebastian Noack, 2015-06-02 21:32:05]

http://codereview.adblockplus.org/6155422901731328/diff/5629499534213120/modu...
File modules/rietveld/files/wrapper.py (right):

http://codereview.adblockplus.org/6155422901731328/diff/5629499534213120/modu...
modules/rietveld/files/wrapper.py:17: sys.path[0:0] =
paths.script_paths(script_name)
Nit: sys.path.insert(0, ..)

http://codereview.adblockplus.org/6155422901731328/diff/5629499534213120/modu...
modules/rietveld/files/wrapper.py:48: def calculate_signature(str):
Nit: Please don't override built-in symbols like "str".

http://codereview.adblockplus.org/6155422901731328/diff/5629499534213120/modu...
modules/rietveld/files/wrapper.py:92: redirect(continue_url or base_url)
It seems inconsistent that you return after error(), but use if/else when
calling redirect().

http://codereview.adblockplus.org/6155422901731328/diff/5629499534213120/modu...
modules/rietveld/files/wrapper.py:119: emails = filter(lambda e: e["type"] ==
"account", json.loads(data).get('emails'))
I'd rather use a list comprehension here.

http://codereview.adblockplus.org/6155422901731328/diff/5629499534213120/modu...
modules/rietveld/files/wrapper.py:135:
redirect('https://accounts.google.com/o/oauth2/auth?' +
urllib.urlencode(authorize_params))
How about a global for 'https://accounts.google.com/o/oauth2/'?

[Sebastian Noack, 2015-06-02 21:41:06]

http://codereview.adblockplus.org/6155422901731328/diff/5629499534213120/modu...
File modules/rietveld/files/wrapper.py (right):

http://codereview.adblockplus.org/6155422901731328/diff/5629499534213120/modu...
modules/rietveld/files/wrapper.py:109: data =
urllib.urlopen('https://accounts.google.com/o/oauth2/token',
urllib.urlencode(token_params)).read()
Please close the file-like object returned by urlopen().

[Wladimir Palant, 2015-06-03 15:42:35]

I replaced cookie_secret by a config.ini file - OAuth2 credentials and admins
are being configured there as well now.

The remaining problem is that upload.py doesn't work. Rietveld cannot use App
Engine's OAuth2 credentials, instead it has its own set of credentials that the
admins set under /restricted/set-client-id-and-secret. This data is supposed to
be stored via NDB API, but for some reason it isn't being saved.

http://codereview.adblockplus.org/6155422901731328/diff/5629499534213120/modu...
File modules/rietveld/files/wrapper.py (right):

http://codereview.adblockplus.org/6155422901731328/diff/5629499534213120/modu...
modules/rietveld/files/wrapper.py:17: sys.path[0:0] =
paths.script_paths(script_name)
On 2015/06/02 21:32:06, Sebastian Noack wrote:
> Nit: sys.path.insert(0, ..)

The App Engine is inserting tons of paths here, not just one. This code matches
what its own wrappers are doing.

http://codereview.adblockplus.org/6155422901731328/diff/5629499534213120/modu...
modules/rietveld/files/wrapper.py:48: def calculate_signature(str):
On 2015/06/02 21:32:06, Sebastian Noack wrote:
> Nit: Please don't override built-in symbols like "str".

Interesting that you seem to be fine with overriding "file" but not with
overriding "str" :)

http://codereview.adblockplus.org/6155422901731328/diff/5629499534213120/modu...
modules/rietveld/files/wrapper.py:92: redirect(continue_url or base_url)
On 2015/06/02 21:32:06, Sebastian Noack wrote:
> It seems inconsistent that you return after error(), but use if/else when
> calling redirect().

Not really, the three branches here represent the functions implemented: logout,
finalize login, start login. Using returns here would make the logic confusing.
I've split up the logic into functions to make that more obvious.

http://codereview.adblockplus.org/6155422901731328/diff/5629499534213120/modu...
modules/rietveld/files/wrapper.py:109: data =
urllib.urlopen('https://accounts.google.com/o/oauth2/token',
urllib.urlencode(token_params)).read()
On 2015/06/02 21:41:06, Sebastian Noack wrote:
> Please close the file-like object returned by urlopen().

Done.

http://codereview.adblockplus.org/6155422901731328/diff/5629499534213120/modu...
modules/rietveld/files/wrapper.py:119: emails = filter(lambda e: e["type"] ==
"account", json.loads(data).get('emails'))
On 2015/06/02 21:32:06, Sebastian Noack wrote:
> I'd rather use a list comprehension here.

Done.

http://codereview.adblockplus.org/6155422901731328/diff/5629499534213120/modu...
modules/rietveld/files/wrapper.py:135:
redirect('https://accounts.google.com/o/oauth2/auth?' +
urllib.urlencode(authorize_params))
On 2015/06/02 21:32:06, Sebastian Noack wrote:
> How about a global for 'https://accounts.google.com/o/oauth2/'?

Done.

[Sebastian Noack, 2015-06-03 15:58:49]

LGTM on the Python code. I leave reviewing the Puppet scripts up to Matze.

http://codereview.adblockplus.org/6155422901731328/diff/5629499534213120/modu...
File modules/rietveld/files/wrapper.py (right):

http://codereview.adblockplus.org/6155422901731328/diff/5629499534213120/modu...
modules/rietveld/files/wrapper.py:17: sys.path[0:0] =
paths.script_paths(script_name)
On 2015/06/03 15:42:35, Wladimir Palant wrote:
> On 2015/06/02 21:32:06, Sebastian Noack wrote:
> > Nit: sys.path.insert(0, ..)
> 
> The App Engine is inserting tons of paths here, not just one. This code
matches
> what its own wrappers are doing.

Sorry, just realized that you insert multiple items not only one here. So yeah,
this is the way to do it.

http://codereview.adblockplus.org/6155422901731328/diff/5629499534213120/modu...
modules/rietveld/files/wrapper.py:48: def calculate_signature(str):
On 2015/06/03 15:42:35, Wladimir Palant wrote:
> On 2015/06/02 21:32:06, Sebastian Noack wrote:
> > Nit: Please don't override built-in symbols like "str".
> 
> Interesting that you seem to be fine with overriding "file" but not with
> overriding "str" :)

I just waited for the day, you notice that. ;P The reason is that "file" is
deprecated and in fact gone in Python 3. So since you should never use the
global "file" anyway, it's fine to use it as local variable name.

http://codereview.adblockplus.org/6155422901731328/diff/5629499534213120/modu...
modules/rietveld/files/wrapper.py:92: redirect(continue_url or base_url)
On 2015/06/03 15:42:35, Wladimir Palant wrote:
> On 2015/06/02 21:32:06, Sebastian Noack wrote:
> > It seems inconsistent that you return after error(), but use if/else when
> > calling redirect().
> 
> Not really, the three branches here represent the functions implemented:
logout,
> finalize login, start login. Using returns here would make the logic
confusing.
> I've split up the logic into functions to make that more obvious.

Fair enough.

[Wladimir Palant, 2015-06-03 18:28:04]

Overriding OAuth validation as well now - Rietveld is using it even though they
have their own flow (and own set of credentials) to generate the token. With
these changes upload.py works now. One problem remains: the token is verified
for each request which is slow and error-prone. I think that this is the same
with our current Rietveld instance - that's what is making base file upload so
slow. We might want to add a bit of caching here.

[mathias, 2015-06-03 18:33:07]

http://codereview.adblockplus.org/6155422901731328/diff/5194384987389952/modu...
File modules/discourse/manifests/init.pp (right):

http://codereview.adblockplus.org/6155422901731328/diff/5194384987389952/modu...
modules/discourse/manifests/init.pp:324: Customservice <| |> {
This will implicitly realize all customservice directives, even if there where
more than those declared in this file (which don't need explicit realization). I
suppose this is not intended, so please remove the <| |> selector.

http://codereview.adblockplus.org/6155422901731328/diff/5673385510043648/modu...
File modules/rietveld/manifests/init.pp (left):

http://codereview.adblockplus.org/6155422901731328/diff/5673385510043648/modu...
modules/rietveld/manifests/init.pp:8: $database = hiera('rietveld::database', {
The $database parameter seems to be obsolete by now (or "for now", at least).
This renders the additional wrapping in module codereview obsolete as well, so
all Postgresql-specific stuff there and the parameter here should be removed by
this patch-set.

Note that the subsequent move of the Nginx directives from module rietfeld to
module codereview is still in queue, so that module will become a
straightforward ("empty") wrapper for now.

[Wladimir Palant, 2015-06-04 21:23:58]

I've addressed Matze's comments and added user info caching so that uploading
base files is reasonably fast now. I also made sure that the user info filled in
by _Dynamic_GetOAuthUser is correct - while it seemed to be working before,
users couldn't edit their reviews. Finally, I now instruct dev_appserver to use
sendmail (not sure whether it was already sending mails before that but now it
definitely does).

[Sebastian Noack, 2015-06-04 21:51:43]

Still LGTM on the Python code.

http://codereview.adblockplus.org/6155422901731328/diff/5631943370604544/modu...
File modules/rietveld/files/wrapper.py (right):

http://codereview.adblockplus.org/6155422901731328/diff/5631943370604544/modu...
modules/rietveld/files/wrapper.py:195: user_id = '1' + ''.join(['%02d' % ord(x)
for x in user_id_digest])[:20]
WTF this algorithm, but I suppose we have to use it.

[mathias, 2015-06-04 22:19:34]

LGTM.

[Wladimir Palant, 2015-06-04 23:19:54]

http://codereview.adblockplus.org/6155422901731328/diff/5631943370604544/modu...
File modules/rietveld/files/wrapper.py (right):

http://codereview.adblockplus.org/6155422901731328/diff/5631943370604544/modu...
modules/rietveld/files/wrapper.py:195: user_id = '1' + ''.join(['%02d' % ord(x)
for x in user_id_digest])[:20]
On 2015/06/04 21:51:43, Sebastian Noack wrote:
> WTF this algorithm, but I suppose we have to use it.

Luckily, it doesn't seem to matter - Rietveld works only with email addresses,
it doesn't care about user IDs. Otherwise this would be a security hole I think,
generating a collision with user IDs shouldn't be hard.

[Wladimir Palant, 2015-06-04 23:23:52]

Pushed: https://hg.adblockplus.org/infrastructure/rev/2f9490cd2f9e

I guess Matze will reimage the server new.