PR#2516: Give Pagure site admins the ability to modify projects using the API - pagure

pagure

#2516 Give Pagure site admins the ability to modify projects using the API

Merged 7 years ago by pingou. Opened 7 years ago by mprahl.

Unknown source modify-project-api-allow-site-admins into master

Give Pagure site admins the ability to modify projects using the API

Matt Prahl • 7 years ago

ab0f462

Fix bug in api_modify_project

Matt Prahl • 7 years ago

58c0b03

pagure/api/project.py

file modified

+8 -3

		`@@ -853,8 +853,13 @@`
		`raise pagure.exceptions.APIError(`
		`404, error_code=APIERROR.ENOPROJECT)`

		`+ is_site_admin = pagure.is_admin()`
		`admins = project.get_project_users('admin')`
		`- if flask.g.fas_user not in admins and flask.g.fas_user != project.user:`
		`+ # Only allow the main admin, the admins of the project, and Pagure site`
		`+ # admins to modify projects, even if the user has the right ACLs on their`
		`+ # token`
		`+ if flask.g.fas_user not in admins and flask.g.fas_user != project.user \`
		`+ and not is_site_admin:`
		`raise pagure.exceptions.APIError(`
		`401, error_code=APIERROR.EMODIFYPROJECTNOTALLOWED)`

		`@@ -872,11 +877,11 @@`
		`400, error_code=APIERROR.EINVALIDREQ)`

		`if 'main_admin' in json:`
		`- if flask.g.fas_user != project.user:`
		`+ if flask.g.fas_user != project.user and not is_site_admin:`
		`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']:`
		`+ if flask.g.fas_user.username == project.user:`
		`return flask.jsonify(project.to_json(public=False, api=True))`

		`try:`

tests/test_pagure_flask_api_project.py

file modified

+71 -2

		`@@ -1118,14 +1118,78 @@`
		`self.assertEqual(output.status_code, 404)`

		`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. """`
		`+ """ 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.lib.get_user(self.session, 'pingou')`
		`+ user.cla_done = True`
		`+ 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'`
		`+ data['date_modified'] = '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",`
		`+ "date_modified": "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)`
		`+`
		`+ @patch.dict('pagure.APP.config', {'PAGURE_ADMIN_USERS': 'foo'})`
		`+ def test_api_modify_project_main_admin_as_site_admin(self):`
		`+ """ Test the api_modify_project method of the flask api when the`
		`+ request is to change the main_admin of the project and the user is a`
		`+ Pagure site admin. """`
		`+ tests.create_projects(self.session)`
		`+ tests.create_tokens(self.session, user_id=2, project_id=None)`
		`+ tests.create_tokens_acl(self.session, 'aaabbbcccddd', 'modify_project')`
		`+ headers = {'Authorization': 'token aaabbbcccddd'}`
		`+`
		`+ user = pagure.lib.get_user(self.session, 'foo')`
		`+ user.cla_done = True`
		`with tests.user_set(pagure.APP, user):`
		`output = self.app.patch(`
		`'/api/0/test', headers=headers,`
		`@@ -1195,6 +1259,7 @@`
		`headers = {'Authorization': 'token aaabbbcccddd'}`

		`user = pagure.lib.get_user(self.session, 'foo')`
		`+ user.cla_done = True`
		`with tests.user_set(pagure.APP, user):`
		`output = self.app.patch(`
		`'/api/0/test', headers=headers,`
		`@@ -1217,6 +1282,7 @@`
		`headers = {'Authorization': 'token aaabbbcccddd'}`

		`user = pagure.lib.get_user(self.session, 'foo')`
		`+ user.cla_done = True`
		`with tests.user_set(pagure.APP, user):`
		`output = self.app.patch(`
		`'/api/0/test', headers=headers,`
		`@@ -1238,6 +1304,7 @@`
		`headers = {'Authorization': 'token aaabbbcccddd'}`

		`user = pagure.lib.get_user(self.session, 'pingou')`
		`+ user.cla_done = True`
		`with tests.user_set(pagure.APP, user):`
		`output = self.app.patch(`
		`'/api/0/test', headers=headers,`
		`@@ -1259,6 +1326,7 @@`
		`headers = {'Authorization': 'token aaabbbcccddd'}`

		`user = pagure.lib.get_user(self.session, 'pingou')`
		`+ user.cla_done = True`
		`with tests.user_set(pagure.APP, user):`
		`output = self.app.patch(`
		`'/api/0/test', headers=headers,`
		`@@ -1281,6 +1349,7 @@`
		`headers = {'Authorization': 'token aaabbbcccddd'}`

		`user = pagure.lib.get_user(self.session, 'pingou')`
		`+ user.cla_done = True`
		`with tests.user_set(pagure.APP, user):`
		`output = self.app.patch(`
		`'/api/0/test', headers=headers,`

mprahl commented 7 years ago

This addresses #2515

pingou commented on line 87 of tests/test_pagure_flask_api_project.py 7 years ago

Let's drop these two or replace them by a single print()

pingou commented 7 years ago

One comment and needs a rebase otherwise looks fine

pingou commented 7 years ago

Won't this suffer the same issue as #2514 regarding tibbs and limb?

rebased

7 years ago

mprahl commented 7 years ago

@pingou I addressed your comment.

As for the issue with tibbs and limb, yes, that will be the case, but Ralph determined that releng would have to handle the ownership changes of existing repos.

rebased

7 years ago