diff --git a/contrib/buildbot/server.py b/contrib/buildbot/server.py --- a/contrib/buildbot/server.py +++ b/contrib/buildbot/server.py @@ -373,12 +373,6 @@ if not comments: return SUCCESS, 200 - # In order to prevent DoS, only ABC members are allowed to call the bot - # to trigger builds. - # FIXME implement a better anti DoS filter. - abc_members = phab.get_project_members(BITCOIN_ABC_PROJECT_PHID) - comments = [c for c in comments if c["authorPHID"] in abc_members] - # Check if there is a specially crafted comment that should trigger a # CI build. Format: # @bot [build_name ...] @@ -389,15 +383,80 @@ # Escape to prevent shell injection and remove duplicates return [quote(token) for token in list(set(tokens))] + def next_token(current_token): + next_token = { + "": "PHID-TOKN-coin-1", + "PHID-TOKN-coin-1": "PHID-TOKN-coin-2", + "PHID-TOKN-coin-2": "PHID-TOKN-coin-3", + "PHID-TOKN-coin-3": "PHID-TOKN-coin-4", + "PHID-TOKN-coin-4": "PHID-TOKN-like-1", + "PHID-TOKN-like-1": "PHID-TOKN-heart-1", + "PHID-TOKN-heart-1": "PHID-TOKN-like-1", + } + return next_token[current_token] if current_token in next_token else "PHID-TOKN-like-1" + + def is_user_allowed_to_trigger_builds(user_PHID, current_token): + if current_token not in [ + "", "PHID-TOKN-coin-1", "PHID-TOKN-coin-2", "PHID-TOKN-coin-3"]: + return False + + return all(role in phab.get_user_roles(user_PHID) for role in [ + "verified", + "approved", + "activated", + ]) + + # Anti DoS filter + # + # Users are allowed to trigger builds if these conditions are met: + # - It is an ABC member + # OR + # | - It is a "verified", "approved" and "activated" user + # | AND + # | - The maximum number of requests for this revision has not been + # | reached yet. + # + # The number of requests is tracked by awarding a coin token to the + # revision each time a build request is submitted (the number of build + # in that request is not taken into account). + # The awarded coin token is graduated as follow: + # "Haypence" => "Piece of Eight" => "Dubloon" => "Mountain of Wealth". + # If the "Mountain of Wealth" token is reached, the next request will be + # refused by the bot. At this stage only ABC members will be able to + # trigger new builds. + abc_members = phab.get_project_members(BITCOIN_ABC_PROJECT_PHID) + current_token = phab.get_object_token(revision_PHID) + builds = [] for comment in comments: - builds += get_builds_from_comment(comment["content"]["raw"]) + comment_builds = get_builds_from_comment(comment["content"]["raw"]) + + # Parsing the string is cheaper than phabricator requests, so check + # if the comment is for us prior to filtering on the user. + if not comment_builds: + continue + + user = comment["authorPHID"] + + # ABC members can always trigger builds + if user in abc_members: + builds += comment_builds + continue + + if is_user_allowed_to_trigger_builds(user, current_token): + builds += comment_builds + # If there is no build provided, this request is not what we are after, # just return. # TODO return an help command to explain how to use the bot. if not builds: return SUCCESS, 200 + # Give (only positive) feedback to user. If several comments are part of + # the same transaction then there is no way to differentiate what the + # token is for; however this is very unlikely to happen in real life. + phab.set_object_token(revision_PHID, next_token(current_token)) + staging_ref = phab.get_latest_diff_staging_ref(revision_PHID) # Trigger the requested builds for build in builds: diff --git a/contrib/buildbot/test/mocks/phabricator.py b/contrib/buildbot/test/mocks/phabricator.py --- a/contrib/buildbot/test/mocks/phabricator.py +++ b/contrib/buildbot/test/mocks/phabricator.py @@ -55,6 +55,7 @@ phab.project.search.return_value = Result([]) phab.token = mock.Mock() + phab.token.given.return_value = [] phab.transaction = mock.Mock() phab.transaction.search.return_value = Result([]) diff --git a/contrib/buildbot/test/test_endpoint_triggerCI.py b/contrib/buildbot/test/test_endpoint_triggerCI.py --- a/contrib/buildbot/test/test_endpoint_triggerCI.py +++ b/contrib/buildbot/test/test_endpoint_triggerCI.py @@ -4,6 +4,7 @@ # Distributed under the MIT software license, see the accompanying # file COPYING or http://www.opensource.org/licenses/mit-license.php. +import itertools import mock import unittest from unittest.mock import call @@ -320,6 +321,118 @@ ] ) + def test_triggerCI_check_user_roles(self): + user_PHID = "PHID-USER-notabc" + + # No build triggered, exit status OK + def check_build_triggered(expect_trigger): + self.teamcity.trigger_build.reset_mock() + self.set_transaction_return_value(["@bot build-1"], user_PHID) + response = self.call_endpoint() + self.phab.user.search.assert_called_with( + constraints={ + "phids": [user_PHID], + } + ) + if not expect_trigger: + self.teamcity.trigger_build.assert_not_called() + else: + self.teamcity.trigger_build.assert_called_once_with( + "BitcoinABC_BitcoinAbcStaging", + "refs/tags/phabricator/diff/{}".format(self.diff_id), + properties=[{ + 'name': 'env.ABC_BUILD_NAME', + 'value': "build-1", + }] + ) + self.assertEqual(response.status_code, 200) + + def set_user_roles(roles): + self.phab.user.search.return_value = test.mocks.phabricator.Result([ + { + "id": 1, + "type": "USER", + "phid": user_PHID, + "fields": { + "roles": roles, + }, + }, + ]) + + roles = [ + "verified", + "approved", + "activated", + ] + + # No role, no chocolate + set_user_roles([]) + check_build_triggered(False) + + # Single role from the required list + for role in roles: + set_user_roles([role]) + check_build_triggered(False) + + # 2 roles out of 3 + for role_combination in itertools.combinations(roles, 2): + set_user_roles(list(role_combination)) + check_build_triggered(False) + + # With all roles the build should be called... + set_user_roles(roles) + check_build_triggered(True) + + permissive_tokens = [ + "", + "PHID-TOKN-coin-1", + "PHID-TOKN-coin-2", + "PHID-TOKN-coin-3", + ] + restrictive_tokens = [ + "PHID-TOKN-coin-4", + "PHID-TOKN-like-1", + "PHID-TOKN-heart-1", + ] + + # ...until some token is awarded... + for token_PHID in permissive_tokens: + self.phab.token.given.return_value = [{"tokenPHID": token_PHID}] + check_build_triggered(True) + + # ...then the build is denied + for token_PHID in restrictive_tokens: + self.phab.token.given.return_value = [{"tokenPHID": token_PHID}] + check_build_triggered(False) + + # If the token is not one from the expected list, the request is denied + self.phab.token.given.return_value = [{"tokenPHID": "PHID-TOKN-dummy"}] + check_build_triggered(False) + + def test_triggerCI_token_auctions(self): + self.set_transaction_return_value(["@bot build-1 build-2"]) + + tokens = [ + "", + "PHID-TOKN-coin-1", + "PHID-TOKN-coin-2", + "PHID-TOKN-coin-3", + "PHID-TOKN-coin-4", + "PHID-TOKN-like-1", + "PHID-TOKN-heart-1", + "PHID-TOKN-like-1", + "PHID-TOKN-heart-1", + ] + + for i in range(len(tokens) - 1): + self.phab.token.given.return_value = [{"tokenPHID": tokens[i]}] + self.call_endpoint() + self.phab.token.give.assert_called_once_with( + objectPHID=self.revision_PHID, + tokenPHID=tokens[i + 1], + ) + self.phab.token.give.reset_mock() + if __name__ == '__main__': unittest.main()