Monitoring proxy for time series data

appengine/chrome_infra_mon_proxy/admin_handler.py

Index: appengine/chrome_infra_mon_proxy/admin_handler.py
diff --git a/appengine/chrome_infra_mon_proxy/admin_handler.py b/appengine/chrome_infra_mon_proxy/admin_handler.py
new file mode 100644
index 0000000000000000000000000000000000000000..266a9d2474c9ac299f88fdfe11aca37b49579807
--- /dev/null
+++ b/appengine/chrome_infra_mon_proxy/admin_handler.py
@@ -0,0 +1,125 @@
+# Copyright 2015 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+import json
+import logging
+import os
+import sys
+import webapp2
+
+from google.appengine.api import users
+from webapp2_extras import jinja2
+
+import common
+
+class AdminCommand(object):
+  """Base class for administrative commands.
+
+  Implement get() and post() methods in the subclasses.
+  """
+
+  def __init__(self, handler):
+    self._handler = handler
+
+  @property
+  def request(self):
+    return self._handler.request
+
+  @property
+  def response(self):
+    return self._handler.response
+
+  def render_response(self, _template, **context):
+    self._handler.render_response(_template, **context)
+
+
+class AdminPage(AdminCommand):
+  """Display the admin page."""
+
+  def get(self):
+    context = {'title': 'Admin: Chrome Infra Monitoring Proxy'}
+    self.render_response('admin.html', **context)
+
+  def post(self):
+    self.response.set_status(403)
+    return
+
+
+def setParams(params, data):

[agable, 2015/04/23 19:29:37]

This should be a staticmethod inside SetCredentials. Could be better named or
accompanied by a docstring as well.

[Sergey Berezin (google), 2015/04/24 19:09:51]

Moved both this and _parsers inside SetCredentials, and added a doc string. I
couldn't come up with a better name (it used to be "serialize", but others asked
to change that too :-)

+  params['url'] = data.url
+  params['credentials'] = json.dumps(data.credentials)
+  params['scopes'] = '\n'.join(data.scopes)
+  params['headers'] = json.dumps(data.headers)
+
+_parsers = {
+    'url': lambda v: v,
+    'credentials': json.loads,
+    'scopes': lambda v: [l for l in v.splitlines() if l],

[agable, 2015/04/23 19:29:37]

Might want to strip whitespace too

[Sergey Berezin (google), 2015/04/24 19:09:52]

Done - good catch, thanks.

+    'headers': json.loads,
+}
+
+
+class SetCredentials(AdminCommand):
+  """Save new credentials for the Monacq endpoint."""
+
+  def get(self):
+    params = {
+        'message': '',
+        'title': 'Config: Chrome Infra Monitoring Proxy',
+    }
+    data = common.MonAcqData.get_by_id(common.CONFIG_DATA_KEY)
+    if data:
+      setParams(params, data)
+    self.render_response('set_credentials.html', **params)
+
+  def post(self):
+    params = {
+        'message': '',
+        'title': 'Config: Chrome Infra Monitoring Proxy',
+    }
+    data = common.MonAcqData.get_or_insert(common.CONFIG_DATA_KEY)
+    setParams(params, data)
+
+    updated_fields = False
+    failed_fields = []
+    for field, parser in _parsers.iteritems():
+      if not self.request.get(field):
+        continue
+      try:
+        setattr(data, field, parser(self.request.get(field)))
+        updated_fields = True
+      except ValueError:
+        failed_fields.append(field)
+      params[field] = self.request.get(field)
+
+    if failed_fields:
+      params['message'] = 'Failed to update %s. Please try again.' % (
+          ', '.join(failed_fields))
+    elif updated_fields:
+      data.put()
+      setParams(params, data)
+      params['message'] = 'Updated configuration.'
+      logging.info('Updated configuration: %r', data)
+    self.render_response('set_credentials.html', **params)
+
+
+commands = {
+  '': AdminPage,
+  'set-credentials': SetCredentials,
+}
+
+class AdminDispatch(common.BaseHandler):
+  """Provide a cached Jinja environment to each request."""
+
+  def get(self, command):
+    if not users.is_current_user_admin():

[agable, 2015/04/23 19:29:37]

If the user is logged in, but not an admin, you should 403 directly rather than
sending them to yet another login url.

[Sergey Berezin (google), 2015/04/24 19:09:52]

Done.

+      self.redirect(users.create_login_url(self.request.url))
+      return
+    commands[command](self).get()

[agable, 2015/04/23 19:29:37]

This dispatch system breaks the ability for url handlers in the WSGIApplication
to accept regular expressions and pass the capture groups on to the handlers.
Since you're only calling with (self), no additional url components can be
captured and passed on.

Why not instead put in app.yaml:

handlers:
- url: /admin/.*
  secure: always
  login: admin
  script: admin.app

And then handle the authentication and dispatch within an admin-only
WSGIApplication in this file?

[Sergey Berezin (google), 2015/04/24 19:09:51]

Honestly, I don't need this functionality now, and I don't expect to need it in
the future. So I'm fine with losing it.

But what I gain is a single place where authentication is implemented, and
command handlers don't need to bother with it.
Done (just because it eliminates a couple of imports :-). Though I still don't
trust "login: admin" in app.yaml, as it is untestable, and is easy to mess up. I
also plan to switch to auth groups for /admin/.*, and will have to strip login:
admin. Added a TODO to that effect.

+
+  def post(self, command):
+    if not users.is_current_user_admin():
+      self.response.set_status(403)
+      return
+    commands[command](self).post()