From a5e1e182570c936b95a7aa19b4ed8e5f395cea93 Mon Sep 17 00:00:00 2001 From: Eric Barbour Date: Jul 05 2016 21:15:30 +0000 Subject: [PATCH 1/3] Token authentication --- diff --git a/.gitignore b/.gitignore index 997adf9..ec3e9ab 100644 --- a/.gitignore +++ b/.gitignore @@ -4,3 +4,4 @@ dist *.db .tox *.pyc +settings.py diff --git a/README.rst b/README.rst index 1a1ffc5..43e0025 100644 --- a/README.rst +++ b/README.rst @@ -52,3 +52,7 @@ Alembic commands can be run by pointing to the included ``ini`` file:: alembic -c plus_plus_service/alembic.ini current The test suite can be run using the ``tox`` command. + +For token authorization, create a ``plus_plus_service/settings.py`` file and add:: + + PLUS_PLUS_TOKEN = 'SECRET_TOKEN' diff --git a/plus_plus_service/__init__.py b/plus_plus_service/__init__.py index ee0ab53..8f50f74 100644 --- a/plus_plus_service/__init__.py +++ b/plus_plus_service/__init__.py @@ -2,6 +2,7 @@ import os import flask from .karma import KarmaManager, NoSuchFASUser +from .auth import api_login_required APP = flask.Flask(__name__) @@ -9,13 +10,6 @@ APP.config.from_object('{}.defaults'.format(__name__)) if 'FLASK_SETTINGS' in os.environ: APP.config.from_envvar('FLASK_SETTINGS') - -def check_auth(): - # TODO something like: - # https://pagure.io/pagure/blob/master/f/pagure/api/__init__.py#_79 - pass - - # # Database # @@ -25,13 +19,13 @@ def check_auth(): def db_connect(): from .database import Database flask.g.db = Database( - APP.name, APP.config["SQLALCHEMY_DATABASE_URI"] + APP.name, APP.config['SQLALCHEMY_DATABASE_URI'] ).get_session() @APP.teardown_appcontext def db_disconnect(exception=None): - if hasattr(flask.g, "db"): + if hasattr(flask.g, 'db'): flask.g.db.remove() @@ -50,15 +44,20 @@ def view_user_get(username): @APP.route('/user/', methods=['POST']) +@api_login_required() def view_user_post(username): + sender = flask.request.form.get('sender', None) + if not sender: + return flask.abort(400) + if sender == username: + return "You may not modify your own karma.", 403 # Change the user's karma karma_manager = KarmaManager(flask.g.db) - if flask.g.fas_user == username: - return "You may not modify your own karma.", 403 - increment = ('decrement' in flask.request.form and - bool(flask.request.form['decrement'])) + increment = 'decrement' not in flask.request.form or \ + ('decrement' in flask.request.form and + not bool(flask.request.form['decrement'])) try: - karma_manager.change(flask.g.fas_user, username, increment) + karma_manager.change(sender, username, increment) except NoSuchFASUser as error: return str(error), 404 stats = karma_manager.stats(username) diff --git a/plus_plus_service/auth.py b/plus_plus_service/auth.py new file mode 100644 index 0000000..cf198c3 --- /dev/null +++ b/plus_plus_service/auth.py @@ -0,0 +1,44 @@ +import functools + +import flask +from .settings import PLUS_PLUS_TOKEN + +def check_auth(): + token_str = None + + if 'Authorization' in flask.request.headers: + authorization = flask.request.headers['Authorization'] + if 'token' in authorization: + token_str = authorization.split('token', 1)[1].strip() + + + if token_str == PLUS_PLUS_TOKEN: + return + + output = { + 'error': 'Invalid token supplied', + } + jsonout = flask.jsonify(output) + jsonout.status_code = 401 + return jsonout + +def api_login_required(): + ''' Decorator used to indicate that authentication is required for some + API endpoint. + ''' + + def decorator(fn): + ''' The decorator of the function ''' + + @functools.wraps(fn) + def decorated_function(*args, **kwargs): + ''' Actually does the job with the arguments provided. ''' + + response = check_auth() + if response: + return response + return fn(*args, **kwargs) + + return decorated_function + + return decorator diff --git a/plus_plus_service/karma.py b/plus_plus_service/karma.py index 7f6367c..f397c88 100644 --- a/plus_plus_service/karma.py +++ b/plus_plus_service/karma.py @@ -44,25 +44,13 @@ class KarmaManager: release = get_current_release() vote = 1 if increment else -1 - # Check our karma db to make sure this hasn't already been done. - try: - existing = self.db.query(Vote).filter_by( - release=release, from_user=agent, to_user=recipient).one() - except NoResultFound: - pass - else: - # Update the existing vote if necessary. - if existing.value != vote: - existing.value = vote - return - self.db.add(Vote( release=release, from_user=agent, to_user=recipient, value=vote, )) - self.db.flush() + self.db.commit() base_query = self.db.query( func.sum(Vote.value)).filter_by(to_user=recipient) diff --git a/plus_plus_service/lib.py b/plus_plus_service/lib.py index 63b1c6b..14a1610 100644 --- a/plus_plus_service/lib.py +++ b/plus_plus_service/lib.py @@ -13,7 +13,7 @@ def make_fas_client(): return AccountSystem( APP.config["FAS_URL"], username=APP.config["FAS_USERNAME"], - password=APP.config["FAS_USERNAME"]) + password=APP.config["FAS_PASSWORD"]) def get_current_release(): diff --git a/plus_plus_service/tests/test_karma.py b/plus_plus_service/tests/test_karma.py index b263ff4..a617ba6 100644 --- a/plus_plus_service/tests/test_karma.py +++ b/plus_plus_service/tests/test_karma.py @@ -7,7 +7,7 @@ from ..karma import KarmaManager class KarmaTestCase(TestCase): def setUp(self): - super().setUp() + super(KarmaTestCase, self).setUp() with patch('plus_plus_service.karma.load_fedmsg_config'): self.karma_manager = KarmaManager(self.db) self.karma_manager.fasclient = Mock() @@ -17,7 +17,7 @@ class KarmaTestCase(TestCase): self.fedmsg = self.fedmsg_patcher.start() def tearDown(self): - super().tearDown() + super(KarmaTestCase, self).tearDown() self.fedmsg_patcher.stop() def test_stats(self): diff --git a/plus_plus_service/tests/test_views.py b/plus_plus_service/tests/test_views.py index e207ccb..d5c02e0 100644 --- a/plus_plus_service/tests/test_views.py +++ b/plus_plus_service/tests/test_views.py @@ -4,19 +4,20 @@ import flask from mock import call, Mock, patch from .utils import TestCase from .. import APP +from ..settings import PLUS_PLUS_TOKEN class ViewTestCase(TestCase): def setUp(self): - super().setUp() + super(ViewTestCase, self).setUp() self.karma_manager = Mock() self.kmgr_patcher = patch("plus_plus_service.KarmaManager") kmgr_class = self.kmgr_patcher.start() kmgr_class.return_value = self.karma_manager def tearDown(self): - super().tearDown() + super(ViewTestCase, self).tearDown() self.kmgr_patcher.stop() def test_get(self): @@ -39,13 +40,15 @@ class ViewTestCase(TestCase): def test_post(self): self.karma_manager.change.return_value = 1 self.karma_manager.stats.return_value = {} + headers = {'Authorization': 'token {}'.format(PLUS_PLUS_TOKEN)} with APP.app_context(): - flask.g.fas_user = "source" - response = self.client.post('/user/target', data={}) + response = self.client.post('/user/target', + data={'sender': 'source'}, + headers=headers) self.assertEqual(response.status_code, 200) self.assertEqual(self.karma_manager.change.call_count, 1) self.assertEqual( self.karma_manager.change.call_args, - call("source", "target", False)) + call("source", "target", True)) result = json.loads(response.data.decode('ascii')) self.assertEqual(result, dict(username="target")) From 6ffd7271e1acd7e462e82cc9aac8782a4ba5fadf Mon Sep 17 00:00:00 2001 From: Eric Barbour Date: Jul 19 2016 13:41:56 +0000 Subject: [PATCH 2/3] Fix flake8 errors, add tests * Run flake8 and make changes * Add test for error catching --- diff --git a/plus_plus_service/__init__.py b/plus_plus_service/__init__.py index 8f50f74..9856d71 100644 --- a/plus_plus_service/__init__.py +++ b/plus_plus_service/__init__.py @@ -2,7 +2,7 @@ import os import flask from .karma import KarmaManager, NoSuchFASUser -from .auth import api_login_required +from .auth import api_token_required APP = flask.Flask(__name__) @@ -20,7 +20,7 @@ def db_connect(): from .database import Database flask.g.db = Database( APP.name, APP.config['SQLALCHEMY_DATABASE_URI'] - ).get_session() + ).get_session() @APP.teardown_appcontext @@ -44,7 +44,7 @@ def view_user_get(username): @APP.route('/user/', methods=['POST']) -@api_login_required() +@api_token_required() def view_user_post(username): sender = flask.request.form.get('sender', None) if not sender: diff --git a/plus_plus_service/auth.py b/plus_plus_service/auth.py index cf198c3..ba4b4bd 100644 --- a/plus_plus_service/auth.py +++ b/plus_plus_service/auth.py @@ -11,7 +11,6 @@ def check_auth(): if 'token' in authorization: token_str = authorization.split('token', 1)[1].strip() - if token_str == PLUS_PLUS_TOKEN: return @@ -22,7 +21,7 @@ def check_auth(): jsonout.status_code = 401 return jsonout -def api_login_required(): +def api_token_required(): ''' Decorator used to indicate that authentication is required for some API endpoint. ''' diff --git a/plus_plus_service/database.py b/plus_plus_service/database.py index 7ffac87..37ba25c 100644 --- a/plus_plus_service/database.py +++ b/plus_plus_service/database.py @@ -67,7 +67,7 @@ def exists_in_db(bind, tablename, columnname=None): return ( tablename in md.tables and columnname in [c.name for c in md.tables[tablename].columns] - ) + ) # vim:set shiftwidth=4 tabstop=4 expandtab textwidth=79: diff --git a/plus_plus_service/karma.py b/plus_plus_service/karma.py index f397c88..6525f1d 100644 --- a/plus_plus_service/karma.py +++ b/plus_plus_service/karma.py @@ -44,13 +44,25 @@ class KarmaManager: release = get_current_release() vote = 1 if increment else -1 + # Check our karma db to make sure this hasn't already been done. + try: + existing = self.db.query(Vote).filter_by( + release=release, from_user=agent, to_user=recipient).one() + except NoResultFound: + pass + else: + # Update the existing vote if necessary. + if existing.value != vote: + existing.value = vote + return + self.db.add(Vote( release=release, from_user=agent, to_user=recipient, value=vote, - )) - self.db.commit() + )) + self.db.flush() base_query = self.db.query( func.sum(Vote.value)).filter_by(to_user=recipient) @@ -88,7 +100,7 @@ class KarmaManager: decrements=dec, current=current, total=total, - ) + ) # vim:set shiftwidth=4 tabstop=4 expandtab textwidth=79: diff --git a/plus_plus_service/models.py b/plus_plus_service/models.py index 1407b07..59be80a 100644 --- a/plus_plus_service/models.py +++ b/plus_plus_service/models.py @@ -6,10 +6,10 @@ from sqlalchemy.ext.declarative import declarative_base Base = declarative_base() Base.metadata.naming_convention = { - "ix": 'ix_%(column_0_label)s', - "uq": "uq_%(table_name)s_%(column_0_name)s", - "fk": "fk_%(table_name)s_%(column_0_name)s_%(referred_table_name)s", - "pk": "pk_%(table_name)s" + "ix": 'ix_%(column_0_label)s', + "uq": "uq_%(table_name)s_%(column_0_name)s", + "fk": "fk_%(table_name)s_%(column_0_name)s_%(referred_table_name)s", + "pk": "pk_%(table_name)s" } diff --git a/plus_plus_service/tests/test_karma.py b/plus_plus_service/tests/test_karma.py index a617ba6..4234206 100644 --- a/plus_plus_service/tests/test_karma.py +++ b/plus_plus_service/tests/test_karma.py @@ -7,7 +7,7 @@ from ..karma import KarmaManager class KarmaTestCase(TestCase): def setUp(self): - super(KarmaTestCase, self).setUp() + super().setUp() with patch('plus_plus_service.karma.load_fedmsg_config'): self.karma_manager = KarmaManager(self.db) self.karma_manager.fasclient = Mock() @@ -17,7 +17,7 @@ class KarmaTestCase(TestCase): self.fedmsg = self.fedmsg_patcher.start() def tearDown(self): - super(KarmaTestCase, self).tearDown() + super().tearDown() self.fedmsg_patcher.stop() def test_stats(self): @@ -53,7 +53,7 @@ class KarmaTestCase(TestCase): 'increments': 2, 'decrements': 1, 'total': 2, - }) + }) def test_change(self): with patch('plus_plus_service.karma.get_current_release') as gcr: @@ -68,7 +68,7 @@ class KarmaTestCase(TestCase): 'total_this_release': 1, 'vote': 1, 'release': "release-1", - }) + }) self.assertEqual(self.db.query(Vote).count(), 1) vote = self.db.query(Vote).one() self.assertEqual(vote.release, "release-1") diff --git a/plus_plus_service/tests/test_views.py b/plus_plus_service/tests/test_views.py index d5c02e0..d23f789 100644 --- a/plus_plus_service/tests/test_views.py +++ b/plus_plus_service/tests/test_views.py @@ -1,5 +1,4 @@ import json -import flask from mock import call, Mock, patch from .utils import TestCase @@ -10,14 +9,14 @@ from ..settings import PLUS_PLUS_TOKEN class ViewTestCase(TestCase): def setUp(self): - super(ViewTestCase, self).setUp() + super().setUp() self.karma_manager = Mock() self.kmgr_patcher = patch("plus_plus_service.KarmaManager") kmgr_class = self.kmgr_patcher.start() kmgr_class.return_value = self.karma_manager def tearDown(self): - super(ViewTestCase, self).tearDown() + super().tearDown() self.kmgr_patcher.stop() def test_get(self): @@ -27,7 +26,7 @@ class ViewTestCase(TestCase): increments=3, decrements=4, total=5, - ) + ) self.karma_manager.stats.return_value = dummy_values response = self.client.get('/user/target') self.assertEqual(response.status_code, 200) @@ -43,7 +42,7 @@ class ViewTestCase(TestCase): headers = {'Authorization': 'token {}'.format(PLUS_PLUS_TOKEN)} with APP.app_context(): response = self.client.post('/user/target', - data={'sender': 'source'}, + data=dict(sender='source'), headers=headers) self.assertEqual(response.status_code, 200) self.assertEqual(self.karma_manager.change.call_count, 1) @@ -52,3 +51,26 @@ class ViewTestCase(TestCase): call("source", "target", True)) result = json.loads(response.data.decode('ascii')) self.assertEqual(result, dict(username="target")) + + def test_return_403_when_sender_and_username_are_same(self): + headers = {'Authorization': 'token {}'.format(PLUS_PLUS_TOKEN)} + with APP.app_context(): + response = self.client.post('/user/target', + data=dict(sender='target'), + headers=headers) + self.assertEqual(response.status_code, 403) + + def test_return_401_when_not_auth_header(self): + self.karma_manager.change.return_value = 1 + self.karma_manager.stats.return_value = {} + with APP.app_context(): + response = self.client.post('/user/target', + data=dict(sender='source')) + self.assertEqual(response.status_code, 401) + + def test_return_400_when_no_sender(self): + headers = {'Authorization': 'token {}'.format(PLUS_PLUS_TOKEN)} + with APP.app_context(): + response = self.client.post('/user/notarealfasuser', + headers=headers) + self.assertEqual(response.status_code, 400) diff --git a/requirements.txt b/requirements.txt index 8420daa..7c7268c 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,6 +1,7 @@ -Flask -SQLAlchemy -alembic -python-fedora -requests -fedmsg +alembic==0.8.6 +fedmsg==0.17.2 +Flask==0.11.1 +python-fedora==0.8.0 +requests==2.10.0 +SQLAlchemy==1.0.13 +tox==2.3.1 diff --git a/setup.py b/setup.py index c0403ca..550b9c7 100755 --- a/setup.py +++ b/setup.py @@ -27,19 +27,19 @@ setup( long_description=open('README.rst').read(), author='Aurelien Bompard', author_email='abompard@fedoraproject.org', - #url="https://pagure.io/plus-plus-service", + # url="https://pagure.io/plus-plus-service", license="AGPLv3+", classifiers=[ "Development Status :: 3 - Alpha", "License :: OSI Approved :: GNU Affero General Public License v3 or later (AGPLv3+)", "Programming Language :: Python :: 3", - ], + ], packages=find_packages(), - #include_package_data=True, + # include_package_data=True, install_requires=reqfile("requirements.txt"), entry_points={ 'console_scripts': [ 'plus-plus-service = plus_plus_service.scripts:main', - ], - }, - ) + ], + }, +) From 396545bdf894240fbfb5a9085700bae36420a2a9 Mon Sep 17 00:00:00 2001 From: Eric Barbour Date: Jul 19 2016 13:56:17 +0000 Subject: [PATCH 3/3] Use APP.config for token configuration --- diff --git a/README.rst b/README.rst index 43e0025..1a1ffc5 100644 --- a/README.rst +++ b/README.rst @@ -52,7 +52,3 @@ Alembic commands can be run by pointing to the included ``ini`` file:: alembic -c plus_plus_service/alembic.ini current The test suite can be run using the ``tox`` command. - -For token authorization, create a ``plus_plus_service/settings.py`` file and add:: - - PLUS_PLUS_TOKEN = 'SECRET_TOKEN' diff --git a/plus_plus_service/auth.py b/plus_plus_service/auth.py index ba4b4bd..a7cd3d5 100644 --- a/plus_plus_service/auth.py +++ b/plus_plus_service/auth.py @@ -1,7 +1,7 @@ import functools import flask -from .settings import PLUS_PLUS_TOKEN +from . import APP def check_auth(): token_str = None @@ -11,7 +11,7 @@ def check_auth(): if 'token' in authorization: token_str = authorization.split('token', 1)[1].strip() - if token_str == PLUS_PLUS_TOKEN: + if token_str == APP.config['PLUS_PLUS_TOKEN']: return output = { diff --git a/plus_plus_service/defaults.py b/plus_plus_service/defaults.py index 400bacd..762d919 100644 --- a/plus_plus_service/defaults.py +++ b/plus_plus_service/defaults.py @@ -8,3 +8,4 @@ SQLALCHEMY_DATABASE_URI = 'sqlite:///' + os.path.join(basedir, '..', 'pps.db') FAS_URL = 'https://admin.fedoraproject.org/accounts/' FAS_USERNAME = 'changeme' FAS_PASSWORD = 'changeme' +PLUS_PLUS_TOKEN = 'changeme' diff --git a/plus_plus_service/tests/test_views.py b/plus_plus_service/tests/test_views.py index d23f789..7caade5 100644 --- a/plus_plus_service/tests/test_views.py +++ b/plus_plus_service/tests/test_views.py @@ -3,8 +3,8 @@ import json from mock import call, Mock, patch from .utils import TestCase from .. import APP -from ..settings import PLUS_PLUS_TOKEN +PLUS_PLUS_TOKEN = APP.config['PLUS_PLUS_TOKEN'] class ViewTestCase(TestCase): diff --git a/requirements.txt b/requirements.txt index 7c7268c..ca2c52d 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,7 +1,7 @@ -alembic==0.8.6 -fedmsg==0.17.2 -Flask==0.11.1 -python-fedora==0.8.0 -requests==2.10.0 -SQLAlchemy==1.0.13 -tox==2.3.1 +Flask +SQLAlchemy +alembic +python-fedora +requests +fedmsg +tox