PR#4223: Add project createapitoken endpoint - pagure

		`@@ -2133,3 +2133,94 @@`
		`raise pagure.exceptions.APIError(400, error_code=APIERROR.EDBERROR)`

		`return flask.jsonify({"message": message, "status": "ok"})`
		`+`
		`+`
		`+ @API.route("/<repo>/token/new", methods=["POST"])`
		`+ @API.route("/<namespace>/<repo>/token/new", methods=["POST"])`
		`+ @API.route("/fork/<username>/<repo>/token/new", methods=["POST"])`
		`+ @API.route("/fork/<username>/<namespace>/<repo>/token/new", methods=["POST"])`
		`+ @api_login_required(acls=["modify_project"])`
		`+ @api_method`
		`+ def api_project_create_api_token(repo, namespace=None, username=None):`
		`+ """`
		`+ Create API project Token`
		`+ ------------------------`
		`+ Create a project token API for the caller user`
		`+`
		`+ This is restricted to project admins.`
		`+`
		`+ ::`
		`+`
		`+ POST /api/0/<repo>/token/new`
		`+ POST /api/0/<namespace>/<repo>/token/new`
		`+`
		`+ ::`
		`+`
		`+ POST /api/0/fork/<username>/<repo>/token/new`
		`+ POST /api/0/fork/<username>/<namespace>/<repo>/token/new`
		`+`
		`+`
		`+ Input`
		`+ ^^^^^`
		`+`
		`+ +------------------+---------+---------------+---------------------------+`
		`+ \| Key \| Type \| Optionality \| Description \|`
		`+ +==================+=========+===============+===========================+`
		+ \| ``description`` \| String \| optional \| A string to specify the \|
		`+ \| \| \| \| description of the token \|`
		`+ \| \| \| \| \|`
		`+ +------------------+---------+---------------+---------------------------+`
		+ \| ``acls`` \| List \| Mandatory \| The ACLs \|
		`+ \| \| \| \| \|`
		`+ +------------------+---------+---------------+---------------------------+`
		`+`
		`+`
		`+ Sample response`
		`+ ^^^^^^^^^^^^^^^`
		`+`
		`+ ::`
		`+`
		`+ {`
		`+ "token": {`
		`+ "description": "My foo token",`
		`+ "id": "aaabbbcccfootoken",`
		`+ },`
		`+ }`
		`+`
		`+ """`
		`+ output = {}`
		`+ project = get_authorized_api_project(`
		`+ flask.g.session, repo, namespace=namespace`
		`+ )`
		`+ if not project:`
		`+ raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOPROJECT)`
		`+`
		`+ if flask.g.token.project and project != flask.g.token.project:`
		`+ raise pagure.exceptions.APIError(401, error_code=APIERROR.EINVALIDTOK)`
		`+`
		`+ authorized_users = [project.user.username]`
		`+ authorized_users.extend(`
		`+ [user.user for user in project.access_users["admin"]]`
		`+ )`
		`+ if flask.g.fas_user.user not in authorized_users:`
		`+ raise pagure.exceptions.APIError(`
		`+ 401, error_code=APIERROR.ENOTHIGHENOUGH`
		`+ )`
		`+`
		`+ authorized_acls = pagure_config.get("USER_ACLS", [])`
		`+ form = pagure.forms.NewTokenForm(csrf_enabled=False, sacls=authorized_acls)`
		`+ if form.validate_on_submit():`
		`+ acls = form.acls.data`
		`+ description = form.description.data`
		`+ else:`
		`+ raise pagure.exceptions.APIError(`
		`+ 400, error_code=APIERROR.EINVALIDREQ, errors=form.errors`
		`+ )`
		`+`
		`+ token = pagure.lib.query.add_token_to_user(`
		`+ flask.g.session, project, acls, flask.g.fas_user.user, description`
		`+ )`
		`+ output = {"token": {"description": token.description, "id": token.id}}`
		`+`
		`+ jsonout = flask.jsonify(output)`
		`+ return jsonout`

pagure/forms.py

file modified

+3 -1

		`@@ -407,7 +407,7 @@`


		`class NewTokenForm(PagureForm):`
		`- """ Form to add/change the status of an issue. """`
		`+ """ Form to add a new token. """`

		`description = wtforms.StringField(`
		`"description", [wtforms.validators.Optional()]`
		`@@ -426,6 +426,8 @@`
		`self.acls.choices = [`
		`(acl.name, acl.name) for acl in kwargs["acls"]`
		`]`
		`+ if "sacls" in kwargs:`
		`+ self.acls.choices = [(acl, acl) for acl in kwargs["sacls"]]`


		`class UpdateIssueForm(PagureForm):`

pagure/lib/query.py

file modified

+12 -2

		`@@ -4176,7 +4176,7 @@`

		`session.commit()`

		`- return "Token created"`
		`+ return token`


		`def _convert_markdown(md_processor, text):`
		`@@ -5205,7 +5205,13 @@`


		`def search_token(`
		`- session, acls, user=None, token=None, active=False, expired=False`
		`+ session,`
		`+ acls,`
		`+ user=None,`
		`+ token=None,`
		`+ active=False,`
		`+ expired=False,`
		`+ description=None,`
		`):`
		`""" Searches the API tokens corresponding to the criterias specified.`

		`@@ -5214,6 +5220,7 @@`
		`:arg user: restrict the API tokens to this given user`
		`:arg token: restrict the API tokens to this specified token (if it`
		`exists)`
		`+ :arg description: restrict the API tokens to this given description`
		`"""`
		`query = (`
		`session.query(model.Token)`
		`@@ -5232,6 +5239,9 @@`
		`model.User.user == user`
		`)`

		`+ if description:`
		`+ query = query.filter(model.Token.description == description)`
		`+`
		`if active:`
		`query = query.filter(`
		`model.Token.expiration > datetime.datetime.utcnow()`

pagure/ui/app.py

file modified

+2 -2

		`@@ -1489,7 +1489,7 @@`

		`if form.validate_on_submit():`
		`try:`
		`- msg = pagure.lib.query.add_token_to_user(`
		`+ pagure.lib.query.add_token_to_user(`
		`flask.g.session,`
		`project=None,`
		`description=form.description.data.strip() or None,`
		`@@ -1497,7 +1497,7 @@`
		`username=user.username,`
		`)`
		`flask.g.session.commit()`
		`- flask.flash(msg)`
		`+ flask.flash("Token created")`
		`return flask.redirect(`
		`flask.url_for("ui_ns.user_settings") + "#nav-api-tab"`
		`)`

pagure/ui/repo.py

file modified

+4 -4

		`@@ -2255,7 +2255,7 @@`

		`if form.validate_on_submit():`
		`try:`
		`- msg = pagure.lib.query.add_token_to_user(`
		`+ pagure.lib.query.add_token_to_user(`
		`flask.g.session,`
		`repo,`
		`description=form.description.data.strip() or None,`
		`@@ -2263,7 +2263,7 @@`
		`username=flask.g.fas_user.username,`
		`)`
		`flask.g.session.commit()`
		`- flask.flash(msg)`
		`+ flask.flash("Token created")`
		`return flask.redirect(`
		`flask.url_for(`
		`"ui_ns.view_settings",`
		`@@ -2324,7 +2324,7 @@`
		`if form.validate_on_submit():`
		`acls = [acl.name for acl in token.acls]`
		`try:`
		`- msg = pagure.lib.query.add_token_to_user(`
		`+ pagure.lib.query.add_token_to_user(`
		`flask.g.session,`
		`repo,`
		`description=token.description or None,`
		`@@ -2332,7 +2332,7 @@`
		`username=flask.g.fas_user.username,`
		`)`
		`flask.g.session.commit()`
		`- flask.flash(msg)`
		`+ flask.flash("Token created")`
		`return flask.redirect(`
		`flask.url_for(`
		`"ui_ns.view_settings",`

tests/test_pagure_flask_api_project.py

file modified

+171

		`@@ -4096,6 +4096,177 @@`
		`before["settings"]["issues_default_to_private"] = True`
		`self.assertEqual(after, before)`

		`+ class PagureFlaskApiProjectCreateAPITokenTests(tests.Modeltests):`
		`+ """ Tests for the flask API of pagure for creating user project API token`
		`+ """`
		`+`
		`+ maxDiff = None`
		`+`
		`+ def setUp(self):`
		`+ """ Set up the environnment, ran before every tests. """`
		`+ super(PagureFlaskApiProjectCreateAPITokenTests, self).setUp()`
		`+ tests.create_projects(self.session)`
		`+ tests.create_tokens(self.session, project_id=None)`
		`+ tests.create_tokens_acl(`
		`+ self.session, 'aaabbbcccddd', 'modify_project')`
		`+`
		`+ def test_api_createapitoken_as_owner(self):`
		`+ """ Test accessing api_project_create_token as owner. """`
		`+`
		`+ headers = {'Authorization': 'token aaabbbcccddd'}`
		`+ project = pagure.lib.query._get_project(self.session, 'test')`
		`+ tdescription = 'my new token'`
		`+`
		`+ # Call the api with pingou user token and verify content`
		`+ data = {`
		`+ 'description': tdescription,`
		`+ 'acls': ['pull_request_merge', 'pull_request_comment']`
		`+ }`
		`+ output = self.app.post('/api/0/test/token/new',`
		`+ headers=headers, data=data)`
		`+ self.assertEqual(output.status_code, 200)`
		`+ data = json.loads(output.get_data(as_text=True))`
		`+ tid = pagure.lib.query.search_token(`
		`+ self.session, None, description=tdescription)[0].id`
		`+ self.assertEqual(`
		`+ data,`
		`+ {"token": {`
		`+ "description": tdescription,`
		`+ "id": tid`
		`+ }`
		`+ }`
		`+ )`
		`+ # Create a second token but with faulty acl`
		`+ # Call the api with pingou user token and error code`
		`+ data = {`
		`+ 'description': tdescription,`
		`+ 'acl': ['foo', 'bar']`
		`+ }`
		`+ output = self.app.post('/api/0/test/token/new',`
		`+ headers=headers, data=data)`
		`+ self.assertEqual(output.status_code, 400)`
		`+`
		`+ def test_api_createapitoken_as_admin(self):`
		`+ """ Test accessing api_project_create_token as admin. """`
		`+`
		`+ project = pagure.lib.query._get_project(self.session, 'test')`
		`+`
		`+ # Set the foo user as test project admin`
		`+ pagure.lib.query.add_user_to_project(`
		`+ self.session, project,`
		`+ new_user='foo',`
		`+ user='pingou',`
		`+ access='admin'`
		`+ )`
		`+ self.session.commit()`
		`+`
		`+ # Create modify_project token for foo user`
		`+ token = pagure.lib.query.add_token_to_user(`
		`+ self.session,`
		`+ project=None,`
		`+ acls=['modify_project'],`
		`+ username='foo')`
		`+`
		`+ # Call the connector with foo user token and verify content`
		`+ headers = {'Authorization': 'token %s' % token.id}`
		`+ tdescription = 'my new token'`
		`+`
		`+ # Call the api with pingou user token and verify content`
		`+ data = {`
		`+ 'description': tdescription,`
		`+ 'acls': ['pull_request_merge', 'pull_request_comment']`
		`+ }`
		`+ output = self.app.post('/api/0/test/token/new',`
		`+ headers=headers, data=data)`
		`+ self.assertEqual(output.status_code, 200)`
		`+ data = json.loads(output.get_data(as_text=True))`
		`+ tid = pagure.lib.query.search_token(`
		`+ self.session, None, user='foo', description=tdescription)[0].id`
		`+ self.assertEqual(`
		`+ data,`
		`+ {"token": {`
		`+ "description": tdescription,`
		`+ "id": tid`
		`+ }`
		`+ }`
		`+ )`
		`+`
		`+ def test_api_createapitoken_as_unauthorized(self):`
		`+ """ Test accessing api_project_create_token as project admin`
		`+ but with unauthorized token ACL.`
		`+ """`
		`+`
		`+ project = pagure.lib.query._get_project(self.session, 'test')`
		`+`
		`+ # Set the foo user as test project admin`
		`+ pagure.lib.query.add_user_to_project(`
		`+ self.session, project,`
		`+ new_user='foo',`
		`+ user='pingou',`
		`+ access='admin'`
		`+ )`
		`+ self.session.commit()`
		`+`
		`+ # Create modify_project token for foo user`
		`+ pagure.lib.query.add_token_to_user(`
		`+ self.session,`
		`+ project=None,`
		`+ acls=['create_branch'],`
		`+ username='foo')`
		`+ mtoken = pagure.lib.query.search_token(`
		`+ self.session, ['create_branch'], user='foo')[0]`
		`+`
		`+ # Call the connector with foo user token and verify content`
		`+ headers = {'Authorization': 'token %s' % mtoken.id}`
		`+ tdescription = 'my new token'`
		`+`
		`+ # Call the api with pingou user token and verify content`
		`+ data = {`
		`+ 'description': tdescription,`
		`+ 'acls': ['pull_request_merge', 'pull_request_comment']`
		`+ }`
		`+ output = self.app.post('/api/0/test/token/new',`
		`+ headers=headers, data=data)`
		`+ self.assertEqual(output.status_code, 401)`
		`+`
		`+ def test_api_createapitoken_as_unauthorized_2(self):`
		`+ """ Test accessing api_project_create_token as project user`
		`+ with unauthorized token ACL.`
		`+ """`
		`+`
		`+ project = pagure.lib.query._get_project(self.session, 'test')`
		`+`
		`+ # Set the foo user as test project admin`
		`+ pagure.lib.query.add_user_to_project(`
		`+ self.session, project,`
		`+ new_user='foo',`
		`+ user='pingou',`
		`+ access='commit'`
		`+ )`
		`+ self.session.commit()`
		`+`
		`+ # Create modify_project token for foo user`
		`+ pagure.lib.query.add_token_to_user(`
		`+ self.session,`
		`+ project=None,`
		`+ acls=['modify_project'],`
		`+ username='foo')`
		`+ mtoken = pagure.lib.query.search_token(`
		`+ self.session, ['modify_project'], user='foo')[0]`
		`+`
		`+ # Call the connector with foo user token and verify content`
		`+ headers = {'Authorization': 'token %s' % mtoken.id}`
		`+ tdescription = 'my new token'`
		`+`
		`+ # Call the api with pingou user token and verify content`
		`+ data = {`
		`+ 'description': tdescription,`
		`+ 'acls': ['pull_request_merge', 'pull_request_comment']`
		`+ }`
		`+ output = self.app.post('/api/0/test/token/new',`
		`+ headers=headers, data=data)`
		`+ self.assertEqual(output.status_code, 401)`
		`+`

		`class PagureFlaskApiProjectConnectorTests(tests.Modeltests):`
		`""" Tests for the flask API of pagure for getting connector of a project`

fbo commented 5 years ago

A project owner or admin (with the 'modify_project') acl can
ask the creation of a project user token. The token
description and acl can be passed to the endpoint.

The endpoint return the token id and the description.

1 new commit added

Attempt to fix wrong code stype

5 years ago

ngompa commented on line 7 of pagure/api/project.py 5 years ago

Wouldn't it be better to do /<repo>/token/create or something? createapitoken is a pretty poorly named endpoint...

1 new commit added

Propose a better API endpoint name repo/token/create

5 years ago

fbo commented 5 years ago

Thanks, yes you are right so here is an update.

pingou commented on line 91 of pagure/api/project.py 5 years ago

You could use a wtforms form here to do the validation for you (disable the csrf protection and dynamically load the list of ACLs), there are some examples of this in the code already (let me know if you don't find one)

pingou commented on line 99 of pagure/api/project.py 5 years ago

Let's specify that it's acls=None here :)

pingou commented on line 29 of tests/test_pagure_flask_api_project.py 5 years ago

We should use a list here, saves us the troubles of splitting the content on comas

pingou commented on line 83 of pagure/api/project.py 5 years ago

You may want to use form.getlist('acl') here ;-)

pingou commented 5 years ago

@fbo if you would have time to update this one it would be nice, this way we could get it into the 5.3-beta I hope to cut tomorrow

fbo commented 5 years ago

Sure, I'll travel tomorrow morning but I'll be able to finish it (and the other PR (hope)) in the train.

1 new commit added

Use wtforms to validate a new token in the projects api

5 years ago

pingou commented on line 79 of pagure/api/project.py 5 years ago

I don't think we want the full list here, more likely the USER_ACLS list only

pingou commented on line 94 of pagure/api/project.py 5 years ago

descriptions are not unique, so depending on the ordering you may get an expired token here. Should we make add_token_to_user return the token id instead?

1 new commit added

Improve add_token_to_user to return the token

5 years ago

rebased onto 7c5cfacce801976a43080fa0c86fa999ead34e97

5 years ago

fbo commented 5 years ago

Any idea why the CI fails this way ? is it related to the patch ?

pretty please pagure-ci rebuild

5 years ago

pretty please pagure-ci rebuild

5 years ago

rebased onto 969d10ecf6cbe31a380de5b6e65c039ddcaf4cf8

5 years ago

pingou commented 5 years ago

Looks like at least one test is failing (genuinely)

rebased onto fa793fd116352d4495f19364e48a427edbb3b33a

5 years ago

pretty please pagure-ci rebuild

5 years ago

pingou commented 5 years ago

It returns three failing test files:

15:50:35 FAILED test: py-test_pagure_repospanner
15:50:35 FAILED test: py-test_pagure_flask_ui_repo
15:50:35 FAILED test: py-test_pagure_flask_ui_app

The first one currently fails on all tests/PRs (I hope the new way to run the tests will help with this), the other two are more concerning me :(

fbo commented 5 years ago

A local run with the container reports:
Failed tests:
FAILED test: py3-test_style
FAILED test: py3-test_pagure_flask_ui_repo
FAILED test: py3-test_pagure_flask_ui_app

fbo commented 5 years ago

Ok so then let me know if the patch needs more edit or if it ready to be merged :)

pretty please pagure-ci rebuild

5 years ago

pretty please pagure-ci rebuild

5 years ago

fbo commented 5 years ago

91 failed :( should I rebase the branch on master ?

rebased onto 0bf7de9

5 years ago

pingou commented on line 7 of pagure/api/project.py 5 years ago

The UI endpoint seems to be /<repo>/token/new, should we align the API with the UI?

pingou commented 5 years ago

Ok, I've had to apply the following two patches to get this PR to pass tests:

3 new commits added

Align new token API endpoint with UI endpoint
Black8 fixes
Fix the tests

5 years ago

pretty please pagure-ci rebuild

5 years ago

pingou commented 5 years ago

Black seems still unhappy: https://ci.centos.org/job/pagure-pr/2212/artifact/pagure/results_f29-rpms-py3/py3-test_style/*view*/

pingou commented 5 years ago

This is the change black does locally:

diff --git a/ pagure/api/project.py b/ pagure/api/project.py
index 20c6e8af..e947a306 100644
--- a/ pagure/api/project.py    
+++ b/ pagure/api/project.py    
@@ -2138,9 +2138,7 @@ def api_modify_project_options(repo, username=None, namespace=None):
 @API.route("/<repo>/token/new", methods=["POST"])
 @API.route("/<namespace>/<repo>/token/new", methods=["POST"])
 @API.route("/fork/<username>/<repo>/token/new", methods=["POST"])
-@API.route(
-    "/fork/<username>/<namespace>/<repo>/token/new", methods=["POST"]
-)
+@API.route("/fork/<username>/<namespace>/<repo>/token/new", methods=["POST"])
 @api_login_required(acls=["modify_project"])
 @api_method
 def api_project_create_api_token(repo, namespace=None, username=None):

fbo commented 5 years ago

Thanks I'll fix that.

fbo commented 5 years ago

Curious, I've run the full test suite locally and black has not complain.

1 new commit added