PR#117: Add API for orphaning process - pagure-dist-git

pagure-dist-git

#117 Add API for orphaning process

Merged 4 years ago by pingou. Opened 4 years ago by zlopez.

zlopez/pagure-dist-git orphan into master

Fix the error when orphan user doesn't exist

Michal Konečný • 4 years ago

2118c60

Add API for orphaning process

Michal Konečný • 4 years ago

b72cee8

pagure_distgit/plugin.py

file modified

+57

		`@@ -19,6 +19,7 @@`
		`import pagure.utils`
		`from pagure.api import APIERROR, api_login_required, api_method`
		`from pagure.api.utils import _check_token, _get_repo`
		`+ from pagure.exceptions import PagureException`

		`import pagure_distgit.forms`
		`from pagure_distgit import model`
		`@@ -182,6 +183,62 @@`
		`return data["results"][0]["active"] is True`


		`+ @DISTGIT_NS.route("/orphan/<namespace>/<repo>", methods=["POST"])`
		`+ @api_login_required(acls=["modify_project"])`
		`+ @api_method`
		`+ def orphan_endpoint(namespace, repo):`
		`+ """ Orphan the package.`
		`+ """`
		`+ _log.info("Received a request to orphan: %s/%s", namespace, repo)`
		`+`
		`+ repo = _get_repo(repo, namespace=namespace)`
		`+ _check_token(repo, project_token=False)`
		`+`
		`+ user_obj = pagure.lib.query.get_user(`
		`+ flask.g.session, flask.g.fas_user.username`
		`+ )`
		`+ if not user_obj:`
		`+ raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOUSER)`
		`+`
		`+ if repo.user.user == "orphan":`
		`+ raise pagure.exceptions.APIError(`
		`+ 401, error_code=APIERROR.EMODIFYPROJECTNOTALLOWED`
		`+ )`
		`+`
		`+ try:`
		`+ orphan_user_obj = pagure.lib.query.get_user(flask.g.session, "orphan")`
		`+ except PagureException:`
		`+ _log.exception("Error when retrieving orphan user")`
		`+ raise pagure.exceptions.APIError(400, error_code=APIERROR.EDBERROR)`
		`+`
		`+ try:`
		`+ pagure.lib.query.set_project_owner(`
		`+ flask.g.session, repo, orphan_user_obj`
		`+ )`
		`+ if user_obj in repo.users:`
		`+ pagure.lib.query.remove_user_of_project(`
		`+ flask.g.session, user_obj, repo, user_obj.user`
		`+ )`
		`+ pagure.lib.query.update_watch_status(`
		`+ flask.g.session, repo, user_obj.username, "-1"`
		`+ )`
		`+ flask.g.session.commit()`
		`+ except SQLAlchemyError: # pragma: no cover`
		`+ flask.g.session.rollback()`
		`+ _log.exception("Error when orphaning project")`
		`+ raise pagure.exceptions.APIError(400, error_code=APIERROR.EDBERROR)`
		`+`
		`+ pagure.lib.notify.log(`
		`+ repo,`
		`+ topic="project.orphan",`
		`+ msg={"project": repo.to_json(public=True), "agent": user_obj.username},`
		`+ )`
		`+`
		`+ output = {"point_of_contact": repo.user.user}`
		`+`
		`+ return flask.jsonify(output)`
		`+`
		`+`
		`@DISTGIT_NS.route("/take_orphan/<namespace>/<repo>", methods=["POST"])`
		`@api_login_required(acls=["modify_project"])`
		`@api_method`

pagure_distgit_tests/test_plugin.py

file added

+145

		`@@ -0,0 +1,145 @@`
		`+ import json`
		`+ import os`
		`+ from unittest.mock import call, patch`
		`+`
		`+ import pagure.lib.query`
		`+ import pagure.lib.model`
		`+`
		`+ from pagure_distgit import plugin`
		`+`
		`+ import tests`
		`+`
		`+`
		`+ class PagureFlaskApiOrphanEndpointTests(tests.Modeltests):`
		`+ """ Tests the orphan endpoints added in pagure-dist-git. """`
		`+`
		`+ def setUp(self):`
		`+ """ Set up the environnment, ran before every tests. """`
		`+ super(PagureFlaskApiOrphanEndpointTests, self).setUp()`
		`+ self.session.flush()`
		`+ tests.create_projects(self.session)`
		`+ tests.create_projects_git(os.path.join(self.path, "repos"), bare=True)`
		`+ tests.create_tokens(self.session)`
		`+ tests.create_tokens_acl(self.session)`
		`+ tests.create_user(`
		`+ self.session, "orphan", "orphan", ["orphan@fedoraproject.org"]`
		`+ )`
		`+ repo = pagure.lib.query.get_authorized_project(`
		`+ self.session, "test3", namespace="somenamespace",`
		`+ )`
		`+ token = pagure.lib.query.get_api_token(self.session, "aaabbbcccddd")`
		`+ token.project = repo`
		`+ self.session.add(token)`
		`+ self.session.commit()`
		`+ self._app.register_blueprint(plugin.DISTGIT_NS)`
		`+`
		`+ def test_token_missing_ACL(self):`
		+ """Test the orphan endpoint with an API token missing the `modify_project` ACL.
		`+ """`
		`+ headers = {"Authorization": "token foo_token"}`
		`+ output = self.app.post(`
		`+ "/_dg/orphan/somenamespace/test3", headers=headers`
		`+ )`
		`+ # invalid token`
		`+ assert output.status_code == 401`
		`+`
		`+ def test_invalid_token(self):`
		`+ """Test the orphan endpoint with an invalid API token. """`
		`+ headers = {"Authorization": "token BBBZZZOOO"}`
		`+ datainput = {}`
		`+ output = self.app.post(`
		`+ "/_dg/orphan/somenamespace/test3", data=datainput, headers=headers,`
		`+ )`
		`+ assert output.status_code == 401`
		`+`
		`+ def test_already_orphaned_package(self):`
		`+ """Assert that already orphaned package can't be orphaned again.`
		`+ """`
		`+ headers = {"Authorization": "token aaabbbcccddd"}`
		`+ datainput = {}`
		`+ orphan_user_obj = pagure.lib.query.get_user(self.session, "orphan")`
		`+ repo = pagure.lib.query.get_authorized_project(`
		`+ self.session, "test3", namespace="somenamespace",`
		`+ )`
		`+ repo.user = orphan_user_obj`
		`+ self.session.add(repo)`
		`+ self.session.commit()`
		`+ output = self.app.post(`
		`+ "/_dg/orphan/somenamespace/test3", data=datainput, headers=headers,`
		`+ )`
		`+ assert output.status_code == 401`
		`+`
		`+ @patch("pagure_distgit.plugin.pagure.lib.notify.log")`
		`+ def test_orphan_package(self, mock_log):`
		`+ """Assert that package is correctly orphaned.`
		`+ """`
		`+ headers = {"Authorization": "token aaabbbcccddd"}`
		`+ datainput = {}`
		`+ repo = pagure.lib.query.get_authorized_project(`
		`+ self.session, "test3", namespace="somenamespace",`
		`+ )`
		`+ user = repo.user`
		`+ project_user = pagure.lib.model.ProjectUser(`
		`+ project_id=repo.id, user_id=repo.user.id, access="admin",`
		`+ )`
		`+ self.session.add(project_user)`
		`+ self.session.commit()`
		`+ output = self.app.post(`
		`+ "/_dg/orphan/somenamespace/test3", data=datainput, headers=headers,`
		`+ )`
		`+ assert output.status_code == 200`
		`+ data = json.loads(output.get_data(as_text=True))`
		`+ self.assertDictEqual(data, {"point_of_contact": "orphan"})`
		`+`
		`+ assert repo.user.user == "orphan"`
		`+`
		`+ watch_level = pagure.lib.query.get_watch_level_on_repo(`
		`+ self.session, user, "test3"`
		`+ )`
		`+ assert watch_level == []`
		`+`
		`+ assert user not in repo.users`
		`+`
		`+ assert mock_log.call_count == 2`
		`+`
		`+ @patch("pagure_distgit.plugin.pagure.lib.notify.log")`
		`+ def test_orphan_package_repo_users_empty(self, mock_log):`
		`+ """Assert that package is correctly orphaned.`
		`+ """`
		`+ headers = {"Authorization": "token aaabbbcccddd"}`
		`+ datainput = {}`
		`+ repo = pagure.lib.query.get_authorized_project(`
		`+ self.session, "test3", namespace="somenamespace",`
		`+ )`
		`+ user = repo.user`
		`+ output = self.app.post(`
		`+ "/_dg/orphan/somenamespace/test3", data=datainput, headers=headers,`
		`+ )`
		`+ assert output.status_code == 200`
		`+ data = json.loads(output.get_data(as_text=True))`
		`+ self.assertDictEqual(data, {"point_of_contact": "orphan"})`
		`+ # refresh the repo object, so we have the current state`
		`+ self.session.refresh(repo)`
		`+`
		`+ assert repo.user.user == "orphan"`
		`+`
		`+ watch_level = pagure.lib.query.get_watch_level_on_repo(`
		`+ self.session, user, "test3"`
		`+ )`
		`+ assert watch_level == []`
		`+`
		`+ assert mock_log.call_count == 1`
		`+`
		`+ def test_orphan_package_repo_no_orphan_user(self):`
		`+ """Assert that missing orphan user is correctly handled.`
		`+ """`
		`+ headers = {"Authorization": "token aaabbbcccddd"}`
		`+ datainput = {}`
		`+ orphan_user_obj = pagure.lib.query.get_user(self.session, "orphan")`
		`+ self.session.delete(orphan_user_obj)`
		`+ self.session.commit()`
		`+`
		`+ output = self.app.post(`
		`+ "/_dg/orphan/somenamespace/test3", data=datainput, headers=headers,`
		`+ )`
		`+ assert output.status_code == 400`

zlopez commented 4 years ago

This PR implements https://pagure.io/pagure-dist-git/issue/110

Signed-off-by: Michal Konečný mkonecny@redhat.com

rebased onto b5b9859cb2372027b1f85eb1ebd06ff36db4b4f6

4 years ago

zlopez commented 4 years ago

During the test I encountered this issue in test_orphan_package:
pkg_resources.VersionConflict: (SQLAlchemy 1.3.18 (/var/home/zlopez/git/pagure-dist-git/.venv/lib/python3.8/site-packages), Requirement.parse('sqlalchemy<1.3.0'))

After doing pip install --upgrade "sqlalchemy<1.3.0" the test worked.

pingou commented on line 43 of pagure_distgit/plugin.py 4 years ago

_log.exception() will automatically log the exception/traceback, so it's better to do something like _log.exception("what we were trying to do when the exception occured") which will be added to the log and give a little bit of context when reviewing the logs to figure how/why/when the exception occured.

pingou commented on line 45 of pagure_distgit_tests/test_plugin.py 4 years ago

Technically the tests we are running here are using the unittest module in stdlib, so they use the self.assert() methods, but I won't block the PR on this.

zlopez commented 4 years ago

_log.exception() will automatically log the exception/traceback, so it's better to do something like _log.exception("what we were trying to do when the exception occured") which will be added to the log and give a little bit of context when reviewing the logs to figure how/why/when the exception occured.

Will update.

Technically the tests we are running here are using the unittest module in stdlib, so they use the self.assert() methods, but I won't block the PR on this.

Because I ported it to pytest, I decided to go more with pytest style.

rebased onto b72cee8

4 years ago

zlopez commented 4 years ago

The _log.exception is now updated.

pingou commented on line 27 of pagure_distgit/plugin.py 4 years ago

This raises an exception when the "orphan" user doesn't exist, we should catch it and raise the appropriate APIError

zlopez commented 4 years ago

This raises an exception when the "orphan" user doesn't exist, we should catch it and raise the appropriate APIError

It's checked in the condition above, I expect that if the repo is already owned by orphan the user must exist.

pingou commented 4 years ago

It's checked in the condition above, I expect that if the repo is already owned by orphan the user must exist.

If the current repo is not owned by orphan and I'm trying to orphan it, your previous check won't apply (current owner == pingou, new owner == orphan, if orphan doesn't exist -> crash). I've triggered it locally :)

zlopez commented 4 years ago

I will add a test for this

1 new commit added