plus-plus-service

Clone
Source Code
GIT

#2 Token authentication
Merged 7 years ago by abompard. Opened 7 years ago by atelic.
atelic/plus-plus-service add-tokens  into  master

Use APP.config for token configuration
Eric Barbour • 7 years ago  
396545b
Fix flake8 errors, add tests
Eric Barbour • 7 years ago  
6ffd727
Token authentication
Eric Barbour • 7 years ago  
a5e1e18
file modified
+1 
@@ -4,3 +4,4 @@ 
 

  *.db
 

  .tox
 

  *.pyc
 

+ settings.py

file modified
+14 -15 
@@ -2,6 +2,7 @@ 
 

  import flask
 

  

  from .karma import KarmaManager, NoSuchFASUser
 

+ from .auth import api_token_required
 

  

  

  APP = flask.Flask(__name__)
 
@@ -9,13 +10,6 @@ 
 

  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 db_connect():
 

      from .database import Database
 

      flask.g.db = Database(
 

-         APP.name, APP.config["SQLALCHEMY_DATABASE_URI"]
 

-         ).get_session()
 

+         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 @@ 
 

  

  

  @APP.route('/user/<username>', methods=['POST'])
 

+ @api_token_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)

file added
+43 
@@ -0,0 +1,43 @@ 
 

+ import functools
 

+ 

+ import flask
 

+ from . import APP
 

+ 

+ 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 == APP.config['PLUS_PLUS_TOKEN']:
 

+         return
 

+ 

+     output = {
 

+         'error': 'Invalid token supplied',
 

+     }
 

+     jsonout = flask.jsonify(output)
 

+     jsonout.status_code = 401
 

+     return jsonout
 

+ 

+ def api_token_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

file modified
+1 -1 
@@ -67,7 +67,7 @@ 
 

          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:

file modified
+1 
@@ -8,3 +8,4 @@ 
 

  FAS_URL = 'https://admin.fedoraproject.org/accounts/'
 

  FAS_USERNAME = 'changeme'
 

  FAS_PASSWORD = 'changeme'
 

+ PLUS_PLUS_TOKEN = 'changeme'

file modified
+2 -2 
@@ -61,7 +61,7 @@ 
 

              from_user=agent,
 

              to_user=recipient,
 

              value=vote,
 

-             ))
 

+         ))
 

          self.db.flush()
 

  

          base_query = self.db.query(
 
@@ -100,7 +100,7 @@ 
 

              decrements=dec,
 

              current=current,
 

              total=total,
 

-             )
 

+         )
 

  

  

  # vim:set shiftwidth=4 tabstop=4 expandtab textwidth=79:

file modified
+1 -1 
@@ -13,7 +13,7 @@ 
 

      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():

file modified
+4 -4 
@@ -6,10 +6,10 @@ 
 

  

  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"
 

  }

file modified
+2 -2 
@@ -53,7 +53,7 @@ 
 

              '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 @@ 
 

              '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")

file modified
+30 -5 
@@ -1,10 +1,10 @@ 
 

  import json
 

- import flask
 

  

  from mock import call, Mock, patch
 

  from .utils import TestCase
 

  from .. import APP
 

  

+ PLUS_PLUS_TOKEN = APP.config['PLUS_PLUS_TOKEN']
 

  

  class ViewTestCase(TestCase):
 

  
@@ -26,7 +26,7 @@ 
 

              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)
 
@@ -39,13 +39,38 @@ 
 

      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=dict(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"))
 

+ 

+     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)

file modified
+1 
@@ -4,3 +4,4 @@ 
 

  python-fedora
 

  requests
 

  fedmsg
 

+ tox

file modified
+6 -6 
@@ -27,19 +27,19 @@ 
 

      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',
 

-             ],
 

-         },
 

-     )
 

+         ],
 

+     },
 

+ )

  • Token authentication and accompanying tests
  • Flake8 fixes

2 new commits added

  • Fix flake8 errors, py 2 compatability, add tests
  • Token authentication
7 years ago

2 new commits added

  • Fix flake8 errors, py 2 compatability, add tests
  • Token authentication
7 years ago

Why not use Flask's configuration system? (APP.config)

Is this still necessary in Python 3? I don't think so.

Same remark, this should not be necessary in Python3

The super() form is preferred in Python3.

Same remark, the super() form is preferred in Python3.

Same comment here, using the Flask config system seems better (APP.config)

Same comment, the super() form is preferred in Python3.

Same comment, the super() form is preferred in Python3.

This is intended to be a Python3 app.

This is intended to be a Python3 app.

Versionned requirements make apps much much harder to package, I would recommend against it.

About Python2 vs Python3, I think there's a discussion to have here. I think it's much trickier to try to retain Python2 compatibility, we would also have to add imports from future at the top of every file (from __future__ import unicode_literals, absolute_import, with_statement, print_statement at least and maybe others).

From the Mailman experience I think it's much safer to target Python3 if we know we have Python3 on the target OS, and that is our case. With our use case, I think it's not very useful to try to support Python2 today, it will only adds support issues.

I spoke with Pierre about the Python2 vs Python3 concerns on #fedora-apps. He said to aim for py2 compatibility since that is the version that is default on RHEL and F24. You're welcome to bring these concerns up to him. However, the application does seem to run on python 2 and 3 locally without the future imports.

That said, I agree that it feels like building for the Windows XP of programming languages. I do think it would be preferable to build new applications in py3 rather than continuing to build things in a version that will be losing support soon.

Yeah, it probably runs without the future imports, but it can bring very convoluted bugs. The nastiest is unicode support: without the future import all the strings in the code are binary strings, but in Python3 (or with the future import) they are unicode strings, and must be converted to binary appropriately. For now Flask does the work for us, but since we'll also be sending fedmsgs, we may have to be extra careful about the status of our strings.

Same thing for absolute imports, you can have a running code in Python3 that will stop working in Python2, or vice-versa, because the name of a submodule is identical to one shipped in base Python (2 or 3). My advice from the Mailman experience is to choose a version and stick to it, unless you have to (for example if you're writing a library that must be compatible).

@pingou , what's your opinon?

Can you explain why? I'm not sure I understand this.

Builds should be stable over time. If a project builds now, it shouldn’t break in the future. Unversioned requirements do not ensure that one build will use the same version that the app was developed with and if a new version is released with breaking changes this will break the application.

Yeah, I know this is a cause of debate, usually between developers and packagers/sysadmins. Here are the use cases:

  • if there's a new version of requests for example, that fixes a security issue, the packager of requests will update the package, and it will break our application (to be precise, the updated package won't be installable unless plus-plus-service is removed)
  • if we want to run pps on a distro version that have different but working version of the libraries, the package won't install and the app won't run
  • it's a pain to maintain, but that's less important than the two previous points

I don't have a perfect solution for this problem, it is a very common issue in software engineering. I usually set minimal versions where I know it needs a recent version.

Our Fedora Packaging Guidelines recommend not setting a version when it is not necessary: http://fedoraproject.org/wiki/Packaging:Guidelines#Package_dependencies
Edited 7 years ago by abompard

3 new commits added

  • Use APP.config for token configuration
  • Fix flake8 errors, add tests
  • Token authentication
7 years ago

3 new commits added

  • Use APP.config for token configuration
  • Fix flake8 errors, add tests
  • Token authentication
7 years ago

Pull-Request has been merged by abompard
7 years ago
Metadata
None
No Tags
Changes Summary 12
+1 -0
file changed
.gitignore
+14 -15
file changed
plus_plus_service/__init__.py
+43
file added
plus_plus_service/auth.py
+1 -1
file changed
plus_plus_service/database.py
+1 -0
file changed
plus_plus_service/defaults.py
+2 -2
file changed
plus_plus_service/karma.py
+1 -1
file changed
plus_plus_service/lib.py
+4 -4
file changed
plus_plus_service/models.py
+2 -2
file changed
plus_plus_service/tests/test_karma.py
+30 -5
file changed
plus_plus_service/tests/test_views.py
+1 -0
file changed
requirements.txt
+6 -6
file changed
setup.py
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.