diff --git a/services/bots.py b/services/bots.py index 21d7c9c01..1e9ed2a4d 100644 --- a/services/bots.py +++ b/services/bots.py @@ -5,6 +5,7 @@ from shared.torngit.base import TokenType from database.models import Owner, Repository +from database.models.core import GITHUB_APP_INSTALLATION_DEFAULT_NAME from helpers.environment import is_enterprise from helpers.exceptions import OwnerWithoutValidBotError, RepositoryWithoutValidBotError from services.encryption import encryptor @@ -13,15 +14,46 @@ log = logging.getLogger(__name__) +def get_owner_installation_id( + owner: Owner, + deprecated_using_integration: bool, + *, + repository: Optional[Repository] = None, + installation_name: str = GITHUB_APP_INSTALLATION_DEFAULT_NAME, +) -> Optional[int]: + default_app_installation_filter = filter( + lambda obj: obj.name == installation_name, owner.github_app_installations or [] + ) + # filter is an Iterator, so we need to scan matches + # in practice we only consider the 1st one + for app_installation in default_app_installation_filter: + if repository: + if app_installation.is_repo_covered_by_integration(repository): + return app_installation.installation_id + # The repo we want to get a token for is not covered by the installation + return None + else: + # Getting owner installation - not tied to any particular repo + return app_installation.installation_id + # DEPRECATED FLOW - begin + if owner.integration_id and deprecated_using_integration: + return owner.integration_id + # DEPRECATED FLOW - end + return None + + def get_repo_appropriate_bot_token(repo: Repository) -> Tuple[Dict, Optional[Owner]]: if is_enterprise() and get_config(repo.service, "bot"): return get_public_bot_token(repo) - if repo.using_integration and repo.owner.integration_id: - github_token = get_github_integration_token( - repo.owner.service, repo.owner.integration_id - ) + + installation_id = get_owner_installation_id( + repo.owner, repo.using_integration, repository=repo + ) + if installation_id: + github_token = get_github_integration_token(repo.owner.service, installation_id) + installation_token = dict(key=github_token) # The token is not owned by an Owner object, so 2nd arg is None - return dict(key=github_token), None + return installation_token, None try: token_dict, appropriate_bot = get_repo_particular_bot_token(repo) return token_dict, appropriate_bot @@ -55,7 +87,10 @@ def get_repo_particular_bot_token(repo) -> Tuple[Dict, Owner]: def get_token_type_mapping(repo: Repository): if repo.private: return None - if repo.using_integration and repo.owner.integration_id: + installation_id = get_owner_installation_id( + repo.owner, repo.using_integration, repository=repo + ) + if installation_id: return None admin_bot = None try: @@ -101,9 +136,11 @@ def _get_repo_appropriate_bot(repo: Repository) -> Owner: def get_owner_appropriate_bot_token(owner, using_integration) -> Dict: - if owner.integration_id and using_integration: - github_token = get_github_integration_token(owner.service, owner.integration_id) + installation_id = get_owner_installation_id(owner, using_integration) + if installation_id: + github_token = get_github_integration_token(owner.service, installation_id) return dict(key=github_token) + return encryptor.decrypt_token(_get_owner_or_appropriate_bot(owner).oauth_token) diff --git a/services/notification/__init__.py b/services/notification/__init__.py index 9f8e6d796..4e2561a87 100644 --- a/services/notification/__init__.py +++ b/services/notification/__init__.py @@ -14,6 +14,7 @@ from shared.helpers.yaml import default_if_true from shared.yaml import UserYaml +from database.models.core import GITHUB_APP_INSTALLATION_DEFAULT_NAME from helpers.metrics import metrics from services.comparison.types import Comparison from services.decoration import Decoration @@ -48,6 +49,30 @@ def __init__( self.current_yaml = current_yaml self.decoration_type = decoration_type + def _should_use_checks_notifier(self) -> bool: + checks_yaml_field = read_yaml_field(self.current_yaml, ("github_checks",)) + if checks_yaml_field is False: + return False + + owner = self.repository.owner + default_app_installation_filter = filter( + lambda obj: obj.name == GITHUB_APP_INSTALLATION_DEFAULT_NAME, + owner.github_app_installations or [], + ) + # filter is an Iterator, so we need to scan matches + # in practice we only consider the 1st one + for app_installation in default_app_installation_filter: + return app_installation.is_repo_covered_by_integration(self.repository) + # DEPRECATED FLOW + return ( + self.repository.using_integration + and self.repository.owner.integration_id + and ( + self.repository.owner.service == "github" + or self.repository.owner.service == "github_enterprise" + ) + ) + def get_notifiers_instances(self) -> Iterator[AbstractBaseNotifier]: mapping = get_all_notifier_classes_mapping() yaml_field = read_yaml_field(self.current_yaml, ("coverage", "notify")) @@ -65,19 +90,11 @@ def get_notifiers_instances(self) -> Iterator[AbstractBaseNotifier]: current_yaml=self.current_yaml, decoration_type=self.decoration_type, ) - checks_yaml_field = read_yaml_field(self.current_yaml, ("github_checks",)) + current_flags = [rf.flag_name for rf in self.repository.flags if not rf.deleted] for key, title, status_config in self.get_statuses(current_flags): status_notifier_class = get_status_notifier_class(key, "status") - if ( - self.repository.using_integration - and self.repository.owner.integration_id - and ( - self.repository.owner.service == "github" - or self.repository.owner.service == "github_enterprise" - ) - and checks_yaml_field is not False - ): + if self._should_use_checks_notifier(): checks_notifier = get_status_notifier_class(key, "checks") yield ChecksWithFallback( checks_notifier=checks_notifier( diff --git a/services/notification/notifiers/mixins/message/__init__.py b/services/notification/notifiers/mixins/message/__init__.py index 5c8d57fa9..dfddc0a05 100644 --- a/services/notification/notifiers/mixins/message/__init__.py +++ b/services/notification/notifiers/mixins/message/__init__.py @@ -169,6 +169,7 @@ async def _possibly_write_gh_app_login_announcement( if ( owner.service == "github" and not owner.integration_id + and owner.github_app_installations == [] and not is_enterprise() ): message_to_display = "Your organization needs to install the [Codecov GitHub app](https://github.com/apps/codecov/installations/select_target) to enable full functionality." diff --git a/services/notification/notifiers/tests/unit/test_comment.py b/services/notification/notifiers/tests/unit/test_comment.py index b182509c3..9e9e3720f 100644 --- a/services/notification/notifiers/tests/unit/test_comment.py +++ b/services/notification/notifiers/tests/unit/test_comment.py @@ -16,6 +16,7 @@ from shared.yaml import UserYaml import services.notification.notifiers.mixins.message.sections as sections +from database.models.core import GithubAppInstallation from database.tests.factories import RepositoryFactory from services.comparison.overlays.critical_path import CriticalPathOverlay from services.decoration import Decoration @@ -481,6 +482,38 @@ async def test__possibly_write_gh_app_login_announcement_already_using_integrati ) assert mock_write.call_count == 0 + @pytest.mark.asyncio + async def test__possibly_write_gh_app_login_announcement_has_ghapp_installation( + self, dbsession, mocker + ): + repository = RepositoryFactory.create( + owner__service="github", owner__integration_id=None + ) + ghapp_installation = GithubAppInstallation( + owner=repository.owner, installation_id=12345, repository_service_ids=None + ) + dbsession.add_all([repository, ghapp_installation]) + dbsession.flush() + mocker.patch( + "services.notification.notifiers.mixins.message.is_enterprise", + return_value=False, + ) + mock_write = mocker.MagicMock() + + notifier = CommentNotifier( + repository=repository, + title="some_title", + notifier_yaml_settings=False, + notifier_site_settings=None, + current_yaml={}, + ) + fake_comparison = mocker.MagicMock() + fake_comparison.head.commit.repository = repository + await notifier._possibly_write_gh_app_login_announcement( + fake_comparison, mock_write + ) + assert mock_write.call_count == 0 + @pytest.mark.asyncio async def test__possibly_write_gh_app_login_announcement_enterprise( self, dbsession, mocker diff --git a/services/notification/tests/unit/test_notification_service.py b/services/notification/tests/unit/test_notification_service.py index b1d451c34..64d4dac7a 100644 --- a/services/notification/tests/unit/test_notification_service.py +++ b/services/notification/tests/unit/test_notification_service.py @@ -9,6 +9,10 @@ from shared.yaml import UserYaml from database.enums import Decoration, Notification, NotificationState +from database.models.core import ( + GITHUB_APP_INSTALLATION_DEFAULT_NAME, + GithubAppInstallation, +) from database.tests.factories import CommitFactory, PullFactory, RepositoryFactory from services.comparison import ComparisonProxy from services.comparison.types import Comparison, EnrichedPull, FullCommit @@ -47,6 +51,91 @@ def sample_comparison(dbsession, request): class TestNotificationService(object): + def test_should_use_checks_notifier_yaml_field_false(self, dbsession): + repository = RepositoryFactory.create() + current_yaml = {"github_checks": False} + service = NotificationService(repository, current_yaml) + assert service._should_use_checks_notifier() == False + + @pytest.mark.parametrize( + "repo_data,outcome", + [ + ( + dict( + using_integration=True, + owner__integration_id=12341234, + owner__service="github", + ), + True, + ), + ( + dict( + using_integration=True, + owner__integration_id=12341234, + owner__service="gitlab", + ), + False, + ), + ( + dict( + using_integration=True, + owner__integration_id=12341234, + owner__service="github_enterprise", + ), + True, + ), + ( + dict( + using_integration=False, + owner__integration_id=None, + owner__service="github", + ), + False, + ), + ], + ) + def test_should_use_checks_notifier_deprecated_flow( + self, repo_data, outcome, dbsession + ): + repository = RepositoryFactory.create(**repo_data) + current_yaml = {"github_checks": True} + assert repository.owner.github_app_installations == [] + service = NotificationService(repository, current_yaml) + assert service._should_use_checks_notifier() == outcome + + def test_should_use_checks_notifier_ghapp_all_repos_covered(self, dbsession): + repository = RepositoryFactory.create() + ghapp_installation = GithubAppInstallation( + name=GITHUB_APP_INSTALLATION_DEFAULT_NAME, + installation_id=456789, + owner=repository.owner, + repository_service_ids=None, + ) + dbsession.add(ghapp_installation) + dbsession.flush() + current_yaml = {"github_checks": True} + assert repository.owner.github_app_installations == [ghapp_installation] + service = NotificationService(repository, current_yaml) + assert service._should_use_checks_notifier() == True + + def test_should_use_checks_notifier_ghapp_some_repos_covered(self, dbsession): + repository = RepositoryFactory.create() + other_repo_same_owner = RepositoryFactory.create(owner=repository.owner) + ghapp_installation = GithubAppInstallation( + name=GITHUB_APP_INSTALLATION_DEFAULT_NAME, + installation_id=456789, + owner=repository.owner, + repository_service_ids=[repository.service_id], + ) + dbsession.add(ghapp_installation) + dbsession.flush() + current_yaml = {"github_checks": True} + assert repository.owner.github_app_installations == [ghapp_installation] + service = NotificationService(repository, current_yaml) + assert service._should_use_checks_notifier() == True + service = NotificationService(other_repo_same_owner, current_yaml) + assert service._should_use_checks_notifier() == False + def test_get_notifiers_instances_only_third_party( self, dbsession, mock_configuration ): diff --git a/services/repository.py b/services/repository.py index b7d425d11..e78b30098 100644 --- a/services/repository.py +++ b/services/repository.py @@ -15,6 +15,10 @@ from sqlalchemy.exc import IntegrityError from database.models import Commit, Owner, Pull, Repository +from database.models.core import ( + GITHUB_APP_INSTALLATION_DEFAULT_NAME, + GithubAppInstallation, +) from helpers.token_refresh import get_token_refresh_callback from services.bots import get_repo_appropriate_bot_token, get_token_type_mapping from services.yaml import read_yaml_field @@ -24,6 +28,20 @@ merged_pull = re.compile(r".*Merged in [^\s]+ \(pull request \#(\d+)\).*").match +def _is_repo_using_integration(repo: Repository) -> bool: + owner = repo.owner + default_ghapp_installation = list( + filter( + lambda obj: obj.name == GITHUB_APP_INSTALLATION_DEFAULT_NAME, + owner.github_app_installations or [], + ) + ) + if default_ghapp_installation: + ghapp_installation = owner.github_app_installations[0] + return ghapp_installation.is_repo_covered_by_integration(repo) + return repo.using_integration + + def get_repo_provider_service( repository, commit=None ) -> torngit.base.TorngitBaseAdapter: @@ -36,7 +54,7 @@ def get_repo_provider_service( adapter_params = dict( repo=dict( name=repository.name, - using_integration=repository.using_integration or False, + using_integration=_is_repo_using_integration(repository), service_id=repository.service_id, repoid=repository.repoid, ), diff --git a/services/tests/test_bots.py b/services/tests/test_bots.py index 3f9138df3..c5a87bef2 100644 --- a/services/tests/test_bots.py +++ b/services/tests/test_bots.py @@ -1,11 +1,16 @@ import pytest +from database.models.core import ( + GITHUB_APP_INSTALLATION_DEFAULT_NAME, + GithubAppInstallation, +) from database.tests.factories import OwnerFactory, RepositoryFactory from services.bots import ( OwnerWithoutValidBotError, RepositoryWithoutValidBotError, TokenType, get_owner_appropriate_bot_token, + get_owner_installation_id, get_repo_appropriate_bot_token, get_repo_particular_bot_token, get_token_type_mapping, @@ -277,6 +282,37 @@ def test_get_repo_appropriate_bot_token_repo_with_user_with_integration_bot_usin expected_result = ({"key": "v1.test50wm4qyel2pbtpbusklcarg7c2etcbunnswp"}, None) assert get_repo_appropriate_bot_token(repo) == expected_result + def test_get_repo_appropriate_bot_token_via_installation_covered_repo( + self, mock_configuration, dbsession, mocker + ): + owner = OwnerFactory.create( + service="github", + integration_id=None, + unencrypted_oauth_token="owner_token", + ) + repo = RepositoryFactory(owner=owner, using_integration=False) + installation = GithubAppInstallation( + installation_id=12341234, + name=GITHUB_APP_INSTALLATION_DEFAULT_NAME, + repository_service_ids=None, # all repos covered + owner=owner, + ) + dbsession.add(installation) + dbsession.flush() + + assert owner.github_app_installations == [installation] + assert installation.is_repo_covered_by_integration(repo) + + mock_get_github_integration_token = mocker.patch( + "services.bots.get_github_integration_token", + return_value="installation_token", + ) + assert get_repo_appropriate_bot_token(repo) == ( + {"key": "installation_token"}, + None, + ) + mock_get_github_integration_token.assert_called_with("github", 12341234) + def test_get_owner_appropriate_bot_token_owner_no_bot_no_integration(self): owner = OwnerFactory.create( unencrypted_oauth_token="owner_token", integration_id=None, bot=None @@ -332,6 +368,72 @@ def test_get_owner_appropriate_bot_token_with_user_with_integration_bot_using_it == expected_result ) + def test_get_owner_installation_id_no_installation_no_legacy_integration( + self, mocker, dbsession + ): + owner = OwnerFactory(service="github", integration_id=None) + assert owner.github_app_installations == [] + assert get_owner_installation_id(owner, True) is None + + def test_get_owner_installation_id_no_installation_yes_legacy_integration( + self, mocker, dbsession + ): + owner = OwnerFactory(service="github", integration_id=12341234) + assert owner.github_app_installations == [] + assert get_owner_installation_id(owner, True) == 12341234 + + def test_get_owner_installation_id_yes_installation_yes_legacy_integration( + self, mocker, dbsession + ): + owner = OwnerFactory(service="github", integration_id=12341234) + installation = GithubAppInstallation( + owner=owner, + name=GITHUB_APP_INSTALLATION_DEFAULT_NAME, + repository_service_ids=None, + installation_id=123456, + ) + dbsession.add(installation) + dbsession.flush() + assert owner.github_app_installations == [installation] + assert get_owner_installation_id(owner, True) == 123456 + + def test_get_owner_installation_id_yes_installation_yes_legacy_integration_specific_repos( + self, mocker, dbsession + ): + owner = OwnerFactory(service="github", integration_id=12341234) + repo_covered_by_installation = RepositoryFactory( + owner=owner, using_integration=True + ) + repo_not_covered_by_installation = RepositoryFactory( + owner=owner, using_integration=True + ) + installation = GithubAppInstallation( + owner=owner, + name=GITHUB_APP_INSTALLATION_DEFAULT_NAME, + repository_service_ids=[repo_covered_by_installation.service_id], + installation_id=123456, + ) + dbsession.add(installation) + dbsession.flush() + assert owner.github_app_installations == [installation] + assert ( + get_owner_installation_id( + owner, + repo_covered_by_installation.using_integration, + repository=repo_covered_by_installation, + ) + == 123456 + ) + # Notice that the installation object overrides the `Repository.using_integration` column completely + assert ( + get_owner_installation_id( + owner, + repo_not_covered_by_installation.using_integration, + repository=repo_not_covered_by_installation, + ) + is None + ) + def test_get_token_type_mapping_public_repo_no_configuration_no_particular_bot( self, mock_configuration, dbsession ): diff --git a/services/tests/test_repository_service.py b/services/tests/test_repository_service.py index 2eaa9952f..268d75873 100644 --- a/services/tests/test_repository_service.py +++ b/services/tests/test_repository_service.py @@ -12,6 +12,10 @@ ) from database.models import Owner +from database.models.core import ( + GITHUB_APP_INSTALLATION_DEFAULT_NAME, + GithubAppInstallation, +) from database.tests.factories import ( CommitFactory, OwnerFactory, @@ -19,6 +23,7 @@ RepositoryFactory, ) from services.repository import ( + _is_repo_using_integration, _pick_best_base_comparedto_pair, fetch_and_update_pull_request_information, fetch_and_update_pull_request_information_from_commit, @@ -30,6 +35,50 @@ ) +@pytest.mark.parametrize("using_integration", [True, False]) +def test__is_repo_using_integration_deprecated_flow(using_integration, dbsession): + repo = RepositoryFactory.create(using_integration=using_integration) + assert _is_repo_using_integration(repo) == using_integration + + +def test__is_repo_using_integration_ghapp_covers_all_repos(dbsession): + owner = OwnerFactory.create(service="github") + repo = RepositoryFactory.create(owner=owner) + other_repo_same_owner = RepositoryFactory.create(owner=owner) + repo_different_owner = RepositoryFactory.create() + assert repo.owner != repo_different_owner.owner + ghapp_installation = GithubAppInstallation( + name=GITHUB_APP_INSTALLATION_DEFAULT_NAME, + owner=owner, + repository_service_ids=None, + installation_id=12345, + ) + dbsession.add(ghapp_installation) + dbsession.flush() + assert _is_repo_using_integration(repo) == True + assert _is_repo_using_integration(other_repo_same_owner) == True + assert _is_repo_using_integration(repo_different_owner) == False + + +def test__is_repo_using_integration_ghapp_covers_some_repos(dbsession): + owner = OwnerFactory.create(service="github") + repo = RepositoryFactory.create(owner=owner) + other_repo_same_owner = RepositoryFactory.create(owner=owner) + repo_different_owner = RepositoryFactory.create() + assert repo.owner != repo_different_owner.owner + ghapp_installation = GithubAppInstallation( + name=GITHUB_APP_INSTALLATION_DEFAULT_NAME, + owner=owner, + repository_service_ids=[repo.service_id], + installation_id=12345, + ) + dbsession.add(ghapp_installation) + dbsession.flush() + assert _is_repo_using_integration(repo) == True + assert _is_repo_using_integration(other_repo_same_owner) == False + assert _is_repo_using_integration(repo_different_owner) == False + + class TestRepositoryServiceTestCase(object): def test_get_repo_provider_service_github(self, dbsession): repo = RepositoryFactory.create( diff --git a/tasks/notify.py b/tasks/notify.py index 98a135c33..4fce917a5 100644 --- a/tasks/notify.py +++ b/tasks/notify.py @@ -15,6 +15,7 @@ from app import celery_app from database.enums import CommitErrorTypes, Decoration from database.models import Commit, Pull +from database.models.core import GITHUB_APP_INSTALLATION_DEFAULT_NAME from helpers.checkpoint_logger import from_kwargs as checkpoints_from_kwargs from helpers.checkpoint_logger.flows import UploadFlow from helpers.exceptions import RepositoryWithoutValidBotError @@ -179,7 +180,24 @@ async def run_async_within_lock( "Not sending notifications yet because we are waiting for CI to finish", extra=dict(repoid=commit.repoid, commit=commit.commitid), ) - if commit.repository.using_integration or commit.repository.hookid: + ghapp_default_installations = list( + filter( + lambda obj: obj.name == GITHUB_APP_INSTALLATION_DEFAULT_NAME, + commit.repository.owner.github_app_installations or [], + ) + ) + rely_on_webhook_ghapp = ( + ghapp_default_installations != [] + and ghapp_default_installations[0].is_repo_covered_by_integration( + commit.repository + ) + ) + rely_on_webhook_legacy = commit.repository.using_integration + if ( + rely_on_webhook_ghapp + or rely_on_webhook_legacy + or commit.repository.hookid + ): # rely on the webhook, but still retry in case we miss the webhook max_retries = 5 countdown = (60 * 3) * 2**self.request.retries diff --git a/tasks/tests/unit/test_upload_task.py b/tasks/tests/unit/test_upload_task.py index 2c74395a0..ab47bf828 100644 --- a/tasks/tests/unit/test_upload_task.py +++ b/tasks/tests/unit/test_upload_task.py @@ -14,6 +14,10 @@ from database.enums import ReportType from database.models import Upload +from database.models.core import ( + GITHUB_APP_INSTALLATION_DEFAULT_NAME, + GithubAppInstallation, +) from database.models.reports import CommitReport from database.tests.factories import CommitFactory, OwnerFactory, RepositoryFactory from database.tests.factories.core import ReportFactory @@ -1329,6 +1333,29 @@ async def test_possibly_setup_webhooks_public_repo( token=None, ) + @pytest.mark.asyncio + async def test_possibly_setup_webhooks_owner_has_ghapp( + self, mocker, dbsession, mock_configuration, mock_repo_provider + ): + mock_configuration.set_params({"github": {"bot": {"key": "somekey"}}}) + commit = CommitFactory.create( + repository__using_integration=False, repository__hookid=None + ) + ghapp = GithubAppInstallation( + name=GITHUB_APP_INSTALLATION_DEFAULT_NAME, + installation_id=1234, + repository_service_ids=None, + owner=commit.repository.owner, + ) + dbsession.add(ghapp) + dbsession.flush() + task = UploadTask() + mock_repo_provider.data = mocker.MagicMock() + mock_repo_provider.service = "github" + res = await task.possibly_setup_webhooks(commit, mock_repo_provider) + assert res is False + mock_repo_provider.post_webhook.assert_not_called() + @pytest.mark.asyncio async def test_possibly_setup_webhooks_gitlab( self, dbsession, mocker, mock_configuration diff --git a/tasks/upload.py b/tasks/upload.py index 056a4899f..06aaa7c66 100644 --- a/tasks/upload.py +++ b/tasks/upload.py @@ -21,6 +21,7 @@ from app import celery_app from database.enums import CommitErrorTypes, ReportType from database.models import Commit, CommitReport +from database.models.core import GITHUB_APP_INSTALLATION_DEFAULT_NAME from helpers.checkpoint_logger import _kwargs_key from helpers.checkpoint_logger import from_kwargs as checkpoints_from_kwargs from helpers.checkpoint_logger.flows import UploadFlow @@ -681,8 +682,24 @@ def _schedule_bundle_analysis_processing_task( async def possibly_setup_webhooks(self, commit, repository_service): repository = commit.repository repo_data = repository_service.data + + ghapp_default_installations = list( + filter( + lambda obj: obj.name == GITHUB_APP_INSTALLATION_DEFAULT_NAME, + commit.repository.owner.github_app_installations or [], + ) + ) + should_post_ghapp = not ( + ghapp_default_installations != [] + and ghapp_default_installations[0].is_repo_covered_by_integration( + commit.repository + ) + ) + should_post_legacy = not repository.using_integration + should_post_webhook = ( - not repository.using_integration + should_post_legacy + and should_post_ghapp and not repository.hookid and hasattr(repository_service, "post_webhook") )