From 09a30db0a3f4e6b20a21ca5c192f72745b127490 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:21 +0000 Subject: [PATCH 1/145] Expand the database schema to support API token with ACLs --- diff --git a/pagure/lib/model.py b/pagure/lib/model.py index 0ad6d51..5766651 100644 --- a/pagure/lib/model.py +++ b/pagure/lib/model.py @@ -893,6 +893,84 @@ class ProjectGroup(BASE): 'project_id', 'group_id'), ) +# +# Class and tables specific for the API/token access +# + +class ACL(BASE): + """ + Table listing all the rights a token can be given + """ + + __tablename__ = 'acls' + + id = sa.Column(sa.Integer, primary_key=True) + name = sa.Column(sa.String(32), unique=True, nullable=False) + description = sa.Column(sa.Text(), nullable=False) + created = sa.Column( + sa.DateTime, nullable=False, default=datetime.datetime.utcnow) + + def __repr__(self): + ''' Return a string representation of this object. ''' + + return 'ACL: %s - name %s' % (self.id, self.name) + + +class Token(BASE): + """ + Table listing all the tokens per user and per project + """ + + __tablename__ = 'tokens' + + id = sa.Column(sa.String(64), primary_key=True) + user_id = sa.Column( + sa.Integer, + sa.ForeignKey('users.id', onupdate='CASCADE'), + nullable=False, + index=True) + project_id = sa.Column( + sa.Integer, + sa.ForeignKey('projects.id', onupdate='CASCADE'), + nullable=False, + index=True) + expiration = sa.Column( + sa.DateTime, nullable=False, default=datetime.datetime.utcnow) + created = sa.Column( + sa.DateTime, nullable=False, default=datetime.datetime.utcnow) + + acls = relation( + "ACL", + secondary="tokens_acls", + primaryjoin="tokens.c.id==tokens_acls.c.token_id", + secondaryjoin="acls.c.id==tokens_acls.c.acl_id", + ) + + def __repr__(self): + ''' Return a string representation of this object. ''' + + return 'ACL: %s - name %s' % (self.id, self.name) + + +class TokenAcl(BASE): + """ + Association table linking the tokens table to the acls table. + This allow linking token to acl. + """ + + __tablename__ = 'tokens_acls' + + token_id = sa.Column( + sa.Integer, sa.ForeignKey('tokens.id'), primary_key=True) + acl_id = sa.Column( + sa.Integer, sa.ForeignKey('acls.id'), primary_key=True) + + # Constraints + __table_args__ = ( + sa.UniqueConstraint( + 'token_id', 'acl_id'), + ) + # ########################################################## # These classes are only used if you're using the `local` From c6d2331939d95a5517d0ab158e26f96a4aa39051 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:21 +0000 Subject: [PATCH 2/145] Make a relation between a token and its user --- diff --git a/pagure/lib/model.py b/pagure/lib/model.py index 5766651..ae8d459 100644 --- a/pagure/lib/model.py +++ b/pagure/lib/model.py @@ -946,6 +946,14 @@ class Token(BASE): secondaryjoin="acls.c.id==tokens_acls.c.acl_id", ) + user = relation( + 'User', + backref=backref( + 'tokens', cascade="delete, delete-orphan", + ), + foreign_keys=[user_id], + remote_side=[User.id]) + def __repr__(self): ''' Return a string representation of this object. ''' From e82230ea734ad95895d6e806eb411c3a768b8cb2 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:22 +0000 Subject: [PATCH 3/145] Add a property to the token to determine if they are expired or not --- diff --git a/pagure/lib/model.py b/pagure/lib/model.py index ae8d459..fb54010 100644 --- a/pagure/lib/model.py +++ b/pagure/lib/model.py @@ -959,6 +959,14 @@ class Token(BASE): return 'ACL: %s - name %s' % (self.id, self.name) + @property + def expired(self): + ''' Returns wether a token has expired or not. ''' + if datetime.datetime.utcnow().date >= self.expiration.date(): + return True + else: + return False + class TokenAcl(BASE): """ From 6e345c42106a41f7670e19274e216a8edebc616c Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:22 +0000 Subject: [PATCH 4/145] Add method in the internal library to retrieve a token object from its id --- diff --git a/pagure/lib/__init__.py b/pagure/lib/__init__.py index b63a71f..848fc7e 100644 --- a/pagure/lib/__init__.py +++ b/pagure/lib/__init__.py @@ -2024,3 +2024,16 @@ def is_group_member(session, user, groupname): return False return groupname in user.groups + + +def get_api_token(session, token_str): + """ Return the Token object corresponding to the provided token string + if there is any, returns None otherwise. + """ + query = session.query( + model.Token + ).filter( + model.Token.id == token_str + ) + + return query.first() From a4d4731e0d469495d108ba0d9ba178d20413d51c Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:22 +0000 Subject: [PATCH 5/145] Add a decorator for the API methods requiring authentification --- diff --git a/pagure/api/__init__.py b/pagure/api/__init__.py index 2fb250c..912473a 100644 --- a/pagure/api/__init__.py +++ b/pagure/api/__init__.py @@ -10,6 +10,8 @@ API namespace version 0. """ +import functools + import flask API = flask.Blueprint('api_ns', __name__, url_prefix='/api/0') @@ -20,6 +22,41 @@ import pagure import pagure.lib +def api_login_required(f, acls=None): + @functools.wraps(f) + def decorated_function(*args, **kwargs): + print args, kwargs + token = None + token_str = None + apt_login = None + if 'Authorization' in flask.request.headers: + authorization = flask.request.headers['Authorization'] + if 'token' in authorization: + token_str = authorization.split('token')[1] + + token_auth = False + if token_str: + token = pagure.lib.get_api_token(SESSION, token_str) + if token and not token.expired: + token_auth = True + flask.g.user = token.user + print token.acls + print 'Add check for token ACLs' + + if not token_auth: + output = { + "output": "notok", + "error": "Login invalid/expired. " + "Please visit %s get or renew your API token." % ( + APP.config['APP_URL']), + } + jsonout = flask.jsonify(output) + jsonout.status_code = 401 + return jsonout + return f(*args, **kwargs) + return decorated_function + + @API.route('/version/') @API.route('/version') def api_version(): From 1afc85b193340123e112f7a464ddf3603d1fb1a4 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:22 +0000 Subject: [PATCH 6/145] Code style fix --- diff --git a/pagure/api/__init__.py b/pagure/api/__init__.py index 912473a..817759b 100644 --- a/pagure/api/__init__.py +++ b/pagure/api/__init__.py @@ -171,7 +171,6 @@ def api_project_tags(repo, username=None): ) - @API.route('/groups/') @API.route('/groups') def api_groups(): From ca76358ec5b21b94a7e0a5a559ff23a13c92e846 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:22 +0000 Subject: [PATCH 7/145] Add API controller to interact with issues and add the first endpoint to create one --- diff --git a/pagure/api/issue.py b/pagure/api/issue.py new file mode 100644 index 0000000..70bcb96 --- /dev/null +++ b/pagure/api/issue.py @@ -0,0 +1,101 @@ +# -*- coding: utf-8 -*- + +""" + (c) 2015 - Copyright Red Hat Inc + + Authors: + Pierre-Yves Chibon + +""" + +import flask + +import pagure +import pagure.lib +from pagure import APP, SESSION +from pagure.api import API, api_login_required, API_ERROR_CODE + + +@API.route('//new_issue', methods=['POST']) +@API.route('/fork///new_issue', methods=['POST']) +@api_login_required +def new_issue(repo, username=None): + """ Create a new issue + """ + repo = pagure.lib.get_project(SESSION, repo, user=username) + httpcode = 200 + output = {} + + if repo is None: + output['error_code'] = 1 + output['error'] = API_ERROR_CODE[1] + jsonout = flask.jsonify(output) + jsonout.status_code = 404 + return jsonout + + if not repo.settings.get('issue_tracker', True): + output['error_code'] = 2 + output['error'] = API_ERROR_CODE[2] + jsonout = flask.jsonify(output) + jsonout.status_code = 404 + return jsonout + + status = pagure.lib.get_issue_statuses(SESSION) + form = pagure.forms.IssueForm(status=status, csrf_token=False) + if form.validate_on_submit(): + title = form.title.data + content = form.issue_content.data + private = form.private.data + + try: + issue = pagure.lib.new_issue( + SESSION, + repo=repo, + title=title, + content=content, + private=private or False, + user=flask.g.fas_user.username, + ticketfolder=APP.config['TICKETS_FOLDER'], + ) + SESSION.commit() + # If there is a file attached, attach it. + filestream = flask.request.files.get('filestream') + if filestream and '' in issue.content: + new_filename = pagure.lib.git.add_file_to_git( + repo=repo, + issue=issue, + ticketfolder=APP.config['TICKETS_FOLDER'], + user=flask.g.fas_user, + filename=filestream.filename, + filestream=filestream.stream, + ) + # Replace the tag in the comment with the link + # to the actual image + filelocation = flask.url_for( + 'view_issue_raw_file', + repo=repo.name, + username=username, + filename=new_filename, + ) + new_filename = new_filename.split('-', 1)[1] + url = '[![%s](%s)](%s)' % ( + new_filename, filelocation, filelocation) + issue.content = issue.content.replace('', url) + SESSION.add(issue) + SESSION.commit() + + output['message'] = 'issue created' + output['message'] = 'issue created' + except pagure.exceptions.PagureException, err: + output['error_code'] = 0 + output['error'] = str(err) + httpcode = 400 + except SQLAlchemyError, err: # pragma: no cover + SESSION.rollback() + output['error_code'] = 3 + output['error'] = API_ERROR_CODE[3] + httpcode = 400 + + jsonout = flask.jsonify(output) + jsonout.status_code = httpcode + return jsonout From 97768420029c0b9afce2026cf678f304f29e5c70 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:22 +0000 Subject: [PATCH 8/145] Create a dictionary of all the errors the API can return --- diff --git a/pagure/api/__init__.py b/pagure/api/__init__.py index 817759b..8b31498 100644 --- a/pagure/api/__init__.py +++ b/pagure/api/__init__.py @@ -22,6 +22,15 @@ import pagure import pagure.lib +API_ERROR_CODE = { + 0: 'Variable message describing the issue', + 1: 'Project not found', + 2: 'No issue tracker found for this project', + 3: 'An error occured at the database level and prevent the action from ' + 'reaching completion', +} + + def api_login_required(f, acls=None): @functools.wraps(f) def decorated_function(*args, **kwargs): From e8915e77865f1de388d3aaf13240241f0f1cbe59 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:22 +0000 Subject: [PATCH 9/145] Include the api.issue controller in the flask application --- diff --git a/pagure/__init__.py b/pagure/__init__.py index 5f623ef..1bf02f7 100644 --- a/pagure/__init__.py +++ b/pagure/__init__.py @@ -389,8 +389,10 @@ import pagure.ui.issues import pagure.ui.plugins import pagure.ui.repo -import pagure.api -APP.register_blueprint(pagure.api.API) +from pagure.api import API +from pagure.api import issue +APP.register_blueprint(API) + import pagure.internal APP.register_blueprint(pagure.internal.PV) diff --git a/pagure/api/__init__.py b/pagure/api/__init__.py index 8b31498..78875af 100644 --- a/pagure/api/__init__.py +++ b/pagure/api/__init__.py @@ -66,6 +66,9 @@ def api_login_required(f, acls=None): return decorated_function +from pagure.api import issue + + @API.route('/version/') @API.route('/version') def api_version(): From f329bb317b1fe1b4b3f4f75f0055e6b6cbd2c349 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:22 +0000 Subject: [PATCH 10/145] Start working on the unit-tests for the API, checking the authentication part --- diff --git a/tests/test_progit_flask_api_auth.py b/tests/test_progit_flask_api_auth.py new file mode 100644 index 0000000..28508c9 --- /dev/null +++ b/tests/test_progit_flask_api_auth.py @@ -0,0 +1,75 @@ +# -*- coding: utf-8 -*- + +""" + (c) 2015 - Copyright Red Hat Inc + + Authors: + Pierre-Yves Chibon + +""" + +__requires__ = ['SQLAlchemy >= 0.8'] +import pkg_resources + +import unittest +import shutil +import sys +import os + +import json +from mock import patch + +sys.path.insert(0, os.path.join(os.path.dirname( + os.path.abspath(__file__)), '..')) + +import pagure.lib +import tests + + +class PagureFlaskApiAuthtests(tests.Modeltests): + """ Tests for the authentication in the flask API of pagure """ + + def setUp(self): + """ Set up the environnment, ran before every tests. """ + super(PagureFlaskApiAuthtests, self).setUp() + + pagure.APP.config['TESTING'] = True + pagure.SESSION = self.session + pagure.api.SESSION = self.session + self.app = pagure.APP.test_client() + + def test_auth_no_data(self): + """ Test the authentication when there is nothing in the database. + """ + + output = self.app.post('/api/0/foo/new_issue') + self.assertEqual(output.status_code, 401) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "Login invalid/expired. Please visit " \ + "https://pagure.org/ get or renew your API token.", + "output": "notok" + } + ) + + headers = {'Authorization': 'token aabbbccc'} + + output = self.app.post('/api/0/foo/new_issue', headers=headers) + self.assertEqual(output.status_code, 401) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "Login invalid/expired. Please visit " \ + "https://pagure.org/ get or renew your API token.", + "output": "notok" + } + ) + + +if __name__ == '__main__': + SUITE = unittest.TestLoader().loadTestsFromTestCase( + PagureFlaskApiAuthtests) + unittest.TextTestRunner(verbosity=2).run(SUITE) From 3e66925c019d68d53a55f26c39f31ec3818f98a1 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:22 +0000 Subject: [PATCH 11/145] Store the token in the g request for use later in the method --- diff --git a/pagure/api/__init__.py b/pagure/api/__init__.py index 78875af..d18e50b 100644 --- a/pagure/api/__init__.py +++ b/pagure/api/__init__.py @@ -32,6 +32,12 @@ API_ERROR_CODE = { def api_login_required(f, acls=None): + ''' Decorator used to indicate that authentication is required for some + API endpoint. + ''' + flask.g.user = None + flask.g.token = None + @functools.wraps(f) def decorated_function(*args, **kwargs): print args, kwargs @@ -49,6 +55,7 @@ def api_login_required(f, acls=None): if token and not token.expired: token_auth = True flask.g.user = token.user + flask.g.token = token print token.acls print 'Add check for token ACLs' From 97646371fe56bfa031d34ead64326ed20c0ce6ed Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:22 +0000 Subject: [PATCH 12/145] Add support for ACL check for a provided API token --- diff --git a/pagure/api/__init__.py b/pagure/api/__init__.py index d18e50b..0e8910a 100644 --- a/pagure/api/__init__.py +++ b/pagure/api/__init__.py @@ -31,46 +31,55 @@ API_ERROR_CODE = { } -def api_login_required(f, acls=None): +def api_login_required(acls=None): ''' Decorator used to indicate that authentication is required for some API endpoint. ''' - flask.g.user = None - flask.g.token = None - - @functools.wraps(f) - def decorated_function(*args, **kwargs): - print args, kwargs - token = None - token_str = None - apt_login = None - if 'Authorization' in flask.request.headers: - authorization = flask.request.headers['Authorization'] - if 'token' in authorization: - token_str = authorization.split('token')[1] - - token_auth = False - if token_str: - token = pagure.lib.get_api_token(SESSION, token_str) - if token and not token.expired: - token_auth = True - flask.g.user = token.user - flask.g.token = token - print token.acls - print 'Add check for token ACLs' - - if not token_auth: - output = { - "output": "notok", - "error": "Login invalid/expired. " - "Please visit %s get or renew your API token." % ( - APP.config['APP_URL']), - } - jsonout = flask.jsonify(output) - jsonout.status_code = 401 - return jsonout - return f(*args, **kwargs) - return decorated_function + + def decorator(fn): + ''' The decorator of the function ''' + + def decorated_function(*args, **kwargs): + ''' Actually does the job with the arguments provided. ''' + + flask.g.token = None + flask.g.user = None + #print args, kwargs + token = None + token_str = None + apt_login = None + if 'Authorization' in flask.request.headers: + authorization = flask.request.headers['Authorization'] + if 'token' in authorization: + token_str = authorization.split('token')[1].strip() + + token_auth = False + if token_str: + token = pagure.lib.get_api_token(SESSION, token_str) + #print token + if token and not token.expired: + if acls and set(token.acls_list).intersection(set(acls)): + token_auth = True + flask.g.user = token.user + flask.g.token = token + print 'Add check for token ACLs' + + if not token_auth: + output = { + "output": "notok", + "error": "Invalid or expired token. " + "Please visit %s get or renew your API token." % ( + APP.config['APP_URL']), + } + jsonout = flask.jsonify(output) + jsonout.status_code = 401 + return jsonout + + return fn(*args, **kwargs) + + return functools.wraps(fn)(decorated_function) + + return decorator from pagure.api import issue From 7ca13c4b852eed0dd34099fd258b2df2550c6e13 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:22 +0000 Subject: [PATCH 13/145] Add a new error when an invalid/incomplete request is made --- diff --git a/pagure/api/__init__.py b/pagure/api/__init__.py index 0e8910a..353c8c8 100644 --- a/pagure/api/__init__.py +++ b/pagure/api/__init__.py @@ -28,6 +28,7 @@ API_ERROR_CODE = { 2: 'No issue tracker found for this project', 3: 'An error occured at the database level and prevent the action from ' 'reaching completion', + 4: 'Invalid or incomplete input submited', } From e8e9bf435f5c934911d22d6d79e34c67af7bbb05 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:22 +0000 Subject: [PATCH 14/145] Return an error if the form is invalid --- diff --git a/pagure/api/issue.py b/pagure/api/issue.py index 70bcb96..02befdc 100644 --- a/pagure/api/issue.py +++ b/pagure/api/issue.py @@ -95,6 +95,10 @@ def new_issue(repo, username=None): output['error_code'] = 3 output['error'] = API_ERROR_CODE[3] httpcode = 400 + else: + output['error_code'] = 4 + output['error'] = API_ERROR_CODE[4] + httpcode = 400 jsonout = flask.jsonify(output) jsonout.status_code = httpcode From b5c93d2293a6acac40061e903a8e5225def07de0 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:22 +0000 Subject: [PATCH 15/145] Specify which ACL is required to create an issue with the API --- diff --git a/pagure/api/issue.py b/pagure/api/issue.py index 02befdc..830bd6a 100644 --- a/pagure/api/issue.py +++ b/pagure/api/issue.py @@ -18,7 +18,7 @@ from pagure.api import API, api_login_required, API_ERROR_CODE @API.route('//new_issue', methods=['POST']) @API.route('/fork///new_issue', methods=['POST']) -@api_login_required +@api_login_required(acls=['create_issue']) def new_issue(repo, username=None): """ Create a new issue """ From 3f19aaf72ceb8dde582c831c06bea0bbf5613506 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:22 +0000 Subject: [PATCH 16/145] Fix the string representation of the Token objects --- diff --git a/pagure/lib/model.py b/pagure/lib/model.py index fb54010..1290b2a 100644 --- a/pagure/lib/model.py +++ b/pagure/lib/model.py @@ -957,7 +957,7 @@ class Token(BASE): def __repr__(self): ''' Return a string representation of this object. ''' - return 'ACL: %s - name %s' % (self.id, self.name) + return 'Token: %s - name %s' % (self.id, self.expiration) @property def expired(self): From 7db4304af36d1512fa50deda58972d1456aa0350 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:22 +0000 Subject: [PATCH 17/145] Compare two objects instead of a function and an object --- diff --git a/pagure/lib/model.py b/pagure/lib/model.py index 1290b2a..b9b65a5 100644 --- a/pagure/lib/model.py +++ b/pagure/lib/model.py @@ -962,7 +962,7 @@ class Token(BASE): @property def expired(self): ''' Returns wether a token has expired or not. ''' - if datetime.datetime.utcnow().date >= self.expiration.date(): + if datetime.datetime.utcnow().date() >= self.expiration.date(): return True else: return False From f32aa5d496566fd7ddbb6029c2ff41c79a5a78af Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:22 +0000 Subject: [PATCH 18/145] Add a property for the Token to return a list of the ACL it has --- diff --git a/pagure/lib/model.py b/pagure/lib/model.py index b9b65a5..c481056 100644 --- a/pagure/lib/model.py +++ b/pagure/lib/model.py @@ -967,6 +967,12 @@ class Token(BASE): else: return False + @property + def acls_list(self): + ''' Return a list containing the name of each ACLs this token has. + ''' + return [acl.name for acl in self.acls] + class TokenAcl(BASE): """ From 5ec12bf0c13de01e559e0ed7aa206386e1230e98 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:22 +0000 Subject: [PATCH 19/145] Make the logic in api_login_required easier to read Thanks @ralph @ralphbean --- diff --git a/pagure/api/__init__.py b/pagure/api/__init__.py index 353c8c8..ee012b0 100644 --- a/pagure/api/__init__.py +++ b/pagure/api/__init__.py @@ -32,6 +32,41 @@ API_ERROR_CODE = { } + +def check_api_acls(acls): + flask.g.token = None + flask.g.user = None + token = None + token_str = None + apt_login = None + if 'Authorization' in flask.request.headers: + authorization = flask.request.headers['Authorization'] + if 'token' in authorization: + token_str = authorization.split('token')[1].strip() + + token_auth = False + if token_str: + token = pagure.lib.get_api_token(SESSION, token_str) + #print token + if token and not token.expired: + if acls and set(token.acls_list).intersection(set(acls)): + token_auth = True + flask.g.user = token.user + flask.g.token = token + print 'Add check for token ACLs' + + if not token_auth: + output = { + "output": "notok", + "error": "Invalid or expired token. " + "Please visit %s get or renew your API token." % ( + APP.config['APP_URL']), + } + jsonout = flask.jsonify(output) + jsonout.status_code = 401 + return jsonout + + def api_login_required(acls=None): ''' Decorator used to indicate that authentication is required for some API endpoint. @@ -40,45 +75,16 @@ def api_login_required(acls=None): def decorator(fn): ''' The decorator of the function ''' + @functools.wraps(fn) def decorated_function(*args, **kwargs): ''' Actually does the job with the arguments provided. ''' - flask.g.token = None - flask.g.user = None - #print args, kwargs - token = None - token_str = None - apt_login = None - if 'Authorization' in flask.request.headers: - authorization = flask.request.headers['Authorization'] - if 'token' in authorization: - token_str = authorization.split('token')[1].strip() - - token_auth = False - if token_str: - token = pagure.lib.get_api_token(SESSION, token_str) - #print token - if token and not token.expired: - if acls and set(token.acls_list).intersection(set(acls)): - token_auth = True - flask.g.user = token.user - flask.g.token = token - print 'Add check for token ACLs' - - if not token_auth: - output = { - "output": "notok", - "error": "Invalid or expired token. " - "Please visit %s get or renew your API token." % ( - APP.config['APP_URL']), - } - jsonout = flask.jsonify(output) - jsonout.status_code = 401 - return jsonout - + response = check_api_acls(acls) + if response: + return response return fn(*args, **kwargs) - return functools.wraps(fn)(decorated_function) + return decorated_function return decorator From 4b845ec8de48e65e6d98547ce65c4a823f2a01fe Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:22 +0000 Subject: [PATCH 20/145] Add a docstring to check_api_acls --- diff --git a/pagure/api/__init__.py b/pagure/api/__init__.py index ee012b0..413b80c 100644 --- a/pagure/api/__init__.py +++ b/pagure/api/__init__.py @@ -34,6 +34,10 @@ API_ERROR_CODE = { def check_api_acls(acls): + ''' Checks if the user provided an API token with its request and if + this token allows the user to access the endpoint desired. + ''' + flask.g.token = None flask.g.user = None token = None From c0b6350566f7ce29dca49cd903a418d0cc628b92 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:22 +0000 Subject: [PATCH 21/145] Add method to insert a few tokens in the database for the tests --- diff --git a/tests/__init__.py b/tests/__init__.py index 4342e5f..6e3df89 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -227,6 +227,35 @@ def create_projects_git(folder, bare=False): return repos +def create_tokens(session, user_id=1): + """ Create some tokens for the project in the database. """ + item = pagure.lib.model.Token( + id='aaabbbcccddd', + user_id=user_id, + project_id=1, + expiration=datetime.utcnow() + timedelta(days=30) + ) + session.add(item) + + item = pagure.lib.model.Token( + id='foo_token', + user_id=user_id, + project_id=1, + expiration=datetime.utcnow() + timedelta(days=30) + ) + session.add(item) + + item = pagure.lib.model.Token( + id='expired_token', + user_id=user_id, + project_id=1, + expiration=datetime.utcnow() - timedelta(days=1) + ) + session.add(item) + + session.commit() + + def add_content_git_repo(folder): """ Create some content for the specified git repo. """ if not os.path.exists(folder): From 51a00a52ddc5e763ce0ebb5d0b636c2a57338429 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:22 +0000 Subject: [PATCH 22/145] Add method to add some ACLs in the database for the tests --- diff --git a/tests/__init__.py b/tests/__init__.py index 6e3df89..948702c 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -256,6 +256,18 @@ def create_tokens(session, user_id=1): session.commit() +def create_acls(session): + """ Create some acls for the tokens. """ + for acl in ['create_issue', 'update_issue', 'create_pull_request']: + item = pagure.lib.model.ACL( + name=acl, + description=acl.replace('_', ' '), + ) + session.add(item) + + session.commit() + + def add_content_git_repo(folder): """ Create some content for the specified git repo. """ if not os.path.exists(folder): From 955bebbe4283c2939bd266f613786b4405a10eca Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:22 +0000 Subject: [PATCH 23/145] Add method to link one token with the ACLs of the database for the tests --- diff --git a/tests/__init__.py b/tests/__init__.py index 948702c..944d0fe 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -268,6 +268,18 @@ def create_acls(session): session.commit() +def create_tokens_acl(session): + """ Create some acls for the tokens. """ + for aclid in range(3): + item = pagure.lib.model.TokenAcl( + token_id='aaabbbcccddd', + acl_id=aclid, + ) + session.add(item) + + session.commit() + + def add_content_git_repo(folder): """ Create some content for the specified git repo. """ if not os.path.exists(folder): From 19062174c0504fc595b39bff06a6700e31930233 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:22 +0000 Subject: [PATCH 24/145] Monkey patch a few more module to be sure we use the right DB session in the tests --- diff --git a/tests/test_progit_flask_api_auth.py b/tests/test_progit_flask_api_auth.py index 28508c9..a3ca99e 100644 --- a/tests/test_progit_flask_api_auth.py +++ b/tests/test_progit_flask_api_auth.py @@ -36,6 +36,8 @@ class PagureFlaskApiAuthtests(tests.Modeltests): pagure.APP.config['TESTING'] = True pagure.SESSION = self.session pagure.api.SESSION = self.session + pagure.api.issue.SESSION = self.session + pagure.lib.SESSION = self.session self.app = pagure.APP.test_client() def test_auth_no_data(self): From 6c26c23e2baee845bfa321a3fb5a2b97ea427691 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:22 +0000 Subject: [PATCH 25/145] Adjust the unit-tests to the change in the error message returned --- diff --git a/tests/test_progit_flask_api_auth.py b/tests/test_progit_flask_api_auth.py index a3ca99e..2a9ad31 100644 --- a/tests/test_progit_flask_api_auth.py +++ b/tests/test_progit_flask_api_auth.py @@ -50,7 +50,7 @@ class PagureFlaskApiAuthtests(tests.Modeltests): self.assertDictEqual( data, { - "error": "Login invalid/expired. Please visit " \ + "error": "Invalid or expired token. Please visit " \ "https://pagure.org/ get or renew your API token.", "output": "notok" } @@ -64,7 +64,7 @@ class PagureFlaskApiAuthtests(tests.Modeltests): self.assertDictEqual( data, { - "error": "Login invalid/expired. Please visit " \ + "error": "Invalid or expired token. Please visit " \ "https://pagure.org/ get or renew your API token.", "output": "notok" } From 4a15116a38fff38fd230a62f57944a7eccc9f416 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:22 +0000 Subject: [PATCH 26/145] Add unit-tests to check an API token when it has no ACLs --- diff --git a/tests/test_progit_flask_api_auth.py b/tests/test_progit_flask_api_auth.py index 2a9ad31..70d109f 100644 --- a/tests/test_progit_flask_api_auth.py +++ b/tests/test_progit_flask_api_auth.py @@ -70,6 +70,38 @@ class PagureFlaskApiAuthtests(tests.Modeltests): } ) + def test_auth_noacl(self): + """ Test the authentication when the token does not have any ACL. + """ + tests.create_projects(self.session) + tests.create_tokens(self.session) + + output = self.app.post('/api/0/test/new_issue') + self.assertEqual(output.status_code, 401) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "Invalid or expired token. Please visit " \ + "https://pagure.org/ get or renew your API token.", + "output": "notok" + } + ) + + headers = {'Authorization': 'token aaabbbcccddd'} + + output = self.app.post('/api/0/test/new_issue', headers=headers) + self.assertEqual(output.status_code, 401) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "Invalid or expired token. Please visit " \ + "https://pagure.org/ get or renew your API token.", + "output": "notok" + } + ) + if __name__ == '__main__': SUITE = unittest.TestLoader().loadTestsFromTestCase( From 37d1e918116c473198417a128be219c1d81d9056 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:22 +0000 Subject: [PATCH 27/145] Add unit-tests to check an API token when it all goes well --- diff --git a/tests/test_progit_flask_api_auth.py b/tests/test_progit_flask_api_auth.py index 70d109f..d72415d 100644 --- a/tests/test_progit_flask_api_auth.py +++ b/tests/test_progit_flask_api_auth.py @@ -102,6 +102,39 @@ class PagureFlaskApiAuthtests(tests.Modeltests): } ) + def test_auth(self): + """ Test the token based authentication. + """ + tests.create_projects(self.session) + tests.create_tokens(self.session) + tests.create_acls(self.session) + tests.create_tokens_acl(self.session) + + output = self.app.post('/api/0/test/new_issue') + self.assertEqual(output.status_code, 401) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "Invalid or expired token. Please visit " \ + "https://pagure.org/ get or renew your API token.", + "output": "notok" + } + ) + + headers = {'Authorization': 'token aaabbbcccddd'} + + output = self.app.post('/api/0/test/new_issue', headers=headers) + self.assertEqual(output.status_code, 400) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "Invalid or incomplete input submited", + "error_code": 4 + } + ) + if __name__ == '__main__': SUITE = unittest.TestLoader().loadTestsFromTestCase( From 254d6540d13ef375ae959542236f74fe466707a8 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:22 +0000 Subject: [PATCH 28/145] Ensure we only split at the first occurrence of the word 'token' --- diff --git a/pagure/api/__init__.py b/pagure/api/__init__.py index 413b80c..e4013e3 100644 --- a/pagure/api/__init__.py +++ b/pagure/api/__init__.py @@ -46,7 +46,7 @@ def check_api_acls(acls): if 'Authorization' in flask.request.headers: authorization = flask.request.headers['Authorization'] if 'token' in authorization: - token_str = authorization.split('token')[1].strip() + token_str = authorization.split('token', 1)[1].strip() token_auth = False if token_str: From dccbc896fe2e03df266a25ac3c908006dc145efb Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:22 +0000 Subject: [PATCH 29/145] Add unit-tests to check the behavior of the API when the API token has expired --- diff --git a/tests/test_progit_flask_api_auth.py b/tests/test_progit_flask_api_auth.py index d72415d..0132d07 100644 --- a/tests/test_progit_flask_api_auth.py +++ b/tests/test_progit_flask_api_auth.py @@ -102,6 +102,38 @@ class PagureFlaskApiAuthtests(tests.Modeltests): } ) + def test_auth_expired(self): + """ Test the authentication when the token has expired. + """ + tests.create_projects(self.session) + tests.create_tokens(self.session) + + output = self.app.post('/api/0/test/new_issue') + self.assertEqual(output.status_code, 401) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "Invalid or expired token. Please visit " \ + "https://pagure.org/ get or renew your API token.", + "output": "notok" + } + ) + + headers = {'Authorization': 'token expired_token'} + + output = self.app.post('/api/0/test/new_issue', headers=headers) + self.assertEqual(output.status_code, 401) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "Invalid or expired token. Please visit " \ + "https://pagure.org/ get or renew your API token.", + "output": "notok" + } + ) + def test_auth(self): """ Test the token based authentication. """ From d403defd6b722604e3d4265980ccc0276a22a16e Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:22 +0000 Subject: [PATCH 30/145] SQLite ßðf!@#$!@#fœ®éöófj³ä, thank you faitout :) --- diff --git a/pagure/lib/model.py b/pagure/lib/model.py index c481056..7908cc7 100644 --- a/pagure/lib/model.py +++ b/pagure/lib/model.py @@ -983,7 +983,7 @@ class TokenAcl(BASE): __tablename__ = 'tokens_acls' token_id = sa.Column( - sa.Integer, sa.ForeignKey('tokens.id'), primary_key=True) + sa.String(64), sa.ForeignKey('tokens.id'), primary_key=True) acl_id = sa.Column( sa.Integer, sa.ForeignKey('acls.id'), primary_key=True) From 35a40f09d90c5aa29c67c79a7282e2bc3fd7ce4d Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:22 +0000 Subject: [PATCH 31/145] Move the error message about the api token to the dict of error code --- diff --git a/pagure/api/__init__.py b/pagure/api/__init__.py index e4013e3..06def23 100644 --- a/pagure/api/__init__.py +++ b/pagure/api/__init__.py @@ -29,10 +29,11 @@ API_ERROR_CODE = { 3: 'An error occured at the database level and prevent the action from ' 'reaching completion', 4: 'Invalid or incomplete input submited', + 5: 'Invalid or expired token. Please visit %s get or renew your ' + 'API token.' % APP.config['APP_URL'] } - def check_api_acls(acls): ''' Checks if the user provided an API token with its request and if this token allows the user to access the endpoint desired. @@ -61,10 +62,8 @@ def check_api_acls(acls): if not token_auth: output = { - "output": "notok", - "error": "Invalid or expired token. " - "Please visit %s get or renew your API token." % ( - APP.config['APP_URL']), + 'error_code': 5, + 'error': API_ERROR_CODE[5], } jsonout = flask.jsonify(output) jsonout.status_code = 401 From 5cb322b4aca33e4de7ecead147326e65726aa5bc Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:22 +0000 Subject: [PATCH 32/145] Check that the provided token is valid against the desired project At this point we checked that the token: - exists - has not expired - is allowed to access this endpoint (which might mean, depending on the endpoint, is allowed to perform the action) But we had not checked if the token is allowed to all this on this project, this is now rectified --- diff --git a/pagure/api/issue.py b/pagure/api/issue.py index 830bd6a..34fbd92 100644 --- a/pagure/api/issue.py +++ b/pagure/api/issue.py @@ -40,6 +40,13 @@ def new_issue(repo, username=None): jsonout.status_code = 404 return jsonout + if repo != flask.g.token.project: + output['error_code'] = 5 + output['error'] = API_ERROR_CODE[5] + jsonout = flask.jsonify(output) + jsonout.status_code = 404 + return jsonout + status = pagure.lib.get_issue_statuses(SESSION) form = pagure.forms.IssueForm(status=status, csrf_token=False) if form.validate_on_submit(): From 9f54cac0fba6340c105275c05f6443d2bf6a4fa3 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:22 +0000 Subject: [PATCH 33/145] Add a new decorator to catch the APIError thrown in the API code This way we have a clean way to returning errors to the user --- diff --git a/pagure/api/__init__.py b/pagure/api/__init__.py index 06def23..9928397 100644 --- a/pagure/api/__init__.py +++ b/pagure/api/__init__.py @@ -12,14 +12,16 @@ API namespace version 0. import functools +import fedmsg import flask API = flask.Blueprint('api_ns', __name__, url_prefix='/api/0') -from pagure import __api_version__, APP, SESSION import pagure import pagure.lib +from pagure import __api_version__, APP, SESSION +from pagure.exceptions import APIError API_ERROR_CODE = { @@ -92,6 +94,47 @@ def api_login_required(acls=None): return decorator +def api_method(function): + ''' Runs an API endpoint and catch all the APIException thrown. ''' + + @functools.wraps(function) + def wrapper(*args, **kwargs): + try: + result = function(*args, **kwargs) + except APIError as e: + if e.error_code in [3]: + APP.log.exception(e) + + if e.error_code in [0]: + response = flask.jsonify( + { + 'error': e.error, + 'error_code': e.error_code + } + ) + else: + response = flask.jsonify( + { + 'error': API_ERROR_CODE[e.error_code], + 'error_code': e.error_code + } + ) + response.status_code = e.status_code + else: + if flask.request.is_xhr: + encoder = fedmsg.encoding.dumps + else: + encoder = fedmsg.encoding.pretty_dumps + + response = flask.Response( + encoder(result), mimetype='application/json') + response.status_code = 200 + + return response + + return wrapper + + from pagure.api import issue From 421a7c29993b60b4a1c851dee7418159d9293445 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:23 +0000 Subject: [PATCH 34/145] Add the APIError class thrown by the API when something goes wrong --- diff --git a/pagure/exceptions.py b/pagure/exceptions.py index 4114069..3b95fdc 100644 --- a/pagure/exceptions.py +++ b/pagure/exceptions.py @@ -28,3 +28,12 @@ class FileNotFoundException(PagureException): exists. ''' pass + + +class APIError(PagureException): + ''' Exception raised by the API when something goes wrong. ''' + + def __init__(self, status_code, error_code, error=None): + self.status_code = status_code + self.error_code = error_code + self.error = error From 1d9dd768966fbd71054250578db836e193c2a54d Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:23 +0000 Subject: [PATCH 35/145] Add the relation between a token and its project --- diff --git a/pagure/lib/model.py b/pagure/lib/model.py index 7908cc7..a19d618 100644 --- a/pagure/lib/model.py +++ b/pagure/lib/model.py @@ -954,6 +954,14 @@ class Token(BASE): foreign_keys=[user_id], remote_side=[User.id]) + project = relation( + 'Project', + backref=backref( + 'tokens', cascade="delete, delete-orphan", + ), + foreign_keys=[project_id], + remote_side=[Project.id]) + def __repr__(self): ''' Return a string representation of this object. ''' From a1b9ea19a8535f61eb04ca2f2b448eddfb98aa2b Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:23 +0000 Subject: [PATCH 36/145] Adjust the unit-tests for the change in the api_login_required to use error_code --- diff --git a/tests/test_progit_flask_api_auth.py b/tests/test_progit_flask_api_auth.py index 0132d07..06a5624 100644 --- a/tests/test_progit_flask_api_auth.py +++ b/tests/test_progit_flask_api_auth.py @@ -52,7 +52,7 @@ class PagureFlaskApiAuthtests(tests.Modeltests): { "error": "Invalid or expired token. Please visit " \ "https://pagure.org/ get or renew your API token.", - "output": "notok" + "error_code": 5, } ) @@ -66,7 +66,7 @@ class PagureFlaskApiAuthtests(tests.Modeltests): { "error": "Invalid or expired token. Please visit " \ "https://pagure.org/ get or renew your API token.", - "output": "notok" + "error_code": 5, } ) @@ -84,7 +84,7 @@ class PagureFlaskApiAuthtests(tests.Modeltests): { "error": "Invalid or expired token. Please visit " \ "https://pagure.org/ get or renew your API token.", - "output": "notok" + "error_code": 5, } ) @@ -98,7 +98,7 @@ class PagureFlaskApiAuthtests(tests.Modeltests): { "error": "Invalid or expired token. Please visit " \ "https://pagure.org/ get or renew your API token.", - "output": "notok" + "error_code": 5, } ) @@ -116,7 +116,7 @@ class PagureFlaskApiAuthtests(tests.Modeltests): { "error": "Invalid or expired token. Please visit " \ "https://pagure.org/ get or renew your API token.", - "output": "notok" + "error_code": 5, } ) @@ -130,7 +130,7 @@ class PagureFlaskApiAuthtests(tests.Modeltests): { "error": "Invalid or expired token. Please visit " \ "https://pagure.org/ get or renew your API token.", - "output": "notok" + "error_code": 5, } ) @@ -150,7 +150,7 @@ class PagureFlaskApiAuthtests(tests.Modeltests): { "error": "Invalid or expired token. Please visit " \ "https://pagure.org/ get or renew your API token.", - "output": "notok" + "error_code": 5, } ) From 5038f2874893820afc71666c8271e17aad9a4e28 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:23 +0000 Subject: [PATCH 37/145] Simplify the API to open a new issue by raising APIError when things go wrong --- diff --git a/pagure/api/issue.py b/pagure/api/issue.py index 34fbd92..fb70e7b 100644 --- a/pagure/api/issue.py +++ b/pagure/api/issue.py @@ -11,14 +11,16 @@ import flask import pagure +import pagure.exceptions import pagure.lib from pagure import APP, SESSION -from pagure.api import API, api_login_required, API_ERROR_CODE +from pagure.api import API, api_method, api_login_required, API_ERROR_CODE @API.route('//new_issue', methods=['POST']) @API.route('/fork///new_issue', methods=['POST']) @api_login_required(acls=['create_issue']) +@api_method def new_issue(repo, username=None): """ Create a new issue """ @@ -27,25 +29,14 @@ def new_issue(repo, username=None): output = {} if repo is None: - output['error_code'] = 1 - output['error'] = API_ERROR_CODE[1] - jsonout = flask.jsonify(output) - jsonout.status_code = 404 - return jsonout + raise pagure.exceptions.APIError(404, error_code=1) if not repo.settings.get('issue_tracker', True): - output['error_code'] = 2 - output['error'] = API_ERROR_CODE[2] - jsonout = flask.jsonify(output) - jsonout.status_code = 404 - return jsonout + raise pagure.exceptions.APIError(404, error_code=2) + if repo != flask.g.token.project: - output['error_code'] = 5 - output['error'] = API_ERROR_CODE[5] - jsonout = flask.jsonify(output) - jsonout.status_code = 404 - return jsonout + raise pagure.exceptions.APIError(401, error_code=5) status = pagure.lib.get_issue_statuses(SESSION) form = pagure.forms.IssueForm(status=status, csrf_token=False) @@ -94,18 +85,13 @@ def new_issue(repo, username=None): output['message'] = 'issue created' output['message'] = 'issue created' except pagure.exceptions.PagureException, err: - output['error_code'] = 0 - output['error'] = str(err) - httpcode = 400 + raise pagure.exceptions.APIError( + 400, error_code=0, error=str(err)) except SQLAlchemyError, err: # pragma: no cover - SESSION.rollback() - output['error_code'] = 3 - output['error'] = API_ERROR_CODE[3] - httpcode = 400 + raise pagure.exceptions.APIError(400, error_code=3) + else: - output['error_code'] = 4 - output['error'] = API_ERROR_CODE[4] - httpcode = 400 + raise pagure.exceptions.APIError(400, error_code=4) jsonout = flask.jsonify(output) jsonout.status_code = httpcode From f51665b74119076caa3683b2376306e7c1a08a93 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:23 +0000 Subject: [PATCH 38/145] Try making fedmsg optional --- diff --git a/pagure/api/__init__.py b/pagure/api/__init__.py index 9928397..615d835 100644 --- a/pagure/api/__init__.py +++ b/pagure/api/__init__.py @@ -12,7 +12,12 @@ API namespace version 0. import functools -import fedmsg + +try: + import fedmsg +except ImportError: + fedmsg = None + import flask API = flask.Blueprint('api_ns', __name__, url_prefix='/api/0') @@ -121,10 +126,13 @@ def api_method(function): ) response.status_code = e.status_code else: - if flask.request.is_xhr: - encoder = fedmsg.encoding.dumps + if fedmsg is None: + encoder = flask.jsonify else: - encoder = fedmsg.encoding.pretty_dumps + if flask.request.is_xhr: + encoder = fedmsg.encoding.dumps + else: + encoder = fedmsg.encoding.pretty_dumps response = flask.Response( encoder(result), mimetype='application/json') From 7f151a8c1ac9ea7e22202aacc445ee6efc7a08e9 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:23 +0000 Subject: [PATCH 39/145] Drop encoding the json message with fedmsg --- diff --git a/pagure/api/__init__.py b/pagure/api/__init__.py index 615d835..bc2316e 100644 --- a/pagure/api/__init__.py +++ b/pagure/api/__init__.py @@ -12,12 +12,6 @@ API namespace version 0. import functools - -try: - import fedmsg -except ImportError: - fedmsg = None - import flask API = flask.Blueprint('api_ns', __name__, url_prefix='/api/0') @@ -63,7 +57,7 @@ def check_api_acls(acls): if token and not token.expired: if acls and set(token.acls_list).intersection(set(acls)): token_auth = True - flask.g.user = token.user + flask.g.fas_user = token.user flask.g.token = token print 'Add check for token ACLs' @@ -126,17 +120,7 @@ def api_method(function): ) response.status_code = e.status_code else: - if fedmsg is None: - encoder = flask.jsonify - else: - if flask.request.is_xhr: - encoder = fedmsg.encoding.dumps - else: - encoder = fedmsg.encoding.pretty_dumps - - response = flask.Response( - encoder(result), mimetype='application/json') - response.status_code = 200 + response = result return response From 6bef76bd8cfba4afbc40c727fa78176014cd04a8 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:23 +0000 Subject: [PATCH 40/145] Add missing import and disable crsf protection on the API --- diff --git a/pagure/api/issue.py b/pagure/api/issue.py index fb70e7b..113631e 100644 --- a/pagure/api/issue.py +++ b/pagure/api/issue.py @@ -10,6 +10,8 @@ import flask +from sqlalchemy.exc import SQLAlchemyError + import pagure import pagure.exceptions import pagure.lib @@ -39,7 +41,7 @@ def new_issue(repo, username=None): raise pagure.exceptions.APIError(401, error_code=5) status = pagure.lib.get_issue_statuses(SESSION) - form = pagure.forms.IssueForm(status=status, csrf_token=False) + form = pagure.forms.IssueForm(status=status, csrf_enabled=False) if form.validate_on_submit(): title = form.title.data content = form.issue_content.data From b86ca6be1ba924df00306b96052b4780fd26b8c9 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:23 +0000 Subject: [PATCH 41/145] Avoid the endpoint name conflicts between UI and API even if the namespace is different --- diff --git a/pagure/api/issue.py b/pagure/api/issue.py index 113631e..fee7425 100644 --- a/pagure/api/issue.py +++ b/pagure/api/issue.py @@ -23,7 +23,7 @@ from pagure.api import API, api_method, api_login_required, API_ERROR_CODE @API.route('/fork///new_issue', methods=['POST']) @api_login_required(acls=['create_issue']) @api_method -def new_issue(repo, username=None): +def api_new_issue(repo, username=None): """ Create a new issue """ repo = pagure.lib.get_project(SESSION, repo, user=username) From 993597847fe614826031c5b158eb9949d5cd4811 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:23 +0000 Subject: [PATCH 42/145] Add unit-tests checking the api_new_issue endpoint --- diff --git a/tests/test_progit_flask_api_issue.py b/tests/test_progit_flask_api_issue.py new file mode 100644 index 0000000..91bcf3a --- /dev/null +++ b/tests/test_progit_flask_api_issue.py @@ -0,0 +1,115 @@ +# -*- coding: utf-8 -*- + +""" + (c) 2015 - Copyright Red Hat Inc + + Authors: + Pierre-Yves Chibon + +""" + +__requires__ = ['SQLAlchemy >= 0.8'] +import pkg_resources + +import unittest +import shutil +import sys +import os + +import json +from mock import patch + +sys.path.insert(0, os.path.join(os.path.dirname( + os.path.abspath(__file__)), '..')) + +import pagure.lib +import tests + + +class PagureFlaskApiIssuetests(tests.Modeltests): + """ Tests for the flask API of pagure for issue """ + + def setUp(self): + """ Set up the environnment, ran before every tests. """ + super(PagureFlaskApiIssuetests, self).setUp() + + pagure.APP.config['TESTING'] = True + pagure.SESSION = self.session + pagure.api.SESSION = self.session + pagure.api.issue.SESSION = self.session + pagure.lib.SESSION = self.session + self.app = pagure.APP.test_client() + + def test_api_new_issue(self): + """ Test the token based authentication. + """ + tests.create_projects(self.session) + tests.create_tokens(self.session) + tests.create_acls(self.session) + tests.create_tokens_acl(self.session) + + headers = {'Authorization': 'token aaabbbcccddd'} + + # Valid token, wrong project + output = self.app.post('/api/0/test2/new_issue', headers=headers) + self.assertEqual(output.status_code, 401) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "Invalid or expired token. Please visit " \ + "https://pagure.org/ get or renew your API token.", + "error_code": 5 + } + ) + + # No input + output = self.app.post('/api/0/test/new_issue', headers=headers) + self.assertEqual(output.status_code, 400) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "Invalid or incomplete input submited", + "error_code": 4 + } + ) + + data = { + 'title': 'test issue', + } + + # Incomplete request + output = self.app.post( + '/api/0/test/new_issue', data=data, headers=headers) + self.assertEqual(output.status_code, 400) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "Invalid or incomplete input submited", + "error_code": 4 + } + ) + + data = { + 'title': 'test issue', + 'issue_content': 'This issue needs attention', + 'status': 'Open', + } + + # Valid request + output = self.app.post( + '/api/0/test/new_issue', data=data, headers=headers) + self.assertEqual(output.status_code, 200) + data = json.loads(output.data) + self.assertDictEqual( + data, + {'message': 'issue created'} + ) + + +if __name__ == '__main__': + SUITE = unittest.TestLoader().loadTestsFromTestCase( + PagureFlaskApiIssuetests) + unittest.TextTestRunner(verbosity=2).run(SUITE) From 9ed075754ee191b8dfdd0b95354894b76dbe6121 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:23 +0000 Subject: [PATCH 43/145] Fix unit-tests on a real DBMS, thanks faitout --- diff --git a/tests/__init__.py b/tests/__init__.py index 944d0fe..7e2eb62 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -273,7 +273,7 @@ def create_tokens_acl(session): for aclid in range(3): item = pagure.lib.model.TokenAcl( token_id='aaabbbcccddd', - acl_id=aclid, + acl_id=aclid + 1, ) session.add(item) From e91e1df3f5ee15c256c1bdd2f2582d441e88bfd6 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:23 +0000 Subject: [PATCH 44/145] Remove debugging statement --- diff --git a/pagure/api/__init__.py b/pagure/api/__init__.py index bc2316e..bef5054 100644 --- a/pagure/api/__init__.py +++ b/pagure/api/__init__.py @@ -59,7 +59,6 @@ def check_api_acls(acls): token_auth = True flask.g.fas_user = token.user flask.g.token = token - print 'Add check for token ACLs' if not token_auth: output = { From e5c5fc712b1e9a9d930a3e96171524a4d8313371 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:23 +0000 Subject: [PATCH 45/145] Create the tickets git repo for the projects of the tests --- diff --git a/tests/test_progit_flask_api_issue.py b/tests/test_progit_flask_api_issue.py index 91bcf3a..2bede50 100644 --- a/tests/test_progit_flask_api_issue.py +++ b/tests/test_progit_flask_api_issue.py @@ -38,12 +38,17 @@ class PagureFlaskApiIssuetests(tests.Modeltests): pagure.api.SESSION = self.session pagure.api.issue.SESSION = self.session pagure.lib.SESSION = self.session + + pagure.APP.config['TICKETS_FOLDER'] = os.path.join( + tests.HERE, 'tickets') + self.app = pagure.APP.test_client() def test_api_new_issue(self): """ Test the token based authentication. """ tests.create_projects(self.session) + tests.create_projects_git(os.path.join(tests.HERE, 'tickets')) tests.create_tokens(self.session) tests.create_acls(self.session) tests.create_tokens_acl(self.session) From 0e5cb20012dbf14b55dfc72f72c761bd5b03e38f Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:23 +0000 Subject: [PATCH 46/145] Set the TICKETS_FOLDER to nothing for these tests --- diff --git a/tests/test_progit_flask_api_issue.py b/tests/test_progit_flask_api_issue.py index 2bede50..046f584 100644 --- a/tests/test_progit_flask_api_issue.py +++ b/tests/test_progit_flask_api_issue.py @@ -39,8 +39,7 @@ class PagureFlaskApiIssuetests(tests.Modeltests): pagure.api.issue.SESSION = self.session pagure.lib.SESSION = self.session - pagure.APP.config['TICKETS_FOLDER'] = os.path.join( - tests.HERE, 'tickets') + pagure.APP.config['TICKETS_FOLDER'] = None self.app = pagure.APP.test_client() From 3e33624e521823208fc813e6108b0f2bdebf96ae Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:23 +0000 Subject: [PATCH 47/145] Adjust doc string to reflect the test --- diff --git a/tests/test_progit_flask_api_issue.py b/tests/test_progit_flask_api_issue.py index 046f584..32a11b1 100644 --- a/tests/test_progit_flask_api_issue.py +++ b/tests/test_progit_flask_api_issue.py @@ -44,8 +44,7 @@ class PagureFlaskApiIssuetests(tests.Modeltests): self.app = pagure.APP.test_client() def test_api_new_issue(self): - """ Test the token based authentication. - """ + """ Test the api_new_issue method of the flask api. """ tests.create_projects(self.session) tests.create_projects_git(os.path.join(tests.HERE, 'tickets')) tests.create_tokens(self.session) From f3d596913a24e81b368ad01f9ef0ffddfe6b2290 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:23 +0000 Subject: [PATCH 48/145] Add a new endpoint to get the description of an issue --- diff --git a/pagure/api/__init__.py b/pagure/api/__init__.py index bef5054..7712c16 100644 --- a/pagure/api/__init__.py +++ b/pagure/api/__init__.py @@ -31,7 +31,9 @@ API_ERROR_CODE = { 'reaching completion', 4: 'Invalid or incomplete input submited', 5: 'Invalid or expired token. Please visit %s get or renew your ' - 'API token.' % APP.config['APP_URL'] + 'API token.' % APP.config['APP_URL'], + 6: 'Issue not found', + 7: 'You are not allowed to view this issue', } diff --git a/pagure/api/issue.py b/pagure/api/issue.py index fee7425..09bf206 100644 --- a/pagure/api/issue.py +++ b/pagure/api/issue.py @@ -36,7 +36,6 @@ def api_new_issue(repo, username=None): if not repo.settings.get('issue_tracker', True): raise pagure.exceptions.APIError(404, error_code=2) - if repo != flask.g.token.project: raise pagure.exceptions.APIError(401, error_code=5) @@ -98,3 +97,36 @@ def api_new_issue(repo, username=None): jsonout = flask.jsonify(output) jsonout.status_code = httpcode return jsonout + + +@API.route('//issue/') +@API.route('/fork///issue/') +@api_method +def api_view_issue(repo, issueid, username=None): + """ List all issues associated to a repo + """ + + repo = pagure.lib.get_project(SESSION, repo, user=username) + httpcode = 200 + output = {} + + if repo is None: + raise pagure.exceptions.APIError(404, error_code=1) + + if not repo.settings.get('issue_tracker', True): + raise pagure.exceptions.APIError(404, error_code=2) + + issue = pagure.lib.search_issues(SESSION, repo, issueid=issueid) + + if issue is None or issue.project != repo: + raise pagure.exceptions.APIError(404, error_code=6) + + if issue.private and not is_repo_admin(repo) \ + and (not authenticated() or + not issue.user.user == flask.g.fas_user.username): + raise pagure.exceptions.APIError(403, error_code=7) + + + jsonout = flask.jsonify(issue.to_json()) + jsonout.status_code = httpcode + return jsonout From 1c76b97dc409a4a2e07d8d7d3b4c72ffdbcb6821 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:23 +0000 Subject: [PATCH 49/145] Add unit-tests for the api_view_issue endpoint --- diff --git a/tests/test_progit_flask_api_issue.py b/tests/test_progit_flask_api_issue.py index 32a11b1..18e9838 100644 --- a/tests/test_progit_flask_api_issue.py +++ b/tests/test_progit_flask_api_issue.py @@ -111,6 +111,53 @@ class PagureFlaskApiIssuetests(tests.Modeltests): {'message': 'issue created'} ) + def test_api_view_issue(self): + """ Test the api_view_issue method of the flask api. """ + self.test_api_new_issue() + + # Valid token, wrong project + output = self.app.get('/api/0/test2/issue/1') + self.assertEqual(output.status_code, 404) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "Issue not found", + "error_code": 6 + } + ) + + # No input + output = self.app.get('/api/0/test/issue/1') + self.assertEqual(output.status_code, 200) + data = json.loads(output.data) + data['date_created'] = '1431414800' + self.assertDictEqual( + data, + { + "assignee": None, + "blocks": [], + "comments": [], + "content": "This issue needs attention", + "date_created": "1431414800", + "depends": [], + "id": 1, + "private": False, + "status": "Open", + "tags": [], + "title": "test issue", + "user": { + "default_email": "bar@pingou.com", + "emails": [ + "bar@pingou.com", + "foo@pingou.com" + ], + "fullname": "PY C", + "name": "pingou" + } + } + ) + if __name__ == '__main__': SUITE = unittest.TestLoader().loadTestsFromTestCase( From 0408b4a3f2ffde5eab9ac444eb9c7b669a5715f9 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:23 +0000 Subject: [PATCH 50/145] Extend the check_api_acls method to support using a token for authentication only This way if someone specifies a token, we can authenticate them but if for GET request that do not require ACLs to access, we can authenticate without checking for authorization. --- diff --git a/pagure/api/__init__.py b/pagure/api/__init__.py index 7712c16..0b48c9a 100644 --- a/pagure/api/__init__.py +++ b/pagure/api/__init__.py @@ -37,7 +37,7 @@ API_ERROR_CODE = { } -def check_api_acls(acls): +def check_api_acls(acls, optional=False): ''' Checks if the user provided an API token with its request and if this token allows the user to access the endpoint desired. ''' @@ -55,12 +55,15 @@ def check_api_acls(acls): token_auth = False if token_str: token = pagure.lib.get_api_token(SESSION, token_str) - #print token if token and not token.expired: if acls and set(token.acls_list).intersection(set(acls)): token_auth = True flask.g.fas_user = token.user flask.g.token = token + elif not acls and optional: + token_auth = True + flask.g.fas_user = token.user + flask.g.token = token if not token_auth: output = { From cd76582f5f40b5b709ce8b512366118d90254bf2 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:23 +0000 Subject: [PATCH 51/145] Add a new API decorator for endpoints not always requiring authentication --- diff --git a/pagure/api/__init__.py b/pagure/api/__init__.py index 0b48c9a..49f60a7 100644 --- a/pagure/api/__init__.py +++ b/pagure/api/__init__.py @@ -97,6 +97,26 @@ def api_login_required(acls=None): return decorator +def api_login_optional(acls=None): + ''' Decorator used to indicate that authentication is optional 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. ''' + + check_api_acls(acls, optional=True) + return fn(*args, **kwargs) + + return decorated_function + + return decorator + + def api_method(function): ''' Runs an API endpoint and catch all the APIException thrown. ''' From 985a064763af87963fc74bf8984cf94f7b038937 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:23 +0000 Subject: [PATCH 52/145] Make the api_view_issue endpoint support optional authentication --- diff --git a/pagure/api/issue.py b/pagure/api/issue.py index 09bf206..94d2f5c 100644 --- a/pagure/api/issue.py +++ b/pagure/api/issue.py @@ -15,8 +15,10 @@ from sqlalchemy.exc import SQLAlchemyError import pagure import pagure.exceptions import pagure.lib -from pagure import APP, SESSION -from pagure.api import API, api_method, api_login_required, API_ERROR_CODE +from pagure import APP, SESSION, is_repo_admin, authenticated +from pagure.api import ( + API, api_method, api_login_required, api_login_optional, API_ERROR_CODE +) @API.route('//new_issue', methods=['POST']) @@ -101,6 +103,7 @@ def api_new_issue(repo, username=None): @API.route('//issue/') @API.route('/fork///issue/') +@api_login_optional() @api_method def api_view_issue(repo, issueid, username=None): """ List all issues associated to a repo From 4a277dcaaf0697950e018d99900bb3ed6446148f Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:23 +0000 Subject: [PATCH 53/145] Expand the unit-tests for the api_view_issue endpoint --- diff --git a/tests/test_progit_flask_api_issue.py b/tests/test_progit_flask_api_issue.py index 18e9838..12aa986 100644 --- a/tests/test_progit_flask_api_issue.py +++ b/tests/test_progit_flask_api_issue.py @@ -115,7 +115,19 @@ class PagureFlaskApiIssuetests(tests.Modeltests): """ Test the api_view_issue method of the flask api. """ self.test_api_new_issue() - # Valid token, wrong project + # Invalid repo + output = self.app.get('/api/0/foo/issue/1') + self.assertEqual(output.status_code, 404) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "Project not found", + "error_code": 1 + } + ) + + # Invalid issue for this repo output = self.app.get('/api/0/test2/issue/1') self.assertEqual(output.status_code, 404) data = json.loads(output.data) @@ -127,7 +139,7 @@ class PagureFlaskApiIssuetests(tests.Modeltests): } ) - # No input + # Valid issue output = self.app.get('/api/0/test/issue/1') self.assertEqual(output.status_code, 200) data = json.loads(output.data) @@ -158,6 +170,79 @@ class PagureFlaskApiIssuetests(tests.Modeltests): } ) + # Create private issue + repo = pagure.lib.get_project(self.session, 'test') + msg = pagure.lib.new_issue( + session=self.session, + repo=repo, + title='Test issue', + content='We should work on this', + user='pingou', + ticketfolder=None, + private=True, + ) + self.session.commit() + self.assertEqual(msg.title, 'Test issue') + + # Access private issue un-authenticated + output = self.app.get('/api/0/test/issue/2') + self.assertEqual(output.status_code, 403) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "You are not allowed to view this issue", + "error_code": 7 + } + ) + + headers = {'Authorization': 'token aaabbbccc'} + + # Access private issue authenticated but wrong token + output = self.app.get('/api/0/test/issue/2', headers=headers) + self.assertEqual(output.status_code, 403) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "You are not allowed to view this issue", + "error_code": 7 + } + ) + + headers = {'Authorization': 'token aaabbbcccddd'} + + # Access private issue authenticated correctly + output = self.app.get('/api/0/test/issue/2', headers=headers) + self.assertEqual(output.status_code, 200) + data = json.loads(output.data) + data['date_created'] = '1431414800' + self.assertDictEqual( + data, + { + "assignee": None, + "blocks": [], + "comments": [], + "content": "We should work on this", + "date_created": "1431414800", + "depends": [], + "id": 2, + "private": True, + "status": "Open", + "tags": [], + "title": "Test issue", + "user": { + "default_email": "bar@pingou.com", + "emails": [ + "bar@pingou.com", + "foo@pingou.com" + ], + "fullname": "PY C", + "name": "pingou" + } + } + ) + if __name__ == '__main__': SUITE = unittest.TestLoader().loadTestsFromTestCase( From e9dd602366528888cb9649317ea54f8034e60b59 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:23 +0000 Subject: [PATCH 54/145] Add a new form to add/change the status of an issue --- diff --git a/pagure/forms.py b/pagure/forms.py index ae43bc5..aeedf7f 100644 --- a/pagure/forms.py +++ b/pagure/forms.py @@ -72,6 +72,26 @@ class AddIssueTagForm(wtf.Form): ) +class StatusForm(wtf.Form): + ''' Form to add/change the status of an issue. ''' + status = wtforms.SelectField( + 'Status', + [wtforms.validators.Required()], + choices=[(item, item) for item in []] + ) + + def __init__(self, *args, **kwargs): + """ Calls the default constructor with the normal argument but + uses the list of collection provided to fill the choices of the + drop-down list. + """ + super(StatusForm, self).__init__(*args, **kwargs) + if 'status' in kwargs: + self.status.choices = [ + (status, status) for status in kwargs['status'] + ] + + class UpdateIssueForm(wtf.Form): ''' Form to add a comment to an issue. ''' tag = wtforms.TextField( From 5abba652b582ea1beeb38c1186308aa004d0b880 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:23 +0000 Subject: [PATCH 55/145] Add a new endpoint to change the status of an issue --- diff --git a/pagure/api/issue.py b/pagure/api/issue.py index 94d2f5c..d3e1478 100644 --- a/pagure/api/issue.py +++ b/pagure/api/issue.py @@ -133,3 +133,65 @@ def api_view_issue(repo, issueid, username=None): jsonout = flask.jsonify(issue.to_json()) jsonout.status_code = httpcode return jsonout + + +@API.route('//issue//status', methods=['POST']) +@API.route('/fork////status', methods=['POST']) +@api_login_required(acls=['change_status_issue']) +@api_method +def api_change_status_issue(repo, issueid, username=None): + """ Change the status of an issue + """ + repo = pagure.lib.get_project(SESSION, repo, user=username) + httpcode = 200 + output = {} + + if repo is None: + raise pagure.exceptions.APIError(404, error_code=1) + + if not repo.settings.get('issue_tracker', True): + raise pagure.exceptions.APIError(404, error_code=2) + + if repo != flask.g.token.project: + raise pagure.exceptions.APIError(401, error_code=5) + + issue = pagure.lib.search_issues(SESSION, repo, issueid=issueid) + + if issue is None or issue.project != repo: + raise pagure.exceptions.APIError(404, error_code=6) + + if issue.private and not is_repo_admin(repo) \ + and (not authenticated() or + not issue.user.user == flask.g.fas_user.username): + raise pagure.exceptions.APIError(403, error_code=7) + + status = pagure.lib.get_issue_statuses(SESSION) + form = pagure.forms.StatusForm(status=status, csrf_enabled=False) + if form.validate_on_submit(): + new_status = form.status.data + try: + # Update status + message = pagure.lib.edit_issue( + SESSION, + issue=issue, + status=new_status, + user=flask.g.fas_user.username, + ticketfolder=APP.config['TICKETS_FOLDER'], + ) + SESSION.commit() + if message: + output['message'] = message + else: + output['message'] = 'No changes' + except pagure.exceptions.PagureException, err: + raise pagure.exceptions.APIError( + 400, error_code=0, error=str(err)) + except SQLAlchemyError, err: # pragma: no cover + raise pagure.exceptions.APIError(400, error_code=3) + + else: + raise pagure.exceptions.APIError(400, error_code=4) + + jsonout = flask.jsonify(output) + jsonout.status_code = httpcode + return jsonout From 41e184c51b904196dbf0a8b03ef79a124ba57ea3 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:23 +0000 Subject: [PATCH 56/145] Expand the ACLs the test token has --- diff --git a/tests/__init__.py b/tests/__init__.py index 7e2eb62..4a4d162 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -258,7 +258,10 @@ def create_tokens(session, user_id=1): def create_acls(session): """ Create some acls for the tokens. """ - for acl in ['create_issue', 'update_issue', 'create_pull_request']: + for acl in [ + 'create_issue', 'update_issue', 'create_pull_request', + 'change_status_issue', + ]: item = pagure.lib.model.ACL( name=acl, description=acl.replace('_', ' '), @@ -270,7 +273,7 @@ def create_acls(session): def create_tokens_acl(session): """ Create some acls for the tokens. """ - for aclid in range(3): + for aclid in range(4): item = pagure.lib.model.TokenAcl( token_id='aaabbbcccddd', acl_id=aclid + 1, From 218ed6a7654e23981f611e42491d2be32b43727a Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:23 +0000 Subject: [PATCH 57/145] Add unit-tests for the api_change_status_issue --- diff --git a/tests/test_progit_flask_api_issue.py b/tests/test_progit_flask_api_issue.py index 12aa986..7e94bc0 100644 --- a/tests/test_progit_flask_api_issue.py +++ b/tests/test_progit_flask_api_issue.py @@ -243,6 +243,128 @@ class PagureFlaskApiIssuetests(tests.Modeltests): } ) + def test_api_change_status_issue(self): + """ Test the api_change_status_issue method of the flask api. """ + tests.create_projects(self.session) + tests.create_projects_git(os.path.join(tests.HERE, 'tickets')) + tests.create_tokens(self.session) + tests.create_acls(self.session) + tests.create_tokens_acl(self.session) + + headers = {'Authorization': 'token aaabbbcccddd'} + + # Valid token, wrong project + output = self.app.post('/api/0/test2/issue/1/status', headers=headers) + self.assertEqual(output.status_code, 401) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "Invalid or expired token. Please visit " \ + "https://pagure.org/ get or renew your API token.", + "error_code": 5 + } + ) + + # No input + output = self.app.post('/api/0/test/issue/1/status', headers=headers) + self.assertEqual(output.status_code, 404) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "Issue not found", + "error_code": 6 + } + ) + + # Create normal issue + repo = pagure.lib.get_project(self.session, 'test') + msg = pagure.lib.new_issue( + session=self.session, + repo=repo, + title='Test issue #1', + content='We should work on this', + user='pingou', + ticketfolder=None, + private=False, + ) + self.session.commit() + self.assertEqual(msg.title, 'Test issue #1') + + # Create private issue + msg = pagure.lib.new_issue( + session=self.session, + repo=repo, + title='Test issue', + content='We should work on this', + user='pingou', + ticketfolder=None, + private=True, + ) + self.session.commit() + self.assertEqual(msg.title, 'Test issue') + + # Check status before + repo = pagure.lib.get_project(self.session, 'test') + issue = pagure.lib.search_issues(self.session, repo, issueid=1) + self.assertEqual(issue.status, 'Open') + + data = { + 'title': 'test issue', + } + + # Incomplete request + output = self.app.post( + '/api/0/test/issue/1/status', data=data, headers=headers) + self.assertEqual(output.status_code, 400) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "Invalid or incomplete input submited", + "error_code": 4 + } + ) + + # No change + repo = pagure.lib.get_project(self.session, 'test') + issue = pagure.lib.search_issues(self.session, repo, issueid=1) + self.assertEqual(issue.status, 'Open') + + data = { + 'status': 'Open', + } + + # Valid request but no change + output = self.app.post( + '/api/0/test/issue/1/status', data=data, headers=headers) + self.assertEqual(output.status_code, 200) + data = json.loads(output.data) + self.assertDictEqual( + data, + {'message': 'No changes'} + ) + + # No change + repo = pagure.lib.get_project(self.session, 'test') + issue = pagure.lib.search_issues(self.session, repo, issueid=1) + self.assertEqual(issue.status, 'Open') + + data = { + 'status': 'Fixed', + } + + # Valid request + output = self.app.post( + '/api/0/test/issue/1/status', data=data, headers=headers) + self.assertEqual(output.status_code, 200) + data = json.loads(output.data) + self.assertDictEqual( + data, + {'message': 'Edited successfully issue #1'} + ) + if __name__ == '__main__': SUITE = unittest.TestLoader().loadTestsFromTestCase( From 0e129fef690a80ec919bbc1d45acd046af8c3c01 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:23 +0000 Subject: [PATCH 58/145] Do not catch an exception that isn't thrown --- diff --git a/pagure/api/issue.py b/pagure/api/issue.py index d3e1478..adebbe1 100644 --- a/pagure/api/issue.py +++ b/pagure/api/issue.py @@ -87,9 +87,6 @@ def api_new_issue(repo, username=None): output['message'] = 'issue created' output['message'] = 'issue created' - except pagure.exceptions.PagureException, err: - raise pagure.exceptions.APIError( - 400, error_code=0, error=str(err)) except SQLAlchemyError, err: # pragma: no cover raise pagure.exceptions.APIError(400, error_code=3) @@ -129,7 +126,6 @@ def api_view_issue(repo, issueid, username=None): not issue.user.user == flask.g.fas_user.username): raise pagure.exceptions.APIError(403, error_code=7) - jsonout = flask.jsonify(issue.to_json()) jsonout.status_code = httpcode return jsonout From f69837a5404e2276b87b76f38c19b6eac86f7d6f Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:23 +0000 Subject: [PATCH 59/145] Expand the unit-test suite to cover some more corners --- diff --git a/tests/test_progit_flask_api_issue.py b/tests/test_progit_flask_api_issue.py index 7e94bc0..e3bf935 100644 --- a/tests/test_progit_flask_api_issue.py +++ b/tests/test_progit_flask_api_issue.py @@ -11,6 +11,7 @@ __requires__ = ['SQLAlchemy >= 0.8'] import pkg_resources +import datetime import unittest import shutil import sys @@ -82,6 +83,19 @@ class PagureFlaskApiIssuetests(tests.Modeltests): 'title': 'test issue', } + # Invalid repo + output = self.app.post( + '/api/0/foo/new_issue', data=data, headers=headers) + self.assertEqual(output.status_code, 404) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "Project not found", + "error_code": 1 + } + ) + # Incomplete request output = self.app.post( '/api/0/test/new_issue', data=data, headers=headers) @@ -253,6 +267,18 @@ class PagureFlaskApiIssuetests(tests.Modeltests): headers = {'Authorization': 'token aaabbbcccddd'} + # Invalid project + output = self.app.post('/api/0/foo/issue/1/status', headers=headers) + self.assertEqual(output.status_code, 404) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "Project not found", + "error_code": 1 + } + ) + # Valid token, wrong project output = self.app.post('/api/0/test2/issue/1/status', headers=headers) self.assertEqual(output.status_code, 401) @@ -292,13 +318,43 @@ class PagureFlaskApiIssuetests(tests.Modeltests): self.session.commit() self.assertEqual(msg.title, 'Test issue #1') + # Create another project + item = pagure.lib.model.Project( + user_id=2, # pingou + name='foo', + description='test project #3', + hook_token='aaabbbdddeee', + ) + self.session.add(item) + self.session.commit() + + # Create a token for pingou for this project + item = pagure.lib.model.Token( + id='pingou_foo', + user_id=1, + project_id=3, + expiration=datetime.datetime.utcnow() + datetime.timedelta( + days=30) + ) + self.session.add(item) + self.session.commit() + + # Give `change_status_issue` to this token + item = pagure.lib.model.TokenAcl( + token_id='pingou_foo', + acl_id=4, + ) + self.session.add(item) + self.session.commit() + + repo = pagure.lib.get_project(self.session, 'foo') # Create private issue msg = pagure.lib.new_issue( session=self.session, repo=repo, title='Test issue', content='We should work on this', - user='pingou', + user='foo', ticketfolder=None, private=True, ) @@ -365,6 +421,21 @@ class PagureFlaskApiIssuetests(tests.Modeltests): {'message': 'Edited successfully issue #1'} ) + headers = {'Authorization': 'token pingou_foo'} + + # Un-authorized issue + output = self.app.post( + '/api/0/foo/issue/1/status', data=data, headers=headers) + self.assertEqual(output.status_code, 403) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "You are not allowed to view this issue", + "error_code": 7 + } + ) + if __name__ == '__main__': SUITE = unittest.TestLoader().loadTestsFromTestCase( From 1981d118ef38d5db96878d0d5f720f512e7fa7c3 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:23 +0000 Subject: [PATCH 60/145] Add an API endpoint to comment on a ticket: api_comment_issue --- diff --git a/pagure/api/issue.py b/pagure/api/issue.py index adebbe1..ccf7bc9 100644 --- a/pagure/api/issue.py +++ b/pagure/api/issue.py @@ -191,3 +191,64 @@ def api_change_status_issue(repo, issueid, username=None): jsonout = flask.jsonify(output) jsonout.status_code = httpcode return jsonout + + +@API.route('//issue//comment', methods=['POST']) +@API.route('/fork////comment', methods=['POST']) +@api_login_required(acls=['comment_issue']) +@api_method +def api_comment_issue(repo, issueid, username=None): + """ Add a comment to an issue + """ + repo = pagure.lib.get_project(SESSION, repo, user=username) + httpcode = 200 + output = {} + + if repo is None: + raise pagure.exceptions.APIError(404, error_code=1) + + if not repo.settings.get('issue_tracker', True): + raise pagure.exceptions.APIError(404, error_code=2) + + if repo.fullname != flask.g.token.project.fullname: + raise pagure.exceptions.APIError(401, error_code=5) + + issue = pagure.lib.search_issues(SESSION, repo, issueid=issueid) + + if issue is None or issue.project != repo: + raise pagure.exceptions.APIError(404, error_code=6) + + if issue.private and not is_repo_admin(repo) \ + and (not authenticated() or + not issue.user.user == flask.g.fas_user.username): + raise pagure.exceptions.APIError(403, error_code=7) + + form = pagure.forms.CommentForm(csrf_enabled=False) + if form.validate_on_submit(): + comment = form.comment.data + try: + # New comment + message = pagure.lib.add_issue_comment( + SESSION, + issue=issue, + comment=comment, + user=flask.g.fas_user.username, + ticketfolder=APP.config['TICKETS_FOLDER'], + ) + SESSION.commit() + if message: + output['message'] = message + else: + output['message'] = 'No changes' + except pagure.exceptions.PagureException, err: + raise pagure.exceptions.APIError( + 400, error_code=0, error=str(err)) + except SQLAlchemyError, err: # pragma: no cover + raise pagure.exceptions.APIError(400, error_code=3) + + else: + raise pagure.exceptions.APIError(400, error_code=4) + + jsonout = flask.jsonify(output) + jsonout.status_code = httpcode + return jsonout From fc6bd47bf4a52a10f27d784a38967f19a8f8a3b2 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:23 +0000 Subject: [PATCH 61/145] Add unit-tests for the api_comment_issue endpoint --- diff --git a/tests/__init__.py b/tests/__init__.py index 4a4d162..241cea8 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -260,7 +260,7 @@ def create_acls(session): """ Create some acls for the tokens. """ for acl in [ 'create_issue', 'update_issue', 'create_pull_request', - 'change_status_issue', + 'change_status_issue', 'comment_issue' ]: item = pagure.lib.model.ACL( name=acl, @@ -273,7 +273,7 @@ def create_acls(session): def create_tokens_acl(session): """ Create some acls for the tokens. """ - for aclid in range(4): + for aclid in range(5): item = pagure.lib.model.TokenAcl( token_id='aaabbbcccddd', acl_id=aclid + 1, diff --git a/tests/test_progit_flask_api_issue.py b/tests/test_progit_flask_api_issue.py index e3bf935..be08b57 100644 --- a/tests/test_progit_flask_api_issue.py +++ b/tests/test_progit_flask_api_issue.py @@ -436,6 +436,183 @@ class PagureFlaskApiIssuetests(tests.Modeltests): } ) + def test_api_comment_issue(self): + """ Test the api_comment_issue method of the flask api. """ + tests.create_projects(self.session) + tests.create_tokens(self.session) + tests.create_acls(self.session) + tests.create_tokens_acl(self.session) + + headers = {'Authorization': 'token aaabbbcccddd'} + + # Invalid project + output = self.app.post('/api/0/foo/issue/1/comment', headers=headers) + self.assertEqual(output.status_code, 404) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "Project not found", + "error_code": 1 + } + ) + + # Valid token, wrong project + output = self.app.post('/api/0/test2/issue/1/comment', headers=headers) + self.assertEqual(output.status_code, 401) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "Invalid or expired token. Please visit " \ + "https://pagure.org/ get or renew your API token.", + "error_code": 5 + } + ) + + # No input + output = self.app.post('/api/0/test/issue/1/comment', headers=headers) + self.assertEqual(output.status_code, 404) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "Issue not found", + "error_code": 6 + } + ) + + # Create normal issue + repo = pagure.lib.get_project(self.session, 'test') + msg = pagure.lib.new_issue( + session=self.session, + repo=repo, + title='Test issue #1', + content='We should work on this', + user='pingou', + ticketfolder=None, + private=False, + ) + self.session.commit() + self.assertEqual(msg.title, 'Test issue #1') + + # Check comments before + repo = pagure.lib.get_project(self.session, 'test') + issue = pagure.lib.search_issues(self.session, repo, issueid=1) + self.assertEqual(len(issue.comments), 0) + + data = { + 'title': 'test issue', + } + + # Incomplete request + output = self.app.post( + '/api/0/test/issue/1/comment', data=data, headers=headers) + self.assertEqual(output.status_code, 400) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "Invalid or incomplete input submited", + "error_code": 4 + } + ) + + # No change + repo = pagure.lib.get_project(self.session, 'test') + issue = pagure.lib.search_issues(self.session, repo, issueid=1) + self.assertEqual(issue.status, 'Open') + + data = { + 'comment': 'This is a very interesting question', + } + + # Valid request + output = self.app.post( + '/api/0/test/issue/1/comment', data=data, headers=headers) + self.assertEqual(output.status_code, 200) + data = json.loads(output.data) + self.assertDictEqual( + data, + {'message': 'Comment added'} + ) + + # One comment added + repo = pagure.lib.get_project(self.session, 'test') + issue = pagure.lib.search_issues(self.session, repo, issueid=1) + self.assertEqual(len(issue.comments), 1) + + # Create another project + item = pagure.lib.model.Project( + user_id=2, # foo + name='foo', + description='test project #3', + hook_token='aaabbbdddeee', + ) + self.session.add(item) + self.session.commit() + + # Create a token for pingou for this project + item = pagure.lib.model.Token( + id='pingou_foo', + user_id=1, + project_id=3, + expiration=datetime.datetime.utcnow() + datetime.timedelta( + days=30) + ) + self.session.add(item) + self.session.commit() + + # Give `change_status_issue` to this token + item = pagure.lib.model.TokenAcl( + token_id='pingou_foo', + acl_id=5, + ) + self.session.add(item) + self.session.commit() + + repo = pagure.lib.get_project(self.session, 'foo') + # Create private issue + msg = pagure.lib.new_issue( + session=self.session, + repo=repo, + title='Test issue', + content='We should work on this', + user='foo', + ticketfolder=None, + private=True, + ) + self.session.commit() + self.assertEqual(msg.title, 'Test issue') + + # Check before + repo = pagure.lib.get_project(self.session, 'foo') + issue = pagure.lib.search_issues(self.session, repo, issueid=1) + self.assertEqual(len(issue.comments), 0) + + data = { + 'comment': 'This is a very interesting question', + } + headers = {'Authorization': 'token pingou_foo'} + + # Valid request but un-authorized + output = self.app.post( + '/api/0/foo/issue/1/comment', data=data, headers=headers) + self.assertEqual(output.status_code, 403) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "You are not allowed to view this issue", + "error_code": 7 + } + ) + + # No comment added + repo = pagure.lib.get_project(self.session, 'foo') + issue = pagure.lib.search_issues(self.session, repo, issueid=1) + self.assertEqual(len(issue.comments), 0) + if __name__ == '__main__': SUITE = unittest.TestLoader().loadTestsFromTestCase( From 3e54af0c0425a158573bf74ca7d7d1097c75805d Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:23 +0000 Subject: [PATCH 62/145] No need to catch an exception that is not thrown --- diff --git a/pagure/api/issue.py b/pagure/api/issue.py index ccf7bc9..3b268b1 100644 --- a/pagure/api/issue.py +++ b/pagure/api/issue.py @@ -240,9 +240,6 @@ def api_comment_issue(repo, issueid, username=None): output['message'] = message else: output['message'] = 'No changes' - except pagure.exceptions.PagureException, err: - raise pagure.exceptions.APIError( - 400, error_code=0, error=str(err)) except SQLAlchemyError, err: # pragma: no cover raise pagure.exceptions.APIError(400, error_code=3) From 4cb77c900ed53cc8e8d274645f5056af09877cda Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:23 +0000 Subject: [PATCH 63/145] There is always a message returned to the user --- diff --git a/pagure/api/issue.py b/pagure/api/issue.py index 3b268b1..f46e392 100644 --- a/pagure/api/issue.py +++ b/pagure/api/issue.py @@ -236,10 +236,7 @@ def api_comment_issue(repo, issueid, username=None): ticketfolder=APP.config['TICKETS_FOLDER'], ) SESSION.commit() - if message: - output['message'] = message - else: - output['message'] = 'No changes' + output['message'] = message except SQLAlchemyError, err: # pragma: no cover raise pagure.exceptions.APIError(400, error_code=3) From 1e194942d8c886ea8747b7fb59f4974b539b7886 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:23 +0000 Subject: [PATCH 64/145] Add new API endpoint to add comments to a pull-request --- diff --git a/pagure/api/fork.py b/pagure/api/fork.py new file mode 100644 index 0000000..42e14bb --- /dev/null +++ b/pagure/api/fork.py @@ -0,0 +1,80 @@ +# -*- coding: utf-8 -*- + +""" + (c) 2015 - Copyright Red Hat Inc + + Authors: + Pierre-Yves Chibon + +""" + +import flask + +from sqlalchemy.exc import SQLAlchemyError + +import pagure +import pagure.exceptions +import pagure.lib +from pagure import APP, SESSION, is_repo_admin, authenticated +from pagure.api import ( + API, api_method, api_login_required, api_login_optional, API_ERROR_CODE +) + + +@API.route('//pull-request//comment', + methods=['POST']) +@API.route('/fork///pull-request//comment', + methods=['POST']) +@api_login_required(acls=['pull_request_comment']) +@api_method +def api_pull_request_add_comment(repo, requestid, username=None): + """ Add a comment to an pull-request + """ + repo = pagure.lib.get_project(SESSION, repo, user=username) + httpcode = 200 + output = {} + + if repo is None: + raise pagure.exceptions.APIError(404, error_code=1) + + if not repo.settings.get('pull_requests', True): + raise pagure.exceptions.APIError(404, error_code=8) + + if repo.fullname != flask.g.token.project.fullname: + raise pagure.exceptions.APIError(401, error_code=5) + + request = pagure.lib.search_pull_requests( + SESSION, project_id=repo.id, requestid=requestid) + + if not request: + raise pagure.exceptions.APIError(404, error_code=9) + + form = pagure.forms.AddPullRequestCommentForm(csrf_enabled=False) + if form.validate_on_submit(): + comment = form.comment.data + commit = form.commit.data + filename = form.filename.data + row = form.row.data + try: + # New comment + message = pagure.lib.add_pull_request_comment( + SESSION, + request=request, + commit=commit, + filename=filename, + row=row, + comment=comment, + user=flask.g.fas_user.username, + requestfolder=APP.config['REQUESTS_FOLDER'], + ) + SESSION.commit() + output['message'] = message + except SQLAlchemyError, err: # pragma: no cover + raise pagure.exceptions.APIError(400, error_code=3) + + else: + raise pagure.exceptions.APIError(400, error_code=4) + + jsonout = flask.jsonify(output) + jsonout.status_code = httpcode + return jsonout From b851ab6aa837a86f620929ca8012857aa1a52f15 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:23 +0000 Subject: [PATCH 65/145] Import the new fork controller from the API namespace --- diff --git a/pagure/__init__.py b/pagure/__init__.py index 1bf02f7..76bd675 100644 --- a/pagure/__init__.py +++ b/pagure/__init__.py @@ -391,6 +391,7 @@ import pagure.ui.repo from pagure.api import API from pagure.api import issue +from pagure.api import fork APP.register_blueprint(API) import pagure.internal diff --git a/pagure/api/__init__.py b/pagure/api/__init__.py index 49f60a7..a36e095 100644 --- a/pagure/api/__init__.py +++ b/pagure/api/__init__.py @@ -152,6 +152,7 @@ def api_method(function): from pagure.api import issue +from pagure.api import fork @API.route('/version/') From 2766215cc0305a7ce9fc05f495f0c0a4250a6db4 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:23 +0000 Subject: [PATCH 66/145] Support a couple more of API error --- diff --git a/pagure/api/__init__.py b/pagure/api/__init__.py index a36e095..b7700d4 100644 --- a/pagure/api/__init__.py +++ b/pagure/api/__init__.py @@ -34,6 +34,8 @@ API_ERROR_CODE = { 'API token.' % APP.config['APP_URL'], 6: 'Issue not found', 7: 'You are not allowed to view this issue', + 8: 'Pull-Request have been deactivated for this project', + 9: 'Pull-Request not found', } From c71c2d280881596d6540f58c400e28ae920ff8c8 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:24 +0000 Subject: [PATCH 67/145] Add unit-tests for the api_pull_request_view --- diff --git a/tests/test_progit_flask_api_fork.py b/tests/test_progit_flask_api_fork.py new file mode 100644 index 0000000..489eb1e --- /dev/null +++ b/tests/test_progit_flask_api_fork.py @@ -0,0 +1,191 @@ +# -*- coding: utf-8 -*- + +""" + (c) 2015 - Copyright Red Hat Inc + + Authors: + Pierre-Yves Chibon + +""" + +__requires__ = ['SQLAlchemy >= 0.8'] +import pkg_resources + +import datetime +import unittest +import shutil +import sys +import os + +import json +from mock import patch + +sys.path.insert(0, os.path.join(os.path.dirname( + os.path.abspath(__file__)), '..')) + +import pagure.lib +import tests + + +class PagureFlaskApiForktests(tests.Modeltests): + """ Tests for the flask API of pagure for issue """ + + def setUp(self): + """ Set up the environnment, ran before every tests. """ + super(PagureFlaskApiForktests, self).setUp() + + pagure.APP.config['TESTING'] = True + pagure.SESSION = self.session + pagure.api.SESSION = self.session + pagure.api.fork.SESSION = self.session + pagure.lib.SESSION = self.session + + pagure.APP.config['REQUESTS_FOLDER'] = None + + self.app = pagure.APP.test_client() + + def test_api_pull_request_view(self): + """ Test the api_pull_request_view method of the flask api. """ + tests.create_projects(self.session) + tests.create_tokens(self.session) + tests.create_acls(self.session) + tests.create_tokens_acl(self.session) + + # Create a pull-request + repo = pagure.lib.get_project(self.session, 'test') + forked_repo = pagure.lib.get_project(self.session, 'test') + msg = pagure.lib.new_pull_request( + session=self.session, + repo_from=forked_repo, + branch_from='master', + repo_to=repo, + branch_to='master', + title='test pull-request', + user='pingou', + requestfolder=None, + ) + self.session.commit() + self.assertEqual(msg, 'Request created') + + # Invalid repo + output = self.app.get('/api/0/foo/pull-request/1') + self.assertEqual(output.status_code, 404) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "Project not found", + "error_code": 1 + } + ) + + # Invalid issue for this repo + output = self.app.get('/api/0/test2/pull-request/1') + self.assertEqual(output.status_code, 404) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "Pull-Request not found", + "error_code": 9 + } + ) + + # Valid issue + output = self.app.get('/api/0/test/pull-request/1') + self.assertEqual(output.status_code, 200) + data = json.loads(output.data) + data['date_created'] = '1431414800' + data['project']['date_created'] = '1431414800' + data['repo_from']['date_created'] = '1431414800' + data['uid'] = '1431414800' + self.assertDictEqual( + data, + { + "assignee": None, + "branch": "master", + "branch_from": "master", + "comments": [], + "commit_start": None, + "commit_stop": None, + "date_created": "1431414800", + "id": 1, + "project": { + "date_created": "1431414800", + "description": "test project #1", + "id": 1, + "name": "test", + "parent": None, + "settings": { + "Minimum_score_to_merge_pull-request": -1, + "Only_assignee_can_merge_pull-request": False, + "Web-hooks": None, + "issue_tracker": True, + "project_documentation": True, + "pull_requests": True + }, + "user": { + "emails": [ + "bar@pingou.com", + "foo@pingou.com" + ], + "fullname": "PY C", + "name": "pingou" + } + }, + "repo_from": { + "date_created": "1431414800", + "description": "test project #1", + "id": 1, + "name": "test", + "parent": None, + "settings": { + "Minimum_score_to_merge_pull-request": -1, + "Only_assignee_can_merge_pull-request": False, + "Web-hooks": None, + "issue_tracker": True, + "project_documentation": True, + "pull_requests": True + }, + "user": { + "emails": [ + "bar@pingou.com", + "foo@pingou.com" + ], + "fullname": "PY C", + "name": "pingou" + } + }, + "status": True, + "title": "test pull-request", + "uid": "1431414800", + "user": { + "default_email": "bar@pingou.com", + "emails": [ + "bar@pingou.com", + "foo@pingou.com" + ], + "fullname": "PY C", + "name": "pingou" + } + } + ) + + headers = {'Authorization': 'token aaabbbcccddd'} + + # Access Pull-Request authenticated + output = self.app.get('/api/0/test/pull-request/1', headers=headers) + self.assertEqual(output.status_code, 200) + data2 = json.loads(output.data) + data2['date_created'] = '1431414800' + data2['project']['date_created'] = '1431414800' + data2['repo_from']['date_created'] = '1431414800' + data2['uid'] = '1431414800' + data2['date_created'] = '1431414800' + self.assertDictEqual(data, data2) + + +if __name__ == '__main__': + SUITE = unittest.TestLoader().loadTestsFromTestCase( + PagureFlaskApiForktests) + unittest.TextTestRunner(verbosity=2).run(SUITE) From bea7bd80ad58e920e97b70f7181b28b6e94f39be Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:24 +0000 Subject: [PATCH 68/145] Add API endpoint to view a specific pull-request --- diff --git a/pagure/api/fork.py b/pagure/api/fork.py index 42e14bb..ae123a3 100644 --- a/pagure/api/fork.py +++ b/pagure/api/fork.py @@ -21,6 +21,34 @@ from pagure.api import ( ) +@API.route('//pull-request/') +@API.route('/fork///pull-request/') +@api_method +def api_pull_request_view(repo, requestid, username=None): + """ List all issues associated to a repo + """ + + repo = pagure.lib.get_project(SESSION, repo, user=username) + httpcode = 200 + output = {} + + if repo is None: + raise pagure.exceptions.APIError(404, error_code=1) + + if not repo.settings.get('pull_requests', True): + raise pagure.exceptions.APIError(404, error_code=8) + + request = pagure.lib.search_pull_requests( + SESSION, project_id=repo.id, requestid=requestid) + + if not request: + raise pagure.exceptions.APIError(404, error_code=9) + + jsonout = flask.jsonify(request.to_json()) + jsonout.status_code = httpcode + return jsonout + + @API.route('//pull-request//comment', methods=['POST']) @API.route('/fork///pull-request//comment', From 9ba77be21f3948e133092d960a59059ee9328119 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:24 +0000 Subject: [PATCH 69/145] Add unit-tests for the api_pull_request_add_comment endpoint --- diff --git a/tests/__init__.py b/tests/__init__.py index 241cea8..a73bbac 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -259,7 +259,7 @@ def create_tokens(session, user_id=1): def create_acls(session): """ Create some acls for the tokens. """ for acl in [ - 'create_issue', 'update_issue', 'create_pull_request', + 'create_issue', 'update_issue', 'pull_request_comment', 'change_status_issue', 'comment_issue' ]: item = pagure.lib.model.ACL( diff --git a/tests/test_progit_flask_api_fork.py b/tests/test_progit_flask_api_fork.py index 489eb1e..de10a97 100644 --- a/tests/test_progit_flask_api_fork.py +++ b/tests/test_progit_flask_api_fork.py @@ -184,6 +184,117 @@ class PagureFlaskApiForktests(tests.Modeltests): data2['date_created'] = '1431414800' self.assertDictEqual(data, data2) + def test_api_pull_request_add_comment(self): + """ Test the api_pull_request_add_comment method of the flask api. """ + tests.create_projects(self.session) + tests.create_tokens(self.session) + tests.create_acls(self.session) + tests.create_tokens_acl(self.session) + + headers = {'Authorization': 'token aaabbbcccddd'} + + # Invalid project + output = self.app.post( + '/api/0/foo/pull-request/1/comment', headers=headers) + self.assertEqual(output.status_code, 404) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "Project not found", + "error_code": 1 + } + ) + + # Valid token, wrong project + output = self.app.post( + '/api/0/test2/pull-request/1/comment', headers=headers) + self.assertEqual(output.status_code, 401) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "Invalid or expired token. Please visit " \ + "https://pagure.org/ get or renew your API token.", + "error_code": 5 + } + ) + + # No input + output = self.app.post( + '/api/0/test/pull-request/1/comment', headers=headers) + self.assertEqual(output.status_code, 404) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "Pull-Request not found", + "error_code": 9 + } + ) + + # Create a pull-request + repo = pagure.lib.get_project(self.session, 'test') + forked_repo = pagure.lib.get_project(self.session, 'test') + msg = pagure.lib.new_pull_request( + session=self.session, + repo_from=forked_repo, + branch_from='master', + repo_to=repo, + branch_to='master', + title='test pull-request', + user='pingou', + requestfolder=None, + ) + self.session.commit() + self.assertEqual(msg, 'Request created') + + # Check comments before + request = pagure.lib.search_pull_requests( + self.session, project_id=1, requestid=1) + self.assertEqual(len(request.comments), 0) + + data = { + 'title': 'test issue', + } + + # Incomplete request + output = self.app.post( + '/api/0/test/pull-request/1/comment', data=data, headers=headers) + self.assertEqual(output.status_code, 400) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "Invalid or incomplete input submited", + "error_code": 4 + } + ) + + # No change + request = pagure.lib.search_pull_requests( + self.session, project_id=1, requestid=1) + self.assertEqual(len(request.comments), 0) + + data = { + 'comment': 'This is a very interesting question', + } + + # Valid request + output = self.app.post( + '/api/0/test/pull-request/1/comment', data=data, headers=headers) + self.assertEqual(output.status_code, 200) + data = json.loads(output.data) + self.assertDictEqual( + data, + {'message': 'Comment added'} + ) + + # One comment added + request = pagure.lib.search_pull_requests( + self.session, project_id=1, requestid=1) + self.assertEqual(len(request.comments), 1) + if __name__ == '__main__': SUITE = unittest.TestLoader().loadTestsFromTestCase( From 0aaeb155682b49a07199fa88030a9f99c8d6558f Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:24 +0000 Subject: [PATCH 70/145] Move the logic to merge a pull-request into the backend This will allow to re-use it in the API --- diff --git a/pagure/lib/git.py b/pagure/lib/git.py index 313a1c2..ddc7837 100644 --- a/pagure/lib/git.py +++ b/pagure/lib/git.py @@ -732,3 +732,108 @@ def get_username(abspath): if username.startswith('/'): username = username[1:] return username + + +def merge_pull_request(session, repo, request, username, request_folder): + ''' Merge the specified pull-request. + ''' + # Get the fork + repopath = pagure.get_repo_path(request.project_from) + fork_obj = pygit2.Repository(repopath) + + # Get the original repo + parentpath = pagure.get_repo_path(request.project) + + # Clone the original repo into a temp folder + newpath = tempfile.mkdtemp(prefix='pagure-pr-merge') + new_repo = pygit2.clone_repository(parentpath, newpath) + + repo_commit = fork_obj[ + fork_obj.lookup_branch(request.branch_from).get_object().hex] + + ori_remote = new_repo.remotes[0] + # Add the fork as remote repo + reponame = '%s_%s' % (request.user.user, repo.name) + remote = new_repo.create_remote(reponame, repopath) + + # Fetch the commits + remote.fetch() + + merge = new_repo.merge(repo_commit.oid) + if merge is None: + mergecode = new_repo.merge_analysis(repo_commit.oid)[0] + + try: + branch_ref = new_repo.lookup_reference( + request.branch).resolve() + except ValueError: + branch_ref = new_repo.lookup_reference( + 'refs/heads/%s' % request.branch).resolve() + + refname = '%s:%s' % (branch_ref.name, branch_ref.name) + if ( + (merge is not None and merge.is_uptodate) + or + (merge is None and + mergecode & pygit2.GIT_MERGE_ANALYSIS_UP_TO_DATE)): + pagure.lib.close_pull_request( + session, request, username, + requestfolder=request_folder) + try: + session.commit() + except SQLAlchemyError as err: # pragma: no cover + session.rollback() + APP.logger.exception(err) + shutil.rmtree(newpath) + raise pagure.exceptions.PagureException( + 'Could not close this pull-request') + raise pagure.exceptions.PagureException( + 'Nothing to do, changes were already merged') + + elif ( + (merge is not None and merge.is_fastforward) + or + (merge is None and + mergecode & pygit2.GIT_MERGE_ANALYSIS_FASTFORWARD)): + if merge is not None: + # This is depending on the pygit2 version + branch_ref.target = merge.fastforward_oid + elif merge is None and mergecode is not None: + branch_ref.set_target(repo_commit.oid.hex) + + ori_remote.push(refname) + + else: + tree = None + try: + tree = new_repo.index.write_tree() + except pygit2.GitError: + shutil.rmtree(newpath) + raise pagure.exceptions.PagureException('Merge conflicts!') + + head = new_repo.lookup_reference('HEAD').get_object() + new_repo.create_commit( + 'refs/heads/master', + repo_commit.author, + repo_commit.committer, + 'Merge #%s `%s`' % (request.id, request.title), + tree, + [head.hex, repo_commit.oid.hex]) + ori_remote.push(refname) + + # Update status + pagure.lib.close_pull_request( + session, request, username, + requestfolder=request_folder, + ) + try: + session.commit() + except SQLAlchemyError as err: # pragma: no cover + session.rollback() + APP.logger.exception(err) + shutil.rmtree(newpath) + raise pagure.exceptions.PagureException( + 'Could not update this pull-request in the database') + shutil.rmtree(newpath) + + return 'Changes merged!' diff --git a/pagure/ui/fork.py b/pagure/ui/fork.py index 85f871d..217019b 100644 --- a/pagure/ui/fork.py +++ b/pagure/ui/fork.py @@ -502,111 +502,16 @@ def merge_request_pull(repo, requestid, username=None): 'request_pull', username=username, repo=repo.name, requestid=requestid)) - error_output = flask.url_for( - 'request_pull', repo=repo.name, requestid=requestid, - username=username) - - # Get the fork - repopath = pagure.get_repo_path(request.project_from) - fork_obj = pygit2.Repository(repopath) - - # Get the original repo - parentpath = pagure.get_repo_path(request.project) - - # Clone the original repo into a temp folder - newpath = tempfile.mkdtemp(prefix='pagure-pr-merge') - new_repo = pygit2.clone_repository(parentpath, newpath) - - repo_commit = fork_obj[ - fork_obj.lookup_branch(request.branch_from).get_object().hex] - - ori_remote = new_repo.remotes[0] - # Add the fork as remote repo - reponame = '%s_%s' % (request.user.user, repo.name) - remote = new_repo.create_remote(reponame, repopath) - - # Fetch the commits - remote.fetch() - - merge = new_repo.merge(repo_commit.oid) - if merge is None: - mergecode = new_repo.merge_analysis(repo_commit.oid)[0] - - try: - branch_ref = new_repo.lookup_reference( - request.branch).resolve() - except ValueError: - branch_ref = new_repo.lookup_reference( - 'refs/heads/%s' % request.branch).resolve() - - refname = '%s:%s' % (branch_ref.name, branch_ref.name) - if ( - (merge is not None and merge.is_uptodate) - or - (merge is None and - mergecode & pygit2.GIT_MERGE_ANALYSIS_UP_TO_DATE)): - flask.flash('Nothing to do, changes were already merged', 'error') - pagure.lib.close_pull_request( - SESSION, request, flask.g.fas_user.username, - requestfolder=APP.config['REQUESTS_FOLDER']) - try: - SESSION.commit() - except SQLAlchemyError as err: # pragma: no cover - SESSION.rollback() - APP.logger.exception(err) - flask.flash('Could not close this pull-request', 'error') - return flask.redirect(error_output) - elif ( - (merge is not None and merge.is_fastforward) - or - (merge is None and - mergecode & pygit2.GIT_MERGE_ANALYSIS_FASTFORWARD)): - if merge is not None: - # This is depending on the pygit2 version - branch_ref.target = merge.fastforward_oid - elif merge is None and mergecode is not None: - branch_ref.set_target(repo_commit.oid.hex) - - ori_remote.push(refname) - flask.flash('Changes merged!') - - else: - tree = None - try: - tree = new_repo.index.write_tree() - except pygit2.GitError: - shutil.rmtree(newpath) - flask.flash('Merge conflicts!', 'error') - return flask.redirect(flask.url_for( - 'request_pull', - repo=repo.name, - username=username, - requestid=requestid)) - head = new_repo.lookup_reference('HEAD').get_object() - new_repo.create_commit( - 'refs/heads/master', - repo_commit.author, - repo_commit.committer, - 'Merge #%s `%s`' % (request.id, request.title), - tree, - [head.hex, repo_commit.oid.hex]) - ori_remote.push(refname) - flask.flash('Changes merged!') - - # Update status - pagure.lib.close_pull_request( - SESSION, request, flask.g.fas_user.username, - requestfolder=APP.config['REQUESTS_FOLDER'], - ) try: - SESSION.commit() - except SQLAlchemyError as err: # pragma: no cover - SESSION.rollback() - APP.logger.exception(err) - flask.flash( - 'Could not update this pull-request in the database', - 'error') - shutil.rmtree(newpath) + message = pagure.lib.git.merge_pull_request( + SESSION, repo, request, flask.g.fas_user.username, + APP.config['REQUESTS_FOLDER']) + flask.flash(message) + except pagure.exceptions.PagureException as err: + flask.flash(str(err), 'error') + return flask.redirect(flask.url_for( + 'request_pull', repo=repo.name, requestid=requestid, + username=username)) return flask.redirect(flask.url_for('view_repo', repo=repo.name)) From 29162615cdb295cfaade5f1eef97f7394a524e15 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:24 +0000 Subject: [PATCH 71/145] Add API endpoint to close a pull-request without merging it --- diff --git a/pagure/api/fork.py b/pagure/api/fork.py index ae123a3..350b8f5 100644 --- a/pagure/api/fork.py +++ b/pagure/api/fork.py @@ -49,6 +49,55 @@ def api_pull_request_view(repo, requestid, username=None): return jsonout +@API.route('//pull-request//close', methods=['POST']) +@API.route('/fork///pull-request//close', + methods=['POST']) +@api_login_required(acls=['pull_request_close']) +@api_method +def api_pull_request_close(repo, issueid, username=None): + """ Close a pull-request without merging it + """ + repo = pagure.lib.get_project(SESSION, repo, user=username) + httpcode = 200 + output = {} + + repo = pagure.lib.get_project(SESSION, repo, user=username) + + if repo is None: + raise pagure.exceptions.APIError(404, error_code=1) + + if not repo.settings.get('pull_requests', True): + raise pagure.exceptions.APIError(404, error_code=8) + + if repo != flask.g.token.project: + raise pagure.exceptions.APIError(401, error_code=5) + + request = pagure.lib.search_pull_requests( + SESSION, project_id=repo.id, requestid=requestid) + + if not request: + raise pagure.exceptions.APIError(404, error_code=9) + + if not is_repo_admin(repo): + raise pagure.exceptions.APIError(403, error_code=10) + + pagure.lib.close_pull_request( + SESSION, request, flask.g.fas_user.username, + requestfolder=APP.config['REQUESTS_FOLDER'], + merged=False) + try: + SESSION.commit() + output['message'] = 'Request pull canceled!' + except SQLAlchemyError as err: # pragma: no cover + SESSION.rollback() + APP.logger.exception(err) + raise pagure.exceptions.APIError(400, error_code=3) + + jsonout = flask.jsonify(output) + jsonout.status_code = httpcode + return jsonout + + @API.route('//pull-request//comment', methods=['POST']) @API.route('/fork///pull-request//comment', From 807fceb588f1945478e54af1cb721be6df5504fa Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:24 +0000 Subject: [PATCH 72/145] Add API endpoint to merge a pull-request --- diff --git a/pagure/api/fork.py b/pagure/api/fork.py index 350b8f5..48dc052 100644 --- a/pagure/api/fork.py +++ b/pagure/api/fork.py @@ -49,6 +49,63 @@ def api_pull_request_view(repo, requestid, username=None): return jsonout +@API.route('//pull-request//merge', methods=['POST']) +@API.route('/fork///pull-request//merge', + methods=['POST']) +@api_login_required(acls=['pull_request_merge']) +@api_method +def api_pull_request_merge(repo, issueid, username=None): + """ Merge a pull-request + """ + repo = pagure.lib.get_project(SESSION, repo, user=username) + httpcode = 200 + output = {} + + repo = pagure.lib.get_project(SESSION, repo, user=username) + + if repo is None: + raise pagure.exceptions.APIError(404, error_code=1) + + if not repo.settings.get('pull_requests', True): + raise pagure.exceptions.APIError(404, error_code=8) + + if repo != flask.g.token.project: + raise pagure.exceptions.APIError(401, error_code=5) + + request = pagure.lib.search_pull_requests( + SESSION, project_id=repo.id, requestid=requestid) + + if not request: + raise pagure.exceptions.APIError(404, error_code=9) + + if not is_repo_admin(repo): + raise pagure.exceptions.APIError(403, error_code=10) + + if repo.settings.get('Only_assignee_can_merge_pull-request', False): + if not request.assignee: + raise pagure.exceptions.APIError(403, error_code=13) + + if request.assignee.username != flask.g.fas_user.username: + raise pagure.exceptions.APIError(403, error_code=12) + + threshold = repo.settings.get('Minimum_score_to_merge_pull-request', -1) + if threshold > 0 and int(request.score) < int(threshold): + raise pagure.exceptions.APIError(403, error_code=11) + + try: + message = pagure.lib.git.merge_pull_request( + SESSION, repo, request, flask.g.fas_user.username, + APP.config['REQUESTS_FOLDER']) + output['message'] = message + except pagure.exceptions.PagureException as err: + raise pagure.exceptions.APIError( + 400, error_code=0, error=str(err)) + + jsonout = flask.jsonify(output) + jsonout.status_code = httpcode + return jsonout + + @API.route('//pull-request//close', methods=['POST']) @API.route('/fork///pull-request//close', methods=['POST']) From ee09232584581e8bc704f71a73451af4b13bfacc Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:24 +0000 Subject: [PATCH 73/145] Add the new API error codes --- diff --git a/pagure/api/__init__.py b/pagure/api/__init__.py index b7700d4..2d0aea6 100644 --- a/pagure/api/__init__.py +++ b/pagure/api/__init__.py @@ -36,6 +36,11 @@ API_ERROR_CODE = { 7: 'You are not allowed to view this issue', 8: 'Pull-Request have been deactivated for this project', 9: 'Pull-Request not found', + 10: 'You are not allowed to merge pull-request for this project', + 11: 'This request does not have the minimum review score necessary to ' + 'be merged', + 12: 'Only the assignee can merge this review', + 13: 'This request must be assigned to be merged', } From 29ee20fd68932e5a3a59374378d85c4c6e1b0813 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:24 +0000 Subject: [PATCH 74/145] More ACLs to the token used in the tests --- diff --git a/tests/__init__.py b/tests/__init__.py index a73bbac..2c88da2 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -259,8 +259,8 @@ def create_tokens(session, user_id=1): def create_acls(session): """ Create some acls for the tokens. """ for acl in [ - 'create_issue', 'update_issue', 'pull_request_comment', - 'change_status_issue', 'comment_issue' + 'create_issue', 'pull_request_merge', 'pull_request_comment', + 'change_status_issue', 'comment_issue', 'pull_request_close', ]: item = pagure.lib.model.ACL( name=acl, From 3d55b301f9c184827c5a0d32f827a77a5928076d Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:24 +0000 Subject: [PATCH 75/145] Fix the api_pull_request_merge and api_pull_request_close methods --- diff --git a/pagure/api/fork.py b/pagure/api/fork.py index 48dc052..83fa17f 100644 --- a/pagure/api/fork.py +++ b/pagure/api/fork.py @@ -54,10 +54,9 @@ def api_pull_request_view(repo, requestid, username=None): methods=['POST']) @api_login_required(acls=['pull_request_merge']) @api_method -def api_pull_request_merge(repo, issueid, username=None): +def api_pull_request_merge(repo, requestid, username=None): """ Merge a pull-request """ - repo = pagure.lib.get_project(SESSION, repo, user=username) httpcode = 200 output = {} @@ -111,10 +110,9 @@ def api_pull_request_merge(repo, issueid, username=None): methods=['POST']) @api_login_required(acls=['pull_request_close']) @api_method -def api_pull_request_close(repo, issueid, username=None): +def api_pull_request_close(repo, requestid, username=None): """ Close a pull-request without merging it """ - repo = pagure.lib.get_project(SESSION, repo, user=username) httpcode = 200 output = {} From b0785b6e0192157a27383378eded8dd491bd867f Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:24 +0000 Subject: [PATCH 76/145] Ensure the new ACLs is associated with the test token --- diff --git a/tests/__init__.py b/tests/__init__.py index 2c88da2..1e44408 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -273,7 +273,7 @@ def create_acls(session): def create_tokens_acl(session): """ Create some acls for the tokens. """ - for aclid in range(5): + for aclid in range(6): item = pagure.lib.model.TokenAcl( token_id='aaabbbcccddd', acl_id=aclid + 1, From 209c7ef97a41771b755287fd59ea5a0f5bdc2f58 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:24 +0000 Subject: [PATCH 77/145] Add unit-tests for the api_pull_request_close endpoint --- diff --git a/tests/test_progit_flask_api_fork.py b/tests/test_progit_flask_api_fork.py index de10a97..9e4793b 100644 --- a/tests/test_progit_flask_api_fork.py +++ b/tests/test_progit_flask_api_fork.py @@ -184,6 +184,68 @@ class PagureFlaskApiForktests(tests.Modeltests): data2['date_created'] = '1431414800' self.assertDictEqual(data, data2) + def test_api_pull_request_close(self): + """ Test the api_pull_request_close method of the flask api. """ + tests.create_projects(self.session) + tests.create_tokens(self.session) + tests.create_acls(self.session) + tests.create_tokens_acl(self.session) + + # Create the pull-request to close + repo = pagure.lib.get_project(self.session, 'test') + forked_repo = pagure.lib.get_project(self.session, 'test') + msg = pagure.lib.new_pull_request( + session=self.session, + repo_from=forked_repo, + branch_from='master', + repo_to=repo, + branch_to='master', + title='test pull-request', + user='pingou', + requestfolder=None, + ) + self.session.commit() + self.assertEqual(msg, 'Request created') + + headers = {'Authorization': 'token aaabbbcccddd'} + + # Invalid project + output = self.app.post( + '/api/0/foo/pull-request/1/close', headers=headers) + self.assertEqual(output.status_code, 404) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "Project not found", + "error_code": 1 + } + ) + + # Valid token, wrong project + output = self.app.post( + '/api/0/test2/pull-request/1/close', headers=headers) + self.assertEqual(output.status_code, 401) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "Invalid or expired token. Please visit " \ + "https://pagure.org/ get or renew your API token.", + "error_code": 5 + } + ) + + # Close PR + output = self.app.post( + '/api/0/test/pull-request/1/close', headers=headers) + self.assertEqual(output.status_code, 200) + data = json.loads(output.data) + self.assertDictEqual( + data, + {"message": "Request pull canceled!"} + ) + def test_api_pull_request_add_comment(self): """ Test the api_pull_request_add_comment method of the flask api. """ tests.create_projects(self.session) From a9c964f831fe3359a4871f83cb08907b6334303c Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:24 +0000 Subject: [PATCH 78/145] Add unit-tests for the api_pull_request_merge endpoint --- diff --git a/tests/test_progit_flask_api_fork.py b/tests/test_progit_flask_api_fork.py index 9e4793b..a5fa82c 100644 --- a/tests/test_progit_flask_api_fork.py +++ b/tests/test_progit_flask_api_fork.py @@ -246,6 +246,71 @@ class PagureFlaskApiForktests(tests.Modeltests): {"message": "Request pull canceled!"} ) + @patch('pagure.lib.git.merge_pull_request') + def test_api_pull_request_merge(self, mpr): + """ Test the api_pull_request_merge method of the flask api. """ + mpr.return_value = 'Changes merged!' + + tests.create_projects(self.session) + tests.create_tokens(self.session) + tests.create_acls(self.session) + tests.create_tokens_acl(self.session) + + # Create the pull-request to close + repo = pagure.lib.get_project(self.session, 'test') + forked_repo = pagure.lib.get_project(self.session, 'test') + msg = pagure.lib.new_pull_request( + session=self.session, + repo_from=forked_repo, + branch_from='master', + repo_to=repo, + branch_to='master', + title='test pull-request', + user='pingou', + requestfolder=None, + ) + self.session.commit() + self.assertEqual(msg, 'Request created') + + headers = {'Authorization': 'token aaabbbcccddd'} + + # Invalid project + output = self.app.post( + '/api/0/foo/pull-request/1/merge', headers=headers) + self.assertEqual(output.status_code, 404) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "Project not found", + "error_code": 1 + } + ) + + # Valid token, wrong project + output = self.app.post( + '/api/0/test2/pull-request/1/merge', headers=headers) + self.assertEqual(output.status_code, 401) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "Invalid or expired token. Please visit " \ + "https://pagure.org/ get or renew your API token.", + "error_code": 5 + } + ) + + # Close PR + output = self.app.post( + '/api/0/test/pull-request/1/merge', headers=headers) + self.assertEqual(output.status_code, 200) + data = json.loads(output.data) + self.assertDictEqual( + data, + {"message": "Changes merged!"} + ) + def test_api_pull_request_add_comment(self): """ Test the api_pull_request_add_comment method of the flask api. """ tests.create_projects(self.session) From f12b4aebe9080b15e0e4742643f1ee016605bab7 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:24 +0000 Subject: [PATCH 79/145] Expand the unit-tests for closing/merging a pull-request and adjust the error message --- diff --git a/pagure/api/__init__.py b/pagure/api/__init__.py index 2d0aea6..c2af9a7 100644 --- a/pagure/api/__init__.py +++ b/pagure/api/__init__.py @@ -36,7 +36,7 @@ API_ERROR_CODE = { 7: 'You are not allowed to view this issue', 8: 'Pull-Request have been deactivated for this project', 9: 'Pull-Request not found', - 10: 'You are not allowed to merge pull-request for this project', + 10: 'You are not allowed to merge/close pull-request for this project', 11: 'This request does not have the minimum review score necessary to ' 'be merged', 12: 'Only the assignee can merge this review', diff --git a/tests/test_progit_flask_api_fork.py b/tests/test_progit_flask_api_fork.py index a5fa82c..e9b960c 100644 --- a/tests/test_progit_flask_api_fork.py +++ b/tests/test_progit_flask_api_fork.py @@ -236,6 +236,51 @@ class PagureFlaskApiForktests(tests.Modeltests): } ) + # Invalid PR + output = self.app.post( + '/api/0/test/pull-request/2/close', headers=headers) + self.assertEqual(output.status_code, 404) + data = json.loads(output.data) + self.assertDictEqual( + data, + {'error': 'Pull-Request not found', 'error_code': 9} + ) + + # Create a token for foo for this project + item = pagure.lib.model.Token( + id='foobar_token', + user_id=2, + project_id=1, + expiration=datetime.datetime.utcnow() + datetime.timedelta( + days=30) + ) + self.session.add(item) + self.session.commit() + item = pagure.lib.model.TokenAcl( + token_id='foobar_token', + acl_id=6, + ) + self.session.add(item) + self.session.commit() + + headers = {'Authorization': 'token foobar_token'} + + # User not admin + output = self.app.post( + '/api/0/test/pull-request/1/close', headers=headers) + self.assertEqual(output.status_code, 403) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + 'error': 'You are not allowed to merge/close pull-request ' + 'for this project', + 'error_code': 10 + } + ) + + headers = {'Authorization': 'token aaabbbcccddd'} + # Close PR output = self.app.post( '/api/0/test/pull-request/1/close', headers=headers) @@ -301,7 +346,52 @@ class PagureFlaskApiForktests(tests.Modeltests): } ) - # Close PR + # Invalid PR + output = self.app.post( + '/api/0/test/pull-request/2/merge', headers=headers) + self.assertEqual(output.status_code, 404) + data = json.loads(output.data) + self.assertDictEqual( + data, + {'error': 'Pull-Request not found', 'error_code': 9} + ) + + # Create a token for foo for this project + item = pagure.lib.model.Token( + id='foobar_token', + user_id=2, + project_id=1, + expiration=datetime.datetime.utcnow() + datetime.timedelta( + days=30) + ) + self.session.add(item) + self.session.commit() + item = pagure.lib.model.TokenAcl( + token_id='foobar_token', + acl_id=2, + ) + self.session.add(item) + self.session.commit() + + headers = {'Authorization': 'token foobar_token'} + + # User not admin + output = self.app.post( + '/api/0/test/pull-request/1/merge', headers=headers) + self.assertEqual(output.status_code, 403) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + 'error': 'You are not allowed to merge/close pull-request ' + 'for this project', + 'error_code': 10 + } + ) + + headers = {'Authorization': 'token aaabbbcccddd'} + + # Merge PR output = self.app.post( '/api/0/test/pull-request/1/merge', headers=headers) self.assertEqual(output.status_code, 200) From b0400aac5d329054665ebd99eabe4b9de3c9cd47 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:24 +0000 Subject: [PATCH 80/145] Adjust the token ACL to be issue_* and pull_request_* --- diff --git a/pagure/api/issue.py b/pagure/api/issue.py index f46e392..caf27c6 100644 --- a/pagure/api/issue.py +++ b/pagure/api/issue.py @@ -23,7 +23,7 @@ from pagure.api import ( @API.route('//new_issue', methods=['POST']) @API.route('/fork///new_issue', methods=['POST']) -@api_login_required(acls=['create_issue']) +@api_login_required(acls=['issue_create']) @api_method def api_new_issue(repo, username=None): """ Create a new issue @@ -133,7 +133,7 @@ def api_view_issue(repo, issueid, username=None): @API.route('//issue//status', methods=['POST']) @API.route('/fork////status', methods=['POST']) -@api_login_required(acls=['change_status_issue']) +@api_login_required(acls=['issue_change_status']) @api_method def api_change_status_issue(repo, issueid, username=None): """ Change the status of an issue @@ -195,7 +195,7 @@ def api_change_status_issue(repo, issueid, username=None): @API.route('//issue//comment', methods=['POST']) @API.route('/fork////comment', methods=['POST']) -@api_login_required(acls=['comment_issue']) +@api_login_required(acls=['issue_comment']) @api_method def api_comment_issue(repo, issueid, username=None): """ Add a comment to an issue diff --git a/tests/__init__.py b/tests/__init__.py index 1e44408..0e6ad63 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -259,8 +259,8 @@ def create_tokens(session, user_id=1): def create_acls(session): """ Create some acls for the tokens. """ for acl in [ - 'create_issue', 'pull_request_merge', 'pull_request_comment', - 'change_status_issue', 'comment_issue', 'pull_request_close', + 'issue_create', 'pull_request_merge', 'pull_request_comment', + 'issue_change_status', 'issue_comment', 'pull_request_close', ]: item = pagure.lib.model.ACL( name=acl, From dea3d8a7dc4e6e1781e38c6765565b5212fb8fb0 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:24 +0000 Subject: [PATCH 81/145] It's APP.logger not APP.log --- diff --git a/pagure/api/__init__.py b/pagure/api/__init__.py index c2af9a7..1907b7e 100644 --- a/pagure/api/__init__.py +++ b/pagure/api/__init__.py @@ -133,7 +133,7 @@ def api_method(function): result = function(*args, **kwargs) except APIError as e: if e.error_code in [3]: - APP.log.exception(e) + APP.logger.exception(e) if e.error_code in [0]: response = flask.jsonify( From 1f6bd5001ecba6e2e5d8ffe4a628b749830e75ab Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:24 +0000 Subject: [PATCH 82/145] Figure a way to populate the ACL table from the configuration file by running createdb.py --- diff --git a/createdb.py b/createdb.py index c7a00ce..eb78c78 100644 --- a/createdb.py +++ b/createdb.py @@ -10,4 +10,5 @@ from pagure.lib import model model.create_tables( APP.config['DB_URL'], APP.config.get('PATH_ALEMBIC_INI', None), + acls=APP.config.get('ACLS', {}), debug=True) diff --git a/pagure/lib/model.py b/pagure/lib/model.py index a19d618..cb68650 100644 --- a/pagure/lib/model.py +++ b/pagure/lib/model.py @@ -31,7 +31,7 @@ ERROR_LOG = logging.getLogger('pagure.model') # pylint: disable=C0103,R0903,W0232,E1101 -def create_tables(db_url, alembic_ini=None, debug=False): +def create_tables(db_url, alembic_ini=None, acls=None, debug=False): """ Create the tables in the database using the information from the url obtained. @@ -72,11 +72,11 @@ def create_tables(db_url, alembic_ini=None, debug=False): scopedsession = scoped_session(sessionmaker(bind=engine)) # Insert the default data into the db - create_default_status(scopedsession) + create_default_status(scopedsession, acls=acls) return scopedsession -def create_default_status(session): +def create_default_status(session, acls=None): """ Insert the defaults status in the status tables. """ @@ -98,6 +98,18 @@ def create_default_status(session): session.rollback() ERROR_LOG.debug('Type %s could not be added', grptype) + for acl in acls or {}: + item = ACL( + name=acl, + description=acls[acl] + ) + session.add(item) + try: + session.commit() + except SQLAlchemyError: # pragma: no cover + session.rollback() + ERROR_LOG.debug('ACL %s could not be added', acl) + class StatusIssue(BASE): """ Stores the status a ticket can have. From adc5dd0b2b18b82baea81dcc2c09ebdd8cefb0a7 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:24 +0000 Subject: [PATCH 83/145] Define the default ACL and their description in the default configuration file --- diff --git a/pagure/default_config.py b/pagure/default_config.py index e576b4c..52f2b23 100644 --- a/pagure/default_config.py +++ b/pagure/default_config.py @@ -137,3 +137,12 @@ APPLICATION_ROOT = '/' # List of blacklisted project names BLACKLISTED_PROJECTS = ['static', 'pv'] + +ACLS = { + 'issue_create': 'Create a new ticket against this project', + 'issue_change_status': 'Change the status of a ticket of this project', + 'issue_comment': 'Comment on a ticket of this project', + 'pull_request_merge': 'Merge a pull-request of this project', + 'pull_request_close': 'Close a pull-request of this project', + 'pull_request_comment': 'Comment on a pull-request of this project', +} From 112bd1bfdb46fa50229d73e61297fd224fd6aa24 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:24 +0000 Subject: [PATCH 84/145] Add a method to list all the ACLs in the database in the backend library --- diff --git a/pagure/lib/__init__.py b/pagure/lib/__init__.py index 848fc7e..25ddbd9 100644 --- a/pagure/lib/__init__.py +++ b/pagure/lib/__init__.py @@ -2037,3 +2037,16 @@ def get_api_token(session, token_str): ) return query.first() + + +def get_acls(session): + """ Returns all the possible ACLs a token can have according to the + database. + """ + query = session.query( + model.ACL + ).order_by( + model.ACL.name + ) + + return query.all() From 0b80a39214328ff31e99fa8da4912624ea65cc02 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:24 +0000 Subject: [PATCH 85/145] Add method to create a new token for an user in a project to the backend library --- diff --git a/pagure/lib/__init__.py b/pagure/lib/__init__.py index 25ddbd9..7137d17 100644 --- a/pagure/lib/__init__.py +++ b/pagure/lib/__init__.py @@ -2050,3 +2050,36 @@ def get_acls(session): ) return query.all() + + +def add_token_to_user(session, project, acls, username): + """ Create a new token for the specified user on the specified project + with the given ACLs. + """ + acls_obj = session.query( + model.ACL + ).filter( + model.ACL.name.in_(acls) + ).all() + + user = search_user(session, username=username) + + token = pagure.lib.model.Token( + id=pagure.lib.login.id_generator(64), + user_id=user.id, + project_id=project.id, + expiration=datetime.datetime.utcnow() + datetime.timedelta(days=60) + ) + session.add(token) + session.flush() + + for acl in acls_obj: + item = pagure.lib.model.TokenAcl( + token_id=token.id, + acl_id=acl.id, + ) + session.add(item) + + session.commit() + + return 'Token created' From 110198c8858562c4a6da08c2aa5714a8f01ffb6c Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:24 +0000 Subject: [PATCH 86/145] Create new endpoint to create the token of an user on a project --- diff --git a/pagure/templates/add_token.html b/pagure/templates/add_token.html new file mode 100644 index 0000000..6c397a0 --- /dev/null +++ b/pagure/templates/add_token.html @@ -0,0 +1,36 @@ +{% extends "master.html" %} +{% from "_formhelper.html" import render_field_in_row %} + +{% block title %}Create token{% endblock %} +{%block tag %}home{% endblock %} + + +{% block content %} + +

Create a new token

+ +
+
+ + + {% for acl in acls %} + + + + + {% endfor %} +
+ + {{ acl.description }}
+

+ + + {{ form.csrf_token }} +

+
+
+ +{% endblock %} diff --git a/pagure/ui/repo.py b/pagure/ui/repo.py index e396789..2900ca1 100644 --- a/pagure/ui/repo.py +++ b/pagure/ui/repo.py @@ -986,3 +986,51 @@ def regenerate_git(repo, username=None): return flask.redirect( flask.url_for('.view_settings', repo=repo.name, username=username) ) + + +@APP.route('//token/new', methods=('GET', 'POST')) +@APP.route('/fork///token/new', methods=('GET', 'POST')) +@cla_required +def add_token(repo, username=None): + """ Add a token to a specified project. + """ + if admin_session_timedout(): + return flask.redirect( + flask.url_for('auth_login', next=flask.request.url)) + + repo = pagure.lib.get_project(SESSION, repo, user=username) + + if not repo: + flask.abort(404, 'Project not found') + + acls = pagure.lib.get_acls(SESSION) + form = pagure.forms.NewTokenForm(acls=acls) + + print flask.request.form + if form.validate_on_submit(): + print form.acls.data + try: + msg = pagure.lib.add_token_to_user( + SESSION, + repo, + acls=form.acls.data, + username=flask.g.fas_user.username, + ) + SESSION.commit() + flask.flash(msg) + return flask.redirect( + flask.url_for( + '.view_settings', repo=repo.name, username=username) + ) + except SQLAlchemyError as err: # pragma: no cover + SESSION.rollback() + APP.logger.exception(err) + flask.flash('User could not be added', 'error') + + return flask.render_template( + 'add_token.html', + form=form, + acls=acls, + username=username, + repo=repo, + ) From bd5f16da31c4b37193ae963b2ef4b3ed54fcedcc Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:24 +0000 Subject: [PATCH 87/145] Add the NewTokenForm --- diff --git a/pagure/forms.py b/pagure/forms.py index aeedf7f..587668d 100644 --- a/pagure/forms.py +++ b/pagure/forms.py @@ -92,6 +92,26 @@ class StatusForm(wtf.Form): ] +class NewTokenForm(wtf.Form): + ''' Form to add/change the status of an issue. ''' + acls = wtforms.SelectMultipleField( + 'ACLs', + [wtforms.validators.Required()], + choices=[(item, item) for item in []] + ) + + def __init__(self, *args, **kwargs): + """ Calls the default constructor with the normal argument but + uses the list of collection provided to fill the choices of the + drop-down list. + """ + super(NewTokenForm, self).__init__(*args, **kwargs) + if 'acls' in kwargs: + self.acls.choices = [ + (acl.name, acl.name) for acl in kwargs['acls'] + ] + + class UpdateIssueForm(wtf.Form): ''' Form to add a comment to an issue. ''' tag = wtforms.TextField( From ce3c2cbf09c4d0308d114d5f36d5e11adbd3149f Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 22 2015 08:49:24 +0000 Subject: [PATCH 88/145] Add UI to create a token of a project on the project's settings page --- diff --git a/pagure/templates/settings.html b/pagure/templates/settings.html index 70b1ff7..7f1dc33 100644 --- a/pagure/templates/settings.html +++ b/pagure/templates/settings.html @@ -54,6 +54,51 @@ +
+

API key

+

+ API keys are tokens used to authenticate you on pagure. They can also + be used to grant access to 3rd party application to behave on this + project on your name. +

+

+ These keys are valid for 60 days. +

+

+ These keys are private to your project, make sure to store in a safe + place and do not share it. +

+ + {% if repo.tokens %} + + {% for token in repo.tokens %} + {% if token.user.username == g.fas_user.username %} + + + + + + {% endif %} + {% endfor %} +
+ {{ token.id }} + + valid until: {{ token.expiration.date() }} + + ACLs +
+ {% endif %} + + + + +
+

Project's options

{{ plugin }}
{% endfor %} - {% endif %} @@ -230,6 +274,25 @@ {% block jscripts %} {{ super() }}