Allow site admins to be specified via AppUser entities. (#1288)

framework/permissions.py

@@ -24,10 +24,20 @@ from google.appengine.api import users
 from internals import models
 
 
-def can_admin_site(unused_user):
+def can_admin_site(user):
   """Return True if the current user is allowed to administer the site."""
-  # TODO(jrobbins): replace this with user.is_admin.
-  return users.is_current_user_admin()
+  # A user is an admin if they are an admin of the GAE project.
+  # TODO(jrobbins): delete this statement after legacy admins moved to AppUser.
+  if users.is_current_user_admin():
+    return True
+
+  # A user is an admin if they have an AppUser entity that has is_admin set.
+  if user:
+    app_user = models.AppUser.get_app_user(user.email())
+    if app_user is not None:
+      return app_user.is_admin
+
+  return False
 
 
 def can_view_feature(unused_user, unused_feature):

framework/permissions_test.py

@@ -25,6 +25,7 @@ from google.appengine.api import users
 
 from framework import basehandlers
 from framework import permissions
+from internals import models
 
 
 class MockHandler(basehandlers.BaseHandler):
@@ -84,6 +85,30 @@ class PermissionFunctionTests(unittest.TestCase):
         permissions.can_admin_site, tuple(),
         normal=False, special=False, admin=True, anon=False)
 
+  def test_can_admin_site__appuser(self):
+    """A registered AppUser that has is_admin set can admin the site."""
+    email = 'app-admin@example.com'
+    testing_config.sign_in(email, 111)
+    user = users.get_current_user()
+
+    # Make sure there is no left over entity from past runs.
+    query = models.AppUser.all().filter('email =', email)
+    for old_app_user in query.fetch(None):
+      old_app_user.delete()
+
+    self.assertFalse(permissions.can_admin_site(user))
+
+    app_user = models.AppUser(email=email)
+    app_user.put()
+    self.assertFalse(permissions.can_admin_site(user))
+
+    app_user.is_admin = True
+    app_user.put()
+    print('user is %r' % user)
+    print('get_app_user is %r' % models.AppUser.get_app_user(email))
+    print('get_app_user.is_admin is %r' % models.AppUser.get_app_user(email).is_admin)
+    self.assertTrue(permissions.can_admin_site(user))
+
   def test_can_view_feature(self):
     self.check_function_results(
         permissions.can_view_feature, (None,),

framework/ramcache_test.py

@@ -85,10 +85,10 @@ class RAMCacheFunctionTests(unittest.TestCase):
 
   @mock.patch('framework.ramcache.SharedInvalidate.invalidate')
   def testDelete_NotFound(self, mock_invalidate):
-    """Deleting an item that is not in the cache is a no-op."""
+    """Deleting an item that is not in the cache still invalidates."""
     ramcache.delete(KEY_5)
 
-    mock_invalidate.assert_not_called()
+    mock_invalidate.assert_called()
 
   @mock.patch('framework.ramcache.SharedInvalidate.invalidate')
   def testDelete_Found(self, mock_invalidate):

internals/models.py

@@ -1219,10 +1219,39 @@ class AppUser(DictModel):
 
   #user = db.UserProperty(required=True, verbose_name='Google Account')
   email = db.EmailProperty(required=True)
-  #is_admin = db.BooleanProperty(default=False)
+  is_admin = db.BooleanProperty(default=False)
   created = db.DateTimeProperty(auto_now_add=True)
   updated = db.DateTimeProperty(auto_now=True)
 
+  def put(self, **kwargs):
+    """when we update an AppUser, also invalidate ramcache."""
+    key = super(DictModel, self).put(**kwargs)
+    cache_key = 'user|%s' % self.email
+    ramcache.delete(cache_key)
+
+  def delete(self, **kwargs):
+    """when we delete an AppUser, also invalidate ramcache."""
+    key = super(DictModel, self).delete(**kwargs)
+    cache_key = 'user|%s' % self.email
+    ramcache.delete(cache_key)
+
+  @classmethod
+  def get_app_user(cls, email):
+    """Return the AppUser for the specified user, or None."""
+    cache_key = 'user|%s' % email
+    cached_app_user = ramcache.get(cache_key)
+    if cached_app_user:
+      return cached_app_user
+
+    query = cls.all()
+    query.filter('email =', email)
+    found_app_user = query.get()
+    if found_app_user:
+      ramcache.set(cache_key, found_app_user)
+      return found_app_user
+
+    return None
+
 
 def list_with_component(l, component):
   return [x for x in l if x.id() == component.key().id()]

pages/users.py

@@ -57,12 +57,13 @@ class CreateUserAPIHandler(basehandlers.FlaskHandler):
 
   @permissions.require_admin_site
   def process_post_data(self):
-    email = flask.request.form['email']
+    email = self.form['email']
 
     # Don't add a duplicate email address.
     user = models.AppUser.all(keys_only=True).filter('email = ', email).get()
     if not user:
       user = models.AppUser(email=db.Email(email))
+      user.is_admin = 'is_admin' in self.form
       user.put()
 
       response_json = user.format_for_template()

static/elements/chromedash-userlist.js

@@ -1,5 +1,6 @@
 import {LitElement, css, html} from 'lit-element';
 import SHARED_STYLES from '../css/shared.css';
+import {nothing} from 'lit-html';
 
 
 class ChromedashUserlist extends LitElement {
@@ -20,6 +21,18 @@ class ChromedashUserlist extends LitElement {
     return [
       SHARED_STYLES,
       css`
+      form {
+        padding: var(--content-padding);
+        background: var(--card-background);
+        border: var(--card-border);
+        box-shadow: var(--card-box-shadow);
+        margin-bottom: var(--content-padding);
+        max-width: 20em;
+      }
+      form > * + * {
+        margin-top: var(--content-padding-half);
+      }
+
       ul {
         margin-top: 10px;
       }
@@ -52,8 +65,12 @@ class ChromedashUserlist extends LitElement {
 
     if (formEl.checkValidity()) {
       const email = formEl.querySelector('input[name="email"]').value;
+      const isAdmin = formEl.querySelector('input[name="is_admin"]').checked;
       const formData = new FormData();
       formData.append('email', email);
+      if (isAdmin) {
+        formData.append('is_admin', 'on');
+      }
       formData.append('token', this.token);
 
       const resp = await fetch(this.actionPath, {
@@ -98,14 +115,25 @@ class ChromedashUserlist extends LitElement {
   render() {
     return html`
       <form id="form" name="user_form" method="POST" action="${this.actionPath}" onsubmit="return false;">
-        <input type="email" placeholder="Email address" name="email" id="id_email" required>
-        <td><input type="submit" @click="${this.ajaxSubmit}">
+        <div>
+          <input type="email" placeholder="Email address" name="email"
+                 required>
+        </div>
+        <div>
+          <label><input type="checkbox" name="is_admin"> User is admin</label>
+        </div>
+        <div>
+          <input type="submit" @click="${this.ajaxSubmit}" value="Add user">
+        </div>
       </form>
+
       <ul id="user-list">
         ${this.users.map((user, index) => html`
           <li>
-            <a href="/admin/users/delete/${user.id}" data-index="${index}" @click="${this.ajaxDelete}">delete</a>
+            <a href="/admin/users/delete/${user.id}"
+               data-index="${index}" @click="${this.ajaxDelete}">delete</a>
             <span>${user.email}</span>
+            ${user.is_admin ? html`(admin)` : nothing}
           </li>
           `)}
       </ul>

templates/admin/users/new.html

@@ -6,7 +6,7 @@
 
 {% block subheader %}
 <div id="subheader">
-  <h2>Allowlist a user</h2>
+  <h2>Application users</h2>
 </div>
 {% endblock %}