From ba2b1caae8a561373cdd93178cf5c5d1e785228c Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Apr 07 2017 15:06:18 +0000 Subject: [PATCH 1/3] Store in the DB the API token used to flag a pull-request --- diff --git a/alembic/versions/4df75d40bafa_log_the_token_used_when_flagging_a_pr.py b/alembic/versions/4df75d40bafa_log_the_token_used_when_flagging_a_pr.py new file mode 100644 index 0000000..8cb887f --- /dev/null +++ b/alembic/versions/4df75d40bafa_log_the_token_used_when_flagging_a_pr.py @@ -0,0 +1,33 @@ +"""Log the token used when flagging a PR + +Revision ID: 4df75d40bafa +Revises: 3ffec872dfdf +Create Date: 2017-04-04 16:26:58.352213 + +""" + +from alembic import op +import sqlalchemy as sa + +# revision identifiers, used by Alembic. +revision = '4df75d40bafa' +down_revision = '3ffec872dfdf' + + +def upgrade(): + ''' Add the foreign key token_id to the table pull_request_flags. + ''' + op.add_column( + 'pull_request_flags', + sa.Column( + 'token_id', + sa.String(64), + sa.ForeignKey('tokens.id'), + nullable=True + ) + ) + +def downgrade(): + ''' Remove the column token_id from the table pull_request_flags. + ''' + op.drop_column('pull_request_flags', 'token_id') diff --git a/pagure/api/fork.py b/pagure/api/fork.py index 35bf16d..925a86c 100644 --- a/pagure/api/fork.py +++ b/pagure/api/fork.py @@ -681,6 +681,7 @@ def api_pull_request_add_flag(repo, requestid, username=None, namespace=None): url=url, uid=uid, user=flask.g.fas_user.username, + token=flask.g.token.id, requestfolder=APP.config['REQUESTS_FOLDER'], ) SESSION.commit() diff --git a/pagure/lib/__init__.py b/pagure/lib/__init__.py index 3c13620..499b99b 100644 --- a/pagure/lib/__init__.py +++ b/pagure/lib/__init__.py @@ -1218,7 +1218,7 @@ def edit_comment(session, parent, comment, user, def add_pull_request_flag(session, request, username, percent, comment, url, - uid, user, requestfolder): + uid, user, token, requestfolder): ''' Add a flag to a pull-request. ''' user_obj = get_user(session, user) @@ -1238,6 +1238,7 @@ def add_pull_request_flag(session, request, username, percent, comment, url, comment=comment, url=url, user_id=user_obj.id, + token_id=token, ) session.add(pr_flag) # Make sure we won't have SQLAlchemy error before we continue diff --git a/pagure/lib/model.py b/pagure/lib/model.py index d6941b8..d258c36 100644 --- a/pagure/lib/model.py +++ b/pagure/lib/model.py @@ -1705,6 +1705,11 @@ class PullRequestFlag(BASE): 'pull_requests.uid', ondelete='CASCADE', onupdate='CASCADE', ), nullable=False) + token_id = sa.Column( + sa.String(64), sa.ForeignKey( + 'tokens.id', + ), + nullable=False) user_id = sa.Column( sa.Integer, sa.ForeignKey( diff --git a/tests/test_pagure_lib.py b/tests/test_pagure_lib.py index 60dcec5..42b0447 100644 --- a/tests/test_pagure_lib.py +++ b/tests/test_pagure_lib.py @@ -2037,6 +2037,7 @@ class PagureLibtests(tests.Modeltests): mockemail.return_value = True self.test_new_pull_request() + tests.create_tokens(self.session) request = pagure.lib.search_pull_requests(self.session, requestid=1) self.assertEqual(len(request.flags), 0) @@ -2050,12 +2051,14 @@ class PagureLibtests(tests.Modeltests): url="http://jenkins.cloud.fedoraproject.org", uid="jenkins_build_pagure_34", user='foo', + token='aaabbbcccddd', requestfolder=None, ) self.assertEqual(msg, 'Flag added') self.session.commit() self.assertEqual(len(request.flags), 1) + self.assertEqual(request.flags[0].token_id, 'aaabbbcccddd') def test_search_pull_requests(self): """ Test search_pull_requests of pagure.lib. """ From 888e4451dfcb77703aab2c5d24c7a04a63576e5b Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Apr 07 2017 15:34:45 +0000 Subject: [PATCH 2/3] Use a validator to keep the model in consistent with the alembic migration This way we ensure the token_id cannot be null in the software level since we cannot do that at the DB level. We cannot do it at the DB level since for existing entries in the DB we cannot find back what was the token_id. So there are two ways: - have the model define nullable=False and the migration nullable=True - be consistent between the model and the migration and enforce nullable=False for new entries in the software. This commit applies the second option. --- diff --git a/pagure/lib/model.py b/pagure/lib/model.py index d258c36..dc8ecbd 100644 --- a/pagure/lib/model.py +++ b/pagure/lib/model.py @@ -27,6 +27,7 @@ from sqlalchemy.orm import backref from sqlalchemy.orm import sessionmaker from sqlalchemy.orm import scoped_session from sqlalchemy.orm import relation +from sqlalchemy.orm import validates CONVENTION = { @@ -1709,7 +1710,7 @@ class PullRequestFlag(BASE): sa.String(64), sa.ForeignKey( 'tokens.id', ), - nullable=False) + nullable=True) user_id = sa.Column( sa.Integer, sa.ForeignKey( @@ -1747,6 +1748,11 @@ class PullRequestFlag(BASE): foreign_keys=[pull_request_uid], remote_side=[PullRequest.uid]) + @validates('token_id') + def validate_token_id(self, _, token_id): + assert token_id is not None + return token_id + def to_json(self, public=False): ''' Returns a dictionnary representation of the pull-request. diff --git a/tests/test_pagure_lib.py b/tests/test_pagure_lib.py index 42b0447..e5fae2a 100644 --- a/tests/test_pagure_lib.py +++ b/tests/test_pagure_lib.py @@ -2060,6 +2060,35 @@ class PagureLibtests(tests.Modeltests): self.assertEqual(len(request.flags), 1) self.assertEqual(request.flags[0].token_id, 'aaabbbcccddd') + @patch('pagure.lib.notify.send_email') + def test_add_pull_request_flag_no_token(self, mockemail): + """ Test add_pull_request_flag of pagure.lib. """ + mockemail.return_value = True + + self.test_new_pull_request() + tests.create_tokens(self.session) + + request = pagure.lib.search_pull_requests(self.session, requestid=1) + self.assertEqual(len(request.flags), 0) + + self.assertRaises( + AssertionError, + pagure.lib.add_pull_request_flag, + session=self.session, + request=request, + username="jenkins", + percent=100, + comment="Build passes", + url="http://jenkins.cloud.fedoraproject.org", + uid="jenkins_build_pagure_34", + user='foo', + token=None, + requestfolder=None, + ) + + request = pagure.lib.search_pull_requests(self.session, requestid=1) + self.assertEqual(len(request.flags), 0) + def test_search_pull_requests(self): """ Test search_pull_requests of pagure.lib. """ From 841124954c27047ad8b1ca89875e99c0d63d09bd Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Apr 10 2017 08:01:46 +0000 Subject: [PATCH 3/3] Adjust the validate_token_id validator as advised by @bowlofeggs --- diff --git a/pagure/lib/model.py b/pagure/lib/model.py index dc8ecbd..b6fbedb 100644 --- a/pagure/lib/model.py +++ b/pagure/lib/model.py @@ -1750,7 +1750,8 @@ class PullRequestFlag(BASE): @validates('token_id') def validate_token_id(self, _, token_id): - assert token_id is not None + if token_id is None: + raise ValueError('token_id may not be None.') return token_id def to_json(self, public=False): diff --git a/tests/test_pagure_lib.py b/tests/test_pagure_lib.py index e5fae2a..39784cc 100644 --- a/tests/test_pagure_lib.py +++ b/tests/test_pagure_lib.py @@ -2072,7 +2072,7 @@ class PagureLibtests(tests.Modeltests): self.assertEqual(len(request.flags), 0) self.assertRaises( - AssertionError, + ValueError, pagure.lib.add_pull_request_flag, session=self.session, request=request,