From 3cb0f9d305b4c1e3aab316a8305d10b006c05e79 Mon Sep 17 00:00:00 2001 From: Nora Shapiro Date: Thu, 10 Oct 2024 16:19:05 -0700 Subject: [PATCH 1/7] True Tokenless for BA and TA --- codecov_auth/authentication/repo_auth.py | 93 +++- .../tests/unit/test_repo_authentication.py | 453 +++++++++++++++++- upload/tests/views/test_bundle_analysis.py | 42 ++ upload/tests/views/test_helpers.py | 96 +++- upload/tests/views/test_test_results.py | 28 ++ upload/views/bundle_analysis.py | 2 + upload/views/helpers.py | 47 ++ upload/views/test_results.py | 4 +- 8 files changed, 756 insertions(+), 9 deletions(-) diff --git a/codecov_auth/authentication/repo_auth.py b/codecov_auth/authentication/repo_auth.py index cfb3b4d2c4..0e855b6c11 100644 --- a/codecov_auth/authentication/repo_auth.py +++ b/codecov_auth/authentication/repo_auth.py @@ -24,6 +24,7 @@ from core.models import Commit, Repository from upload.helpers import get_global_tokens, get_repo_with_github_actions_oidc_token from upload.views.helpers import ( + get_repository_and_owner_from_slug_and_commit, get_repository_and_owner_from_string, get_repository_from_string, ) @@ -345,7 +346,7 @@ def get_branch(self, request, repoid=None, commitid=None): body = json.loads(str(request.body, "utf8")) # If commit is not created yet (ie first upload for this commit), we just validate branch format. - # However if a commit exists already (ie not the first upload for this commit), we must additionally + # However, if a commit exists already (ie not the first upload for this commit), we must additionally # validate the saved commit branch matches what is requested in this upload call. commit = Commit.objects.filter(repository_id=repoid, commitid=commitid).first() if commit and commit.branch != body.get("branch"): @@ -381,10 +382,15 @@ def _get_info_from_request_path( return repository, owner + def get_repository_and_owner( + self, request: HttpRequest + ) -> tuple[Repository | None, Owner | None]: + return self._get_info_from_request_path(request) + def authenticate( self, request: HttpRequest ) -> tuple[RepositoryAsUser, TokenlessAuth] | None: - repository, owner = self._get_info_from_request_path(request) + repository, owner = self.get_repository_and_owner(request) if ( repository is None @@ -398,3 +404,86 @@ def authenticate( RepositoryAsUser(repository), TokenlessAuth(repository), ) + + +class TestResultsUploadTokenRequiredAuthenticationCheck( + UploadTokenRequiredAuthenticationCheck +): + """ + Repository and Owner are not in the path for this endpoint, have to get them another way, + then use the same authenticate() as parent class. + """ + + def _get_info_from_request_data( + self, request: HttpRequest + ) -> tuple[Repository | None, Owner | None]: + from upload.views.test_results import UploadSerializer # circular imports + + try: + body = json.loads(str(request.body, "utf8")) + except json.JSONDecodeError: + return None, None # continue to next auth class + + serializer = UploadSerializer(data=body) + if not serializer.is_valid(): + return None, None # continue to next auth class + + # these fields are required=True + slug = serializer.validated_data["slug"] + commitid = serializer.validated_data["commit"] + + return get_repository_and_owner_from_slug_and_commit( + slug=slug, commitid=commitid + ) + + def get_repository_and_owner( + self, request: HttpRequest + ) -> tuple[Repository | None, Owner | None]: + return self._get_info_from_request_data(request) + + +class BundleAnalysisUploadTokenRequiredAuthenticationCheck( + UploadTokenRequiredAuthenticationCheck +): + """ + Repository and Owner are not in the path for this endpoint, have to get them another way, + then use the same authenticate() as parent class. + """ + + def _get_info_from_request_data( + self, request: HttpRequest + ) -> tuple[Repository | None, Owner | None]: + from upload.views.bundle_analysis import UploadSerializer # circular imports + + try: + body = json.loads(str(request.body, "utf8")) + except json.JSONDecodeError: + return None, None # continue to next auth class + + serializer = UploadSerializer(data=body) + if not serializer.is_valid(): + return None, None # continue to next auth class + + # these fields are required=True + slug = serializer.validated_data["slug"] + commitid = serializer.validated_data["commit"] + + # this field is required=False but is much better for getting repository and owner + service = serializer.validated_data.get("git_service") + if service: + try: + service_enum = Service(service) + except ValueError: + return None, None + return get_repository_and_owner_from_string( + service=service_enum, repo_identifier=slug + ) + + return get_repository_and_owner_from_slug_and_commit( + slug=slug, commitid=commitid + ) + + def get_repository_and_owner( + self, request: HttpRequest + ) -> tuple[Repository | None, Owner | None]: + return self._get_info_from_request_data(request) diff --git a/codecov_auth/tests/unit/test_repo_authentication.py b/codecov_auth/tests/unit/test_repo_authentication.py index 1f44d99bcc..1da5deb5aa 100644 --- a/codecov_auth/tests/unit/test_repo_authentication.py +++ b/codecov_auth/tests/unit/test_repo_authentication.py @@ -10,14 +10,18 @@ from jwt import PyJWTError from rest_framework import exceptions from rest_framework.test import APIRequestFactory +from shared.django_apps.codecov_auth.models import Owner, Service +from shared.django_apps.core.models import Repository from codecov_auth.authentication.repo_auth import ( + BundleAnalysisUploadTokenRequiredAuthenticationCheck, GitHubOIDCTokenAuthentication, GlobalTokenAuthentication, OrgLevelTokenAuthentication, RepositoryLegacyQueryTokenAuthentication, RepositoryLegacyTokenAuthentication, RepositoryTokenAuthentication, + TestResultsUploadTokenRequiredAuthenticationCheck, TokenlessAuth, TokenlessAuthentication, UploadTokenRequiredAuthenticationCheck, @@ -597,7 +601,7 @@ def test_token_not_required_unknown_owner(self, db): @pytest.mark.parametrize("request_uri,repo_slug,commitid", valid_params_to_test) @pytest.mark.parametrize("private", [False, True]) @pytest.mark.parametrize("token_required", [False, True]) - def test_token_not_required_matches_paths( + def test_get_repository_and_owner( self, request_uri, repo_slug, commitid, private, token_required, db ): author_name, repo_name = repo_slug.split("/") @@ -612,7 +616,7 @@ def test_token_not_required_matches_paths( request_uri, {"branch": "fork:branch"}, format="json" ) authentication = UploadTokenRequiredAuthenticationCheck() - assert authentication._get_info_from_request_path(request) == ( + assert authentication.get_repository_and_owner(request) == ( repo, repo.author, ) @@ -670,3 +674,448 @@ def test_token_not_required_fork_branch_public_private( else: res = authentication.authenticate(request) assert res is None + + +class TestTestResultsUploadTokenRequiredAuthenticationCheck(object): + def test_token_not_required_invalid_data(self): + request = APIRequestFactory().post( + "/endpoint", + data={"slug": 123, "commit": 456}, + format="json", + ) + authentication = TestResultsUploadTokenRequiredAuthenticationCheck() + res = authentication.authenticate(request) + assert res is None + + def test_token_not_required_no_data(self): + request = APIRequestFactory().post( + "/endpoint", + format="json", + ) + authentication = TestResultsUploadTokenRequiredAuthenticationCheck() + res = authentication.authenticate(request) + assert res is None + + def test_token_not_required_unknown_repository(self, db): + an_owner = OwnerFactory(upload_token_required_for_public_repos=False) + # their repo + RepositoryFactory(author=an_owner, private=False) + request_uri = f"/upload/github/{an_owner.username}::::bad/commits" + request = APIRequestFactory().post( + request_uri, + data={ + "slug": f"{an_owner.username}::::bad", + "commit": "this is a commitid", + }, + format="json", + ) + authentication = TestResultsUploadTokenRequiredAuthenticationCheck() + res = authentication.authenticate(request) + assert res is None + + def test_token_not_required_unknown_owner(self, db): + repo = RepositoryFactory( + private=False, author__upload_token_required_for_public_repos=False + ) + request_uri = f"/upload/github/bad::::{repo.name}/commits" + request = APIRequestFactory().post( + request_uri, + data={"slug": f"bad::::{repo.name}", "commit": "this is a commitid"}, + format="json", + ) + authentication = TestResultsUploadTokenRequiredAuthenticationCheck() + res = authentication.authenticate(request) + assert res is None + + @pytest.mark.parametrize("request_uri,repo_slug,commitid", valid_params_to_test) + @pytest.mark.parametrize("private", [False, True]) + @pytest.mark.parametrize("token_required", [False, True]) + def test_get_repository_and_owner( + self, request_uri, repo_slug, commitid, private, token_required, db + ): + author_name, repo_name = repo_slug.split("/") + repo = RepositoryFactory( + name=repo_name, + author__username=author_name, + private=private, + author__upload_token_required_for_public_repos=token_required, + ) + assert repo.service == "github" + request = APIRequestFactory().post( + request_uri, + data={"slug": f"{author_name}::::{repo_name}", "commit": commitid}, + format="json", + ) + authentication = TestResultsUploadTokenRequiredAuthenticationCheck() + + if commitid is None: + # fails serializer validation for this endpoint + assert authentication.get_repository_and_owner(request) == (None, None) + else: + assert authentication.get_repository_and_owner(request) == ( + repo, + repo.author, + ) + + def test_get_repository_and_owner_by_commitid(self, db): + authentication = TestResultsUploadTokenRequiredAuthenticationCheck() + repo_name = "the-repo" + owner_username = "the-author" + for name, _ in Service.choices: + owner = OwnerFactory(service=name, username=owner_username) + RepositoryFactory(name=repo_name, author=owner) + + request = APIRequestFactory().post( + "endpoint/", + data={"slug": f"{owner_username}::::{repo_name}", "commit": "new commit"}, + format="json", + ) + assert authentication.get_repository_and_owner(request) == (None, None) + + commit_on_random_repo = CommitFactory() + request = APIRequestFactory().post( + "endpoint/", + data={ + "slug": f"{owner_username}::::{repo_name}", + "commit": commit_on_random_repo.commitid, + }, + format="json", + ) + assert authentication.get_repository_and_owner(request) == (None, None) + + matching_repo = Repository.objects.filter( + name=repo_name, author__username=owner_username + ).first() + commit_on_matching_repo = CommitFactory(repository=matching_repo) + request = APIRequestFactory().post( + "endpoint/", + data={ + "slug": f"{owner_username}::::{repo_name}", + "commit": commit_on_matching_repo.commitid, + }, + format="json", + ) + assert authentication.get_repository_and_owner(request) == ( + matching_repo, + matching_repo.author, + ) + + @pytest.mark.parametrize("private", [False, True]) + @pytest.mark.parametrize("branch", ["branch", "fork:branch"]) + @pytest.mark.parametrize( + "existing_commit,commit_branch", + [(False, None), (True, "branch"), (True, "fork:branch")], + ) + @pytest.mark.parametrize("token_required", [False, True]) + def test_token_not_required_fork_branch_public_private( + self, + db, + mocker, + private, + branch, + existing_commit, + commit_branch, + token_required, + ): + repo = RepositoryFactory( + private=private, + author__upload_token_required_for_public_repos=token_required, + ) + + if existing_commit: + commit = CommitFactory() + commit.branch = commit_branch + commit.repository = repo + commit.save() + + request = APIRequestFactory().post( + f"/upload/github/{repo.author.username}::::{repo.name}/commits/{commit.commitid}/reports/report_code/uploads", + data={ + "slug": f"{repo.author.username}::::{repo.name}", + "commit": commit.commitid, + }, + format="json", + ) + + else: + request = APIRequestFactory().post( + f"/upload/github/{repo.author.username}::::{repo.name}/commits", + data={ + "slug": f"{repo.author.username}::::{repo.name}", + "commit": "this is a commitid", + }, + format="json", + ) + + authentication = TestResultsUploadTokenRequiredAuthenticationCheck() + + if not private and not token_required: + res = authentication.authenticate(request) + assert res is not None + repo_as_user, auth_class = res + + assert repo_as_user.is_authenticated() is True + assert isinstance(auth_class, TokenlessAuth) + else: + res = authentication.authenticate(request) + assert res is None + + +class TestBundleAnalysisUploadTokenRequiredAuthenticationCheck(object): + def test_token_not_required_invalid_data(self): + request = APIRequestFactory().post( + "/endpoint", + data={"slug": 123, "commit": 456}, + format="json", + ) + authentication = BundleAnalysisUploadTokenRequiredAuthenticationCheck() + res = authentication.authenticate(request) + assert res is None + + def test_token_not_required_no_data(self): + request = APIRequestFactory().post( + "/endpoint", + format="json", + ) + authentication = BundleAnalysisUploadTokenRequiredAuthenticationCheck() + res = authentication.authenticate(request) + assert res is None + + def test_token_not_required_unknown_repository(self, db): + an_owner = OwnerFactory(upload_token_required_for_public_repos=False) + # their repo + RepositoryFactory(author=an_owner, private=False) + request_uri = f"/upload/github/{an_owner.username}::::bad/commits" + request = APIRequestFactory().post( + request_uri, + data={ + "slug": f"{an_owner.username}::::bad", + "commit": "this is a commitid", + "service": an_owner.service, + }, + format="json", + ) + authentication = BundleAnalysisUploadTokenRequiredAuthenticationCheck() + assert authentication.get_repository_and_owner(request) == (None, None) + res = authentication.authenticate(request) + assert res is None + + def test_token_not_required_unknown_owner(self, db): + repo = RepositoryFactory( + private=False, author__upload_token_required_for_public_repos=False + ) + request_uri = f"/upload/github/bad::::{repo.name}/commits" + request = APIRequestFactory().post( + request_uri, + data={"slug": f"bad::::{repo.name}", "commit": "this is a commitid"}, + format="json", + ) + authentication = BundleAnalysisUploadTokenRequiredAuthenticationCheck() + res = authentication.authenticate(request) + assert res is None + + @pytest.mark.parametrize("request_uri,repo_slug,commitid", valid_params_to_test) + @pytest.mark.parametrize("private", [False, True]) + @pytest.mark.parametrize("token_required", [False, True]) + def test_get_repository_and_owner( + self, request_uri, repo_slug, commitid, private, token_required, db + ): + author_name, repo_name = repo_slug.split("/") + repo = RepositoryFactory( + name=repo_name, + author__username=author_name, + private=private, + author__upload_token_required_for_public_repos=token_required, + ) + assert repo.service == "github" + request = APIRequestFactory().post( + request_uri, + data={"slug": f"{author_name}::::{repo_name}", "commit": commitid}, + format="json", + ) + authentication = BundleAnalysisUploadTokenRequiredAuthenticationCheck() + + if commitid is None: + # fails serializer validation for this endpoint + assert authentication.get_repository_and_owner(request) == (None, None) + else: + assert authentication.get_repository_and_owner(request) == ( + repo, + repo.author, + ) + + def test_get_repository_and_owner_by_commitid(self, db): + authentication = BundleAnalysisUploadTokenRequiredAuthenticationCheck() + repo_name = "the-repo" + owner_username = "the-author" + for name, _ in Service.choices: + owner = OwnerFactory(service=name, username=owner_username) + RepositoryFactory(name=repo_name, author=owner) + + request = APIRequestFactory().post( + "endpoint/", + data={"slug": f"{owner_username}::::{repo_name}", "commit": "new commit"}, + format="json", + ) + assert authentication.get_repository_and_owner(request) == (None, None) + + commit_on_random_repo = CommitFactory() + request = APIRequestFactory().post( + "endpoint/", + data={ + "slug": f"{owner_username}::::{repo_name}", + "commit": commit_on_random_repo.commitid, + }, + format="json", + ) + assert authentication.get_repository_and_owner(request) == (None, None) + + matching_repo = Repository.objects.filter( + name=repo_name, author__username=owner_username + ).first() + commit_on_matching_repo = CommitFactory(repository=matching_repo) + request = APIRequestFactory().post( + "endpoint/", + data={ + "slug": f"{owner_username}::::{repo_name}", + "commit": commit_on_matching_repo.commitid, + }, + format="json", + ) + assert authentication.get_repository_and_owner(request) == ( + matching_repo, + matching_repo.author, + ) + + def test_get_repository_and_owner_with_service(self, db): + authentication = BundleAnalysisUploadTokenRequiredAuthenticationCheck() + repo_name = "the-repo" + owner_username = "the-author" + for name, _ in Service.choices: + owner = OwnerFactory(service=name, username=owner_username) + RepositoryFactory(name=repo_name, author=owner) + + request = APIRequestFactory().post( + "endpoint/", + data={ + "slug": f"{owner_username}::::{repo_name}", + "commit": "new commit", + "git_service": Service.BITBUCKET.value, + }, + format="json", + ) + matching_owner = Owner.objects.get( + service=Service.BITBUCKET.value, username=owner_username + ) + matching_repo = Repository.objects.get( + name=repo_name, author__service=Service.BITBUCKET.value + ) + assert authentication.get_repository_and_owner(request) == ( + matching_repo, + matching_owner, + ) + + commit_on_random_repo = CommitFactory() + request = APIRequestFactory().post( + "endpoint/", + data={ + "slug": f"{owner_username}::::{repo_name}", + "commit": commit_on_random_repo.commitid, + "git_service": Service.GITLAB.value, + }, + format="json", + ) + matching_owner = Owner.objects.get( + service=Service.GITLAB.value, username=owner_username + ) + matching_repo = Repository.objects.get( + name=repo_name, author__service=Service.GITLAB.value + ) + assert authentication.get_repository_and_owner(request) == ( + matching_repo, + matching_owner, + ) + + matching_repo = Repository.objects.filter( + name=repo_name, author__username=owner_username + ).first() + commit_on_matching_repo = CommitFactory(repository=matching_repo) + request = APIRequestFactory().post( + "endpoint/", + data={ + "slug": f"{owner_username}::::{repo_name}", + "commit": commit_on_matching_repo.commitid, + "git_service": Service.GITHUB.value, + }, + format="json", + ) + matching_owner = Owner.objects.get( + service=Service.GITHUB.value, username=owner_username + ) + matching_repo = Repository.objects.get( + name=repo_name, author__service=Service.GITHUB.value + ) + assert authentication.get_repository_and_owner(request) == ( + matching_repo, + matching_owner, + ) + + @pytest.mark.parametrize("private", [False, True]) + @pytest.mark.parametrize("branch", ["branch", "fork:branch"]) + @pytest.mark.parametrize( + "existing_commit,commit_branch", + [(False, None), (True, "branch"), (True, "fork:branch")], + ) + @pytest.mark.parametrize("token_required", [False, True]) + def test_token_not_required_fork_branch_public_private( + self, + db, + mocker, + private, + branch, + existing_commit, + commit_branch, + token_required, + ): + repo = RepositoryFactory( + private=private, + author__upload_token_required_for_public_repos=token_required, + ) + + if existing_commit: + commit = CommitFactory() + commit.branch = commit_branch + commit.repository = repo + commit.save() + + request = APIRequestFactory().post( + f"/upload/github/{repo.author.username}::::{repo.name}/commits/{commit.commitid}/reports/report_code/uploads", + data={ + "slug": f"{repo.author.username}::::{repo.name}", + "commit": commit.commitid, + }, + format="json", + ) + + else: + request = APIRequestFactory().post( + f"/upload/github/{repo.author.username}::::{repo.name}/commits", + data={ + "slug": f"{repo.author.username}::::{repo.name}", + "commit": "this is a commitid", + }, + format="json", + ) + + authentication = BundleAnalysisUploadTokenRequiredAuthenticationCheck() + + if not private and not token_required: + res = authentication.authenticate(request) + assert res is not None + repo_as_user, auth_class = res + + assert repo_as_user.is_authenticated() is True + assert isinstance(auth_class, TokenlessAuth) + else: + res = authentication.authenticate(request) + assert res is None diff --git a/upload/tests/views/test_bundle_analysis.py b/upload/tests/views/test_bundle_analysis.py index 8703e9c754..2ce25d5c27 100644 --- a/upload/tests/views/test_bundle_analysis.py +++ b/upload/tests/views/test_bundle_analysis.py @@ -418,6 +418,48 @@ def test_upload_bundle_analysis_tokenless_success(db, client, mocker, mock_redis create_presigned_put.assert_called_once_with("bundle-analysis", ANY, 30) +@pytest.mark.django_db(databases={"default", "timeseries"}) +def test_upload_bundle_analysis_true_tokenless_success(db, client, mocker, mock_redis): + upload = mocker.patch.object(TaskService, "upload") + + create_presigned_put = mocker.patch( + "services.archive.StorageService.create_presigned_put", + return_value="test-presigned-put", + ) + + repository = RepositoryFactory.create( + private=False, + author__upload_token_required_for_public_repos=False, + author__service="github", + ) + client = APIClient() + + res = client.post( + reverse("upload-bundle-analysis"), + { + "commit": "any", + "slug": f"{repository.author.username}::::{repository.name}", + "build": "test-build", + "buildURL": "test-build-url", + "job": "test-job", + "service": "test-service", + "compareSha": "6fd5b89357fc8cdf34d6197549ac7c6d7e5aaaaa", + "branch": "f1:main", + "git_service": "github", + }, + format="json", + headers={"User-Agent": "codecov-cli/0.4.7"}, + ) + + assert res.status_code == 201 + + # returns presigned storage URL + assert res.json() == {"url": "test-presigned-put"} + + assert upload.called + create_presigned_put.assert_called_once_with("bundle-analysis", ANY, 30) + + @pytest.mark.django_db(databases={"default", "timeseries"}) def test_upload_bundle_analysis_tokenless_no_repo(db, client, mocker, mock_redis): upload = mocker.patch.object(TaskService, "upload") diff --git a/upload/tests/views/test_helpers.py b/upload/tests/views/test_helpers.py index 439f65c9fa..afff7b7c83 100644 --- a/upload/tests/views/test_helpers.py +++ b/upload/tests/views/test_helpers.py @@ -1,8 +1,11 @@ from django.test import TestCase +from shared.django_apps.codecov_auth.models import Owner +from shared.django_apps.core.models import Repository from codecov_auth.models import Service -from core.tests.factories import OwnerFactory, RepositoryFactory +from core.tests.factories import CommitFactory, OwnerFactory, RepositoryFactory from upload.views.helpers import ( + get_repository_and_owner_from_slug_and_commit, get_repository_and_owner_from_string, get_repository_from_string, ) @@ -21,6 +24,7 @@ def setUpClass(cls): cls.group = "somegroup" cls.subgroup = "somesubgroup" cls.sub_subgroup = "subsub" + cls.commitid = CommitFactory().commitid owners_to_repos_mapping = { cls.gh: {cls.uname: [cls.repo_1_name, cls.repo_2_name]}, cls.bb: {cls.uname: [cls.repo_1_name, cls.repo_2_name]}, @@ -36,10 +40,10 @@ def setUpClass(cls): } for service, owner_data in owners_to_repos_mapping.items(): for username, repo_names in owner_data.items(): - RepositoryFactory() o = OwnerFactory(service=service, username=username) for r_name in repo_names: - RepositoryFactory(author=o, name=r_name) + r = RepositoryFactory(author=o, name=r_name) + CommitFactory(repository=r, commitid=cls.commitid) def test_get_repository_from_string(self): first_result = get_repository_from_string( @@ -149,3 +153,89 @@ def test_get_repository_and_owner_from_string(self): assert get_repository_and_owner_from_string( self.gl, f"{self.group}:::somebadsubgroup::::{self.repo_1_name}" ) == (None, None) + + def test_get_repository_and_owner_from_slug_and_commit(self): + # the test cases from setUpClass won't work on this method, they all have + # same username, repo name, and commitid + assert get_repository_and_owner_from_slug_and_commit( + slug=f"{self.uname}::::{self.repo_1_name}", commitid=self.commitid + ) == (None, None) + + # can identify by unique commitid + gh_owner = Owner.objects.get(service=self.gh, username=self.uname) + gh_repo = Repository.objects.get(author=gh_owner, name=self.repo_1_name) + unique_commit = CommitFactory(repository=gh_repo) + assert get_repository_and_owner_from_slug_and_commit( + slug=f"{self.uname}::::{self.repo_1_name}", commitid=unique_commit.commitid + ) == (gh_repo, gh_owner) + + # can identify by unique username + gl_owner = Owner.objects.get(service=self.gl, username=self.uname) + gl_repo = Repository.objects.get(author=gl_owner, name=self.repo_1_name) + gl_owner.username = "other" + gl_owner.save() + assert get_repository_and_owner_from_slug_and_commit( + slug=f"other::::{self.repo_1_name}", commitid=self.commitid + ) == (gl_repo, gl_owner) + + # can identify by unique repo name + another_gh_repo = RepositoryFactory(author=gh_owner, name="another") + CommitFactory(repository=another_gh_repo, commitid=self.commitid) + assert get_repository_and_owner_from_slug_and_commit( + slug=f"{self.uname}::::another", commitid=self.commitid + ) == (another_gh_repo, gh_owner) + + assert get_repository_and_owner_from_slug_and_commit( + slug=f"somerandomlalala::::{self.repo_1_name}", commitid=self.commitid + ) == (None, None) + assert get_repository_and_owner_from_slug_and_commit( + slug=f"{self.uname}::::{self.repo_1_name}wrongname", commitid=self.commitid + ) == (None, None) + assert get_repository_and_owner_from_slug_and_commit( + slug=f"{self.uname}::::{self.repo_1_name}", commitid="nonsense" + ) == (None, None) + assert get_repository_and_owner_from_slug_and_commit( + slug=f"{self.group}:::somebadsubgroup::::{self.repo_1_name}", + commitid=self.commitid, + ) == (None, None) + + # gitlab group + gitlab_group_repository, gitlab_group = ( + get_repository_and_owner_from_slug_and_commit( + slug=f"{self.group}::::{self.repo_1_name}", commitid=self.commitid + ) + ) + assert gitlab_group_repository.name == self.repo_1_name + assert gitlab_group_repository.author.username == self.group + assert gitlab_group_repository.author.service == self.gl + assert gitlab_group_repository.author == gitlab_group + + # gitlab subgroup + gitlab_subgroup_repository, gitlab_subgroup = ( + get_repository_and_owner_from_slug_and_commit( + slug=f"{self.group}:::{self.subgroup}::::{self.repo_1_name}", + commitid=self.commitid, + ) + ) + assert gitlab_subgroup_repository.name == self.repo_1_name + assert ( + gitlab_subgroup_repository.author.username + == f"{self.group}:{self.subgroup}" + ) + assert gitlab_subgroup_repository.author.service == self.gl + assert gitlab_subgroup_repository.author == gitlab_subgroup + + # gitlab subgroup of a subgroup + gitlab_sub_subgroup_repository, gitlab_sub_subgroup = ( + get_repository_and_owner_from_slug_and_commit( + slug=f"{self.group}:::{self.subgroup}:::{self.sub_subgroup}::::{self.repo_1_name}", + commitid=self.commitid, + ) + ) + assert gitlab_sub_subgroup_repository.name == self.repo_1_name + assert ( + gitlab_sub_subgroup_repository.author.username + == f"{self.group}:{self.subgroup}:{self.sub_subgroup}" + ) + assert gitlab_sub_subgroup_repository.author.service == self.gl + assert gitlab_sub_subgroup_repository.author == gitlab_sub_subgroup diff --git a/upload/tests/views/test_test_results.py b/upload/tests/views/test_test_results.py index 9035c4e9d3..ee0b5b2421 100644 --- a/upload/tests/views/test_test_results.py +++ b/upload/tests/views/test_test_results.py @@ -169,6 +169,34 @@ def test_test_results_github_oidc_token( assert res.status_code == 201 +def test_test_results_upload_token_not_required(db, client, mocker, mock_redis): + mocker.patch.object(TaskService, "upload") + mocker.patch( + "services.archive.StorageService.create_presigned_put", + return_value="test-presigned-put", + ) + + owner = OwnerFactory( + service="github", + username="codecov", + upload_token_required_for_public_repos=False, + ) + repository = RepositoryFactory.create(author=owner, private=False) + + client = APIClient() + + res = client.post( + reverse("upload-test-results"), + { + "commit": "6fd5b89357fc8cdf34d6197549ac7c6d7e5977ef", + "slug": f"{repository.author.username}::::{repository.name}", + "branch": "aaaaaa", + }, + format="json", + ) + assert res.status_code == 201 + + def test_test_results_no_auth(db, client, mocker, mock_redis): owner = OwnerFactory(service="github", username="codecov") repository = RepositoryFactory.create(author=owner) diff --git a/upload/views/bundle_analysis.py b/upload/views/bundle_analysis.py index 6b2d095501..ad9ebb29c8 100644 --- a/upload/views/bundle_analysis.py +++ b/upload/views/bundle_analysis.py @@ -14,6 +14,7 @@ from codecov_auth.authentication.repo_auth import ( BundleAnalysisTokenlessAuthentication, + BundleAnalysisUploadTokenRequiredAuthenticationCheck, GitHubOIDCTokenAuthentication, OrgLevelTokenAuthentication, RepositoryLegacyTokenAuthentication, @@ -54,6 +55,7 @@ class UploadSerializer(serializers.Serializer): class BundleAnalysisView(APIView, ShelterMixin): permission_classes = [UploadBundleAnalysisPermission] authentication_classes = [ + BundleAnalysisUploadTokenRequiredAuthenticationCheck, OrgLevelTokenAuthentication, GitHubOIDCTokenAuthentication, RepositoryLegacyTokenAuthentication, diff --git a/upload/views/helpers.py b/upload/views/helpers.py index cd61677680..d6b52ab146 100644 --- a/upload/views/helpers.py +++ b/upload/views/helpers.py @@ -1,8 +1,13 @@ +import logging import typing +from shared.django_apps.core.models import Commit + from codecov_auth.models import Owner, Service from core.models import Repository +log = logging.getLogger(__name__) + def get_repository_and_owner_from_string( service: Service, repo_identifier: str @@ -45,3 +50,45 @@ def get_repository_from_string( service=service, repo_identifier=repo_identifier ) return repository + + +def get_repository_and_owner_from_slug_and_commit( + slug: str, commitid: str +) -> tuple[Repository | None, Owner | None]: + if "::::" not in slug: + return None, None + + owner_username, repository_name = slug.rsplit("::::", 1) + + # formatting for GL subgroups + if ":::" in owner_username: + owner_username = owner_username.replace(":::", ":") + + matching_repositories = Repository.objects.filter( + author__username=owner_username, name=repository_name + ).select_related("author") + if matching_repositories.count() == 1: + log.info( + "get_repository_and_owner_from_slug_and_commit success", + extra=dict(slug=slug), + ) + repository = matching_repositories.first() + return repository, repository.author + else: + # commit might not exist yet (if this is first upload for it) + try: + # Commit has UniqueConstraint on commitid + repository + commit = Commit.objects.select_related( + "repository", "repository__author" + ).get(commitid=commitid, repository__in=matching_repositories) + log.info( + "get_repository_and_owner_from_slug_and_commit multiple matches success", + extra=dict(slug=slug), + ) + return commit.repository, commit.repository.author + except (Commit.DoesNotExist, Commit.MultipleObjectsReturned): + log.info( + "get_repository_and_owner_from_slug_and_commit fail", + extra=dict(slug=slug), + ) + return None, None diff --git a/upload/views/test_results.py b/upload/views/test_results.py index b3e2609ebd..865ff665aa 100644 --- a/upload/views/test_results.py +++ b/upload/views/test_results.py @@ -13,8 +13,8 @@ GitHubOIDCTokenAuthentication, OrgLevelTokenAuthentication, RepositoryLegacyTokenAuthentication, + TestResultsUploadTokenRequiredAuthenticationCheck, TokenlessAuthentication, - UploadTokenRequiredAuthenticationCheck, repo_auth_custom_exception_handler, ) from codecov_auth.authentication.types import RepositoryAsUser @@ -55,7 +55,7 @@ class TestResultsView( ): permission_classes = [UploadTestResultsPermission] authentication_classes = [ - UploadTokenRequiredAuthenticationCheck, + TestResultsUploadTokenRequiredAuthenticationCheck, OrgLevelTokenAuthentication, GitHubOIDCTokenAuthentication, RepositoryLegacyTokenAuthentication, From f9d830733633587e9249fd638d8bf50deee66be2 Mon Sep 17 00:00:00 2001 From: Nora Shapiro Date: Fri, 11 Oct 2024 18:06:51 -0700 Subject: [PATCH 2/7] redo the whole thing, condense into one class that both views share --- codecov_auth/authentication/repo_auth.py | 95 +++--- .../tests/unit/test_repo_authentication.py | 272 ++---------------- upload/tests/views/test_helpers.py | 96 +------ upload/tests/views/test_test_results.py | 4 +- upload/views/bundle_analysis.py | 4 +- upload/views/test_results.py | 8 +- 6 files changed, 68 insertions(+), 411 deletions(-) diff --git a/codecov_auth/authentication/repo_auth.py b/codecov_auth/authentication/repo_auth.py index 0e855b6c11..c036b2d89b 100644 --- a/codecov_auth/authentication/repo_auth.py +++ b/codecov_auth/authentication/repo_auth.py @@ -8,7 +8,7 @@ from django.http import HttpRequest from django.utils import timezone from jwt import PyJWTError -from rest_framework import authentication, exceptions +from rest_framework import authentication, exceptions, serializers from rest_framework.exceptions import NotAuthenticated from rest_framework.views import exception_handler from shared.django_apps.codecov_auth.models import Owner @@ -24,7 +24,6 @@ from core.models import Commit, Repository from upload.helpers import get_global_tokens, get_repo_with_github_actions_oidc_token from upload.views.helpers import ( - get_repository_and_owner_from_slug_and_commit, get_repository_and_owner_from_string, get_repository_from_string, ) @@ -406,84 +405,54 @@ def authenticate( ) -class TestResultsUploadTokenRequiredAuthenticationCheck( - UploadTokenRequiredAuthenticationCheck -): - """ - Repository and Owner are not in the path for this endpoint, have to get them another way, - then use the same authenticate() as parent class. - """ - - def _get_info_from_request_data( - self, request: HttpRequest - ) -> tuple[Repository | None, Owner | None]: - from upload.views.test_results import UploadSerializer # circular imports - - try: - body = json.loads(str(request.body, "utf8")) - except json.JSONDecodeError: - return None, None # continue to next auth class - - serializer = UploadSerializer(data=body) - if not serializer.is_valid(): - return None, None # continue to next auth class - - # these fields are required=True - slug = serializer.validated_data["slug"] - commitid = serializer.validated_data["commit"] - - return get_repository_and_owner_from_slug_and_commit( - slug=slug, commitid=commitid - ) - - def get_repository_and_owner( - self, request: HttpRequest - ) -> tuple[Repository | None, Owner | None]: - return self._get_info_from_request_data(request) +class UploadTokenRequiredGetFromBodySerializer(serializers.Serializer): + slug = serializers.CharField(required=True) + service = serializers.CharField(required=False) # git_service from TA + git_service = serializers.CharField(required=False) # git_service from BA -class BundleAnalysisUploadTokenRequiredAuthenticationCheck( +class UploadTokenRequiredGetFromBodyAuthenticationCheck( UploadTokenRequiredAuthenticationCheck ): """ - Repository and Owner are not in the path for this endpoint, have to get them another way, + Get Repository and Owner from request body instead of path, then use the same authenticate() as parent class. """ - def _get_info_from_request_data( + def _get_git(self, validated_data): + """ + BA sends this in as git_service, TA sends this in as service. + Use this function so this Check class can be used by both views. + """ + git_service = validated_data.get("git_service") + if not git_service: + git_service = validated_data.get("service") + return git_service + + def _get_info_from_request_body( self, request: HttpRequest ) -> tuple[Repository | None, Owner | None]: - from upload.views.bundle_analysis import UploadSerializer # circular imports - try: body = json.loads(str(request.body, "utf8")) - except json.JSONDecodeError: - return None, None # continue to next auth class - serializer = UploadSerializer(data=body) - if not serializer.is_valid(): - return None, None # continue to next auth class + serializer = UploadTokenRequiredGetFromBodySerializer(data=body) - # these fields are required=True - slug = serializer.validated_data["slug"] - commitid = serializer.validated_data["commit"] + if serializer.is_valid(): + git_service = self._get_git(validated_data=serializer.validated_data) + service_enum = Service(git_service) + return get_repository_and_owner_from_string( + service=service_enum, + repo_identifier=serializer.validated_data["slug"], + ) - # this field is required=False but is much better for getting repository and owner - service = serializer.validated_data.get("git_service") - if service: - try: - service_enum = Service(service) - except ValueError: - return None, None - return get_repository_and_owner_from_string( - service=service_enum, repo_identifier=slug - ) + except (json.JSONDecodeError, ValueError): + # exceptions raised by json.loads() and Service() + # catch rather than raise to continue to next auth class + pass - return get_repository_and_owner_from_slug_and_commit( - slug=slug, commitid=commitid - ) + return None, None # continue to next auth class def get_repository_and_owner( self, request: HttpRequest ) -> tuple[Repository | None, Owner | None]: - return self._get_info_from_request_data(request) + return self._get_info_from_request_body(request) diff --git a/codecov_auth/tests/unit/test_repo_authentication.py b/codecov_auth/tests/unit/test_repo_authentication.py index 1da5deb5aa..1329856585 100644 --- a/codecov_auth/tests/unit/test_repo_authentication.py +++ b/codecov_auth/tests/unit/test_repo_authentication.py @@ -14,17 +14,16 @@ from shared.django_apps.core.models import Repository from codecov_auth.authentication.repo_auth import ( - BundleAnalysisUploadTokenRequiredAuthenticationCheck, GitHubOIDCTokenAuthentication, GlobalTokenAuthentication, OrgLevelTokenAuthentication, RepositoryLegacyQueryTokenAuthentication, RepositoryLegacyTokenAuthentication, RepositoryTokenAuthentication, - TestResultsUploadTokenRequiredAuthenticationCheck, TokenlessAuth, TokenlessAuthentication, UploadTokenRequiredAuthenticationCheck, + UploadTokenRequiredGetFromBodyAuthenticationCheck, ) from codecov_auth.models import SERVICE_GITHUB, OrganizationLevelToken, RepositoryToken from codecov_auth.tests.factories import OwnerFactory @@ -676,14 +675,14 @@ def test_token_not_required_fork_branch_public_private( assert res is None -class TestTestResultsUploadTokenRequiredAuthenticationCheck(object): +class TestUploadTokenRequiredGetFromBodyAuthenticationCheck(object): def test_token_not_required_invalid_data(self): request = APIRequestFactory().post( "/endpoint", - data={"slug": 123, "commit": 456}, + data={"slug": 123, "git_service": "github"}, format="json", ) - authentication = TestResultsUploadTokenRequiredAuthenticationCheck() + authentication = UploadTokenRequiredGetFromBodyAuthenticationCheck() res = authentication.authenticate(request) assert res is None @@ -692,192 +691,24 @@ def test_token_not_required_no_data(self): "/endpoint", format="json", ) - authentication = TestResultsUploadTokenRequiredAuthenticationCheck() + authentication = UploadTokenRequiredGetFromBodyAuthenticationCheck() res = authentication.authenticate(request) assert res is None - def test_token_not_required_unknown_repository(self, db): - an_owner = OwnerFactory(upload_token_required_for_public_repos=False) + def test_token_not_required_no_git_service(self, db): + owner = OwnerFactory(upload_token_required_for_public_repos=False) # their repo - RepositoryFactory(author=an_owner, private=False) - request_uri = f"/upload/github/{an_owner.username}::::bad/commits" + repo = RepositoryFactory(author=owner, private=False) + request_uri = f"/upload/github/{owner.username}::::{repo.name}/commits" request = APIRequestFactory().post( request_uri, data={ - "slug": f"{an_owner.username}::::bad", - "commit": "this is a commitid", - }, - format="json", - ) - authentication = TestResultsUploadTokenRequiredAuthenticationCheck() - res = authentication.authenticate(request) - assert res is None - - def test_token_not_required_unknown_owner(self, db): - repo = RepositoryFactory( - private=False, author__upload_token_required_for_public_repos=False - ) - request_uri = f"/upload/github/bad::::{repo.name}/commits" - request = APIRequestFactory().post( - request_uri, - data={"slug": f"bad::::{repo.name}", "commit": "this is a commitid"}, - format="json", - ) - authentication = TestResultsUploadTokenRequiredAuthenticationCheck() - res = authentication.authenticate(request) - assert res is None - - @pytest.mark.parametrize("request_uri,repo_slug,commitid", valid_params_to_test) - @pytest.mark.parametrize("private", [False, True]) - @pytest.mark.parametrize("token_required", [False, True]) - def test_get_repository_and_owner( - self, request_uri, repo_slug, commitid, private, token_required, db - ): - author_name, repo_name = repo_slug.split("/") - repo = RepositoryFactory( - name=repo_name, - author__username=author_name, - private=private, - author__upload_token_required_for_public_repos=token_required, - ) - assert repo.service == "github" - request = APIRequestFactory().post( - request_uri, - data={"slug": f"{author_name}::::{repo_name}", "commit": commitid}, - format="json", - ) - authentication = TestResultsUploadTokenRequiredAuthenticationCheck() - - if commitid is None: - # fails serializer validation for this endpoint - assert authentication.get_repository_and_owner(request) == (None, None) - else: - assert authentication.get_repository_and_owner(request) == ( - repo, - repo.author, - ) - - def test_get_repository_and_owner_by_commitid(self, db): - authentication = TestResultsUploadTokenRequiredAuthenticationCheck() - repo_name = "the-repo" - owner_username = "the-author" - for name, _ in Service.choices: - owner = OwnerFactory(service=name, username=owner_username) - RepositoryFactory(name=repo_name, author=owner) - - request = APIRequestFactory().post( - "endpoint/", - data={"slug": f"{owner_username}::::{repo_name}", "commit": "new commit"}, - format="json", - ) - assert authentication.get_repository_and_owner(request) == (None, None) - - commit_on_random_repo = CommitFactory() - request = APIRequestFactory().post( - "endpoint/", - data={ - "slug": f"{owner_username}::::{repo_name}", - "commit": commit_on_random_repo.commitid, + "slug": f"{owner.username}::::{repo.name}", }, format="json", ) + authentication = UploadTokenRequiredGetFromBodyAuthenticationCheck() assert authentication.get_repository_and_owner(request) == (None, None) - - matching_repo = Repository.objects.filter( - name=repo_name, author__username=owner_username - ).first() - commit_on_matching_repo = CommitFactory(repository=matching_repo) - request = APIRequestFactory().post( - "endpoint/", - data={ - "slug": f"{owner_username}::::{repo_name}", - "commit": commit_on_matching_repo.commitid, - }, - format="json", - ) - assert authentication.get_repository_and_owner(request) == ( - matching_repo, - matching_repo.author, - ) - - @pytest.mark.parametrize("private", [False, True]) - @pytest.mark.parametrize("branch", ["branch", "fork:branch"]) - @pytest.mark.parametrize( - "existing_commit,commit_branch", - [(False, None), (True, "branch"), (True, "fork:branch")], - ) - @pytest.mark.parametrize("token_required", [False, True]) - def test_token_not_required_fork_branch_public_private( - self, - db, - mocker, - private, - branch, - existing_commit, - commit_branch, - token_required, - ): - repo = RepositoryFactory( - private=private, - author__upload_token_required_for_public_repos=token_required, - ) - - if existing_commit: - commit = CommitFactory() - commit.branch = commit_branch - commit.repository = repo - commit.save() - - request = APIRequestFactory().post( - f"/upload/github/{repo.author.username}::::{repo.name}/commits/{commit.commitid}/reports/report_code/uploads", - data={ - "slug": f"{repo.author.username}::::{repo.name}", - "commit": commit.commitid, - }, - format="json", - ) - - else: - request = APIRequestFactory().post( - f"/upload/github/{repo.author.username}::::{repo.name}/commits", - data={ - "slug": f"{repo.author.username}::::{repo.name}", - "commit": "this is a commitid", - }, - format="json", - ) - - authentication = TestResultsUploadTokenRequiredAuthenticationCheck() - - if not private and not token_required: - res = authentication.authenticate(request) - assert res is not None - repo_as_user, auth_class = res - - assert repo_as_user.is_authenticated() is True - assert isinstance(auth_class, TokenlessAuth) - else: - res = authentication.authenticate(request) - assert res is None - - -class TestBundleAnalysisUploadTokenRequiredAuthenticationCheck(object): - def test_token_not_required_invalid_data(self): - request = APIRequestFactory().post( - "/endpoint", - data={"slug": 123, "commit": 456}, - format="json", - ) - authentication = BundleAnalysisUploadTokenRequiredAuthenticationCheck() - res = authentication.authenticate(request) - assert res is None - - def test_token_not_required_no_data(self): - request = APIRequestFactory().post( - "/endpoint", - format="json", - ) - authentication = BundleAnalysisUploadTokenRequiredAuthenticationCheck() res = authentication.authenticate(request) assert res is None @@ -890,12 +721,11 @@ def test_token_not_required_unknown_repository(self, db): request_uri, data={ "slug": f"{an_owner.username}::::bad", - "commit": "this is a commitid", "service": an_owner.service, }, format="json", ) - authentication = BundleAnalysisUploadTokenRequiredAuthenticationCheck() + authentication = UploadTokenRequiredGetFromBodyAuthenticationCheck() assert authentication.get_repository_and_owner(request) == (None, None) res = authentication.authenticate(request) assert res is None @@ -907,10 +737,13 @@ def test_token_not_required_unknown_owner(self, db): request_uri = f"/upload/github/bad::::{repo.name}/commits" request = APIRequestFactory().post( request_uri, - data={"slug": f"bad::::{repo.name}", "commit": "this is a commitid"}, + data={ + "slug": f"bad::::{repo.name}", + "service": "github", + }, format="json", ) - authentication = BundleAnalysisUploadTokenRequiredAuthenticationCheck() + authentication = UploadTokenRequiredGetFromBodyAuthenticationCheck() res = authentication.authenticate(request) assert res is None @@ -930,65 +763,18 @@ def test_get_repository_and_owner( assert repo.service == "github" request = APIRequestFactory().post( request_uri, - data={"slug": f"{author_name}::::{repo_name}", "commit": commitid}, + data={"slug": f"{author_name}::::{repo_name}", "service": "github"}, format="json", ) - authentication = BundleAnalysisUploadTokenRequiredAuthenticationCheck() + authentication = UploadTokenRequiredGetFromBodyAuthenticationCheck() - if commitid is None: - # fails serializer validation for this endpoint - assert authentication.get_repository_and_owner(request) == (None, None) - else: - assert authentication.get_repository_and_owner(request) == ( - repo, - repo.author, - ) - - def test_get_repository_and_owner_by_commitid(self, db): - authentication = BundleAnalysisUploadTokenRequiredAuthenticationCheck() - repo_name = "the-repo" - owner_username = "the-author" - for name, _ in Service.choices: - owner = OwnerFactory(service=name, username=owner_username) - RepositoryFactory(name=repo_name, author=owner) - - request = APIRequestFactory().post( - "endpoint/", - data={"slug": f"{owner_username}::::{repo_name}", "commit": "new commit"}, - format="json", - ) - assert authentication.get_repository_and_owner(request) == (None, None) - - commit_on_random_repo = CommitFactory() - request = APIRequestFactory().post( - "endpoint/", - data={ - "slug": f"{owner_username}::::{repo_name}", - "commit": commit_on_random_repo.commitid, - }, - format="json", - ) - assert authentication.get_repository_and_owner(request) == (None, None) - - matching_repo = Repository.objects.filter( - name=repo_name, author__username=owner_username - ).first() - commit_on_matching_repo = CommitFactory(repository=matching_repo) - request = APIRequestFactory().post( - "endpoint/", - data={ - "slug": f"{owner_username}::::{repo_name}", - "commit": commit_on_matching_repo.commitid, - }, - format="json", - ) assert authentication.get_repository_and_owner(request) == ( - matching_repo, - matching_repo.author, + repo, + repo.author, ) def test_get_repository_and_owner_with_service(self, db): - authentication = BundleAnalysisUploadTokenRequiredAuthenticationCheck() + authentication = UploadTokenRequiredGetFromBodyAuthenticationCheck() repo_name = "the-repo" owner_username = "the-author" for name, _ in Service.choices: @@ -999,7 +785,6 @@ def test_get_repository_and_owner_with_service(self, db): "endpoint/", data={ "slug": f"{owner_username}::::{repo_name}", - "commit": "new commit", "git_service": Service.BITBUCKET.value, }, format="json", @@ -1015,12 +800,10 @@ def test_get_repository_and_owner_with_service(self, db): matching_owner, ) - commit_on_random_repo = CommitFactory() request = APIRequestFactory().post( "endpoint/", data={ "slug": f"{owner_username}::::{repo_name}", - "commit": commit_on_random_repo.commitid, "git_service": Service.GITLAB.value, }, format="json", @@ -1036,15 +819,10 @@ def test_get_repository_and_owner_with_service(self, db): matching_owner, ) - matching_repo = Repository.objects.filter( - name=repo_name, author__username=owner_username - ).first() - commit_on_matching_repo = CommitFactory(repository=matching_repo) request = APIRequestFactory().post( "endpoint/", data={ "slug": f"{owner_username}::::{repo_name}", - "commit": commit_on_matching_repo.commitid, "git_service": Service.GITHUB.value, }, format="json", @@ -1092,7 +870,7 @@ def test_token_not_required_fork_branch_public_private( f"/upload/github/{repo.author.username}::::{repo.name}/commits/{commit.commitid}/reports/report_code/uploads", data={ "slug": f"{repo.author.username}::::{repo.name}", - "commit": commit.commitid, + "git_service": repo.author.service, }, format="json", ) @@ -1102,12 +880,12 @@ def test_token_not_required_fork_branch_public_private( f"/upload/github/{repo.author.username}::::{repo.name}/commits", data={ "slug": f"{repo.author.username}::::{repo.name}", - "commit": "this is a commitid", + "git_service": repo.author.service, }, format="json", ) - authentication = BundleAnalysisUploadTokenRequiredAuthenticationCheck() + authentication = UploadTokenRequiredGetFromBodyAuthenticationCheck() if not private and not token_required: res = authentication.authenticate(request) diff --git a/upload/tests/views/test_helpers.py b/upload/tests/views/test_helpers.py index afff7b7c83..439f65c9fa 100644 --- a/upload/tests/views/test_helpers.py +++ b/upload/tests/views/test_helpers.py @@ -1,11 +1,8 @@ from django.test import TestCase -from shared.django_apps.codecov_auth.models import Owner -from shared.django_apps.core.models import Repository from codecov_auth.models import Service -from core.tests.factories import CommitFactory, OwnerFactory, RepositoryFactory +from core.tests.factories import OwnerFactory, RepositoryFactory from upload.views.helpers import ( - get_repository_and_owner_from_slug_and_commit, get_repository_and_owner_from_string, get_repository_from_string, ) @@ -24,7 +21,6 @@ def setUpClass(cls): cls.group = "somegroup" cls.subgroup = "somesubgroup" cls.sub_subgroup = "subsub" - cls.commitid = CommitFactory().commitid owners_to_repos_mapping = { cls.gh: {cls.uname: [cls.repo_1_name, cls.repo_2_name]}, cls.bb: {cls.uname: [cls.repo_1_name, cls.repo_2_name]}, @@ -40,10 +36,10 @@ def setUpClass(cls): } for service, owner_data in owners_to_repos_mapping.items(): for username, repo_names in owner_data.items(): + RepositoryFactory() o = OwnerFactory(service=service, username=username) for r_name in repo_names: - r = RepositoryFactory(author=o, name=r_name) - CommitFactory(repository=r, commitid=cls.commitid) + RepositoryFactory(author=o, name=r_name) def test_get_repository_from_string(self): first_result = get_repository_from_string( @@ -153,89 +149,3 @@ def test_get_repository_and_owner_from_string(self): assert get_repository_and_owner_from_string( self.gl, f"{self.group}:::somebadsubgroup::::{self.repo_1_name}" ) == (None, None) - - def test_get_repository_and_owner_from_slug_and_commit(self): - # the test cases from setUpClass won't work on this method, they all have - # same username, repo name, and commitid - assert get_repository_and_owner_from_slug_and_commit( - slug=f"{self.uname}::::{self.repo_1_name}", commitid=self.commitid - ) == (None, None) - - # can identify by unique commitid - gh_owner = Owner.objects.get(service=self.gh, username=self.uname) - gh_repo = Repository.objects.get(author=gh_owner, name=self.repo_1_name) - unique_commit = CommitFactory(repository=gh_repo) - assert get_repository_and_owner_from_slug_and_commit( - slug=f"{self.uname}::::{self.repo_1_name}", commitid=unique_commit.commitid - ) == (gh_repo, gh_owner) - - # can identify by unique username - gl_owner = Owner.objects.get(service=self.gl, username=self.uname) - gl_repo = Repository.objects.get(author=gl_owner, name=self.repo_1_name) - gl_owner.username = "other" - gl_owner.save() - assert get_repository_and_owner_from_slug_and_commit( - slug=f"other::::{self.repo_1_name}", commitid=self.commitid - ) == (gl_repo, gl_owner) - - # can identify by unique repo name - another_gh_repo = RepositoryFactory(author=gh_owner, name="another") - CommitFactory(repository=another_gh_repo, commitid=self.commitid) - assert get_repository_and_owner_from_slug_and_commit( - slug=f"{self.uname}::::another", commitid=self.commitid - ) == (another_gh_repo, gh_owner) - - assert get_repository_and_owner_from_slug_and_commit( - slug=f"somerandomlalala::::{self.repo_1_name}", commitid=self.commitid - ) == (None, None) - assert get_repository_and_owner_from_slug_and_commit( - slug=f"{self.uname}::::{self.repo_1_name}wrongname", commitid=self.commitid - ) == (None, None) - assert get_repository_and_owner_from_slug_and_commit( - slug=f"{self.uname}::::{self.repo_1_name}", commitid="nonsense" - ) == (None, None) - assert get_repository_and_owner_from_slug_and_commit( - slug=f"{self.group}:::somebadsubgroup::::{self.repo_1_name}", - commitid=self.commitid, - ) == (None, None) - - # gitlab group - gitlab_group_repository, gitlab_group = ( - get_repository_and_owner_from_slug_and_commit( - slug=f"{self.group}::::{self.repo_1_name}", commitid=self.commitid - ) - ) - assert gitlab_group_repository.name == self.repo_1_name - assert gitlab_group_repository.author.username == self.group - assert gitlab_group_repository.author.service == self.gl - assert gitlab_group_repository.author == gitlab_group - - # gitlab subgroup - gitlab_subgroup_repository, gitlab_subgroup = ( - get_repository_and_owner_from_slug_and_commit( - slug=f"{self.group}:::{self.subgroup}::::{self.repo_1_name}", - commitid=self.commitid, - ) - ) - assert gitlab_subgroup_repository.name == self.repo_1_name - assert ( - gitlab_subgroup_repository.author.username - == f"{self.group}:{self.subgroup}" - ) - assert gitlab_subgroup_repository.author.service == self.gl - assert gitlab_subgroup_repository.author == gitlab_subgroup - - # gitlab subgroup of a subgroup - gitlab_sub_subgroup_repository, gitlab_sub_subgroup = ( - get_repository_and_owner_from_slug_and_commit( - slug=f"{self.group}:::{self.subgroup}:::{self.sub_subgroup}::::{self.repo_1_name}", - commitid=self.commitid, - ) - ) - assert gitlab_sub_subgroup_repository.name == self.repo_1_name - assert ( - gitlab_sub_subgroup_repository.author.username - == f"{self.group}:{self.subgroup}:{self.sub_subgroup}" - ) - assert gitlab_sub_subgroup_repository.author.service == self.gl - assert gitlab_sub_subgroup_repository.author == gitlab_sub_subgroup diff --git a/upload/tests/views/test_test_results.py b/upload/tests/views/test_test_results.py index ee0b5b2421..7276f9fb51 100644 --- a/upload/tests/views/test_test_results.py +++ b/upload/tests/views/test_test_results.py @@ -35,8 +35,7 @@ def test_upload_test_results(db, client, mocker, mock_redis): "build": "test-build", "buildURL": "test-build-url", "job": "test-job", - "service": "test-service", - "ci_service": "github-actions", + "service": "github-actions", "branch": "aaaaaa", }, format="json", @@ -191,6 +190,7 @@ def test_test_results_upload_token_not_required(db, client, mocker, mock_redis): "commit": "6fd5b89357fc8cdf34d6197549ac7c6d7e5977ef", "slug": f"{repository.author.username}::::{repository.name}", "branch": "aaaaaa", + "service": owner.service, }, format="json", ) diff --git a/upload/views/bundle_analysis.py b/upload/views/bundle_analysis.py index ad9ebb29c8..7713b38e49 100644 --- a/upload/views/bundle_analysis.py +++ b/upload/views/bundle_analysis.py @@ -14,10 +14,10 @@ from codecov_auth.authentication.repo_auth import ( BundleAnalysisTokenlessAuthentication, - BundleAnalysisUploadTokenRequiredAuthenticationCheck, GitHubOIDCTokenAuthentication, OrgLevelTokenAuthentication, RepositoryLegacyTokenAuthentication, + UploadTokenRequiredGetFromBodyAuthenticationCheck, repo_auth_custom_exception_handler, ) from codecov_auth.authentication.types import RepositoryAsUser @@ -55,7 +55,7 @@ class UploadSerializer(serializers.Serializer): class BundleAnalysisView(APIView, ShelterMixin): permission_classes = [UploadBundleAnalysisPermission] authentication_classes = [ - BundleAnalysisUploadTokenRequiredAuthenticationCheck, + UploadTokenRequiredGetFromBodyAuthenticationCheck, OrgLevelTokenAuthentication, GitHubOIDCTokenAuthentication, RepositoryLegacyTokenAuthentication, diff --git a/upload/views/test_results.py b/upload/views/test_results.py index 865ff665aa..2ee6d2af50 100644 --- a/upload/views/test_results.py +++ b/upload/views/test_results.py @@ -13,8 +13,8 @@ GitHubOIDCTokenAuthentication, OrgLevelTokenAuthentication, RepositoryLegacyTokenAuthentication, - TestResultsUploadTokenRequiredAuthenticationCheck, TokenlessAuthentication, + UploadTokenRequiredGetFromBodyAuthenticationCheck, repo_auth_custom_exception_handler, ) from codecov_auth.authentication.types import RepositoryAsUser @@ -39,13 +39,13 @@ def has_permission(self, request, view): class UploadSerializer(serializers.Serializer): commit = serializers.CharField(required=True) slug = serializers.CharField(required=True) + service = serializers.CharField(required=False) # git_service build = serializers.CharField(required=False) buildURL = serializers.CharField(required=False) job = serializers.CharField(required=False) flags = FlagListField(required=False) pr = serializers.CharField(required=False) branch = serializers.CharField(required=False, allow_null=True) - ci_service = serializers.CharField(required=False) storage_path = serializers.CharField(required=False) @@ -55,7 +55,7 @@ class TestResultsView( ): permission_classes = [UploadTestResultsPermission] authentication_classes = [ - TestResultsUploadTokenRequiredAuthenticationCheck, + UploadTokenRequiredGetFromBodyAuthenticationCheck, OrgLevelTokenAuthentication, GitHubOIDCTokenAuthentication, RepositoryLegacyTokenAuthentication, @@ -154,7 +154,7 @@ def post(self, request): "build_url": data.get("buildURL"), # build_url "job": data.get("job"), # job_code "flags": data.get("flags"), - "service": data.get("ci_service"), # provider + "service": data.get("service"), # git provider "url": storage_path, # storage_path # these are used for dispatching the task below "commit": commit.commitid, From e5a700035be8738ba289cabf94cf8f8965a0486d Mon Sep 17 00:00:00 2001 From: Nora Shapiro Date: Tue, 15 Oct 2024 13:16:08 -0700 Subject: [PATCH 3/7] remove this chunk --- upload/views/helpers.py | 47 ----------------------------------------- 1 file changed, 47 deletions(-) diff --git a/upload/views/helpers.py b/upload/views/helpers.py index d6b52ab146..cd61677680 100644 --- a/upload/views/helpers.py +++ b/upload/views/helpers.py @@ -1,13 +1,8 @@ -import logging import typing -from shared.django_apps.core.models import Commit - from codecov_auth.models import Owner, Service from core.models import Repository -log = logging.getLogger(__name__) - def get_repository_and_owner_from_string( service: Service, repo_identifier: str @@ -50,45 +45,3 @@ def get_repository_from_string( service=service, repo_identifier=repo_identifier ) return repository - - -def get_repository_and_owner_from_slug_and_commit( - slug: str, commitid: str -) -> tuple[Repository | None, Owner | None]: - if "::::" not in slug: - return None, None - - owner_username, repository_name = slug.rsplit("::::", 1) - - # formatting for GL subgroups - if ":::" in owner_username: - owner_username = owner_username.replace(":::", ":") - - matching_repositories = Repository.objects.filter( - author__username=owner_username, name=repository_name - ).select_related("author") - if matching_repositories.count() == 1: - log.info( - "get_repository_and_owner_from_slug_and_commit success", - extra=dict(slug=slug), - ) - repository = matching_repositories.first() - return repository, repository.author - else: - # commit might not exist yet (if this is first upload for it) - try: - # Commit has UniqueConstraint on commitid + repository - commit = Commit.objects.select_related( - "repository", "repository__author" - ).get(commitid=commitid, repository__in=matching_repositories) - log.info( - "get_repository_and_owner_from_slug_and_commit multiple matches success", - extra=dict(slug=slug), - ) - return commit.repository, commit.repository.author - except (Commit.DoesNotExist, Commit.MultipleObjectsReturned): - log.info( - "get_repository_and_owner_from_slug_and_commit fail", - extra=dict(slug=slug), - ) - return None, None From dfbfb8ebe95ace1aa930844013fc042bfa7ac6ad Mon Sep 17 00:00:00 2001 From: Nora Shapiro Date: Thu, 10 Oct 2024 16:19:05 -0700 Subject: [PATCH 4/7] True Tokenless for BA and TA --- codecov_auth/authentication/repo_auth.py | 93 +++- .../tests/unit/test_repo_authentication.py | 453 +++++++++++++++++- upload/tests/views/test_bundle_analysis.py | 42 ++ upload/tests/views/test_helpers.py | 96 +++- upload/tests/views/test_test_results.py | 28 ++ upload/views/bundle_analysis.py | 2 + upload/views/helpers.py | 47 ++ upload/views/test_results.py | 4 +- 8 files changed, 756 insertions(+), 9 deletions(-) diff --git a/codecov_auth/authentication/repo_auth.py b/codecov_auth/authentication/repo_auth.py index cfb3b4d2c4..0e855b6c11 100644 --- a/codecov_auth/authentication/repo_auth.py +++ b/codecov_auth/authentication/repo_auth.py @@ -24,6 +24,7 @@ from core.models import Commit, Repository from upload.helpers import get_global_tokens, get_repo_with_github_actions_oidc_token from upload.views.helpers import ( + get_repository_and_owner_from_slug_and_commit, get_repository_and_owner_from_string, get_repository_from_string, ) @@ -345,7 +346,7 @@ def get_branch(self, request, repoid=None, commitid=None): body = json.loads(str(request.body, "utf8")) # If commit is not created yet (ie first upload for this commit), we just validate branch format. - # However if a commit exists already (ie not the first upload for this commit), we must additionally + # However, if a commit exists already (ie not the first upload for this commit), we must additionally # validate the saved commit branch matches what is requested in this upload call. commit = Commit.objects.filter(repository_id=repoid, commitid=commitid).first() if commit and commit.branch != body.get("branch"): @@ -381,10 +382,15 @@ def _get_info_from_request_path( return repository, owner + def get_repository_and_owner( + self, request: HttpRequest + ) -> tuple[Repository | None, Owner | None]: + return self._get_info_from_request_path(request) + def authenticate( self, request: HttpRequest ) -> tuple[RepositoryAsUser, TokenlessAuth] | None: - repository, owner = self._get_info_from_request_path(request) + repository, owner = self.get_repository_and_owner(request) if ( repository is None @@ -398,3 +404,86 @@ def authenticate( RepositoryAsUser(repository), TokenlessAuth(repository), ) + + +class TestResultsUploadTokenRequiredAuthenticationCheck( + UploadTokenRequiredAuthenticationCheck +): + """ + Repository and Owner are not in the path for this endpoint, have to get them another way, + then use the same authenticate() as parent class. + """ + + def _get_info_from_request_data( + self, request: HttpRequest + ) -> tuple[Repository | None, Owner | None]: + from upload.views.test_results import UploadSerializer # circular imports + + try: + body = json.loads(str(request.body, "utf8")) + except json.JSONDecodeError: + return None, None # continue to next auth class + + serializer = UploadSerializer(data=body) + if not serializer.is_valid(): + return None, None # continue to next auth class + + # these fields are required=True + slug = serializer.validated_data["slug"] + commitid = serializer.validated_data["commit"] + + return get_repository_and_owner_from_slug_and_commit( + slug=slug, commitid=commitid + ) + + def get_repository_and_owner( + self, request: HttpRequest + ) -> tuple[Repository | None, Owner | None]: + return self._get_info_from_request_data(request) + + +class BundleAnalysisUploadTokenRequiredAuthenticationCheck( + UploadTokenRequiredAuthenticationCheck +): + """ + Repository and Owner are not in the path for this endpoint, have to get them another way, + then use the same authenticate() as parent class. + """ + + def _get_info_from_request_data( + self, request: HttpRequest + ) -> tuple[Repository | None, Owner | None]: + from upload.views.bundle_analysis import UploadSerializer # circular imports + + try: + body = json.loads(str(request.body, "utf8")) + except json.JSONDecodeError: + return None, None # continue to next auth class + + serializer = UploadSerializer(data=body) + if not serializer.is_valid(): + return None, None # continue to next auth class + + # these fields are required=True + slug = serializer.validated_data["slug"] + commitid = serializer.validated_data["commit"] + + # this field is required=False but is much better for getting repository and owner + service = serializer.validated_data.get("git_service") + if service: + try: + service_enum = Service(service) + except ValueError: + return None, None + return get_repository_and_owner_from_string( + service=service_enum, repo_identifier=slug + ) + + return get_repository_and_owner_from_slug_and_commit( + slug=slug, commitid=commitid + ) + + def get_repository_and_owner( + self, request: HttpRequest + ) -> tuple[Repository | None, Owner | None]: + return self._get_info_from_request_data(request) diff --git a/codecov_auth/tests/unit/test_repo_authentication.py b/codecov_auth/tests/unit/test_repo_authentication.py index 1f44d99bcc..1da5deb5aa 100644 --- a/codecov_auth/tests/unit/test_repo_authentication.py +++ b/codecov_auth/tests/unit/test_repo_authentication.py @@ -10,14 +10,18 @@ from jwt import PyJWTError from rest_framework import exceptions from rest_framework.test import APIRequestFactory +from shared.django_apps.codecov_auth.models import Owner, Service +from shared.django_apps.core.models import Repository from codecov_auth.authentication.repo_auth import ( + BundleAnalysisUploadTokenRequiredAuthenticationCheck, GitHubOIDCTokenAuthentication, GlobalTokenAuthentication, OrgLevelTokenAuthentication, RepositoryLegacyQueryTokenAuthentication, RepositoryLegacyTokenAuthentication, RepositoryTokenAuthentication, + TestResultsUploadTokenRequiredAuthenticationCheck, TokenlessAuth, TokenlessAuthentication, UploadTokenRequiredAuthenticationCheck, @@ -597,7 +601,7 @@ def test_token_not_required_unknown_owner(self, db): @pytest.mark.parametrize("request_uri,repo_slug,commitid", valid_params_to_test) @pytest.mark.parametrize("private", [False, True]) @pytest.mark.parametrize("token_required", [False, True]) - def test_token_not_required_matches_paths( + def test_get_repository_and_owner( self, request_uri, repo_slug, commitid, private, token_required, db ): author_name, repo_name = repo_slug.split("/") @@ -612,7 +616,7 @@ def test_token_not_required_matches_paths( request_uri, {"branch": "fork:branch"}, format="json" ) authentication = UploadTokenRequiredAuthenticationCheck() - assert authentication._get_info_from_request_path(request) == ( + assert authentication.get_repository_and_owner(request) == ( repo, repo.author, ) @@ -670,3 +674,448 @@ def test_token_not_required_fork_branch_public_private( else: res = authentication.authenticate(request) assert res is None + + +class TestTestResultsUploadTokenRequiredAuthenticationCheck(object): + def test_token_not_required_invalid_data(self): + request = APIRequestFactory().post( + "/endpoint", + data={"slug": 123, "commit": 456}, + format="json", + ) + authentication = TestResultsUploadTokenRequiredAuthenticationCheck() + res = authentication.authenticate(request) + assert res is None + + def test_token_not_required_no_data(self): + request = APIRequestFactory().post( + "/endpoint", + format="json", + ) + authentication = TestResultsUploadTokenRequiredAuthenticationCheck() + res = authentication.authenticate(request) + assert res is None + + def test_token_not_required_unknown_repository(self, db): + an_owner = OwnerFactory(upload_token_required_for_public_repos=False) + # their repo + RepositoryFactory(author=an_owner, private=False) + request_uri = f"/upload/github/{an_owner.username}::::bad/commits" + request = APIRequestFactory().post( + request_uri, + data={ + "slug": f"{an_owner.username}::::bad", + "commit": "this is a commitid", + }, + format="json", + ) + authentication = TestResultsUploadTokenRequiredAuthenticationCheck() + res = authentication.authenticate(request) + assert res is None + + def test_token_not_required_unknown_owner(self, db): + repo = RepositoryFactory( + private=False, author__upload_token_required_for_public_repos=False + ) + request_uri = f"/upload/github/bad::::{repo.name}/commits" + request = APIRequestFactory().post( + request_uri, + data={"slug": f"bad::::{repo.name}", "commit": "this is a commitid"}, + format="json", + ) + authentication = TestResultsUploadTokenRequiredAuthenticationCheck() + res = authentication.authenticate(request) + assert res is None + + @pytest.mark.parametrize("request_uri,repo_slug,commitid", valid_params_to_test) + @pytest.mark.parametrize("private", [False, True]) + @pytest.mark.parametrize("token_required", [False, True]) + def test_get_repository_and_owner( + self, request_uri, repo_slug, commitid, private, token_required, db + ): + author_name, repo_name = repo_slug.split("/") + repo = RepositoryFactory( + name=repo_name, + author__username=author_name, + private=private, + author__upload_token_required_for_public_repos=token_required, + ) + assert repo.service == "github" + request = APIRequestFactory().post( + request_uri, + data={"slug": f"{author_name}::::{repo_name}", "commit": commitid}, + format="json", + ) + authentication = TestResultsUploadTokenRequiredAuthenticationCheck() + + if commitid is None: + # fails serializer validation for this endpoint + assert authentication.get_repository_and_owner(request) == (None, None) + else: + assert authentication.get_repository_and_owner(request) == ( + repo, + repo.author, + ) + + def test_get_repository_and_owner_by_commitid(self, db): + authentication = TestResultsUploadTokenRequiredAuthenticationCheck() + repo_name = "the-repo" + owner_username = "the-author" + for name, _ in Service.choices: + owner = OwnerFactory(service=name, username=owner_username) + RepositoryFactory(name=repo_name, author=owner) + + request = APIRequestFactory().post( + "endpoint/", + data={"slug": f"{owner_username}::::{repo_name}", "commit": "new commit"}, + format="json", + ) + assert authentication.get_repository_and_owner(request) == (None, None) + + commit_on_random_repo = CommitFactory() + request = APIRequestFactory().post( + "endpoint/", + data={ + "slug": f"{owner_username}::::{repo_name}", + "commit": commit_on_random_repo.commitid, + }, + format="json", + ) + assert authentication.get_repository_and_owner(request) == (None, None) + + matching_repo = Repository.objects.filter( + name=repo_name, author__username=owner_username + ).first() + commit_on_matching_repo = CommitFactory(repository=matching_repo) + request = APIRequestFactory().post( + "endpoint/", + data={ + "slug": f"{owner_username}::::{repo_name}", + "commit": commit_on_matching_repo.commitid, + }, + format="json", + ) + assert authentication.get_repository_and_owner(request) == ( + matching_repo, + matching_repo.author, + ) + + @pytest.mark.parametrize("private", [False, True]) + @pytest.mark.parametrize("branch", ["branch", "fork:branch"]) + @pytest.mark.parametrize( + "existing_commit,commit_branch", + [(False, None), (True, "branch"), (True, "fork:branch")], + ) + @pytest.mark.parametrize("token_required", [False, True]) + def test_token_not_required_fork_branch_public_private( + self, + db, + mocker, + private, + branch, + existing_commit, + commit_branch, + token_required, + ): + repo = RepositoryFactory( + private=private, + author__upload_token_required_for_public_repos=token_required, + ) + + if existing_commit: + commit = CommitFactory() + commit.branch = commit_branch + commit.repository = repo + commit.save() + + request = APIRequestFactory().post( + f"/upload/github/{repo.author.username}::::{repo.name}/commits/{commit.commitid}/reports/report_code/uploads", + data={ + "slug": f"{repo.author.username}::::{repo.name}", + "commit": commit.commitid, + }, + format="json", + ) + + else: + request = APIRequestFactory().post( + f"/upload/github/{repo.author.username}::::{repo.name}/commits", + data={ + "slug": f"{repo.author.username}::::{repo.name}", + "commit": "this is a commitid", + }, + format="json", + ) + + authentication = TestResultsUploadTokenRequiredAuthenticationCheck() + + if not private and not token_required: + res = authentication.authenticate(request) + assert res is not None + repo_as_user, auth_class = res + + assert repo_as_user.is_authenticated() is True + assert isinstance(auth_class, TokenlessAuth) + else: + res = authentication.authenticate(request) + assert res is None + + +class TestBundleAnalysisUploadTokenRequiredAuthenticationCheck(object): + def test_token_not_required_invalid_data(self): + request = APIRequestFactory().post( + "/endpoint", + data={"slug": 123, "commit": 456}, + format="json", + ) + authentication = BundleAnalysisUploadTokenRequiredAuthenticationCheck() + res = authentication.authenticate(request) + assert res is None + + def test_token_not_required_no_data(self): + request = APIRequestFactory().post( + "/endpoint", + format="json", + ) + authentication = BundleAnalysisUploadTokenRequiredAuthenticationCheck() + res = authentication.authenticate(request) + assert res is None + + def test_token_not_required_unknown_repository(self, db): + an_owner = OwnerFactory(upload_token_required_for_public_repos=False) + # their repo + RepositoryFactory(author=an_owner, private=False) + request_uri = f"/upload/github/{an_owner.username}::::bad/commits" + request = APIRequestFactory().post( + request_uri, + data={ + "slug": f"{an_owner.username}::::bad", + "commit": "this is a commitid", + "service": an_owner.service, + }, + format="json", + ) + authentication = BundleAnalysisUploadTokenRequiredAuthenticationCheck() + assert authentication.get_repository_and_owner(request) == (None, None) + res = authentication.authenticate(request) + assert res is None + + def test_token_not_required_unknown_owner(self, db): + repo = RepositoryFactory( + private=False, author__upload_token_required_for_public_repos=False + ) + request_uri = f"/upload/github/bad::::{repo.name}/commits" + request = APIRequestFactory().post( + request_uri, + data={"slug": f"bad::::{repo.name}", "commit": "this is a commitid"}, + format="json", + ) + authentication = BundleAnalysisUploadTokenRequiredAuthenticationCheck() + res = authentication.authenticate(request) + assert res is None + + @pytest.mark.parametrize("request_uri,repo_slug,commitid", valid_params_to_test) + @pytest.mark.parametrize("private", [False, True]) + @pytest.mark.parametrize("token_required", [False, True]) + def test_get_repository_and_owner( + self, request_uri, repo_slug, commitid, private, token_required, db + ): + author_name, repo_name = repo_slug.split("/") + repo = RepositoryFactory( + name=repo_name, + author__username=author_name, + private=private, + author__upload_token_required_for_public_repos=token_required, + ) + assert repo.service == "github" + request = APIRequestFactory().post( + request_uri, + data={"slug": f"{author_name}::::{repo_name}", "commit": commitid}, + format="json", + ) + authentication = BundleAnalysisUploadTokenRequiredAuthenticationCheck() + + if commitid is None: + # fails serializer validation for this endpoint + assert authentication.get_repository_and_owner(request) == (None, None) + else: + assert authentication.get_repository_and_owner(request) == ( + repo, + repo.author, + ) + + def test_get_repository_and_owner_by_commitid(self, db): + authentication = BundleAnalysisUploadTokenRequiredAuthenticationCheck() + repo_name = "the-repo" + owner_username = "the-author" + for name, _ in Service.choices: + owner = OwnerFactory(service=name, username=owner_username) + RepositoryFactory(name=repo_name, author=owner) + + request = APIRequestFactory().post( + "endpoint/", + data={"slug": f"{owner_username}::::{repo_name}", "commit": "new commit"}, + format="json", + ) + assert authentication.get_repository_and_owner(request) == (None, None) + + commit_on_random_repo = CommitFactory() + request = APIRequestFactory().post( + "endpoint/", + data={ + "slug": f"{owner_username}::::{repo_name}", + "commit": commit_on_random_repo.commitid, + }, + format="json", + ) + assert authentication.get_repository_and_owner(request) == (None, None) + + matching_repo = Repository.objects.filter( + name=repo_name, author__username=owner_username + ).first() + commit_on_matching_repo = CommitFactory(repository=matching_repo) + request = APIRequestFactory().post( + "endpoint/", + data={ + "slug": f"{owner_username}::::{repo_name}", + "commit": commit_on_matching_repo.commitid, + }, + format="json", + ) + assert authentication.get_repository_and_owner(request) == ( + matching_repo, + matching_repo.author, + ) + + def test_get_repository_and_owner_with_service(self, db): + authentication = BundleAnalysisUploadTokenRequiredAuthenticationCheck() + repo_name = "the-repo" + owner_username = "the-author" + for name, _ in Service.choices: + owner = OwnerFactory(service=name, username=owner_username) + RepositoryFactory(name=repo_name, author=owner) + + request = APIRequestFactory().post( + "endpoint/", + data={ + "slug": f"{owner_username}::::{repo_name}", + "commit": "new commit", + "git_service": Service.BITBUCKET.value, + }, + format="json", + ) + matching_owner = Owner.objects.get( + service=Service.BITBUCKET.value, username=owner_username + ) + matching_repo = Repository.objects.get( + name=repo_name, author__service=Service.BITBUCKET.value + ) + assert authentication.get_repository_and_owner(request) == ( + matching_repo, + matching_owner, + ) + + commit_on_random_repo = CommitFactory() + request = APIRequestFactory().post( + "endpoint/", + data={ + "slug": f"{owner_username}::::{repo_name}", + "commit": commit_on_random_repo.commitid, + "git_service": Service.GITLAB.value, + }, + format="json", + ) + matching_owner = Owner.objects.get( + service=Service.GITLAB.value, username=owner_username + ) + matching_repo = Repository.objects.get( + name=repo_name, author__service=Service.GITLAB.value + ) + assert authentication.get_repository_and_owner(request) == ( + matching_repo, + matching_owner, + ) + + matching_repo = Repository.objects.filter( + name=repo_name, author__username=owner_username + ).first() + commit_on_matching_repo = CommitFactory(repository=matching_repo) + request = APIRequestFactory().post( + "endpoint/", + data={ + "slug": f"{owner_username}::::{repo_name}", + "commit": commit_on_matching_repo.commitid, + "git_service": Service.GITHUB.value, + }, + format="json", + ) + matching_owner = Owner.objects.get( + service=Service.GITHUB.value, username=owner_username + ) + matching_repo = Repository.objects.get( + name=repo_name, author__service=Service.GITHUB.value + ) + assert authentication.get_repository_and_owner(request) == ( + matching_repo, + matching_owner, + ) + + @pytest.mark.parametrize("private", [False, True]) + @pytest.mark.parametrize("branch", ["branch", "fork:branch"]) + @pytest.mark.parametrize( + "existing_commit,commit_branch", + [(False, None), (True, "branch"), (True, "fork:branch")], + ) + @pytest.mark.parametrize("token_required", [False, True]) + def test_token_not_required_fork_branch_public_private( + self, + db, + mocker, + private, + branch, + existing_commit, + commit_branch, + token_required, + ): + repo = RepositoryFactory( + private=private, + author__upload_token_required_for_public_repos=token_required, + ) + + if existing_commit: + commit = CommitFactory() + commit.branch = commit_branch + commit.repository = repo + commit.save() + + request = APIRequestFactory().post( + f"/upload/github/{repo.author.username}::::{repo.name}/commits/{commit.commitid}/reports/report_code/uploads", + data={ + "slug": f"{repo.author.username}::::{repo.name}", + "commit": commit.commitid, + }, + format="json", + ) + + else: + request = APIRequestFactory().post( + f"/upload/github/{repo.author.username}::::{repo.name}/commits", + data={ + "slug": f"{repo.author.username}::::{repo.name}", + "commit": "this is a commitid", + }, + format="json", + ) + + authentication = BundleAnalysisUploadTokenRequiredAuthenticationCheck() + + if not private and not token_required: + res = authentication.authenticate(request) + assert res is not None + repo_as_user, auth_class = res + + assert repo_as_user.is_authenticated() is True + assert isinstance(auth_class, TokenlessAuth) + else: + res = authentication.authenticate(request) + assert res is None diff --git a/upload/tests/views/test_bundle_analysis.py b/upload/tests/views/test_bundle_analysis.py index 8703e9c754..2ce25d5c27 100644 --- a/upload/tests/views/test_bundle_analysis.py +++ b/upload/tests/views/test_bundle_analysis.py @@ -418,6 +418,48 @@ def test_upload_bundle_analysis_tokenless_success(db, client, mocker, mock_redis create_presigned_put.assert_called_once_with("bundle-analysis", ANY, 30) +@pytest.mark.django_db(databases={"default", "timeseries"}) +def test_upload_bundle_analysis_true_tokenless_success(db, client, mocker, mock_redis): + upload = mocker.patch.object(TaskService, "upload") + + create_presigned_put = mocker.patch( + "services.archive.StorageService.create_presigned_put", + return_value="test-presigned-put", + ) + + repository = RepositoryFactory.create( + private=False, + author__upload_token_required_for_public_repos=False, + author__service="github", + ) + client = APIClient() + + res = client.post( + reverse("upload-bundle-analysis"), + { + "commit": "any", + "slug": f"{repository.author.username}::::{repository.name}", + "build": "test-build", + "buildURL": "test-build-url", + "job": "test-job", + "service": "test-service", + "compareSha": "6fd5b89357fc8cdf34d6197549ac7c6d7e5aaaaa", + "branch": "f1:main", + "git_service": "github", + }, + format="json", + headers={"User-Agent": "codecov-cli/0.4.7"}, + ) + + assert res.status_code == 201 + + # returns presigned storage URL + assert res.json() == {"url": "test-presigned-put"} + + assert upload.called + create_presigned_put.assert_called_once_with("bundle-analysis", ANY, 30) + + @pytest.mark.django_db(databases={"default", "timeseries"}) def test_upload_bundle_analysis_tokenless_no_repo(db, client, mocker, mock_redis): upload = mocker.patch.object(TaskService, "upload") diff --git a/upload/tests/views/test_helpers.py b/upload/tests/views/test_helpers.py index 439f65c9fa..afff7b7c83 100644 --- a/upload/tests/views/test_helpers.py +++ b/upload/tests/views/test_helpers.py @@ -1,8 +1,11 @@ from django.test import TestCase +from shared.django_apps.codecov_auth.models import Owner +from shared.django_apps.core.models import Repository from codecov_auth.models import Service -from core.tests.factories import OwnerFactory, RepositoryFactory +from core.tests.factories import CommitFactory, OwnerFactory, RepositoryFactory from upload.views.helpers import ( + get_repository_and_owner_from_slug_and_commit, get_repository_and_owner_from_string, get_repository_from_string, ) @@ -21,6 +24,7 @@ def setUpClass(cls): cls.group = "somegroup" cls.subgroup = "somesubgroup" cls.sub_subgroup = "subsub" + cls.commitid = CommitFactory().commitid owners_to_repos_mapping = { cls.gh: {cls.uname: [cls.repo_1_name, cls.repo_2_name]}, cls.bb: {cls.uname: [cls.repo_1_name, cls.repo_2_name]}, @@ -36,10 +40,10 @@ def setUpClass(cls): } for service, owner_data in owners_to_repos_mapping.items(): for username, repo_names in owner_data.items(): - RepositoryFactory() o = OwnerFactory(service=service, username=username) for r_name in repo_names: - RepositoryFactory(author=o, name=r_name) + r = RepositoryFactory(author=o, name=r_name) + CommitFactory(repository=r, commitid=cls.commitid) def test_get_repository_from_string(self): first_result = get_repository_from_string( @@ -149,3 +153,89 @@ def test_get_repository_and_owner_from_string(self): assert get_repository_and_owner_from_string( self.gl, f"{self.group}:::somebadsubgroup::::{self.repo_1_name}" ) == (None, None) + + def test_get_repository_and_owner_from_slug_and_commit(self): + # the test cases from setUpClass won't work on this method, they all have + # same username, repo name, and commitid + assert get_repository_and_owner_from_slug_and_commit( + slug=f"{self.uname}::::{self.repo_1_name}", commitid=self.commitid + ) == (None, None) + + # can identify by unique commitid + gh_owner = Owner.objects.get(service=self.gh, username=self.uname) + gh_repo = Repository.objects.get(author=gh_owner, name=self.repo_1_name) + unique_commit = CommitFactory(repository=gh_repo) + assert get_repository_and_owner_from_slug_and_commit( + slug=f"{self.uname}::::{self.repo_1_name}", commitid=unique_commit.commitid + ) == (gh_repo, gh_owner) + + # can identify by unique username + gl_owner = Owner.objects.get(service=self.gl, username=self.uname) + gl_repo = Repository.objects.get(author=gl_owner, name=self.repo_1_name) + gl_owner.username = "other" + gl_owner.save() + assert get_repository_and_owner_from_slug_and_commit( + slug=f"other::::{self.repo_1_name}", commitid=self.commitid + ) == (gl_repo, gl_owner) + + # can identify by unique repo name + another_gh_repo = RepositoryFactory(author=gh_owner, name="another") + CommitFactory(repository=another_gh_repo, commitid=self.commitid) + assert get_repository_and_owner_from_slug_and_commit( + slug=f"{self.uname}::::another", commitid=self.commitid + ) == (another_gh_repo, gh_owner) + + assert get_repository_and_owner_from_slug_and_commit( + slug=f"somerandomlalala::::{self.repo_1_name}", commitid=self.commitid + ) == (None, None) + assert get_repository_and_owner_from_slug_and_commit( + slug=f"{self.uname}::::{self.repo_1_name}wrongname", commitid=self.commitid + ) == (None, None) + assert get_repository_and_owner_from_slug_and_commit( + slug=f"{self.uname}::::{self.repo_1_name}", commitid="nonsense" + ) == (None, None) + assert get_repository_and_owner_from_slug_and_commit( + slug=f"{self.group}:::somebadsubgroup::::{self.repo_1_name}", + commitid=self.commitid, + ) == (None, None) + + # gitlab group + gitlab_group_repository, gitlab_group = ( + get_repository_and_owner_from_slug_and_commit( + slug=f"{self.group}::::{self.repo_1_name}", commitid=self.commitid + ) + ) + assert gitlab_group_repository.name == self.repo_1_name + assert gitlab_group_repository.author.username == self.group + assert gitlab_group_repository.author.service == self.gl + assert gitlab_group_repository.author == gitlab_group + + # gitlab subgroup + gitlab_subgroup_repository, gitlab_subgroup = ( + get_repository_and_owner_from_slug_and_commit( + slug=f"{self.group}:::{self.subgroup}::::{self.repo_1_name}", + commitid=self.commitid, + ) + ) + assert gitlab_subgroup_repository.name == self.repo_1_name + assert ( + gitlab_subgroup_repository.author.username + == f"{self.group}:{self.subgroup}" + ) + assert gitlab_subgroup_repository.author.service == self.gl + assert gitlab_subgroup_repository.author == gitlab_subgroup + + # gitlab subgroup of a subgroup + gitlab_sub_subgroup_repository, gitlab_sub_subgroup = ( + get_repository_and_owner_from_slug_and_commit( + slug=f"{self.group}:::{self.subgroup}:::{self.sub_subgroup}::::{self.repo_1_name}", + commitid=self.commitid, + ) + ) + assert gitlab_sub_subgroup_repository.name == self.repo_1_name + assert ( + gitlab_sub_subgroup_repository.author.username + == f"{self.group}:{self.subgroup}:{self.sub_subgroup}" + ) + assert gitlab_sub_subgroup_repository.author.service == self.gl + assert gitlab_sub_subgroup_repository.author == gitlab_sub_subgroup diff --git a/upload/tests/views/test_test_results.py b/upload/tests/views/test_test_results.py index 9035c4e9d3..ee0b5b2421 100644 --- a/upload/tests/views/test_test_results.py +++ b/upload/tests/views/test_test_results.py @@ -169,6 +169,34 @@ def test_test_results_github_oidc_token( assert res.status_code == 201 +def test_test_results_upload_token_not_required(db, client, mocker, mock_redis): + mocker.patch.object(TaskService, "upload") + mocker.patch( + "services.archive.StorageService.create_presigned_put", + return_value="test-presigned-put", + ) + + owner = OwnerFactory( + service="github", + username="codecov", + upload_token_required_for_public_repos=False, + ) + repository = RepositoryFactory.create(author=owner, private=False) + + client = APIClient() + + res = client.post( + reverse("upload-test-results"), + { + "commit": "6fd5b89357fc8cdf34d6197549ac7c6d7e5977ef", + "slug": f"{repository.author.username}::::{repository.name}", + "branch": "aaaaaa", + }, + format="json", + ) + assert res.status_code == 201 + + def test_test_results_no_auth(db, client, mocker, mock_redis): owner = OwnerFactory(service="github", username="codecov") repository = RepositoryFactory.create(author=owner) diff --git a/upload/views/bundle_analysis.py b/upload/views/bundle_analysis.py index 6b2d095501..ad9ebb29c8 100644 --- a/upload/views/bundle_analysis.py +++ b/upload/views/bundle_analysis.py @@ -14,6 +14,7 @@ from codecov_auth.authentication.repo_auth import ( BundleAnalysisTokenlessAuthentication, + BundleAnalysisUploadTokenRequiredAuthenticationCheck, GitHubOIDCTokenAuthentication, OrgLevelTokenAuthentication, RepositoryLegacyTokenAuthentication, @@ -54,6 +55,7 @@ class UploadSerializer(serializers.Serializer): class BundleAnalysisView(APIView, ShelterMixin): permission_classes = [UploadBundleAnalysisPermission] authentication_classes = [ + BundleAnalysisUploadTokenRequiredAuthenticationCheck, OrgLevelTokenAuthentication, GitHubOIDCTokenAuthentication, RepositoryLegacyTokenAuthentication, diff --git a/upload/views/helpers.py b/upload/views/helpers.py index cd61677680..d6b52ab146 100644 --- a/upload/views/helpers.py +++ b/upload/views/helpers.py @@ -1,8 +1,13 @@ +import logging import typing +from shared.django_apps.core.models import Commit + from codecov_auth.models import Owner, Service from core.models import Repository +log = logging.getLogger(__name__) + def get_repository_and_owner_from_string( service: Service, repo_identifier: str @@ -45,3 +50,45 @@ def get_repository_from_string( service=service, repo_identifier=repo_identifier ) return repository + + +def get_repository_and_owner_from_slug_and_commit( + slug: str, commitid: str +) -> tuple[Repository | None, Owner | None]: + if "::::" not in slug: + return None, None + + owner_username, repository_name = slug.rsplit("::::", 1) + + # formatting for GL subgroups + if ":::" in owner_username: + owner_username = owner_username.replace(":::", ":") + + matching_repositories = Repository.objects.filter( + author__username=owner_username, name=repository_name + ).select_related("author") + if matching_repositories.count() == 1: + log.info( + "get_repository_and_owner_from_slug_and_commit success", + extra=dict(slug=slug), + ) + repository = matching_repositories.first() + return repository, repository.author + else: + # commit might not exist yet (if this is first upload for it) + try: + # Commit has UniqueConstraint on commitid + repository + commit = Commit.objects.select_related( + "repository", "repository__author" + ).get(commitid=commitid, repository__in=matching_repositories) + log.info( + "get_repository_and_owner_from_slug_and_commit multiple matches success", + extra=dict(slug=slug), + ) + return commit.repository, commit.repository.author + except (Commit.DoesNotExist, Commit.MultipleObjectsReturned): + log.info( + "get_repository_and_owner_from_slug_and_commit fail", + extra=dict(slug=slug), + ) + return None, None diff --git a/upload/views/test_results.py b/upload/views/test_results.py index b3e2609ebd..865ff665aa 100644 --- a/upload/views/test_results.py +++ b/upload/views/test_results.py @@ -13,8 +13,8 @@ GitHubOIDCTokenAuthentication, OrgLevelTokenAuthentication, RepositoryLegacyTokenAuthentication, + TestResultsUploadTokenRequiredAuthenticationCheck, TokenlessAuthentication, - UploadTokenRequiredAuthenticationCheck, repo_auth_custom_exception_handler, ) from codecov_auth.authentication.types import RepositoryAsUser @@ -55,7 +55,7 @@ class TestResultsView( ): permission_classes = [UploadTestResultsPermission] authentication_classes = [ - UploadTokenRequiredAuthenticationCheck, + TestResultsUploadTokenRequiredAuthenticationCheck, OrgLevelTokenAuthentication, GitHubOIDCTokenAuthentication, RepositoryLegacyTokenAuthentication, From db650f1b7ee2bb3cf5e7de5286dfbf1d2634898c Mon Sep 17 00:00:00 2001 From: Nora Shapiro Date: Fri, 11 Oct 2024 18:06:51 -0700 Subject: [PATCH 5/7] redo the whole thing, condense into one class that both views share --- codecov_auth/authentication/repo_auth.py | 95 +++--- .../tests/unit/test_repo_authentication.py | 272 ++---------------- upload/tests/views/test_helpers.py | 96 +------ upload/tests/views/test_test_results.py | 4 +- upload/views/bundle_analysis.py | 4 +- upload/views/test_results.py | 8 +- 6 files changed, 68 insertions(+), 411 deletions(-) diff --git a/codecov_auth/authentication/repo_auth.py b/codecov_auth/authentication/repo_auth.py index 0e855b6c11..c036b2d89b 100644 --- a/codecov_auth/authentication/repo_auth.py +++ b/codecov_auth/authentication/repo_auth.py @@ -8,7 +8,7 @@ from django.http import HttpRequest from django.utils import timezone from jwt import PyJWTError -from rest_framework import authentication, exceptions +from rest_framework import authentication, exceptions, serializers from rest_framework.exceptions import NotAuthenticated from rest_framework.views import exception_handler from shared.django_apps.codecov_auth.models import Owner @@ -24,7 +24,6 @@ from core.models import Commit, Repository from upload.helpers import get_global_tokens, get_repo_with_github_actions_oidc_token from upload.views.helpers import ( - get_repository_and_owner_from_slug_and_commit, get_repository_and_owner_from_string, get_repository_from_string, ) @@ -406,84 +405,54 @@ def authenticate( ) -class TestResultsUploadTokenRequiredAuthenticationCheck( - UploadTokenRequiredAuthenticationCheck -): - """ - Repository and Owner are not in the path for this endpoint, have to get them another way, - then use the same authenticate() as parent class. - """ - - def _get_info_from_request_data( - self, request: HttpRequest - ) -> tuple[Repository | None, Owner | None]: - from upload.views.test_results import UploadSerializer # circular imports - - try: - body = json.loads(str(request.body, "utf8")) - except json.JSONDecodeError: - return None, None # continue to next auth class - - serializer = UploadSerializer(data=body) - if not serializer.is_valid(): - return None, None # continue to next auth class - - # these fields are required=True - slug = serializer.validated_data["slug"] - commitid = serializer.validated_data["commit"] - - return get_repository_and_owner_from_slug_and_commit( - slug=slug, commitid=commitid - ) - - def get_repository_and_owner( - self, request: HttpRequest - ) -> tuple[Repository | None, Owner | None]: - return self._get_info_from_request_data(request) +class UploadTokenRequiredGetFromBodySerializer(serializers.Serializer): + slug = serializers.CharField(required=True) + service = serializers.CharField(required=False) # git_service from TA + git_service = serializers.CharField(required=False) # git_service from BA -class BundleAnalysisUploadTokenRequiredAuthenticationCheck( +class UploadTokenRequiredGetFromBodyAuthenticationCheck( UploadTokenRequiredAuthenticationCheck ): """ - Repository and Owner are not in the path for this endpoint, have to get them another way, + Get Repository and Owner from request body instead of path, then use the same authenticate() as parent class. """ - def _get_info_from_request_data( + def _get_git(self, validated_data): + """ + BA sends this in as git_service, TA sends this in as service. + Use this function so this Check class can be used by both views. + """ + git_service = validated_data.get("git_service") + if not git_service: + git_service = validated_data.get("service") + return git_service + + def _get_info_from_request_body( self, request: HttpRequest ) -> tuple[Repository | None, Owner | None]: - from upload.views.bundle_analysis import UploadSerializer # circular imports - try: body = json.loads(str(request.body, "utf8")) - except json.JSONDecodeError: - return None, None # continue to next auth class - serializer = UploadSerializer(data=body) - if not serializer.is_valid(): - return None, None # continue to next auth class + serializer = UploadTokenRequiredGetFromBodySerializer(data=body) - # these fields are required=True - slug = serializer.validated_data["slug"] - commitid = serializer.validated_data["commit"] + if serializer.is_valid(): + git_service = self._get_git(validated_data=serializer.validated_data) + service_enum = Service(git_service) + return get_repository_and_owner_from_string( + service=service_enum, + repo_identifier=serializer.validated_data["slug"], + ) - # this field is required=False but is much better for getting repository and owner - service = serializer.validated_data.get("git_service") - if service: - try: - service_enum = Service(service) - except ValueError: - return None, None - return get_repository_and_owner_from_string( - service=service_enum, repo_identifier=slug - ) + except (json.JSONDecodeError, ValueError): + # exceptions raised by json.loads() and Service() + # catch rather than raise to continue to next auth class + pass - return get_repository_and_owner_from_slug_and_commit( - slug=slug, commitid=commitid - ) + return None, None # continue to next auth class def get_repository_and_owner( self, request: HttpRequest ) -> tuple[Repository | None, Owner | None]: - return self._get_info_from_request_data(request) + return self._get_info_from_request_body(request) diff --git a/codecov_auth/tests/unit/test_repo_authentication.py b/codecov_auth/tests/unit/test_repo_authentication.py index 1da5deb5aa..1329856585 100644 --- a/codecov_auth/tests/unit/test_repo_authentication.py +++ b/codecov_auth/tests/unit/test_repo_authentication.py @@ -14,17 +14,16 @@ from shared.django_apps.core.models import Repository from codecov_auth.authentication.repo_auth import ( - BundleAnalysisUploadTokenRequiredAuthenticationCheck, GitHubOIDCTokenAuthentication, GlobalTokenAuthentication, OrgLevelTokenAuthentication, RepositoryLegacyQueryTokenAuthentication, RepositoryLegacyTokenAuthentication, RepositoryTokenAuthentication, - TestResultsUploadTokenRequiredAuthenticationCheck, TokenlessAuth, TokenlessAuthentication, UploadTokenRequiredAuthenticationCheck, + UploadTokenRequiredGetFromBodyAuthenticationCheck, ) from codecov_auth.models import SERVICE_GITHUB, OrganizationLevelToken, RepositoryToken from codecov_auth.tests.factories import OwnerFactory @@ -676,14 +675,14 @@ def test_token_not_required_fork_branch_public_private( assert res is None -class TestTestResultsUploadTokenRequiredAuthenticationCheck(object): +class TestUploadTokenRequiredGetFromBodyAuthenticationCheck(object): def test_token_not_required_invalid_data(self): request = APIRequestFactory().post( "/endpoint", - data={"slug": 123, "commit": 456}, + data={"slug": 123, "git_service": "github"}, format="json", ) - authentication = TestResultsUploadTokenRequiredAuthenticationCheck() + authentication = UploadTokenRequiredGetFromBodyAuthenticationCheck() res = authentication.authenticate(request) assert res is None @@ -692,192 +691,24 @@ def test_token_not_required_no_data(self): "/endpoint", format="json", ) - authentication = TestResultsUploadTokenRequiredAuthenticationCheck() + authentication = UploadTokenRequiredGetFromBodyAuthenticationCheck() res = authentication.authenticate(request) assert res is None - def test_token_not_required_unknown_repository(self, db): - an_owner = OwnerFactory(upload_token_required_for_public_repos=False) + def test_token_not_required_no_git_service(self, db): + owner = OwnerFactory(upload_token_required_for_public_repos=False) # their repo - RepositoryFactory(author=an_owner, private=False) - request_uri = f"/upload/github/{an_owner.username}::::bad/commits" + repo = RepositoryFactory(author=owner, private=False) + request_uri = f"/upload/github/{owner.username}::::{repo.name}/commits" request = APIRequestFactory().post( request_uri, data={ - "slug": f"{an_owner.username}::::bad", - "commit": "this is a commitid", - }, - format="json", - ) - authentication = TestResultsUploadTokenRequiredAuthenticationCheck() - res = authentication.authenticate(request) - assert res is None - - def test_token_not_required_unknown_owner(self, db): - repo = RepositoryFactory( - private=False, author__upload_token_required_for_public_repos=False - ) - request_uri = f"/upload/github/bad::::{repo.name}/commits" - request = APIRequestFactory().post( - request_uri, - data={"slug": f"bad::::{repo.name}", "commit": "this is a commitid"}, - format="json", - ) - authentication = TestResultsUploadTokenRequiredAuthenticationCheck() - res = authentication.authenticate(request) - assert res is None - - @pytest.mark.parametrize("request_uri,repo_slug,commitid", valid_params_to_test) - @pytest.mark.parametrize("private", [False, True]) - @pytest.mark.parametrize("token_required", [False, True]) - def test_get_repository_and_owner( - self, request_uri, repo_slug, commitid, private, token_required, db - ): - author_name, repo_name = repo_slug.split("/") - repo = RepositoryFactory( - name=repo_name, - author__username=author_name, - private=private, - author__upload_token_required_for_public_repos=token_required, - ) - assert repo.service == "github" - request = APIRequestFactory().post( - request_uri, - data={"slug": f"{author_name}::::{repo_name}", "commit": commitid}, - format="json", - ) - authentication = TestResultsUploadTokenRequiredAuthenticationCheck() - - if commitid is None: - # fails serializer validation for this endpoint - assert authentication.get_repository_and_owner(request) == (None, None) - else: - assert authentication.get_repository_and_owner(request) == ( - repo, - repo.author, - ) - - def test_get_repository_and_owner_by_commitid(self, db): - authentication = TestResultsUploadTokenRequiredAuthenticationCheck() - repo_name = "the-repo" - owner_username = "the-author" - for name, _ in Service.choices: - owner = OwnerFactory(service=name, username=owner_username) - RepositoryFactory(name=repo_name, author=owner) - - request = APIRequestFactory().post( - "endpoint/", - data={"slug": f"{owner_username}::::{repo_name}", "commit": "new commit"}, - format="json", - ) - assert authentication.get_repository_and_owner(request) == (None, None) - - commit_on_random_repo = CommitFactory() - request = APIRequestFactory().post( - "endpoint/", - data={ - "slug": f"{owner_username}::::{repo_name}", - "commit": commit_on_random_repo.commitid, + "slug": f"{owner.username}::::{repo.name}", }, format="json", ) + authentication = UploadTokenRequiredGetFromBodyAuthenticationCheck() assert authentication.get_repository_and_owner(request) == (None, None) - - matching_repo = Repository.objects.filter( - name=repo_name, author__username=owner_username - ).first() - commit_on_matching_repo = CommitFactory(repository=matching_repo) - request = APIRequestFactory().post( - "endpoint/", - data={ - "slug": f"{owner_username}::::{repo_name}", - "commit": commit_on_matching_repo.commitid, - }, - format="json", - ) - assert authentication.get_repository_and_owner(request) == ( - matching_repo, - matching_repo.author, - ) - - @pytest.mark.parametrize("private", [False, True]) - @pytest.mark.parametrize("branch", ["branch", "fork:branch"]) - @pytest.mark.parametrize( - "existing_commit,commit_branch", - [(False, None), (True, "branch"), (True, "fork:branch")], - ) - @pytest.mark.parametrize("token_required", [False, True]) - def test_token_not_required_fork_branch_public_private( - self, - db, - mocker, - private, - branch, - existing_commit, - commit_branch, - token_required, - ): - repo = RepositoryFactory( - private=private, - author__upload_token_required_for_public_repos=token_required, - ) - - if existing_commit: - commit = CommitFactory() - commit.branch = commit_branch - commit.repository = repo - commit.save() - - request = APIRequestFactory().post( - f"/upload/github/{repo.author.username}::::{repo.name}/commits/{commit.commitid}/reports/report_code/uploads", - data={ - "slug": f"{repo.author.username}::::{repo.name}", - "commit": commit.commitid, - }, - format="json", - ) - - else: - request = APIRequestFactory().post( - f"/upload/github/{repo.author.username}::::{repo.name}/commits", - data={ - "slug": f"{repo.author.username}::::{repo.name}", - "commit": "this is a commitid", - }, - format="json", - ) - - authentication = TestResultsUploadTokenRequiredAuthenticationCheck() - - if not private and not token_required: - res = authentication.authenticate(request) - assert res is not None - repo_as_user, auth_class = res - - assert repo_as_user.is_authenticated() is True - assert isinstance(auth_class, TokenlessAuth) - else: - res = authentication.authenticate(request) - assert res is None - - -class TestBundleAnalysisUploadTokenRequiredAuthenticationCheck(object): - def test_token_not_required_invalid_data(self): - request = APIRequestFactory().post( - "/endpoint", - data={"slug": 123, "commit": 456}, - format="json", - ) - authentication = BundleAnalysisUploadTokenRequiredAuthenticationCheck() - res = authentication.authenticate(request) - assert res is None - - def test_token_not_required_no_data(self): - request = APIRequestFactory().post( - "/endpoint", - format="json", - ) - authentication = BundleAnalysisUploadTokenRequiredAuthenticationCheck() res = authentication.authenticate(request) assert res is None @@ -890,12 +721,11 @@ def test_token_not_required_unknown_repository(self, db): request_uri, data={ "slug": f"{an_owner.username}::::bad", - "commit": "this is a commitid", "service": an_owner.service, }, format="json", ) - authentication = BundleAnalysisUploadTokenRequiredAuthenticationCheck() + authentication = UploadTokenRequiredGetFromBodyAuthenticationCheck() assert authentication.get_repository_and_owner(request) == (None, None) res = authentication.authenticate(request) assert res is None @@ -907,10 +737,13 @@ def test_token_not_required_unknown_owner(self, db): request_uri = f"/upload/github/bad::::{repo.name}/commits" request = APIRequestFactory().post( request_uri, - data={"slug": f"bad::::{repo.name}", "commit": "this is a commitid"}, + data={ + "slug": f"bad::::{repo.name}", + "service": "github", + }, format="json", ) - authentication = BundleAnalysisUploadTokenRequiredAuthenticationCheck() + authentication = UploadTokenRequiredGetFromBodyAuthenticationCheck() res = authentication.authenticate(request) assert res is None @@ -930,65 +763,18 @@ def test_get_repository_and_owner( assert repo.service == "github" request = APIRequestFactory().post( request_uri, - data={"slug": f"{author_name}::::{repo_name}", "commit": commitid}, + data={"slug": f"{author_name}::::{repo_name}", "service": "github"}, format="json", ) - authentication = BundleAnalysisUploadTokenRequiredAuthenticationCheck() + authentication = UploadTokenRequiredGetFromBodyAuthenticationCheck() - if commitid is None: - # fails serializer validation for this endpoint - assert authentication.get_repository_and_owner(request) == (None, None) - else: - assert authentication.get_repository_and_owner(request) == ( - repo, - repo.author, - ) - - def test_get_repository_and_owner_by_commitid(self, db): - authentication = BundleAnalysisUploadTokenRequiredAuthenticationCheck() - repo_name = "the-repo" - owner_username = "the-author" - for name, _ in Service.choices: - owner = OwnerFactory(service=name, username=owner_username) - RepositoryFactory(name=repo_name, author=owner) - - request = APIRequestFactory().post( - "endpoint/", - data={"slug": f"{owner_username}::::{repo_name}", "commit": "new commit"}, - format="json", - ) - assert authentication.get_repository_and_owner(request) == (None, None) - - commit_on_random_repo = CommitFactory() - request = APIRequestFactory().post( - "endpoint/", - data={ - "slug": f"{owner_username}::::{repo_name}", - "commit": commit_on_random_repo.commitid, - }, - format="json", - ) - assert authentication.get_repository_and_owner(request) == (None, None) - - matching_repo = Repository.objects.filter( - name=repo_name, author__username=owner_username - ).first() - commit_on_matching_repo = CommitFactory(repository=matching_repo) - request = APIRequestFactory().post( - "endpoint/", - data={ - "slug": f"{owner_username}::::{repo_name}", - "commit": commit_on_matching_repo.commitid, - }, - format="json", - ) assert authentication.get_repository_and_owner(request) == ( - matching_repo, - matching_repo.author, + repo, + repo.author, ) def test_get_repository_and_owner_with_service(self, db): - authentication = BundleAnalysisUploadTokenRequiredAuthenticationCheck() + authentication = UploadTokenRequiredGetFromBodyAuthenticationCheck() repo_name = "the-repo" owner_username = "the-author" for name, _ in Service.choices: @@ -999,7 +785,6 @@ def test_get_repository_and_owner_with_service(self, db): "endpoint/", data={ "slug": f"{owner_username}::::{repo_name}", - "commit": "new commit", "git_service": Service.BITBUCKET.value, }, format="json", @@ -1015,12 +800,10 @@ def test_get_repository_and_owner_with_service(self, db): matching_owner, ) - commit_on_random_repo = CommitFactory() request = APIRequestFactory().post( "endpoint/", data={ "slug": f"{owner_username}::::{repo_name}", - "commit": commit_on_random_repo.commitid, "git_service": Service.GITLAB.value, }, format="json", @@ -1036,15 +819,10 @@ def test_get_repository_and_owner_with_service(self, db): matching_owner, ) - matching_repo = Repository.objects.filter( - name=repo_name, author__username=owner_username - ).first() - commit_on_matching_repo = CommitFactory(repository=matching_repo) request = APIRequestFactory().post( "endpoint/", data={ "slug": f"{owner_username}::::{repo_name}", - "commit": commit_on_matching_repo.commitid, "git_service": Service.GITHUB.value, }, format="json", @@ -1092,7 +870,7 @@ def test_token_not_required_fork_branch_public_private( f"/upload/github/{repo.author.username}::::{repo.name}/commits/{commit.commitid}/reports/report_code/uploads", data={ "slug": f"{repo.author.username}::::{repo.name}", - "commit": commit.commitid, + "git_service": repo.author.service, }, format="json", ) @@ -1102,12 +880,12 @@ def test_token_not_required_fork_branch_public_private( f"/upload/github/{repo.author.username}::::{repo.name}/commits", data={ "slug": f"{repo.author.username}::::{repo.name}", - "commit": "this is a commitid", + "git_service": repo.author.service, }, format="json", ) - authentication = BundleAnalysisUploadTokenRequiredAuthenticationCheck() + authentication = UploadTokenRequiredGetFromBodyAuthenticationCheck() if not private and not token_required: res = authentication.authenticate(request) diff --git a/upload/tests/views/test_helpers.py b/upload/tests/views/test_helpers.py index afff7b7c83..439f65c9fa 100644 --- a/upload/tests/views/test_helpers.py +++ b/upload/tests/views/test_helpers.py @@ -1,11 +1,8 @@ from django.test import TestCase -from shared.django_apps.codecov_auth.models import Owner -from shared.django_apps.core.models import Repository from codecov_auth.models import Service -from core.tests.factories import CommitFactory, OwnerFactory, RepositoryFactory +from core.tests.factories import OwnerFactory, RepositoryFactory from upload.views.helpers import ( - get_repository_and_owner_from_slug_and_commit, get_repository_and_owner_from_string, get_repository_from_string, ) @@ -24,7 +21,6 @@ def setUpClass(cls): cls.group = "somegroup" cls.subgroup = "somesubgroup" cls.sub_subgroup = "subsub" - cls.commitid = CommitFactory().commitid owners_to_repos_mapping = { cls.gh: {cls.uname: [cls.repo_1_name, cls.repo_2_name]}, cls.bb: {cls.uname: [cls.repo_1_name, cls.repo_2_name]}, @@ -40,10 +36,10 @@ def setUpClass(cls): } for service, owner_data in owners_to_repos_mapping.items(): for username, repo_names in owner_data.items(): + RepositoryFactory() o = OwnerFactory(service=service, username=username) for r_name in repo_names: - r = RepositoryFactory(author=o, name=r_name) - CommitFactory(repository=r, commitid=cls.commitid) + RepositoryFactory(author=o, name=r_name) def test_get_repository_from_string(self): first_result = get_repository_from_string( @@ -153,89 +149,3 @@ def test_get_repository_and_owner_from_string(self): assert get_repository_and_owner_from_string( self.gl, f"{self.group}:::somebadsubgroup::::{self.repo_1_name}" ) == (None, None) - - def test_get_repository_and_owner_from_slug_and_commit(self): - # the test cases from setUpClass won't work on this method, they all have - # same username, repo name, and commitid - assert get_repository_and_owner_from_slug_and_commit( - slug=f"{self.uname}::::{self.repo_1_name}", commitid=self.commitid - ) == (None, None) - - # can identify by unique commitid - gh_owner = Owner.objects.get(service=self.gh, username=self.uname) - gh_repo = Repository.objects.get(author=gh_owner, name=self.repo_1_name) - unique_commit = CommitFactory(repository=gh_repo) - assert get_repository_and_owner_from_slug_and_commit( - slug=f"{self.uname}::::{self.repo_1_name}", commitid=unique_commit.commitid - ) == (gh_repo, gh_owner) - - # can identify by unique username - gl_owner = Owner.objects.get(service=self.gl, username=self.uname) - gl_repo = Repository.objects.get(author=gl_owner, name=self.repo_1_name) - gl_owner.username = "other" - gl_owner.save() - assert get_repository_and_owner_from_slug_and_commit( - slug=f"other::::{self.repo_1_name}", commitid=self.commitid - ) == (gl_repo, gl_owner) - - # can identify by unique repo name - another_gh_repo = RepositoryFactory(author=gh_owner, name="another") - CommitFactory(repository=another_gh_repo, commitid=self.commitid) - assert get_repository_and_owner_from_slug_and_commit( - slug=f"{self.uname}::::another", commitid=self.commitid - ) == (another_gh_repo, gh_owner) - - assert get_repository_and_owner_from_slug_and_commit( - slug=f"somerandomlalala::::{self.repo_1_name}", commitid=self.commitid - ) == (None, None) - assert get_repository_and_owner_from_slug_and_commit( - slug=f"{self.uname}::::{self.repo_1_name}wrongname", commitid=self.commitid - ) == (None, None) - assert get_repository_and_owner_from_slug_and_commit( - slug=f"{self.uname}::::{self.repo_1_name}", commitid="nonsense" - ) == (None, None) - assert get_repository_and_owner_from_slug_and_commit( - slug=f"{self.group}:::somebadsubgroup::::{self.repo_1_name}", - commitid=self.commitid, - ) == (None, None) - - # gitlab group - gitlab_group_repository, gitlab_group = ( - get_repository_and_owner_from_slug_and_commit( - slug=f"{self.group}::::{self.repo_1_name}", commitid=self.commitid - ) - ) - assert gitlab_group_repository.name == self.repo_1_name - assert gitlab_group_repository.author.username == self.group - assert gitlab_group_repository.author.service == self.gl - assert gitlab_group_repository.author == gitlab_group - - # gitlab subgroup - gitlab_subgroup_repository, gitlab_subgroup = ( - get_repository_and_owner_from_slug_and_commit( - slug=f"{self.group}:::{self.subgroup}::::{self.repo_1_name}", - commitid=self.commitid, - ) - ) - assert gitlab_subgroup_repository.name == self.repo_1_name - assert ( - gitlab_subgroup_repository.author.username - == f"{self.group}:{self.subgroup}" - ) - assert gitlab_subgroup_repository.author.service == self.gl - assert gitlab_subgroup_repository.author == gitlab_subgroup - - # gitlab subgroup of a subgroup - gitlab_sub_subgroup_repository, gitlab_sub_subgroup = ( - get_repository_and_owner_from_slug_and_commit( - slug=f"{self.group}:::{self.subgroup}:::{self.sub_subgroup}::::{self.repo_1_name}", - commitid=self.commitid, - ) - ) - assert gitlab_sub_subgroup_repository.name == self.repo_1_name - assert ( - gitlab_sub_subgroup_repository.author.username - == f"{self.group}:{self.subgroup}:{self.sub_subgroup}" - ) - assert gitlab_sub_subgroup_repository.author.service == self.gl - assert gitlab_sub_subgroup_repository.author == gitlab_sub_subgroup diff --git a/upload/tests/views/test_test_results.py b/upload/tests/views/test_test_results.py index ee0b5b2421..7276f9fb51 100644 --- a/upload/tests/views/test_test_results.py +++ b/upload/tests/views/test_test_results.py @@ -35,8 +35,7 @@ def test_upload_test_results(db, client, mocker, mock_redis): "build": "test-build", "buildURL": "test-build-url", "job": "test-job", - "service": "test-service", - "ci_service": "github-actions", + "service": "github-actions", "branch": "aaaaaa", }, format="json", @@ -191,6 +190,7 @@ def test_test_results_upload_token_not_required(db, client, mocker, mock_redis): "commit": "6fd5b89357fc8cdf34d6197549ac7c6d7e5977ef", "slug": f"{repository.author.username}::::{repository.name}", "branch": "aaaaaa", + "service": owner.service, }, format="json", ) diff --git a/upload/views/bundle_analysis.py b/upload/views/bundle_analysis.py index ad9ebb29c8..7713b38e49 100644 --- a/upload/views/bundle_analysis.py +++ b/upload/views/bundle_analysis.py @@ -14,10 +14,10 @@ from codecov_auth.authentication.repo_auth import ( BundleAnalysisTokenlessAuthentication, - BundleAnalysisUploadTokenRequiredAuthenticationCheck, GitHubOIDCTokenAuthentication, OrgLevelTokenAuthentication, RepositoryLegacyTokenAuthentication, + UploadTokenRequiredGetFromBodyAuthenticationCheck, repo_auth_custom_exception_handler, ) from codecov_auth.authentication.types import RepositoryAsUser @@ -55,7 +55,7 @@ class UploadSerializer(serializers.Serializer): class BundleAnalysisView(APIView, ShelterMixin): permission_classes = [UploadBundleAnalysisPermission] authentication_classes = [ - BundleAnalysisUploadTokenRequiredAuthenticationCheck, + UploadTokenRequiredGetFromBodyAuthenticationCheck, OrgLevelTokenAuthentication, GitHubOIDCTokenAuthentication, RepositoryLegacyTokenAuthentication, diff --git a/upload/views/test_results.py b/upload/views/test_results.py index 865ff665aa..2ee6d2af50 100644 --- a/upload/views/test_results.py +++ b/upload/views/test_results.py @@ -13,8 +13,8 @@ GitHubOIDCTokenAuthentication, OrgLevelTokenAuthentication, RepositoryLegacyTokenAuthentication, - TestResultsUploadTokenRequiredAuthenticationCheck, TokenlessAuthentication, + UploadTokenRequiredGetFromBodyAuthenticationCheck, repo_auth_custom_exception_handler, ) from codecov_auth.authentication.types import RepositoryAsUser @@ -39,13 +39,13 @@ def has_permission(self, request, view): class UploadSerializer(serializers.Serializer): commit = serializers.CharField(required=True) slug = serializers.CharField(required=True) + service = serializers.CharField(required=False) # git_service build = serializers.CharField(required=False) buildURL = serializers.CharField(required=False) job = serializers.CharField(required=False) flags = FlagListField(required=False) pr = serializers.CharField(required=False) branch = serializers.CharField(required=False, allow_null=True) - ci_service = serializers.CharField(required=False) storage_path = serializers.CharField(required=False) @@ -55,7 +55,7 @@ class TestResultsView( ): permission_classes = [UploadTestResultsPermission] authentication_classes = [ - TestResultsUploadTokenRequiredAuthenticationCheck, + UploadTokenRequiredGetFromBodyAuthenticationCheck, OrgLevelTokenAuthentication, GitHubOIDCTokenAuthentication, RepositoryLegacyTokenAuthentication, @@ -154,7 +154,7 @@ def post(self, request): "build_url": data.get("buildURL"), # build_url "job": data.get("job"), # job_code "flags": data.get("flags"), - "service": data.get("ci_service"), # provider + "service": data.get("service"), # git provider "url": storage_path, # storage_path # these are used for dispatching the task below "commit": commit.commitid, From c91f69ef7bd631ff7d34ed3e02fa8f3a111ecfb6 Mon Sep 17 00:00:00 2001 From: Nora Shapiro Date: Tue, 15 Oct 2024 13:16:08 -0700 Subject: [PATCH 6/7] remove this chunk --- upload/views/helpers.py | 47 ----------------------------------------- 1 file changed, 47 deletions(-) diff --git a/upload/views/helpers.py b/upload/views/helpers.py index d6b52ab146..cd61677680 100644 --- a/upload/views/helpers.py +++ b/upload/views/helpers.py @@ -1,13 +1,8 @@ -import logging import typing -from shared.django_apps.core.models import Commit - from codecov_auth.models import Owner, Service from core.models import Repository -log = logging.getLogger(__name__) - def get_repository_and_owner_from_string( service: Service, repo_identifier: str @@ -50,45 +45,3 @@ def get_repository_from_string( service=service, repo_identifier=repo_identifier ) return repository - - -def get_repository_and_owner_from_slug_and_commit( - slug: str, commitid: str -) -> tuple[Repository | None, Owner | None]: - if "::::" not in slug: - return None, None - - owner_username, repository_name = slug.rsplit("::::", 1) - - # formatting for GL subgroups - if ":::" in owner_username: - owner_username = owner_username.replace(":::", ":") - - matching_repositories = Repository.objects.filter( - author__username=owner_username, name=repository_name - ).select_related("author") - if matching_repositories.count() == 1: - log.info( - "get_repository_and_owner_from_slug_and_commit success", - extra=dict(slug=slug), - ) - repository = matching_repositories.first() - return repository, repository.author - else: - # commit might not exist yet (if this is first upload for it) - try: - # Commit has UniqueConstraint on commitid + repository - commit = Commit.objects.select_related( - "repository", "repository__author" - ).get(commitid=commitid, repository__in=matching_repositories) - log.info( - "get_repository_and_owner_from_slug_and_commit multiple matches success", - extra=dict(slug=slug), - ) - return commit.repository, commit.repository.author - except (Commit.DoesNotExist, Commit.MultipleObjectsReturned): - log.info( - "get_repository_and_owner_from_slug_and_commit fail", - extra=dict(slug=slug), - ) - return None, None From 56f613bf9bf66aca4ac6eab4a19449cdf73cbfb9 Mon Sep 17 00:00:00 2001 From: Nora Shapiro Date: Fri, 8 Nov 2024 16:27:45 -0800 Subject: [PATCH 7/7] get service the same way as shelter --- codecov_auth/authentication/repo_auth.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/codecov_auth/authentication/repo_auth.py b/codecov_auth/authentication/repo_auth.py index c036b2d89b..3258fd33ec 100644 --- a/codecov_auth/authentication/repo_auth.py +++ b/codecov_auth/authentication/repo_auth.py @@ -424,9 +424,7 @@ def _get_git(self, validated_data): BA sends this in as git_service, TA sends this in as service. Use this function so this Check class can be used by both views. """ - git_service = validated_data.get("git_service") - if not git_service: - git_service = validated_data.get("service") + git_service = validated_data.get("git_service") or validated_data.get("service") return git_service def _get_info_from_request_body(