From 65844bd3d877e3e735027a56684617ae74120da1 Mon Sep 17 00:00:00 2001 From: Matt Prahl Date: Jun 07 2017 07:58:27 +0000 Subject: [PATCH 1/3] Move the logic of setting the project owner to another function --- diff --git a/pagure/lib/__init__.py b/pagure/lib/__init__.py index f074ccd..482801b 100644 --- a/pagure/lib/__init__.py +++ b/pagure/lib/__init__.py @@ -42,6 +42,7 @@ from sqlalchemy import asc from sqlalchemy.orm import aliased from sqlalchemy.orm import sessionmaker from sqlalchemy.orm import scoped_session +from sqlalchemy.exc import SQLAlchemyError import pagure import pagure.exceptions @@ -4212,3 +4213,15 @@ def search_token( return query.first() else: return query.all() + + +def set_project_owner(session, project, user): + ''' Set the ownership of a project + :arg session: the session to use to connect to the database. + :arg project: a Project object representing the project's ownership to + change. + :arg user: a User object representing the new owner of the project. + :return: None + ''' + project.user = user + session.add(project) diff --git a/pagure/ui/repo.py b/pagure/ui/repo.py index 9b8ed55..d749bc5 100644 --- a/pagure/ui/repo.py +++ b/pagure/ui/repo.py @@ -2597,8 +2597,7 @@ def give_project(repo, username=None, namespace=None): 404, 'No such user %s found' % new_username) try: - repo.user = new_owner - SESSION.add(repo) + pagure.lib.set_project_owner(SESSION, repo, new_owner) SESSION.commit() flask.flash( 'The project has been transferred to %s' % new_username) From 30f8b1f7a042e99f720fea77e28dcb4ffe9fe200 Mon Sep 17 00:00:00 2001 From: Matt Prahl Date: Jun 07 2017 07:58:27 +0000 Subject: [PATCH 2/3] Add an API to modify a Pagure project's owner --- diff --git a/alembic/versions/27a79ff0fb41_add_modify_project_acl.py b/alembic/versions/27a79ff0fb41_add_modify_project_acl.py new file mode 100644 index 0000000..33fb2a6 --- /dev/null +++ b/alembic/versions/27a79ff0fb41_add_modify_project_acl.py @@ -0,0 +1,40 @@ +"""Add modify_project ACL + +Revision ID: 27a79ff0fb41 +Revises: d4d2c5aa8a0 +Create Date: 2017-06-01 14:20:06.769321 + +""" + +# revision identifiers, used by Alembic. +revision = '27a79ff0fb41' +down_revision = '5179e99d35a5' + +from alembic import op +import sqlalchemy as sa + +from pagure.lib import model + + +def get_session(): + engine = op.get_bind() + Session = sa.orm.scoped_session(sa.orm.sessionmaker()) + Session.configure(bind=engine) + return Session() + + +def upgrade(): + session = get_session() + modify_project_acl = model.ACL() + modify_project_acl.name = 'modify_project' + modify_project_acl.description = 'Modify a project' + session.add(modify_project_acl) + session.commit() + + +def downgrade(): + session = get_session() + modify_project_acl = session.query(model.ACL).filter_by( + name='modify_project').one() + session.delete(modify_project_acl) + session.commit() diff --git a/pagure/api/__init__.py b/pagure/api/__init__.py index a07f195..bd65803 100644 --- a/pagure/api/__init__.py +++ b/pagure/api/__init__.py @@ -86,6 +86,8 @@ class APIERROR(enum.Enum): 'is not a link' EINVALIDPRIORITY = 'Invalid priority submitted' ENOGROUP = 'Group not found' + ENOTMAINADMIN = 'Only the main admin can set the main admin of a project' + EMODIFYPROJECTNOTALLOWED = 'You are not allowed to modify this project' def get_authorized_api_project(SESSION, repo, user=None, namespace=None): @@ -457,6 +459,7 @@ def api(): api_pull_request_add_flag_doc = load_doc(fork.api_pull_request_add_flag) api_new_project_doc = load_doc(project.api_new_project) + api_modify_project_doc = load_doc(project.api_modify_project) api_version_doc = load_doc(api_version) api_users_doc = load_doc(api_users) @@ -487,6 +490,7 @@ def api(): api_doc=APIDOC, projects=[ api_new_project_doc, + api_modify_project_doc, api_project_doc, api_projects_doc, api_git_tags_doc, diff --git a/pagure/api/project.py b/pagure/api/project.py index 9839ead..b93a687 100644 --- a/pagure/api/project.py +++ b/pagure/api/project.py @@ -554,6 +554,120 @@ def api_new_project(): return jsonout +@API.route('/', methods=['PATCH']) +@API.route('//', methods=['PATCH']) +@api_login_required(acls=['modify_project']) +@api_method +def api_modify_project(repo, namespace=None): + """ + Modify a project + ---------------- + Modify an existing project on this Pagure instance. + + :: + + PATCH /api/0/ + + + Input + ^^^^^ + + +------------------+---------+--------------+---------------------------+ + | Key | Type | Optionality | Description | + +==================+=========+==============+===========================+ + | ``main_admin`` | string | Mandatory | | The new main admin of | + | | | | the project. | + +------------------+---------+--------------+---------------------------+ + + Sample response + ^^^^^^^^^^^^^^^ + + :: + + { + "access_groups": { + "admin": [], + "commit": [], + "ticket": [] + }, + "access_users": { + "admin": [], + "commit": [], + "owner": [ + "testuser1" + ], + "ticket": [] + }, + "close_status": [], + "custom_keys": [], + "date_created": "1496326387", + "description": "Test", + "fullname": "test-project2", + "id": 2, + "milestones": {}, + "name": "test-project2", + "namespace": null, + "parent": null, + "priorities": {}, + "tags": [], + "user": { + "default_email": "testuser1@domain.local", + "emails": [], + "fullname": "Test User1", + "name": "testuser1" + } + } + + """ + project = get_authorized_api_project( + SESSION, repo, namespace=namespace) + if not project: + raise pagure.exceptions.APIError( + 404, error_code=APIERROR.ENOPROJECT) + + admins = project.get_project_users('admin') + if flask.g.fas_user not in admins and flask.g.fas_user != project.user: + raise pagure.exceptions.APIError( + 401, error_code=APIERROR.EMODIFYPROJECTNOTALLOWED) + + valid_keys = ['main_admin'] + # Set force to True to ignore the mimetype. Set silent so that None is + # returned if it's invalid JSON. + json = flask.request.get_json(force=True, silent=True) + if not json: + raise pagure.exceptions.APIError(400, error_code=APIERROR.EINVALIDREQ) + + # Check to make sure there aren't parameters we don't support + for key in json.keys(): + if key not in valid_keys: + raise pagure.exceptions.APIError( + 400, error_code=APIERROR.EINVALIDREQ) + + if 'main_admin' in json: + if flask.g.fas_user != project.user: + raise pagure.exceptions.APIError( + 401, error_code=APIERROR.ENOTMAINADMIN) + # If the main_admin is already set correctly, don't do anything + if flask.g.fas_user.username == json['main_admin']: + return flask.jsonify(project.to_json(public=False, api=True)) + + try: + new_main_admin = pagure.lib.get_user(SESSION, json['main_admin']) + except pagure.exceptions.PagureException: + raise pagure.exceptions.APIError(400, error_code=APIERROR.ENOUSER) + + pagure.lib.set_project_owner(SESSION, project, new_main_admin) + + try: + SESSION.commit() + except SQLAlchemyError: # pragma: no cover + SESSION.rollback() + raise pagure.exceptions.APIError( + 400, error_code=APIERROR.EDBERROR) + + return flask.jsonify(project.to_json(public=False, api=True)) + + @API.route('/fork/', methods=['POST']) @API.route('/fork', methods=['POST']) @api_login_required(acls=['fork_project']) diff --git a/pagure/default_config.py b/pagure/default_config.py index d0c59b0..fca3695 100644 --- a/pagure/default_config.py +++ b/pagure/default_config.py @@ -236,6 +236,7 @@ ACLS = { 'issue_update': 'Update an issue, status, comments, custom fields...', 'issue_update_custom_fields': 'Update the custom fields of an issue', 'issue_update_milestone': 'Update the milestone of an issue', + 'modify_project': 'Modify an existing project' } # From the ACLs above lists which ones are tolerated to be associated with @@ -243,6 +244,7 @@ ACLS = { CROSS_PROJECT_ACLS = [ 'create_project', 'fork_project', + 'modify_project' ] # ACLs with which admins are allowed to create project-less API tokens diff --git a/tests/__init__.py b/tests/__init__.py index dbe48b2..6297aa4 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -439,14 +439,25 @@ def create_tokens(session, user_id=1, project_id=1): session.commit() -def create_tokens_acl(session, token_id='aaabbbcccddd'): - """ Create some acls for the tokens. """ - for aclid in range(len(pagure.APP.config['ACLS'])): - item = pagure.lib.model.TokenAcl( +def create_tokens_acl(session, token_id='aaabbbcccddd', acl_name=None): + """ Create some ACLs for the token. If acl_name is not set, the token will + have all the ACLs enabled. + """ + if acl_name is None: + for aclid in range(len(pagure.APP.config['ACLS'])): + token_acl = pagure.lib.model.TokenAcl( + token_id=token_id, + acl_id=aclid + 1, + ) + session.add(token_acl) + else: + acl = session.query(pagure.lib.model.ACL).filter_by( + name=acl_name).one() + token_acl = pagure.lib.model.TokenAcl( token_id=token_id, - acl_id=aclid + 1, + acl_id=acl.id, ) - session.add(item) + session.add(token_acl) session.commit() diff --git a/tests/test_pagure_flask_api_project.py b/tests/test_pagure_flask_api_project.py index 5ff8624..0d5cf4f 100644 --- a/tests/test_pagure_flask_api_project.py +++ b/tests/test_pagure_flask_api_project.py @@ -11,7 +11,6 @@ __requires__ = ['SQLAlchemy >= 0.8'] import pkg_resources -import datetime import json import unittest import shutil @@ -44,7 +43,6 @@ class PagureFlaskApiProjecttests(tests.Modeltests): pagure.api.project.SESSION = self.session pagure.lib.SESSION = self.session - def test_api_git_tags(self): """ Test the api_git_tags method of the flask api. """ tests.create_projects(self.session) @@ -650,6 +648,173 @@ class PagureFlaskApiProjecttests(tests.Modeltests): } self.assertDictEqual(data, expected_data) + def test_api_modify_project_main_admin(self): + """ Test the api_modify_project method of the flask api when the request + is to change the main_admin of the project. """ + tests.create_projects(self.session) + tests.create_tokens(self.session, project_id=None) + tests.create_tokens_acl(self.session, 'aaabbbcccddd', 'modify_project') + headers = {'Authorization': 'token aaabbbcccddd'} + user = pagure.SESSION.query(pagure.lib.model.User).filter_by( + user='pingou').one() + with tests.user_set(pagure.APP, user): + output = self.app.patch('/api/0/test', headers=headers, + data=json.dumps({'main_admin': 'foo'})) + self.assertEqual(output.status_code, 200) + data = json.loads(output.data) + data['date_created'] = '1496338274' + expected_output = { + "access_groups": { + "admin": [], + "commit": [], + "ticket": [] + }, + "access_users": { + "admin": [], + "commit": [], + "owner": [ + "foo" + ], + "ticket": [] + }, + "close_status": [ + "Invalid", + "Insufficient data", + "Fixed", + "Duplicate" + ], + "custom_keys": [], + "date_created": "1496338274", + "description": "test project #1", + "fullname": "test", + "id": 1, + "milestones": {}, + "name": "test", + "namespace": None, + "parent": None, + "priorities": {}, + "tags": [], + "user": { + "default_email": "foo@bar.com", + "emails": [ + "foo@bar.com" + ], + "fullname": "foo bar", + "name": "foo" + } + } + self.assertEqual(data, expected_output) + + def test_api_modify_project_main_admin_not_main_admin(self): + """ Test the api_modify_project method of the flask api when the + requester is not the main_admin of the project and requests to change + the main_admin. + """ + tests.create_projects(self.session) + project_user = pagure.lib.model.ProjectUser( + project_id=1, + user_id=2, + access='admin', + ) + self.session.add(project_user) + self.session.commit() + tests.create_tokens(self.session, project_id=None, user_id=2) + tests.create_tokens_acl(self.session, 'aaabbbcccddd', 'modify_project') + headers = {'Authorization': 'token aaabbbcccddd'} + user = pagure.SESSION.query(pagure.lib.model.User).filter_by( + user='foo').one() + with tests.user_set(pagure.APP, user): + output = self.app.patch('/api/0/test', headers=headers, + data=json.dumps({'main_admin': 'foo'})) + self.assertEqual(output.status_code, 401) + expected_error = { + 'error': ('Only the main admin can set the main admin of a ' + 'project'), + 'error_code': 'ENOTMAINADMIN' + } + self.assertEqual(json.loads(output.data), expected_error) + + def test_api_modify_project_not_admin(self): + """ Test the api_modify_project method of the flask api when the + requester is not an admin of the project. + """ + tests.create_projects(self.session) + tests.create_tokens(self.session, project_id=None, user_id=2) + tests.create_tokens_acl(self.session, 'aaabbbcccddd', 'modify_project') + headers = {'Authorization': 'token aaabbbcccddd'} + user = pagure.SESSION.query(pagure.lib.model.User).filter_by( + user='foo').one() + with tests.user_set(pagure.APP, user): + output = self.app.patch('/api/0/test', headers=headers, + data=json.dumps({'main_admin': 'foo'})) + self.assertEqual(output.status_code, 401) + expected_error = { + 'error': 'You are not allowed to modify this project', + 'error_code': 'EMODIFYPROJECTNOTALLOWED' + } + self.assertEqual(json.loads(output.data), expected_error) + + def test_api_modify_project_invalid_request(self): + """ Test the api_modify_project method of the flask api when the + request data is invalid. + """ + tests.create_projects(self.session) + tests.create_tokens(self.session, project_id=None) + tests.create_tokens_acl(self.session, 'aaabbbcccddd', 'modify_project') + headers = {'Authorization': 'token aaabbbcccddd'} + user = pagure.SESSION.query(pagure.lib.model.User).filter_by( + user='pingou').one() + with tests.user_set(pagure.APP, user): + output = self.app.patch('/api/0/test', headers=headers, + data='invalid') + self.assertEqual(output.status_code, 400) + expected_error = { + 'error': 'Invalid or incomplete input submited', + 'error_code': 'EINVALIDREQ' + } + self.assertEqual(json.loads(output.data), expected_error) + + def test_api_modify_project_invalid_keys(self): + """ Test the api_modify_project method of the flask api when the + request data contains an invalid key. + """ + tests.create_projects(self.session) + tests.create_tokens(self.session, project_id=None) + tests.create_tokens_acl(self.session, 'aaabbbcccddd', 'modify_project') + headers = {'Authorization': 'token aaabbbcccddd'} + user = pagure.SESSION.query(pagure.lib.model.User).filter_by( + user='pingou').one() + with tests.user_set(pagure.APP, user): + output = self.app.patch('/api/0/test', headers=headers, + data=json.dumps({'invalid': 'invalid'})) + self.assertEqual(output.status_code, 400) + expected_error = { + 'error': 'Invalid or incomplete input submited', + 'error_code': 'EINVALIDREQ' + } + self.assertEqual(json.loads(output.data), expected_error) + + def test_api_modify_project_invalid_new_main_admin(self): + """ Test the api_modify_project method of the flask api when the + request is to change the main_admin of the project to a main_admin + that doesn't exist. + """ + tests.create_projects(self.session) + tests.create_tokens(self.session, project_id=None) + tests.create_tokens_acl(self.session, 'aaabbbcccddd', 'modify_project') + headers = {'Authorization': 'token aaabbbcccddd'} + user = pagure.SESSION.query(pagure.lib.model.User).filter_by( + user='pingou').one() + with tests.user_set(pagure.APP, user): + output = self.app.patch('/api/0/test', headers=headers, + data=json.dumps({'main_admin': 'tbrady'})) + self.assertEqual(output.status_code, 400) + expected_error = { + 'error': 'No such user found', + 'error_code': 'ENOUSER' + } + self.assertEqual(json.loads(output.data), expected_error) + def test_api_project_watchers(self): """ Test the api_project_watchers method of the flask api. """ tests.create_projects(self.session) From 0e867dd4e2855cd60b6e7d60256eebab842a9ad9 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jun 07 2017 09:13:25 +0000 Subject: [PATCH 3/3] Style and tests fixes to make the unit-tests pass --- diff --git a/alembic/versions/27a79ff0fb41_add_modify_project_acl.py b/alembic/versions/27a79ff0fb41_add_modify_project_acl.py index 33fb2a6..9811339 100644 --- a/alembic/versions/27a79ff0fb41_add_modify_project_acl.py +++ b/alembic/versions/27a79ff0fb41_add_modify_project_acl.py @@ -13,7 +13,12 @@ down_revision = '5179e99d35a5' from alembic import op import sqlalchemy as sa -from pagure.lib import model +try: + from pagure.lib import model +except ImportError: + import sys + sys.path.insert(0, '.') + from pagure.lib import model def get_session(): diff --git a/pagure/lib/__init__.py b/pagure/lib/__init__.py index 482801b..c0a6313 100644 --- a/pagure/lib/__init__.py +++ b/pagure/lib/__init__.py @@ -42,7 +42,6 @@ from sqlalchemy import asc from sqlalchemy.orm import aliased from sqlalchemy.orm import sessionmaker from sqlalchemy.orm import scoped_session -from sqlalchemy.exc import SQLAlchemyError import pagure import pagure.exceptions diff --git a/pagure/ui/app.py b/pagure/ui/app.py index feab07c..6d5c779 100644 --- a/pagure/ui/app.py +++ b/pagure/ui/app.py @@ -590,7 +590,7 @@ def update_user_settings(): message = pagure.lib.update_user_settings( SESSION, settings=settings, - user=flask.g.fas_user.username, + user=user.username, ) SESSION.commit() flask.flash(message) @@ -832,7 +832,7 @@ def add_api_user_token(): project=None, description=form.description.data.strip() or None, acls=form.acls.data, - username=flask.g.fas_user.username, + username=user.username, ) SESSION.commit() flask.flash(msg) diff --git a/tests/test_pagure_lib.py b/tests/test_pagure_lib.py index 2b289f0..4271b94 100644 --- a/tests/test_pagure_lib.py +++ b/tests/test_pagure_lib.py @@ -5360,6 +5360,7 @@ foo bar 'issue_update', 'issue_update_custom_fields', 'issue_update_milestone', + 'modify_project', 'pull_request_close', 'pull_request_comment', 'pull_request_flag',