From 9e5ef7ed0f512e953951075010d86d9a5cd3e88a Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Dec 22 2016 14:07:17 +0000 Subject: [PATCH 1/12] Move the regex to validate an URL to the pagure module so it can easily be re-used --- diff --git a/pagure/__init__.py b/pagure/__init__.py index 488f1ce..493a4c1 100644 --- a/pagure/__init__.py +++ b/pagure/__init__.py @@ -19,6 +19,7 @@ __api_version__ = '0.10' import datetime import logging import os +import re import subprocess import urlparse from logging.handlers import SMTPHandler @@ -608,6 +609,52 @@ def get_remote_repo_path(remote_git, branch_from, loop=False): return repopath +ip_middle_octet = u"(?:\.(?:1?\d{1,2}|2[0-4]\d|25[0-5]))" +ip_last_octet = u"(?:\.(?:[1-9]\d?|1\d\d|2[0-4]\d|25[0-4]))" + +""" +regex based on https://github.com/kvesteri/validators/blob/master/validators/url.py +""" +urlregex = re.compile( + u"^" + # protocol identifier + u"(?:(?:https?|ftp)://)" + # user:pass authentication + u"(?:\S+(?::\S*)?@)?" + u"(?:" + u"(?P" + # IP address exclusion + # private & local networks + u"(?:(?:10|127)" + ip_middle_octet + u"{2}" + ip_last_octet + u")|" + u"(?:(?:169\.254|192\.168)" + ip_middle_octet + ip_last_octet + u")|" + u"(?:172\.(?:1[6-9]|2\d|3[0-1])" + ip_middle_octet + ip_last_octet + u"))" + u"|" + # IP address dotted notation octets + # excludes loopback network 0.0.0.0 + # excludes reserved space >= 224.0.0.0 + # excludes network & broadcast addresses + # (first & last IP address of each class) + u"(?P" + u"(?:[1-9]\d?|1\d\d|2[01]\d|22[0-3])" + u"" + ip_middle_octet + u"{2}" + u"" + ip_last_octet + u")" + u"|" + # host name + u"(?:(?:[a-z\u00a1-\uffff0-9]-?)*[a-z\u00a1-\uffff0-9]+)" + # domain name + u"(?:\.(?:[a-z\u00a1-\uffff0-9]-?)*[a-z\u00a1-\uffff0-9]+)*" + # TLD identifier + u"(?:\.(?:[a-z\u00a1-\uffff]{2,}))" + u")" + # port number + u"(?::\d{2,5})?" + # resource path + u"(?:/\S*)?" + u"$", + re.UNICODE | re.IGNORECASE +) +urlpattern = re.compile(urlregex) + # Import the application import pagure.ui.app import pagure.ui.admin diff --git a/pagure/ui/issues.py b/pagure/ui/issues.py index 9f9fb23..0aad641 100644 --- a/pagure/ui/issues.py +++ b/pagure/ui/issues.py @@ -26,7 +26,6 @@ from binaryornot.helpers import is_binary_string import kitchen.text.converters as ktc import mimetypes -import re import pagure.doc_utils import pagure.exceptions @@ -34,54 +33,7 @@ import pagure.lib import pagure.lib.encoding_utils import pagure.forms from pagure import (APP, SESSION, LOG, __get_file_in_tree, - login_required, authenticated) - - -ip_middle_octet = u"(?:\.(?:1?\d{1,2}|2[0-4]\d|25[0-5]))" -ip_last_octet = u"(?:\.(?:[1-9]\d?|1\d\d|2[0-4]\d|25[0-4]))" - -""" -regex based on https://github.com/kvesteri/validators/blob/master/validators/url.py -""" -urlregex = re.compile( - u"^" - # protocol identifier - u"(?:(?:https?|ftp)://)" - # user:pass authentication - u"(?:\S+(?::\S*)?@)?" - u"(?:" - u"(?P" - # IP address exclusion - # private & local networks - u"(?:(?:10|127)" + ip_middle_octet + u"{2}" + ip_last_octet + u")|" - u"(?:(?:169\.254|192\.168)" + ip_middle_octet + ip_last_octet + u")|" - u"(?:172\.(?:1[6-9]|2\d|3[0-1])" + ip_middle_octet + ip_last_octet + u"))" - u"|" - # IP address dotted notation octets - # excludes loopback network 0.0.0.0 - # excludes reserved space >= 224.0.0.0 - # excludes network & broadcast addresses - # (first & last IP address of each class) - u"(?P" - u"(?:[1-9]\d?|1\d\d|2[01]\d|22[0-3])" - u"" + ip_middle_octet + u"{2}" - u"" + ip_last_octet + u")" - u"|" - # host name - u"(?:(?:[a-z\u00a1-\uffff0-9]-?)*[a-z\u00a1-\uffff0-9]+)" - # domain name - u"(?:\.(?:[a-z\u00a1-\uffff0-9]-?)*[a-z\u00a1-\uffff0-9]+)*" - # TLD identifier - u"(?:\.(?:[a-z\u00a1-\uffff]{2,}))" - u")" - # port number - u"(?::\d{2,5})?" - # resource path - u"(?:/\S*)?" - u"$", - re.UNICODE | re.IGNORECASE -) -urlpattern = re.compile(urlregex) + login_required, authenticated, urlpattern) # URLs From 6f8dc22ffd6abb3ffaa29db3c0919b9d404f1900 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Dec 22 2016 14:07:17 +0000 Subject: [PATCH 2/12] Create an ``issue_update`` ACLs allowing to do multiple type of updates at once --- diff --git a/pagure/api/issue.py b/pagure/api/issue.py index 41ed130..42b6685 100644 --- a/pagure/api/issue.py +++ b/pagure/api/issue.py @@ -536,7 +536,7 @@ def api_view_issue_comment( @API.route( '/fork////issue//status', methods=['POST']) -@api_login_required(acls=['issue_change_status']) +@api_login_required(acls=['issue_change_status', 'issue_update']) @api_method def api_change_status_issue(repo, issueid, username=None, namespace=None): """ @@ -659,7 +659,7 @@ def api_change_status_issue(repo, issueid, username=None, namespace=None): @API.route( '/fork////issue//comment', methods=['POST']) -@api_login_required(acls=['issue_comment']) +@api_login_required(acls=['issue_comment', 'issue_update']) @api_method def api_comment_issue(repo, issueid, username=None, namespace=None): """ @@ -758,7 +758,7 @@ def api_comment_issue(repo, issueid, username=None, namespace=None): @API.route( '/fork////issue//assign', methods=['POST']) -@api_login_required(acls=['issue_assign']) +@api_login_required(acls=['issue_assign', 'issue_update']) @api_method def api_assign_issue(repo, issueid, username=None, namespace=None): """ diff --git a/pagure/default_config.py b/pagure/default_config.py index edb8302..da90503 100644 --- a/pagure/default_config.py +++ b/pagure/default_config.py @@ -217,6 +217,7 @@ ACLS = { 'pull_request_flag': 'Flag a pull-request of this project', 'pull_request_merge': 'Merge a pull-request of this project', 'issue_subscribe': 'Subscribe the user with this token to an issue', + 'issue_update': 'Update an issue, status, comments, custom fields...', } # Bootstrap URLS From 7ef397a0b6a38d5b16328a1ca8dc93d23bc71f1c Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Dec 22 2016 14:07:18 +0000 Subject: [PATCH 3/12] Add a new API endpoint to set or reset the custom field of an issue Fixes https://pagure.io/pagure/issue/1607 --- diff --git a/pagure/api/__init__.py b/pagure/api/__init__.py index eb591fe..3bc2f23 100644 --- a/pagure/api/__init__.py +++ b/pagure/api/__init__.py @@ -80,6 +80,9 @@ class APIERROR(enum.Enum): 'instance' ETIMESTAMP = 'Invalid timestamp format' EDATETIME = 'Invalid datetime format' + EINVALIDISSUEFIELD = 'Invalid custom field submitted' + EINVALIDISSUEFIELD_LINK = 'Invalid custom field submitted, the value '\ + 'is not a link' def check_api_acls(acls, optional=False): diff --git a/pagure/api/issue.py b/pagure/api/issue.py index 42b6685..78eea11 100644 --- a/pagure/api/issue.py +++ b/pagure/api/issue.py @@ -16,7 +16,9 @@ from sqlalchemy.exc import SQLAlchemyError import pagure import pagure.exceptions import pagure.lib -from pagure import APP, SESSION, is_repo_admin, api_authenticated +from pagure import ( + APP, SESSION, is_repo_admin, api_authenticated, urlpattern +) from pagure.api import ( API, api_method, api_login_required, api_login_optional, APIERROR ) @@ -952,3 +954,116 @@ def api_subscribe_issue(repo, issueid, username=None, namespace=None): jsonout = flask.jsonify(output) return jsonout + + +@API.route('//issue//custom/', methods=['POST']) +@API.route( + '///issue//custom/', + methods=['POST']) +@API.route( + '/fork///issue//custom/', + methods=['POST']) +@API.route( + '/fork////issue//custom/', + methods=['POST']) +@api_login_required(acls=['issue_update_custom_fields', 'issue_update']) +@api_method +def api_update_custom_field( + repo, issueid, field, username=None, namespace=None): + """ + Update custom field + ------------------- + Update or reset the content of a custom field associated to an issue. + + :: + + POST /api/0//issue//custom/ + POST /api/0///issue//custom/ + + :: + + POST /api/0/fork///issue//custom/ + POST /api/0/fork////issue//custom/ + + Input + ^^^^^ + + +----------------- +---------+--------------+-------------------------+ + | Key | Type | Optionality | Description | + +==================+=========+==============+=========================+ + | ``value`` | string | Optional | The new value of the | + | | | | custom field of interest| + +----------------- +---------+--------------+-------------------------+ + + Sample response + ^^^^^^^^^^^^^^^ + + :: + + { + "message": "Custom key adjusted" + } + + """ + repo = pagure.lib.get_project( + SESSION, repo, user=username, namespace=namespace) + + output = {} + + if repo is None: + raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOPROJECT) + + if not repo.settings.get('issue_tracker', True): + raise pagure.exceptions.APIError( + 404, error_code=APIERROR.ETRACKERDISABLED) + + if api_authenticated(): + if repo != flask.g.token.project: + raise pagure.exceptions.APIError( + 401, error_code=APIERROR.EINVALIDTOK) + + issue = pagure.lib.search_issues(SESSION, repo, issueid=issueid) + + if issue is None or issue.project != repo: + raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOISSUE) + + if issue.private and not is_repo_admin(repo) \ + and (not api_authenticated() or + not issue.user.user == flask.g.fas_user.username): + raise pagure.exceptions.APIError( + 403, error_code=APIERROR.EISSUENOTALLOWED) + + fields = {k.name: k for k in repo.issue_keys} + if field not in fields: + raise pagure.exceptions.APIError( + 400, error_code=APIERROR.EINVALIDISSUEFIELD) + + key = fields[field] + value = flask.request.form.get('value') + if value: + if key.key_type == 'link': + links = value.split(',') + for link in links: + link = link.replace(' ', '') + if not urlpattern.match(link): + raise pagure.exceptions.APIError( + 400, error_code=APIERROR.EINVALIDISSUEFIELD_LINK) + try: + message = pagure.lib.set_custom_key_value( + SESSION, issue, key, value) + + SESSION.commit() + if message: + output['message'] = message + else: + output['message'] = 'No changes' + except pagure.exceptions.PagureException as err: + raise pagure.exceptions.APIError( + 400, error_code=APIERROR.ENOCODE, error=str(err)) + except SQLAlchemyError as err: # pragma: no cover + print err + SESSION.rollback() + raise pagure.exceptions.APIError(400, error_code=APIERROR.EDBERROR) + + jsonout = flask.jsonify(output) + return jsonout diff --git a/pagure/default_config.py b/pagure/default_config.py index da90503..6dff333 100644 --- a/pagure/default_config.py +++ b/pagure/default_config.py @@ -218,6 +218,7 @@ ACLS = { 'pull_request_merge': 'Merge a pull-request of this project', 'issue_subscribe': 'Subscribe the user with this token to an issue', 'issue_update': 'Update an issue, status, comments, custom fields...', + 'issue_update_custom_fields': 'Update the custom fields of an issue', } # Bootstrap URLS From 52aac41eb41d93b8a5d403796fbb9958afdbed05 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Dec 22 2016 14:07:18 +0000 Subject: [PATCH 4/12] Adjust set_custom_key_value to allow resetting a custom field --- diff --git a/pagure/lib/__init__.py b/pagure/lib/__init__.py index 9cedfd6..f9e95d3 100644 --- a/pagure/lib/__init__.py +++ b/pagure/lib/__init__.py @@ -3420,20 +3420,30 @@ def set_custom_key_value(session, issue, key, value): current_field = query.first() updated = False + delete = False if current_field: if current_field.key.key_type == 'boolean': value = value or False - if current_field.value != value: + if value is None: + session.delete(current_field) + updated = True + delete = True + elif current_field.value != value: current_field.value = value updated = True else: + if not value: + raise pagure.exceptions.PagureException( + 'No value given to this new custom field: %s' % key.name + ) current_field = model.IssueValues( issue_uid=issue.uid, key_id=key.id, value=value, ) updated = True - session.add(current_field) + if not delete: + session.add(current_field) if REDIS and updated: if issue.private: From 7bbefe310c37d01bb6d33189d360c90497dea701 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Dec 22 2016 14:07:18 +0000 Subject: [PATCH 5/12] Add unit-tests for the new API endpoint setting/resetting the custom fields --- diff --git a/tests/test_pagure_flask_api_issue.py b/tests/test_pagure_flask_api_issue.py index da78b94..82aa0b5 100644 --- a/tests/test_pagure_flask_api_issue.py +++ b/tests/test_pagure_flask_api_issue.py @@ -920,7 +920,7 @@ class PagureFlaskApiIssuetests(tests.Modeltests): data['error_code']) self.assertEqual(pagure.api.APIERROR.EINVALIDTOK.value, data['error']) - # No input + # No issue output = self.app.post('/api/0/test/issue/1/status', headers=headers) self.assertEqual(output.status_code, 404) data = json.loads(output.data) @@ -1780,6 +1780,198 @@ class PagureFlaskApiIssuetests(tests.Modeltests): pagure.lib.get_watch_list(self.session, issue), set(['pingou', 'foo'])) + def test_api_update_custom_field(self): + """ Test the api_update_custom_field method of the flask api. """ + tests.create_projects(self.session) + tests.create_projects_git(os.path.join(self.path, 'tickets')) + tests.create_tokens(self.session) + tests.create_tokens_acl(self.session) + + headers = {'Authorization': 'token aaabbbcccddd'} + + # Invalid project + output = self.app.post( + '/api/0/foo/issue/1/custom/bugzilla', headers=headers) + self.assertEqual(output.status_code, 404) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "Project not found", + "error_code": "ENOPROJECT", + } + ) + + # Valid token, wrong project + output = self.app.post( + '/api/0/test2/issue/1/custom/bugzilla', headers=headers) + self.assertEqual(output.status_code, 401) + data = json.loads(output.data) + self.assertEqual(pagure.api.APIERROR.EINVALIDTOK.name, + data['error_code']) + self.assertEqual(pagure.api.APIERROR.EINVALIDTOK.value, data['error']) + + # No issue + output = self.app.post( + '/api/0/test/issue/1/custom/bugzilla', headers=headers) + self.assertEqual(output.status_code, 404) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "Issue not found", + "error_code": "ENOISSUE", + } + ) + + # 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') + + # Project does not have this custom field + output = self.app.post( + '/api/0/test/issue/1/custom/bugzilla', headers=headers) + self.assertEqual(output.status_code, 400) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "Invalid custom field submitted", + "error_code": "EINVALIDISSUEFIELD", + } + ) + + # Check the behavior if the project disabled the issue tracker + repo = pagure.lib.get_project(self.session, 'test') + settings = repo.settings + settings['issue_tracker'] = False + repo.settings = settings + self.session.add(repo) + self.session.commit() + + output = self.app.post( + '/api/0/test/issue/1/custom/bugzilla', headers=headers) + self.assertEqual(output.status_code, 404) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "Issue tracker disabled for this project", + "error_code": "ETRACKERDISABLED", + } + ) + + repo = pagure.lib.get_project(self.session, 'test') + settings = repo.settings + settings['issue_tracker'] = True + repo.settings = settings + self.session.add(repo) + self.session.commit() + + # Invalid API token + headers = {'Authorization': 'token foobar'} + + output = self.app.post( + '/api/0/test/issue/1/custom/bugzilla', 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/ to get or renew your API token.", + "error_code": "EINVALIDTOK", + } + ) + + headers = {'Authorization': 'token aaabbbcccddd'} + + # Set some custom fields + repo = pagure.lib.get_project(self.session, 'test') + msg = pagure.lib.set_custom_key_fields( + self.session, repo, + ['bugzilla', 'upstream'], ['link', 'boolean']) + self.session.commit() + self.assertEqual(msg, 'List of custom fields updated') + + # No value specified while we try to create the field + output = self.app.post( + '/api/0/test/issue/1/custom/bugzilla', headers=headers) + self.assertEqual(output.status_code, 400) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "No value given to this new custom field: bugzilla", + "error_code": "ENOCODE", + } + ) + + # Invalid value + output = self.app.post( + '/api/0/test/issue/1/custom/bugzilla', headers=headers, + data={'value': 'foobar'}) + self.assertEqual(output.status_code, 400) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "Invalid custom field submitted, the value is not " + "a link", + "error_code": "EINVALIDISSUEFIELD_LINK", + } + ) + + issue = pagure.lib.search_issues(self.session, repo, issueid=1) + self.assertEqual(issue.other_fields, []) + self.assertEqual(len(issue.other_fields), 0) + + # All good + output = self.app.post( + '/api/0/test/issue/1/custom/bugzilla', headers=headers, + data={'value': 'https://bugzilla.redhat.com/1234'}) + self.assertEqual(output.status_code, 200) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "message": "Custom key adjusted" + } + ) + + issue = pagure.lib.search_issues(self.session, repo, issueid=1) + self.assertEqual(len(issue.other_fields), 1) + self.assertEqual(issue.other_fields[0].key.name, 'bugzilla') + self.assertEqual( + issue.other_fields[0].value, + 'https://bugzilla.redhat.com/1234') + + # Reset the value + output = self.app.post( + '/api/0/test/issue/1/custom/bugzilla', headers=headers) + self.assertEqual(output.status_code, 200) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "message": "Custom key adjusted" + } + ) + + issue = pagure.lib.search_issues(self.session, repo, issueid=1) + self.assertEqual(issue.other_fields, []) + self.assertEqual(len(issue.other_fields), 0) + if __name__ == '__main__': SUITE = unittest.TestLoader().loadTestsFromTestCase( From 84ceb8ea59f40bf030913ff19901b2adb66c6c96 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Dec 22 2016 14:07:18 +0000 Subject: [PATCH 6/12] Fix missing import --- diff --git a/pagure/ui/issues.py b/pagure/ui/issues.py index 0aad641..e611fc0 100644 --- a/pagure/ui/issues.py +++ b/pagure/ui/issues.py @@ -13,9 +13,10 @@ # pylint: disable=too-many-locals # pylint: disable=too-many-statements + +import datetime import flask import os -import datetime from collections import defaultdict from math import ceil @@ -233,6 +234,11 @@ def update_issue(repo, issueid, username=None, namespace=None): username=flask.g.fas_user.username, ticketfolder=APP.config['TICKETS_FOLDER'] ))) + + # The meta-data can be changed by admins and issue creator, + # where issue creators can only change status of their issue while + # other fields will be missing for non-admin and thus reset if we let them + if repo_admin: # Assign or update assignee of the ticket message = pagure.lib.add_issue_assignee( SESSION, From 29929891231aa45f58e3a62cacbe18160e364f53 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Dec 22 2016 14:07:18 +0000 Subject: [PATCH 7/12] Give unicode string to the markdown processor --- diff --git a/pagure/doc_utils.py b/pagure/doc_utils.py index 7878d04..99c3099 100644 --- a/pagure/doc_utils.py +++ b/pagure/doc_utils.py @@ -103,7 +103,7 @@ def convert_readme(content, ext, view_file_url=None): safe = True output = convert_doc(output, view_file_url) elif ext and ext in ['.mk', '.md', '.markdown']: - output = pagure.lib.text2markdown(content, readme=True) + output = pagure.lib.text2markdown(output, readme=True) safe = True elif not ext or (ext and ext in ['.text', '.txt']): safe = True From 866ceaaabf1b0f4b6a3730c986777a88c92a725e Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Dec 22 2016 14:07:18 +0000 Subject: [PATCH 8/12] Drop left-over code --- diff --git a/tests/test_pagure_flask_ui_app.py b/tests/test_pagure_flask_ui_app.py index c2c1f19..a5d0207 100644 --- a/tests/test_pagure_flask_ui_app.py +++ b/tests/test_pagure_flask_ui_app.py @@ -1050,7 +1050,6 @@ class PagureFlaskApptests(tests.Modeltests): output = self.app.get('/user/pingou/issues') - print output.data self.assertEqual(output.status_code, 200) self.assertIn('Test issue #1', output.data) self.assertIn('Test issue #2', output.data) From d73d04d4c9e89550e84ea620cc378c816dc15f58 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Dec 22 2016 14:07:18 +0000 Subject: [PATCH 9/12] Fix unit-test broken by when we changed the UI on merge commits --- diff --git a/tests/test_pagure_flask_ui_repo.py b/tests/test_pagure_flask_ui_repo.py index 2d570c3..5523497 100644 --- a/tests/test_pagure_flask_ui_repo.py +++ b/tests/test_pagure_flask_ui_repo.py @@ -1727,7 +1727,7 @@ class PagureFlaskRepotests(tests.Modeltests): self.assertTrue( '