Reduce usage of GAE users. (#1355)

api/accounts_api_test.py

@@ -31,6 +31,9 @@ from internals import models
 class AccountsAPITest(unittest.TestCase):
 
   def setUp(self):
+    self.app_admin = models.AppUser(email='admin@example.com')
+    self.app_admin.is_admin = True
+    self.app_admin.put()
     self.appuser_1 = models.AppUser(email='user@example.com')
     self.appuser_1.put()
     self.appuser_id = self.appuser_1.key().id()
@@ -39,11 +42,12 @@ class AccountsAPITest(unittest.TestCase):
     self.handler = accounts_api.AccountsAPI()
 
   def tearDown(self):
+    self.app_admin.delete()
     self.appuser_1.delete()
 
   def test_create__normal_valid(self):
     """Admin wants to register a normal account."""
-    testing_config.sign_in('admin@example.com', 123567890, is_admin=True)
+    testing_config.sign_in('admin@example.com', 123567890)
 
     json_data = {'email': 'new@example.com', 'isAdmin': False}
     with register.app.test_request_context(self.request_path, json=json_data):
@@ -58,7 +62,7 @@ class AccountsAPITest(unittest.TestCase):
 
   def test_create__admin_valid(self):
     """Admin wants to register a new admin account."""
-    testing_config.sign_in('admin@example.com', 123567890, is_admin=True)
+    testing_config.sign_in('admin@example.com', 123567890)
 
     json_data = {'email': 'new_admin@example.com', 'isAdmin': True}
     with register.app.test_request_context(self.request_path, json=json_data):
@@ -85,7 +89,7 @@ class AccountsAPITest(unittest.TestCase):
 
   def test_create__invalid(self):
     """We cannot create an account without an email address."""
-    testing_config.sign_in('admin@example.com', 123567890, is_admin=True)
+    testing_config.sign_in('admin@example.com', 123567890)
 
     json_data = {'isAdmin': False}  # No email
     with register.app.test_request_context(self.request_path):
@@ -98,7 +102,7 @@ class AccountsAPITest(unittest.TestCase):
 
   def test_create__duplicate(self):
     """We cannot create an account with a duplicate email."""
-    testing_config.sign_in('admin@example.com', 123567890, is_admin=True)
+    testing_config.sign_in('admin@example.com', 123567890)
 
     json_data = {'email': 'user@example.com'}
     with register.app.test_request_context(self.request_path, json=json_data):
@@ -110,7 +114,7 @@ class AccountsAPITest(unittest.TestCase):
 
   def test_delete__valid(self):
     """Admin wants to delete an account."""
-    testing_config.sign_in('admin@example.com', 123567890, is_admin=True)
+    testing_config.sign_in('admin@example.com', 123567890)
 
     with register.app.test_request_context(self.request_path):
       actual_json = self.handler.do_delete(self.appuser_id)
@@ -132,7 +136,7 @@ class AccountsAPITest(unittest.TestCase):
 
   def test_delete__invalid(self):
     """We cannot delete an account without an account_id."""
-    testing_config.sign_in('admin@example.com', 123567890, is_admin=True)
+    testing_config.sign_in('admin@example.com', 123567890)
 
     with register.app.test_request_context(self.request_path):
       with self.assertRaises(werkzeug.exceptions.BadRequest):
@@ -144,7 +148,7 @@ class AccountsAPITest(unittest.TestCase):
 
   def test_delete__not_found(self):
     """We cannot delete an account with the wrong account_id."""
-    testing_config.sign_in('admin@example.com', 123567890, is_admin=True)
+    testing_config.sign_in('admin@example.com', 123567890)
 
     with register.app.test_request_context(self.request_path):
       with self.assertRaises(werkzeug.exceptions.NotFound):

api/features_api_test.py

@@ -40,12 +40,17 @@ class FeaturesAPITest(unittest.TestCase):
     self.request_path = '/api/v0/features/%d' % self.feature_id
     self.handler = features_api.FeaturesAPI()
 
+    self.app_admin = models.AppUser(email='admin@example.com')
+    self.app_admin.is_admin = True
+    self.app_admin.put()
+
   def tearDown(self):
     self.feature_1.delete()
+    self.app_admin.delete()
 
   def test_delete__valid(self):
     """Admin wants to soft-delete a feature."""
-    testing_config.sign_in('admin@example.com', 123567890, is_admin=True)
+    testing_config.sign_in('admin@example.com', 123567890)
 
     with register.app.test_request_context(self.request_path):
       actual_json = self.handler.do_delete(self.feature_id)
@@ -67,7 +72,7 @@ class FeaturesAPITest(unittest.TestCase):
 
   def test_delete__invalid(self):
     """We cannot soft-delete a feature without a feature_id."""
-    testing_config.sign_in('admin@example.com', 123567890, is_admin=True)
+    testing_config.sign_in('admin@example.com', 123567890)
 
     with register.app.test_request_context(self.request_path):
       with self.assertRaises(werkzeug.exceptions.BadRequest):
@@ -78,7 +83,7 @@ class FeaturesAPITest(unittest.TestCase):
 
   def test_delete__not_found(self):
     """We cannot soft-delete a feature with the wrong feature_id."""
-    testing_config.sign_in('admin@example.com', 123567890, is_admin=True)
+    testing_config.sign_in('admin@example.com', 123567890)
 
     with register.app.test_request_context(self.request_path):
       with self.assertRaises(werkzeug.exceptions.NotFound):

framework/basehandlers.py

@@ -25,7 +25,6 @@ import flask
 import flask.views
 import werkzeug.exceptions
 
-from google.appengine.api import users as gae_users
 from google.appengine.ext import db
 
 import settings
@@ -78,11 +77,7 @@ class BaseHandler(flask.views.MethodView):
 
   def get_current_user(self, required=False):
     # TODO(jrobbins): oauth support
-    current_user = None
-    if not settings.UNIT_TEST_MODE and self.request.method == 'POST':
-      current_user = users.get_current_user() or gae_users.get_current_user()
-    else:
-      current_user = users.get_current_user()
+    current_user = users.get_current_user()
 
     if required and not current_user:
       self.abort(403, msg='User must be signed in')

framework/permissions.py

@@ -19,17 +19,11 @@ from __future__ import print_function
 import logging
 import flask
 
-from google.appengine.api import users as gae_users
 from framework import users
 from internals import models
 
 def can_admin_site(user):
   """Return True if the current user is allowed to administer the site."""
-  # 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 gae_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())

framework/permissions_test.py

@@ -53,14 +53,32 @@ test_app = basehandlers.FlaskApplication(
 
 class PermissionFunctionTests(unittest.TestCase):
 
+  def setUp(self):
+    self.app_user = models.AppUser(email='registered@example.com')
+    self.app_user.put()
+
+    self.app_admin = models.AppUser(email='admin@example.com')
+    self.app_admin.is_admin = True
+    self.app_admin.put()
+
+  def tearDown(self):
+    self.app_user.delete()
+    self.app_admin.delete()
+
   def check_function_results(
       self, func, additional_args,
-      normal='missing', special='missing', admin='missing', anon='missing'):
+      unregistered='missing', registered='missing',
+      special='missing', admin='missing', anon='missing'):
     """Test func under four conditions and check expected results."""
-    # Test normal users
-    testing_config.sign_in('user@example.com', 123)
+    # Test unregistered users
+    testing_config.sign_in('unregistered@example.com', 123)
     user = users.get_current_user()
-    self.assertEqual(normal, func(user, *additional_args))
+    self.assertEqual(unregistered, func(user, *additional_args))
+
+    # Test registered users
+    testing_config.sign_in('registered@example.com', 123)
+    user = users.get_current_user()
+    self.assertEqual(registered, func(user, *additional_args))
 
     # Test special users
     # TODO(jrobbins): generalize this.
@@ -72,7 +90,7 @@ class PermissionFunctionTests(unittest.TestCase):
     self.assertEqual(special, func(user, *additional_args))
 
     # Test admin users
-    testing_config.sign_in('user@example.com', 123, is_admin=True)
+    testing_config.sign_in('admin@example.com', 123)
     user = users.get_current_user()
     self.assertEqual(admin, func(user, *additional_args))
 
@@ -84,70 +102,74 @@ class PermissionFunctionTests(unittest.TestCase):
   def test_can_admin_site(self):
     self.check_function_results(
         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))
+        unregistered=False, registered=False,
+        special=False, admin=True, anon=False)
 
   def test_can_view_feature(self):
     self.check_function_results(
         permissions.can_view_feature, (None,),
-        normal=True, special=True, admin=True, anon=True)
+        unregistered=True, registered=True,
+        special=True, admin=True, anon=True)
 
   def test_can_create_feature(self):
     self.check_function_results(
         permissions.can_create_feature, tuple(),
-        normal=False, special=True, admin=True, anon=False)
+        unregistered=False, registered=True,
+        special=True, admin=True, anon=False)
 
   def test_can_edit_any_feature(self):
     self.check_function_results(
         permissions.can_edit_any_feature, tuple(),
-        normal=False, special=True, admin=True, anon=False)
+        unregistered=False, registered=True,
+        special=True, admin=True, anon=False)
 
   def test_can_edit_feature(self):
     self.check_function_results(
         permissions.can_edit_feature, (None,),
-        normal=False, special=True, admin=True, anon=False)
+        unregistered=False, registered=True,
+        special=True, admin=True, anon=False)
 
   def test_can_approve_feature(self):
     approvers = []
     self.check_function_results(
         permissions.can_approve_feature, (None, approvers),
-        normal=False, special=False, admin=True, anon=False)
+        unregistered=False, registered=False,
+        special=False, admin=True, anon=False)
 
-    approvers = ['user@example.com']
+    approvers = ['registered@example.com']
     self.check_function_results(
         permissions.can_approve_feature, (None, approvers),
-        normal=True, special=False, admin=True, anon=False)
+        unregistered=False, registered=True,
+        special=False, admin=True, anon=False)
 
 
 class RequireAdminSiteTests(unittest.TestCase):
 
-  def test_require_admin_site__normal_user(self):
-    """Wrapped method rejects call from normal user."""
+  def setUp(self):
+    self.app_user = models.AppUser(email='registered@example.com')
+    self.app_user.put()
+
+    self.app_admin = models.AppUser(email='admin@example.com')
+    self.app_admin.is_admin = True
+    self.app_admin.put()
+
+  def tearDown(self):
+    self.app_user.delete()
+    self.app_admin.delete()
+
+  def test_require_admin_site__unregistered_user(self):
+    """Wrapped method rejects call from an unregistered user."""
     handler = MockHandler()
-    testing_config.sign_in('user@example.com', 123)
+    testing_config.sign_in('unregistered@example.com', 123)
+    with test_app.test_request_context('/path', method='POST'):
+      with self.assertRaises(werkzeug.exceptions.Forbidden):
+        handler.do_post()
+    self.assertEqual(handler.called_with, None)
+
+  def test_require_admin_site__registered_user(self):
+    """Wrapped method rejects call from registered non-admin user."""
+    handler = MockHandler()
+    testing_config.sign_in('registered@example.com', 123)
     with test_app.test_request_context('/path', method='POST'):
       with self.assertRaises(werkzeug.exceptions.Forbidden):
         handler.do_post()
@@ -165,7 +187,7 @@ class RequireAdminSiteTests(unittest.TestCase):
   def test_require_admin_site__admin(self):
     """Wrapped method accepts call from an admin user."""
     handler = MockHandler()
-    testing_config.sign_in('admin@example.com', 123, is_admin=True)
+    testing_config.sign_in('admin@example.com', 123)
     with test_app.test_request_context('/path'):
       actual_response = handler.do_get(123, 234)
     self.assertEqual(handler.called_with, (123, 234))

testing_config.py

@@ -117,10 +117,10 @@ def sign_out():
       user_email='', user_id='', user_is_admin='0', overwrite=True)
 
 
-def sign_in(user_email, user_id, is_admin=False):
+def sign_in(user_email, user_id):
   """Set env variables to represent a signed out user."""
   ourTestbed.setup_env(
       user_email=user_email,
       user_id=str(user_id),
-      user_is_admin='1' if is_admin else '0',
+      user_is_admin='0',  # This was for GAE user admin, we use AppUser.
       overwrite=True)