From 8a96647d6e98ccead1f1493e114d1f8b2eb412c4 Mon Sep 17 00:00:00 2001
From: Joseph Schorr <josephschorr@users.noreply.github.com>
Date: Fri, 21 Jul 2017 11:06:21 -0400
Subject: [PATCH] Add feature flag to enable team syncing setup when not a
 superuser

---
 config.py                           |  3 +++
 endpoints/api/team.py               | 16 +++++++++++-----
 endpoints/api/test/test_security.py | 28 +++++++++++++++++++++++++++-
 endpoints/test/shared.py            | 10 ++++++++++
 4 files changed, 51 insertions(+), 6 deletions(-)

diff --git a/config.py b/config.py
index 795443317..c4e06d14a 100644
--- a/config.py
+++ b/config.py
@@ -462,6 +462,9 @@ class DefaultConfig(ImmutableConfig):
   TEAM_RESYNC_STALE_TIME = '30m'
   TEAM_SYNC_WORKER_FREQUENCY = 60 # seconds
 
+  # Feature Flag: If enabled, non-superusers can setup team syncing.
+  FEATURE_NONSUPERUSER_TEAM_SYNCING_SETUP = False
+
   # The default configurable tag expiration time for time machine.
   DEFAULT_TAG_EXPIRATION = '2w'
 
diff --git a/endpoints/api/team.py b/endpoints/api/team.py
index 981e0f1cc..2b9564ae9 100644
--- a/endpoints/api/team.py
+++ b/endpoints/api/team.py
@@ -209,6 +209,14 @@ class OrganizationTeam(ApiResource):
     raise Unauthorized()
 
 
+def _syncing_setup_allowed(orgname):
+  """ Returns whether syncing setup is allowed for the current user over the matching org. """
+  if not features.NONSUPERUSER_TEAM_SYNCING_SETUP and not SuperUserPermission().can():
+    return False
+
+  return AdministerOrganizationPermission(orgname).can()
+
+
 @resource('/v1/organization/<orgname>/team/<teamname>/syncing')
 @path_param('orgname', 'The name of the organization')
 @path_param('teamname', 'The name of the team')
@@ -221,8 +229,7 @@ class OrganizationTeamSyncing(ApiResource):
   @verify_not_prod
   @require_fresh_login
   def post(self, orgname, teamname):
-    # User must be both the org admin AND a superuser.
-    if SuperUserPermission().can() and AdministerOrganizationPermission(orgname).can():
+    if _syncing_setup_allowed(orgname):
       try:
         team = model.team.get_organization_team(orgname, teamname)
       except model.InvalidTeamException:
@@ -248,8 +255,7 @@ class OrganizationTeamSyncing(ApiResource):
   @verify_not_prod
   @require_fresh_login
   def delete(self, orgname, teamname):
-    # User must be both the org admin AND a superuser.
-    if SuperUserPermission().can() and AdministerOrganizationPermission(orgname).can():
+    if _syncing_setup_allowed(orgname):
       try:
         team = model.team.get_organization_team(orgname, teamname)
       except model.InvalidTeamException:
@@ -296,7 +302,7 @@ class TeamMemberList(ApiResource):
       }
 
       if features.TEAM_SYNCING and authentication.federated_service:
-        if SuperUserPermission().can() and AdministerOrganizationPermission(orgname).can():
+        if _syncing_setup_allowed(orgname):
           data['can_sync'] = {
             'service': authentication.federated_service,
           }
diff --git a/endpoints/api/test/test_security.py b/endpoints/api/test/test_security.py
index a2dee07f2..50c58f39d 100644
--- a/endpoints/api/test/test_security.py
+++ b/endpoints/api/test/test_security.py
@@ -1,3 +1,5 @@
+from mock import patch
+
 import pytest
 from flask_principal import AnonymousIdentity
 
@@ -10,7 +12,7 @@ from endpoints.api.signing import RepositorySignatures
 from endpoints.api.search import ConductRepositorySearch
 from endpoints.api.superuser import SuperUserRepositoryBuildLogs, SuperUserRepositoryBuildResource
 from endpoints.api.superuser import SuperUserRepositoryBuildStatus
-from endpoints.test.shared import client_with_identity
+from endpoints.test.shared import client_with_identity, toggle_feature
 
 from test.fixtures import *
 
@@ -69,3 +71,27 @@ NOTIFICATION_PARAMS = {'namespace': 'devtable', 'repository': 'devtable/simple',
 def test_api_security(resource, method, params, body, identity, expected, client):
   with client_with_identity(identity, client) as cl:
     conduct_api_call(cl, resource, method, params, body, expected)
+
+
+@pytest.mark.parametrize('is_superuser', [
+  (True),
+  (False),
+])
+@pytest.mark.parametrize('allow_nonsuperuser', [
+  (True),
+  (False),
+])
+@pytest.mark.parametrize('method, expected', [
+  ('POST', 400),
+  ('DELETE', 200),
+])
+def test_team_sync_security(is_superuser, allow_nonsuperuser, method, expected, client):
+  def is_superuser_method(_):
+    return is_superuser
+
+  with patch('auth.permissions.superusers.is_superuser', is_superuser_method):
+    with toggle_feature('NONSUPERUSER_TEAM_SYNCING_SETUP', allow_nonsuperuser):
+      with client_with_identity('devtable', client) as cl:
+        expect_success = is_superuser or allow_nonsuperuser
+        expected_status = expected if expect_success else 403
+        conduct_api_call(cl, OrganizationTeamSyncing, method, TEAM_PARAMS, {}, expected_status)
diff --git a/endpoints/test/shared.py b/endpoints/test/shared.py
index abb22ded9..d2888938c 100644
--- a/endpoints/test/shared.py
+++ b/endpoints/test/shared.py
@@ -30,6 +30,16 @@ def client_with_identity(auth_username, client):
     sess[CSRF_TOKEN_KEY] = None
 
 
+@contextmanager
+def toggle_feature(name, enabled):
+  """ Context manager which temporarily toggles a feature. """
+  import features
+  previous_value = getattr(features, name)
+  setattr(features, name, enabled)
+  yield
+  setattr(features, name, previous_value)
+
+
 def add_csrf_param(params):
   """ Returns a params dict with the CSRF parameter added. """
   params = params or {}