From 4ae4d7051b827fbb0f01f8030e8bb04469f3ee77 Mon Sep 17 00:00:00 2001 From: Clemens Wolff Date: Fri, 22 Nov 2019 18:24:48 -0500 Subject: [PATCH] Enable authentication via Github access token (#247) * Make naming of responses imports consistent * Enable authentication via Github access token * Use Github authentication for live service tests * Make live/local test mode distinction clearer --- .env | 1 + .travis.yml | 17 +- docker-compose.yml | 2 + docker/integtest/1-register-client.sh | 4 +- opwen_email_server/config.py | 2 + opwen_email_server/constants/github.py | 3 + opwen_email_server/integration/connexion.py | 14 +- opwen_email_server/services/auth.py | 92 +++++++++++ .../opwen_email_server/services/test_auth.py | 146 ++++++++++++++++++ .../utils/test_email_parser.py | 24 +-- 10 files changed, 284 insertions(+), 21 deletions(-) create mode 100644 opwen_email_server/constants/github.py diff --git a/.env b/.env index 18f7d0c7..87fb5ab0 100644 --- a/.env +++ b/.env @@ -10,6 +10,7 @@ LOKOLE_EMAIL_SERVER_QUEUES_SAS_NAME= LOKOLE_EMAIL_SERVER_QUEUES_SAS_KEY= LOKOLE_EMAIL_SERVER_QUEUES_NAMESPACE= LOKOLE_SENDGRID_KEY= +REGISTRATION_CREDENTIALS=admin:password CLOUDBROWSER_PORT=10001 AZURITE_PORT=10000 diff --git a/.travis.yml b/.travis.yml index ac2ea3db..79d528fc 100644 --- a/.travis.yml +++ b/.travis.yml @@ -2,8 +2,8 @@ language: minimal env: matrix: - - LOKOLE_QUEUE_BROKER_SCHEME=azureservicebus - - LOKOLE_QUEUE_BROKER_SCHEME=amqp + - TEST_MODE=live + - TEST_MODE=local services: - docker @@ -21,7 +21,14 @@ script: else echo "Skipping cert renewal for on-demand build" fi - if [[ "$TRAVIS_PULL_REQUEST" = "false" ]] && [[ "$LOKOLE_QUEUE_BROKER_SCHEME" = "azureservicebus" ]]; then + if [[ "$TEST_MODE" = "live" ]]; then + export REGISTRATION_CREDENTIALS="$GITHUB_AUTH_USERNAME:$GITHUB_AUTH_TOKEN" + export LOKOLE_QUEUE_BROKER_SCHEME="azureservicebus" + else + export REGISTRATION_CREDENTIALS="admin:password" + export LOKOLE_QUEUE_BROKER_SCHEME="amqp" + fi + if [[ "$TRAVIS_PULL_REQUEST" = "false" ]] && [[ "$TEST_MODE" = "live" ]]; then echo "Skipping live service test for branch build" else make build verify-build @@ -31,7 +38,7 @@ script: after_success: - | - if [[ "$LOKOLE_QUEUE_BROKER_SCHEME" = "amqp" ]]; then + if [[ "$TEST_MODE" = "local" ]]; then bash <(curl -s https://codecov.io/bash) fi @@ -50,4 +57,4 @@ deploy: on: # yamllint disable rule:truthy repo: ascoderu/opwen-cloudserver tags: true - condition: $LOKOLE_QUEUE_BROKER_SCHEME = azureservicebus + condition: $TEST_MODE = live diff --git a/docker-compose.yml b/docker-compose.yml index c05a3bd2..0122a54c 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -106,6 +106,8 @@ services: context: . dockerfile: docker/integtest/Dockerfile command: echo skipped + environment: + REGISTRATION_CREDENTIALS: ${REGISTRATION_CREDENTIALS} volumes: - /var/run/docker.sock:/var/run/docker.sock diff --git a/docker/integtest/1-register-client.sh b/docker/integtest/1-register-client.sh index 15e621bc..8911a089 100755 --- a/docker/integtest/1-register-client.sh +++ b/docker/integtest/1-register-client.sh @@ -11,7 +11,7 @@ mkdir -p "${out_dir}" # normally this endpoint would be called during a new lokole device setup curl -fs \ -H "Content-Type: application/json" \ - -u "admin:password" \ + -u "${REGISTRATION_CREDENTIALS}" \ -d '{"domain":"developer1.lokole.ca"}' \ "http://nginx:8888/api/email/register/" \ | tee "${out_dir}/register1.json" @@ -27,7 +27,7 @@ if curl -fs \ # also register another client to simulate multi-client emails curl -fs \ -H "Content-Type: application/json" \ - -u "admin:password" \ + -u "${REGISTRATION_CREDENTIALS}" \ -d '{"domain":"developer2.lokole.ca"}' \ "http://nginx:8888/api/email/register/" \ | tee "${out_dir}/register2.json" diff --git a/opwen_email_server/config.py b/opwen_email_server/config.py index 172af46f..b0cd5183 100644 --- a/opwen_email_server/config.py +++ b/opwen_email_server/config.py @@ -33,6 +33,8 @@ REGISTRATION_USERNAME = env('REGISTRATION_USERNAME', '') REGISTRATION_PASSWORD = env('REGISTRATION_PASSWORD', '') +REGISTRATION_GITHUB_ORGANIZATION = env('REGISTRATION_GITHUB_ORGANIZATION', 'ascoderu') +REGISTRATION_GITHUB_TEAM = env('REGISTRATION_GITHUB_ORGANIZATION', 'lokole-registration') MAX_WIDTH_IMAGES = env.int('LOKOLE_MAX_WIDTH_EMAIL_IMAGES', 200) MAX_HEIGHT_IMAGES = env.int('LOKOLE_MAX_HEIGHT_EMAIL_IMAGES', 200) diff --git a/opwen_email_server/constants/github.py b/opwen_email_server/constants/github.py new file mode 100644 index 00000000..eef0c298 --- /dev/null +++ b/opwen_email_server/constants/github.py @@ -0,0 +1,3 @@ +from typing_extensions import Final # noqa: F401 + +GRAPHQL_URL = 'https://api.github.com/graphql' # type: Final # noqa: E501 diff --git a/opwen_email_server/integration/connexion.py b/opwen_email_server/integration/connexion.py index 22944c07..e1015ac7 100644 --- a/opwen_email_server/integration/connexion.py +++ b/opwen_email_server/integration/connexion.py @@ -14,7 +14,9 @@ from opwen_email_server.integration.azure import get_raw_email_storage from opwen_email_server.integration.celery import inbound_store from opwen_email_server.integration.celery import written_store +from opwen_email_server.services.auth import AnyOfBasicAuth from opwen_email_server.services.auth import BasicAuth +from opwen_email_server.services.auth import GithubBasicAuth email_receive = ReceiveInboundEmail( auth=get_auth(), @@ -46,8 +48,14 @@ pending_factory=get_pending_storage, ) -basic_auth = BasicAuth(users={config.REGISTRATION_USERNAME: { - 'password': config.REGISTRATION_PASSWORD, -}}) +basic_auth = AnyOfBasicAuth(auths=[ + BasicAuth(users={ + config.REGISTRATION_USERNAME: {'password': config.REGISTRATION_PASSWORD}, + }), + GithubBasicAuth( + organization=config.REGISTRATION_GITHUB_ORGANIZATION, + team=config.REGISTRATION_GITHUB_TEAM, + ), +]) healthcheck = Ping() diff --git a/opwen_email_server/services/auth.py b/opwen_email_server/services/auth.py index 23e87670..4a2d4957 100644 --- a/opwen_email_server/services/auth.py +++ b/opwen_email_server/services/auth.py @@ -1,14 +1,34 @@ from functools import lru_cache +from typing import Callable +from typing import Dict +from typing import Iterable +from typing import List from typing import Optional from libcloud.storage.types import ObjectDoesNotExistError +from requests import RequestException +from requests import post as http_post from opwen_email_server.constants import events +from opwen_email_server.constants import github from opwen_email_server.constants.cache import AUTH_DOMAIN_CACHE_SIZE from opwen_email_server.services.storage import AzureTextStorage from opwen_email_server.utils.log import LogMixin +class AnyOfBasicAuth(LogMixin): + def __init__(self, auths: Iterable[Callable[[str, str, Optional[List[str]]], Optional[Dict[str, str]]]]): + self._auths = list(auths) + + def __call__(self, username, password, required_scopes=None): + for auth in self._auths: + user = auth(username, password, required_scopes) + if user is not None: + return user + + return None + + class BasicAuth(LogMixin): def __init__(self, users: dict): self._users = dict(users) @@ -41,6 +61,78 @@ def _has_scopes(cls, user, required_scopes) -> bool: return set(required_scopes).issubset(user.get('scopes', [])) +class GithubBasicAuth(LogMixin): + def __init__(self, organization: str, team: str, page_size: int = 50): + self._organization = organization + self._team = team + self._page_size = page_size + + def __call__(self, username, password, required_scopes=None): + if not username or not password or not self._organization or not self._team: + return None + + try: + team_members = self._fetch_team_members(access_token=password) + user_exists = any(username == team_member for team_member in team_members) + except RequestException: + self.log_event(events.BAD_PASSWORD, {'username': username}) # noqa: E501 # yapf: disable + return None + + if not user_exists: + self.log_event(events.UNKNOWN_USER, {'username': username}) # noqa: E501 # yapf: disable + return None + + return {'sub': username} + + def _fetch_team_members(self, access_token: str) -> Iterable[str]: + cursor = None + + while True: + response = http_post( + url=github.GRAPHQL_URL, + json={ + 'query': ''' + query($organization:String!, $team:String!, $cursor:String, $first:Int!) { + organization(login:$organization) { + team(slug:$team) { + members(after:$cursor, first:$first, orderBy:{ field:LOGIN, direction:DESC }) { + edges { + cursor + } + nodes { + login + } + } + } + } + } + ''', + 'variables': { + 'organization': self._organization, + 'team': self._team, + 'cursor': cursor, + 'first': self._page_size, + }, + }, + headers={ + 'Authorization': f'Bearer {access_token}', + }, + ) + response.raise_for_status() + + members = response.json()['data']['organization']['team']['members'] + nodes = members['nodes'] + edges = members['edges'] + + for member in nodes: + yield member['login'] + + if len(nodes) < self._page_size: + break + + cursor = edges[-1]['cursor'] + + class AzureAuth(LogMixin): def __init__(self, storage: AzureTextStorage) -> None: self._storage = storage diff --git a/tests/opwen_email_server/services/test_auth.py b/tests/opwen_email_server/services/test_auth.py index 76d17839..9f0ec292 100644 --- a/tests/opwen_email_server/services/test_auth.py +++ b/tests/opwen_email_server/services/test_auth.py @@ -1,12 +1,68 @@ from shutil import rmtree from tempfile import mkdtemp from unittest import TestCase +from unittest.mock import Mock +from responses import mock as mock_responses + +from opwen_email_server.constants import github from opwen_email_server.services.auth import AzureAuth +from opwen_email_server.services.auth import AnyOfBasicAuth from opwen_email_server.services.auth import BasicAuth +from opwen_email_server.services.auth import GithubBasicAuth from opwen_email_server.services.storage import AzureTextStorage +class _MockResponses: + def __init__(self, responses): + self._i = 0 + self._responses = responses + + def __call__(self, *args, **kwargs): + try: + status, headers, body = self._responses[self._i] + except ValueError: + body = self._responses[self._i] + status = 200 + headers = {} + + self._i += 1 + + return status, headers, body + + +class AnyOfBasicAuthTests(TestCase): + def setUp(self): + self._auth1 = Mock() + self._auth2 = Mock() + self._auth3 = Mock() + self._auth = AnyOfBasicAuth(auths=[self._auth1, self._auth2, self._auth3]) + + def test_with_failing_sub_auth(self): + self._auth1.return_value = None + self._auth2.return_value = None + self._auth3.return_value = None + + user = self._auth('username', 'password') + + self.assertIsNone(user) + self.assertEqual(self._auth1.call_count, 1) + self.assertEqual(self._auth2.call_count, 1) + self.assertEqual(self._auth3.call_count, 1) + + def test_with_passing_sub_auth(self): + self._auth1.return_value = None + self._auth2.return_value = {'sub': 'username'} + self._auth3.return_value = None + + user = self._auth('username', 'password') + + self.assertIsNotNone(user) + self.assertEqual(self._auth1.call_count, 1) + self.assertEqual(self._auth2.call_count, 1) + self.assertEqual(self._auth3.call_count, 0) + + class BasicAuthTests(TestCase): def setUp(self): self._auth = BasicAuth({ @@ -38,6 +94,96 @@ def test_with_correct_password(self): self.assertIsNotNone(self._auth(username='user1', password='pass', required_scopes=['scope1', 'scopeA'])) +class GithubBasicAuthTests(TestCase): + def setUp(self): + self._auth = GithubBasicAuth(organization='organization', team='team', page_size=2) + + def test_with_bad_user(self): + self.assertIsNone(self._auth(username='', password='pass')) + + @mock_responses.activate + def test_with_missing_user(self): + mock_responses.add( + mock_responses.POST, + github.GRAPHQL_URL, + body='''{ + "data": { + "organization": { + "team": { + "members": { + "edges": [ + {"cursor": "cursor1"} + ], + "nodes": [ + {"login": "user1"} + ] + } + } + } + } + }''', + status=200, + ) + + self.assertIsNone(self._auth(username='does-not-exist', password='pass')) + + @mock_responses.activate + def test_with_bad_password(self): + mock_responses.add( + mock_responses.POST, + github.GRAPHQL_URL, + json={'message': 'Bad credentials'}, + status=401, + ) + + self.assertIsNone(self._auth(username='user1', password='incorrect')) + + @mock_responses.activate + def test_with_correct_password(self): + mock_responses.add_callback( + mock_responses.POST, + github.GRAPHQL_URL, + callback=_MockResponses([ + '''{ + "data": { + "organization": { + "team": { + "members": { + "edges": [ + {"cursor": "cursor1"}, + {"cursor": "cursor2"} + ], + "nodes": [ + {"login": "user1"}, + {"login": "user2"} + ] + } + } + } + } + }''', + '''{ + "data": { + "organization": { + "team": { + "members": { + "edges": [ + {"cursor": "cursor3"} + ], + "nodes": [ + {"login": "user3"} + ] + } + } + } + } + }''', + ]), + ) + + self.assertIsNotNone(self._auth(username='user3', password='pass')) + + class AzureAuthTests(TestCase): def setUp(self): self._folder = mkdtemp() diff --git a/tests/opwen_email_server/utils/test_email_parser.py b/tests/opwen_email_server/utils/test_email_parser.py index 95d9cf2e..06aaf3fc 100644 --- a/tests/opwen_email_server/utils/test_email_parser.py +++ b/tests/opwen_email_server/utils/test_email_parser.py @@ -6,7 +6,7 @@ from unittest import TestCase from unittest.mock import patch -from responses import mock +from responses import mock as mock_responses from opwen_email_server.utils import email_parser @@ -129,7 +129,7 @@ def test_change_image_size_when_already_small(self): class ConvertImgUrlToBase64Tests(TestCase): - @mock.activate + @mock_responses.activate def test_format_inline_images_with_img_tag(self): self.givenTestImage() input_email = {'body': '

test image

'} @@ -138,7 +138,7 @@ def test_format_inline_images_with_img_tag(self): self.assertStartsWith(output_email['body'], '

test image

'} @@ -181,7 +181,7 @@ def test_format_inline_images_with_bad_request(self): self.assertEqual(output_email, input_email) - @mock.activate + @mock_responses.activate def test_format_inline_images_with_many_img_tags(self): self.givenTestImage() input_email = {'body': '
'} @@ -197,7 +197,7 @@ def test_format_inline_images_without_img_tags(self): self.assertEqual(output_email, input_email) - @mock.activate + @mock_responses.activate def test_format_inline_images_without_content_type(self): self.givenTestImage(content_type='') input_email = {'body': '
'} @@ -219,11 +219,13 @@ def givenTestImage(cls, content_type='image/png', status=200): with open(join(TEST_DATA_DIRECTORY, 'test_image.png'), 'rb') as image: image_bytes = image.read() - mock.add(mock.GET, - 'http://test-url.png', - headers={'Content-Type': content_type}, - body=image_bytes, - status=status) + mock_responses.add( + mock_responses.GET, + 'http://test-url.png', + headers={'Content-Type': content_type}, + body=image_bytes, + status=status, + ) def fail_if_called(self, message, *args): self.fail(message % args)