From b37bd9db7364e32dbd046c5dbcfe15dbe6cf378c Mon Sep 17 00:00:00 2001 From: Trent Schmidt Date: Wed, 15 May 2024 11:09:23 -0400 Subject: [PATCH 01/11] Improving upload metrics (#562) --- upload/helpers.py | 3 ++- upload/tests/views/test_bundle_analysis.py | 15 +++++++++++++++ upload/tests/views/test_commits.py | 14 +++++++++++++- upload/tests/views/test_empty_upload.py | 16 +++++++++++++--- upload/tests/views/test_reports.py | 16 +++++++++++++++- upload/tests/views/test_test_results.py | 13 +++++++++++++ upload/tests/views/test_upload_completion.py | 19 +++++++++++++++---- upload/tests/views/test_uploads.py | 2 ++ upload/views/bundle_analysis.py | 18 ++++++++++++++---- upload/views/commits.py | 13 ++++++++++++- upload/views/empty_upload.py | 19 ++++++++++++++++--- upload/views/legacy.py | 1 + upload/views/reports.py | 13 ++++++++++++- upload/views/test_results.py | 1 + upload/views/upload_completion.py | 13 ++++++++++++- upload/views/uploads.py | 1 + 16 files changed, 157 insertions(+), 20 deletions(-) diff --git a/upload/helpers.py b/upload/helpers.py index 88142b5ba8..83ce35b28b 100644 --- a/upload/helpers.py +++ b/upload/helpers.py @@ -780,12 +780,13 @@ def get_version_from_headers(headers): def generate_upload_sentry_metrics_tags( - action, request, repository, is_shelter_request + action, request, repository, is_shelter_request, endpoint: Optional[str] = None ): return dict( agent=get_agent_from_headers(request.headers), version=get_version_from_headers(request.headers), action=action, + endpoint=endpoint, repo_visibility="private" if repository.private is True else "public", is_using_shelter="yes" if is_shelter_request else "no", ) diff --git a/upload/tests/views/test_bundle_analysis.py b/upload/tests/views/test_bundle_analysis.py index 82679bfc1f..90de3c1147 100644 --- a/upload/tests/views/test_bundle_analysis.py +++ b/upload/tests/views/test_bundle_analysis.py @@ -14,6 +14,9 @@ def test_upload_bundle_analysis(db, client, mocker, mock_redis): upload = mocker.patch.object(TaskService, "upload") + mock_sentry_metrics = mocker.patch( + "upload.views.bundle_analysis.sentry_metrics.incr" + ) create_presigned_put = mocker.patch( "services.archive.StorageService.create_presigned_put", return_value="test-presigned-put", @@ -36,6 +39,7 @@ def test_upload_bundle_analysis(db, client, mocker, mock_redis): "service": "test-service", }, format="json", + headers={"User-Agent": "codecov-cli/0.4.7"}, ) assert res.status_code == 201 @@ -79,6 +83,17 @@ def test_upload_bundle_analysis(db, client, mocker, mock_redis): report_code=None, report_type="bundle_analysis", ) + mock_sentry_metrics.assert_called_with( + "upload", + tags={ + "agent": "cli", + "version": "0.4.7", + "action": "bundle_analysis", + "endpoint": "bundle_analysis", + "repo_visibility": "private", + "is_using_shelter": "no", + }, + ) def test_upload_bundle_analysis_org_token(db, client, mocker, mock_redis): diff --git a/upload/tests/views/test_commits.py b/upload/tests/views/test_commits.py index 3992d504e3..c574f8ed9b 100644 --- a/upload/tests/views/test_commits.py +++ b/upload/tests/views/test_commits.py @@ -295,6 +295,7 @@ def test_commit_github_oidc_auth(mock_jwks_client, mock_jwt_decode, db, mocker): private=False, author__username="codecov", name="the_repo" ) mocked_call = mocker.patch.object(TaskService, "update_commit") + mock_sentry_metrics = mocker.patch("upload.views.commits.sentry_metrics.incr") mock_jwt_decode.return_value = { "repository": f"url/{repository.name}", "repository_owner": repository.author.username, @@ -315,7 +316,7 @@ def test_commit_github_oidc_auth(mock_jwks_client, mock_jwt_decode, db, mocker): "pullid": "4", }, format="json", - headers={"Authorization": f"token {token}"}, + headers={"Authorization": f"token {token}", "User-Agent": "codecov-cli/0.4.7"}, ) assert response.status_code == 201 response_json = response.json() @@ -340,3 +341,14 @@ def test_commit_github_oidc_auth(mock_jwks_client, mock_jwt_decode, db, mocker): } assert expected_response == response_json mocked_call.assert_called_with(commitid="commit_sha", repoid=repository.repoid) + mock_sentry_metrics.assert_called_with( + "upload", + tags={ + "agent": "cli", + "version": "0.4.7", + "action": "coverage", + "endpoint": "create_commit", + "repo_visibility": "public", + "is_using_shelter": "no", + }, + ) diff --git a/upload/tests/views/test_empty_upload.py b/upload/tests/views/test_empty_upload.py index b0b6581a3c..f7bfadcc44 100644 --- a/upload/tests/views/test_empty_upload.py +++ b/upload/tests/views/test_empty_upload.py @@ -48,6 +48,7 @@ def test_empty_upload_with_yaml_ignored_files( mocker.patch.object( CanDoCoverageUploadsPermission, "has_permission", return_value=True ) + mock_sentry_metrics = mocker.patch("upload.views.empty_upload.sentry_metrics.incr") mock_final_yaml.return_value = UserYaml( { "ignore": [ @@ -79,9 +80,7 @@ def test_empty_upload_with_yaml_ignored_files( commit.commitid, ], ) - response = client.post( - url, - ) + response = client.post(url, headers={"User-Agent": "codecov-cli/0.4.7"}) response_json = response.json() assert response.status_code == 200 assert ( @@ -91,6 +90,17 @@ def test_empty_upload_with_yaml_ignored_files( notify_mock.assert_called_once_with( repoid=repository.repoid, commitid=commit.commitid, empty_upload="pass" ) + mock_sentry_metrics.assert_called_with( + "upload", + tags={ + "agent": "cli", + "version": "0.4.7", + "action": "coverage", + "endpoint": "empty_upload", + "repo_visibility": "private", + "is_using_shelter": "no", + }, + ) @patch("services.task.TaskService.notify") diff --git a/upload/tests/views/test_reports.py b/upload/tests/views/test_reports.py index 91a47be1e9..6c648fa546 100644 --- a/upload/tests/views/test_reports.py +++ b/upload/tests/views/test_reports.py @@ -24,6 +24,7 @@ def test_reports_get_not_allowed(client, mocker): def test_reports_post(client, db, mocker): mocked_call = mocker.patch.object(TaskService, "preprocess_upload") + mock_sentry_metrics = mocker.patch("upload.views.reports.sentry_metrics.incr") repository = RepositoryFactory( name="the_repo", author__username="codecov", author__service="github" ) @@ -35,7 +36,9 @@ def test_reports_post(client, db, mocker): "new_upload.reports", args=["github", "codecov::::the_repo", commit.commitid], ) - response = client.post(url, data={"code": "code1"}) + response = client.post( + url, data={"code": "code1"}, headers={"User-Agent": "codecov-cli/0.4.7"} + ) assert ( url == f"/upload/github/codecov::::the_repo/commits/{commit.commitid}/reports" @@ -45,6 +48,17 @@ def test_reports_post(client, db, mocker): commit_id=commit.id, code="code1", report_type=CommitReport.ReportType.COVERAGE ).exists() mocked_call.assert_called_with(repository.repoid, commit.commitid, "code1") + mock_sentry_metrics.assert_called_with( + "upload", + tags={ + "agent": "cli", + "version": "0.4.7", + "action": "coverage", + "endpoint": "create_report", + "repo_visibility": "private", + "is_using_shelter": "no", + }, + ) @patch("upload.helpers.jwt.decode") diff --git a/upload/tests/views/test_test_results.py b/upload/tests/views/test_test_results.py index bbac051a7b..5c0e76b384 100644 --- a/upload/tests/views/test_test_results.py +++ b/upload/tests/views/test_test_results.py @@ -14,6 +14,7 @@ def test_upload_test_results(db, client, mocker, mock_redis): upload = mocker.patch.object(TaskService, "upload") + mock_sentry_metrics = mocker.patch("upload.views.test_results.metrics.incr") create_presigned_put = mocker.patch( "services.archive.StorageService.create_presigned_put", return_value="test-presigned-put", @@ -37,6 +38,7 @@ def test_upload_test_results(db, client, mocker, mock_redis): "service": "test-service", }, format="json", + headers={"User-Agent": "codecov-cli/0.4.7"}, ) assert res.status_code == 201 @@ -89,6 +91,17 @@ def test_upload_test_results(db, client, mocker, mock_redis): report_code=None, report_type="test_results", ) + mock_sentry_metrics.assert_called_with( + "upload", + tags={ + "agent": "cli", + "version": "0.4.7", + "action": "test_results", + "endpoint": "test_results", + "repo_visibility": "private", + "is_using_shelter": "no", + }, + ) def test_test_results_org_token(db, client, mocker, mock_redis): diff --git a/upload/tests/views/test_upload_completion.py b/upload/tests/views/test_upload_completion.py index 0df6bea9fc..59ba798d49 100644 --- a/upload/tests/views/test_upload_completion.py +++ b/upload/tests/views/test_upload_completion.py @@ -49,7 +49,9 @@ def test_upload_completion_view_processed_uploads(mocked_manual_trigger, db, moc mocker.patch.object( CanDoCoverageUploadsPermission, "has_permission", return_value=True ) - + mock_sentry_metrics = mocker.patch( + "upload.views.upload_completion.sentry_metrics.incr" + ) repository = RepositoryFactory( name="the_repo", author__username="codecov", author__service="github" ) @@ -74,9 +76,7 @@ def test_upload_completion_view_processed_uploads(mocked_manual_trigger, db, moc commit.commitid, ], ) - response = client.post( - url, - ) + response = client.post(url, headers={"User-Agent": "codecov-cli/0.4.7"}) response_json = response.json() assert response.status_code == 200 assert response_json == { @@ -86,6 +86,17 @@ def test_upload_completion_view_processed_uploads(mocked_manual_trigger, db, moc "uploads_error": 0, } mocked_manual_trigger.assert_called_once_with(repository.repoid, commit.commitid) + mock_sentry_metrics.assert_called_with( + "upload", + tags={ + "agent": "cli", + "version": "0.4.7", + "action": "coverage", + "endpoint": "upload_complete", + "repo_visibility": "private", + "is_using_shelter": "no", + }, + ) @patch("services.task.TaskService.manual_upload_completion_trigger") diff --git a/upload/tests/views/test_uploads.py b/upload/tests/views/test_uploads.py index 41b999878d..4a69112008 100644 --- a/upload/tests/views/test_uploads.py +++ b/upload/tests/views/test_uploads.py @@ -655,6 +655,7 @@ def test_uploads_post_shelter(db, mocker, mock_redis): "agent": "cli", "version": "0.4.7", "action": "coverage", + "endpoint": "create_upload", "repo_visibility": "private", "is_using_shelter": "yes", }, @@ -667,6 +668,7 @@ def test_uploads_post_shelter(db, mocker, mock_redis): "agent": "cli", "version": "0.4.7", "action": "coverage", + "endpoint": "create_upload", "repo_visibility": "private", "is_using_shelter": "yes", }, diff --git a/upload/views/bundle_analysis.py b/upload/views/bundle_analysis.py index 4a2742a117..41e995c9e3 100644 --- a/upload/views/bundle_analysis.py +++ b/upload/views/bundle_analysis.py @@ -7,7 +7,7 @@ from rest_framework.response import Response from rest_framework.views import APIView from shared.bundle_analysis.storage import StoragePaths, get_bucket_name - +from sentry_sdk import metrics as sentry_metrics from codecov_auth.authentication.repo_auth import ( GitHubOIDCTokenAuthentication, OrgLevelTokenAuthentication, @@ -20,7 +20,8 @@ from reports.models import CommitReport from services.archive import ArchiveService from services.redis_configuration import get_redis_connection -from upload.helpers import dispatch_upload_task +from upload.helpers import dispatch_upload_task, generate_upload_sentry_metrics_tags +from upload.views.base import ShelterMixin from upload.views.helpers import get_repository_from_string log = logging.getLogger(__name__) @@ -42,7 +43,7 @@ class UploadSerializer(serializers.Serializer): branch = serializers.CharField(required=False, allow_null=True) -class BundleAnalysisView(APIView): +class BundleAnalysisView(APIView, ShelterMixin): permission_classes = [UploadBundleAnalysisPermission] authentication_classes = [ OrgLevelTokenAuthentication, @@ -130,5 +131,14 @@ def post(self, request): get_redis_connection(), report_type=CommitReport.ReportType.BUNDLE_ANALYSIS, ) - + sentry_metrics.incr( + "upload", + tags=generate_upload_sentry_metrics_tags( + action="bundle_analysis", + endpoint="bundle_analysis", + request=self.request, + repository=repo, + is_shelter_request=self.is_shelter_request(), + ), + ) return Response({"url": url}, status=201) diff --git a/upload/views/commits.py b/upload/views/commits.py index b8f1330ef5..fe9eafdda5 100644 --- a/upload/views/commits.py +++ b/upload/views/commits.py @@ -2,7 +2,7 @@ from rest_framework.exceptions import ValidationError from rest_framework.generics import ListCreateAPIView - +from sentry_sdk import metrics as sentry_metrics from codecov_auth.authentication.repo_auth import ( GitHubOIDCTokenAuthentication, GlobalTokenAuthentication, @@ -14,6 +14,7 @@ ) from core.models import Commit from services.task import TaskService +from upload.helpers import generate_upload_sentry_metrics_tags from upload.serializers import CommitSerializer from upload.views.base import GetterMixin from upload.views.uploads import CanDoCoverageUploadsPermission @@ -77,4 +78,14 @@ def perform_create(self, serializer): TaskService().update_commit( commitid=commit.commitid, repoid=commit.repository.repoid ) + sentry_metrics.incr( + "upload", + tags=generate_upload_sentry_metrics_tags( + action="coverage", + endpoint="create_commit", + request=self.request, + repository=repository, + is_shelter_request=self.is_shelter_request(), + ), + ) return commit diff --git a/upload/views/empty_upload.py b/upload/views/empty_upload.py index 2cd6b78cd7..ef77308447 100644 --- a/upload/views/empty_upload.py +++ b/upload/views/empty_upload.py @@ -9,7 +9,7 @@ from rest_framework.generics import CreateAPIView from rest_framework.response import Response from shared.torngit.exceptions import TorngitClientError, TorngitClientGeneralError - +from sentry_sdk import metrics as sentry_metrics from codecov_auth.authentication.repo_auth import ( GitHubOIDCTokenAuthentication, GlobalTokenAuthentication, @@ -20,7 +20,10 @@ from services.repo_providers import RepoProviderService from services.task import TaskService from services.yaml import final_commit_yaml -from upload.helpers import try_to_get_best_possible_bot_token +from upload.helpers import ( + try_to_get_best_possible_bot_token, + generate_upload_sentry_metrics_tags, +) from upload.views.base import GetterMixin from upload.views.uploads import CanDoCoverageUploadsPermission @@ -129,7 +132,16 @@ def post(self, request, *args, **kwargs): ) ) ] - + sentry_metrics.incr( + "upload", + tags=generate_upload_sentry_metrics_tags( + action="coverage", + endpoint="empty_upload", + request=self.request, + repository=repo, + is_shelter_request=self.is_shelter_request(), + ), + ) if set(changed_files) == set(ignored_changed_files): TaskService().notify( repoid=repo.repoid, commitid=commit.commitid, empty_upload="pass" @@ -146,6 +158,7 @@ def post(self, request, *args, **kwargs): TaskService().notify( repoid=repo.repoid, commitid=commit.commitid, empty_upload="fail" ) + return Response( data={ "result": "Some files cannot be ignored. Triggering failing notifications.", diff --git a/upload/views/legacy.py b/upload/views/legacy.py index c895ee01c1..996f78abd0 100644 --- a/upload/views/legacy.py +++ b/upload/views/legacy.py @@ -159,6 +159,7 @@ def post(self, request, *args, **kwargs): sentry_tags = generate_upload_sentry_metrics_tags( action="coverage", + endpoint="legacy_upload", request=self.request, repository=repository, is_shelter_request=self.is_shelter_request(), diff --git a/upload/views/reports.py b/upload/views/reports.py index 65c7744813..d207ac49c5 100644 --- a/upload/views/reports.py +++ b/upload/views/reports.py @@ -3,7 +3,7 @@ from django.http import HttpRequest, HttpResponseNotAllowed from rest_framework.exceptions import ValidationError from rest_framework.generics import CreateAPIView, ListCreateAPIView, RetrieveAPIView - +from sentry_sdk import metrics as sentry_metrics from codecov_auth.authentication.repo_auth import ( GitHubOIDCTokenAuthentication, GlobalTokenAuthentication, @@ -14,6 +14,7 @@ ) from reports.models import CommitReport, ReportResults from services.task import TaskService +from upload.helpers import generate_upload_sentry_metrics_tags from upload.serializers import CommitReportSerializer, ReportResultsSerializer from upload.views.base import GetterMixin from upload.views.uploads import CanDoCoverageUploadsPermission @@ -52,6 +53,16 @@ def perform_create(self, serializer): TaskService().preprocess_upload( repository.repoid, commit.commitid, instance.code ) + sentry_metrics.incr( + "upload", + tags=generate_upload_sentry_metrics_tags( + action="coverage", + endpoint="create_report", + request=self.request, + repository=repository, + is_shelter_request=self.is_shelter_request(), + ), + ) return instance def list(self, request: HttpRequest, service: str, repo: str, commit_sha: str): diff --git a/upload/views/test_results.py b/upload/views/test_results.py index 5778fcc023..812bdae6f8 100644 --- a/upload/views/test_results.py +++ b/upload/views/test_results.py @@ -80,6 +80,7 @@ def post(self, request): "upload", tags=generate_upload_sentry_metrics_tags( action="test_results", + endpoint="test_results", request=request, repository=repo, is_shelter_request=self.is_shelter_request(), diff --git a/upload/views/upload_completion.py b/upload/views/upload_completion.py index e2d9a39aa9..7a6071a2b7 100644 --- a/upload/views/upload_completion.py +++ b/upload/views/upload_completion.py @@ -3,7 +3,7 @@ from rest_framework import status from rest_framework.generics import CreateAPIView from rest_framework.response import Response - +from sentry_sdk import metrics as sentry_metrics from codecov_auth.authentication.repo_auth import ( GitHubOIDCTokenAuthentication, GlobalTokenAuthentication, @@ -13,6 +13,7 @@ ) from reports.models import ReportSession from services.task import TaskService +from upload.helpers import generate_upload_sentry_metrics_tags from upload.views.base import GetterMixin from upload.views.uploads import CanDoCoverageUploadsPermission @@ -66,6 +67,16 @@ def post(self, request, *args, **kwargs): errored_uploads += 1 TaskService().manual_upload_completion_trigger(repo.repoid, commit.commitid) + sentry_metrics.incr( + "upload", + tags=generate_upload_sentry_metrics_tags( + action="coverage", + endpoint="upload_complete", + request=self.request, + repository=repo, + is_shelter_request=self.is_shelter_request(), + ), + ) return Response( data={ "uploads_total": uploads_count, diff --git a/upload/views/uploads.py b/upload/views/uploads.py index ece5523039..2bea0cdf12 100644 --- a/upload/views/uploads.py +++ b/upload/views/uploads.py @@ -73,6 +73,7 @@ def perform_create(self, serializer: UploadSerializer): sentry_tags = generate_upload_sentry_metrics_tags( action="coverage", + endpoint="create_upload", request=self.request, repository=repository, is_shelter_request=self.is_shelter_request(), From bd05e844211fbe953d1e4a85e0f12a57c2b643b0 Mon Sep 17 00:00:00 2001 From: JerrySentry <142266253+JerrySentry@users.noreply.github.com> Date: Wed, 15 May 2024 11:47:29 -0400 Subject: [PATCH 02/11] delete session security fix (#564) --- api/shared/permissions.py | 34 ++++++------ .../owner/interactors/delete_session.py | 22 +++++--- .../tests/mutation/test_delete_session.py | 54 ++++++++++++++++--- 3 files changed, 81 insertions(+), 29 deletions(-) diff --git a/api/shared/permissions.py b/api/shared/permissions.py index 9f101bb549..bcbd98104e 100644 --- a/api/shared/permissions.py +++ b/api/shared/permissions.py @@ -1,13 +1,15 @@ import logging +from typing import Any, Tuple from asgiref.sync import async_to_sync from django.conf import settings from django.http import Http404 -from rest_framework.permissions import ( - SAFE_METHODS, # ['GET', 'HEAD', 'OPTIONS'] - BasePermission, -) +from rest_framework.permissions import SAFE_METHODS # ['GET', 'HEAD', 'OPTIONS'] +from rest_framework.permissions import BasePermission +from django.http import HttpRequest +from codecov_auth.models import Owner +from core.models import Repository import services.self_hosted as self_hosted from api.shared.mixins import InternalPermissionsMixin, SuperPermissionsMixin from api.shared.repo.repository_accessors import RepoAccessors @@ -20,7 +22,9 @@ class RepositoryPermissionsService: @torngit_safe - def _fetch_provider_permissions(self, owner, repo): + def _fetch_provider_permissions( + self, owner: Owner, repo: Repository + ) -> Tuple[bool, bool]: can_view, can_edit = RepoAccessors().get_repo_permissions(owner, repo) if can_view: @@ -30,7 +34,7 @@ def _fetch_provider_permissions(self, owner, repo): return can_view, can_edit - def has_read_permissions(self, owner, repo): + def has_read_permissions(self, owner: Owner, repo: Repository) -> bool: return not repo.private or ( owner is not None and ( @@ -41,13 +45,13 @@ def has_read_permissions(self, owner, repo): ) ) - def has_write_permissions(self, user, repo): + def has_write_permissions(self, user: Owner, repo: Repository) -> bool: return user.is_authenticated and ( repo.author.ownerid == user.ownerid or self._fetch_provider_permissions(user, repo)[1] ) - def user_is_activated(self, current_owner, owner): + def user_is_activated(self, current_owner: Owner, owner: Owner) -> bool: if not current_owner or not owner: return False if current_owner.ownerid == owner.ownerid: @@ -82,7 +86,7 @@ class RepositoryArtifactPermissions(BasePermission): "trying to view a private repo but is not activated." ) - def has_permission(self, request, view): + def has_permission(self, request: HttpRequest, view: Any) -> bool: if view.repo.private: user_activated_permissions = ( request.user.is_authenticated @@ -107,19 +111,19 @@ def has_permission(self, request, view): class SuperTokenPermissions(BasePermission, SuperPermissionsMixin): - def has_permission(self, request, view): + def has_permission(self, request: HttpRequest, view: Any) -> bool: return self.has_super_token_permissions(request) class InternalTokenPermissions(BasePermission, InternalPermissionsMixin): - def has_permission(self, request, view): + def has_permission(self, request: HttpRequest, view: Any) -> bool: return self.has_internal_token_permissions(request) class ChartPermissions(BasePermission): permissions_service = RepositoryPermissionsService() - def has_permission(self, request, view): + def has_permission(self, request: HttpRequest, view: Any) -> bool: log.info( f"Coverage chart has repositories {view.repositories}", extra=dict(user=request.current_owner), @@ -142,7 +146,7 @@ class UserIsAdminPermissions(BasePermission): returns this owner. """ - def has_permission(self, request, view): + def has_permission(self, request: HttpRequest, view: Any) -> bool: if settings.IS_ENTERPRISE: return request.user.is_authenticated and self_hosted.is_admin_owner( request.current_owner @@ -158,7 +162,7 @@ def has_permission(self, request, view): ) @torngit_safe - def _is_admin_on_provider(self, user, owner): + def _is_admin_on_provider(self, user: Owner, owner: Owner) -> bool: torngit_provider_adapter = get_provider( owner.service, { @@ -183,7 +187,7 @@ class MemberOfOrgPermissions(BasePermission): Requires that the view has a '.owner' property that returns this owner. """ - def has_permission(self, request, view): + def has_permission(self, request: HttpRequest, view: Any) -> bool: if not request.user.is_authenticated: return False diff --git a/codecov_auth/commands/owner/interactors/delete_session.py b/codecov_auth/commands/owner/interactors/delete_session.py index 691e80c475..6e90239703 100644 --- a/codecov_auth/commands/owner/interactors/delete_session.py +++ b/codecov_auth/commands/owner/interactors/delete_session.py @@ -7,14 +7,24 @@ class DeleteSessionInteractor(BaseInteractor): - def validate(self): + def validate(self) -> None: if not self.current_user.is_authenticated: raise Unauthenticated() @sync_to_async - def execute(self, sessionid: int): + def execute(self, sessionid: int) -> None: self.validate() - session_to_delete = Session.objects.get(sessionid=sessionid) - DjangoSession.objects.filter( - session_key=session_to_delete.login_session_id - ).delete() + + try: + session_to_delete = Session.objects.get(sessionid=sessionid) + django_session_to_delete = DjangoSession.objects.get( + session_key=session_to_delete.login_session_id + ) + user_id_to_delete = int( + django_session_to_delete.get_decoded().get("_auth_user_id", "0") + ) + + if user_id_to_delete == self.current_user.id: + django_session_to_delete.delete() + except (Session.DoesNotExist, DjangoSession.DoesNotExist): + pass diff --git a/graphql_api/tests/mutation/test_delete_session.py b/graphql_api/tests/mutation/test_delete_session.py index bab5bec9c1..c00aa67da7 100644 --- a/graphql_api/tests/mutation/test_delete_session.py +++ b/graphql_api/tests/mutation/test_delete_session.py @@ -1,8 +1,9 @@ +from django.contrib import auth from django.test import TransactionTestCase from django.utils import timezone from codecov_auth.models import DjangoSession, Session -from codecov_auth.tests.factories import OwnerFactory +from codecov_auth.tests.factories import OwnerFactory, UserFactory from graphql_api.tests.helper import GraphQLTestHelper query = """ @@ -25,15 +26,26 @@ def test_when_unauthenticated(self): assert data["deleteSession"]["error"]["__typename"] == "UnauthenticatedError" def test_when_authenticated(self): - django_session = DjangoSession.objects.create( - expire_date=timezone.now(), - session_key="123abc", - ) + user = UserFactory() + self.owner.user = user + self.owner.save() + + login_query = "{ me { user { username }} }" + self.gql_request(login_query, owner=self.owner) + + user = auth.get_user(self.client) + assert user.is_authenticated + + django_session_id = DjangoSession.objects.all() + assert len(django_session_id) == 1 + + django_session_id = django_session_id[0] + sessionid = Session.objects.create( lastseen=timezone.now(), useragent="Firefox", ip="0.0.0.0", - login_session=django_session, + login_session=django_session_id, type=Session.SessionType.LOGIN, owner=self.owner, ).sessionid @@ -41,6 +53,32 @@ def test_when_authenticated(self): self.gql_request( query, owner=self.owner, variables={"input": {"sessionid": sessionid}} ) - - assert len(DjangoSession.objects.filter(session_key="123abc")) == 0 assert len(Session.objects.filter(sessionid=sessionid)) == 0 + + def test_when_authenticated_session_not_valid(self): + user = UserFactory() + self.owner.user = user + self.owner.save() + + login_query = "{ me { user { username }} }" + self.gql_request(login_query, owner=self.owner) + + user = auth.get_user(self.client) + assert user.is_authenticated + + django_session_id = DjangoSession.objects.all() + assert len(django_session_id) == 1 + + django_session_id = django_session_id[0] + + Session.objects.create( + lastseen=timezone.now(), + useragent="Firefox", + ip="0.0.0.0", + login_session=django_session_id, + type=Session.SessionType.LOGIN, + owner=self.owner, + ).sessionid + + self.gql_request(query, owner=self.owner, variables={"input": {"sessionid": 0}}) + assert len(Session.objects.all()) == 1 From 2ee4871017155c3bf215c41a3905ff0759dd7dfe Mon Sep 17 00:00:00 2001 From: Trent Schmidt Date: Wed, 15 May 2024 14:34:58 -0400 Subject: [PATCH 03/11] Adding metrics report_results (#566) --- upload/tests/views/test_reports.py | 19 ++++++++++++++++++- upload/views/reports.py | 10 ++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/upload/tests/views/test_reports.py b/upload/tests/views/test_reports.py index 6c648fa546..752f4764c5 100644 --- a/upload/tests/views/test_reports.py +++ b/upload/tests/views/test_reports.py @@ -300,6 +300,7 @@ def test_reports_results_post_successful_github_oidc_auth( mock_jwks_client, mock_jwt_decode, client, db, mocker ): mocked_task = mocker.patch("services.task.TaskService.create_report_results") + mock_sentry_metrics = mocker.patch("upload.views.reports.sentry_metrics.incr") mocker.patch.object( CanDoCoverageUploadsPermission, "has_permission", return_value=True ) @@ -323,7 +324,12 @@ def test_reports_results_post_successful_github_oidc_auth( "new_upload.reports_results", args=["github", "codecov::::the_repo", commit.commitid, "code"], ) - response = client.post(url, content_type="application/json", data={}) + response = client.post( + url, + content_type="application/json", + data={}, + headers={"User-Agent": "codecov-cli/0.4.7"}, + ) assert ( url @@ -334,6 +340,17 @@ def test_reports_results_post_successful_github_oidc_auth( report_id=commit_report.id, ).exists() mocked_task.assert_called_once() + mock_sentry_metrics.assert_called_with( + "upload", + tags={ + "agent": "cli", + "version": "0.4.7", + "action": "coverage", + "endpoint": "create_report_results", + "repo_visibility": "private", + "is_using_shelter": "no", + }, + ) def test_reports_results_already_exists_post_successful(client, db, mocker): diff --git a/upload/views/reports.py b/upload/views/reports.py index d207ac49c5..39f78e28f9 100644 --- a/upload/views/reports.py +++ b/upload/views/reports.py @@ -103,6 +103,16 @@ def perform_create(self, serializer): repoid=repository.repoid, report_code=report.code, ) + sentry_metrics.incr( + "upload", + tags=generate_upload_sentry_metrics_tags( + action="coverage", + endpoint="create_report_results", + request=self.request, + repository=repository, + is_shelter_request=self.is_shelter_request(), + ), + ) return instance def get_object(self): From 35fa4874d7c2952d56e6e251a483466ec2d4c46f Mon Sep 17 00:00:00 2001 From: ajay-sentry <159853603+ajay-sentry@users.noreply.github.com> Date: Wed, 15 May 2024 11:54:42 -0700 Subject: [PATCH 04/11] dev: pin stripe version to V8 (#565) --- billing/views.py | 1 + services/billing.py | 1 + 2 files changed, 2 insertions(+) diff --git a/billing/views.py b/billing/views.py index 9ccaf9c85a..400dcf55e8 100644 --- a/billing/views.py +++ b/billing/views.py @@ -16,6 +16,7 @@ if settings.STRIPE_API_KEY: stripe.api_key = settings.STRIPE_API_KEY + stripe.api_version = "2023-10-16" log = logging.getLogger(__name__) diff --git a/services/billing.py b/services/billing.py index 1d94986920..dec7f4a9bf 100644 --- a/services/billing.py +++ b/services/billing.py @@ -22,6 +22,7 @@ if settings.STRIPE_API_KEY: stripe.api_key = settings.STRIPE_API_KEY + stripe.api_version = "2023-10-16" def _log_stripe_error(method): From d5ebabe1c02e3c2e52da4d6db68da570e7fa5b1a Mon Sep 17 00:00:00 2001 From: ajay-sentry <159853603+ajay-sentry@users.noreply.github.com> Date: Wed, 15 May 2024 15:50:59 -0700 Subject: [PATCH 05/11] fix: Use correct proration param for new stripe version (#567) --- services/billing.py | 4 +++- services/tests/test_billing.py | 8 ++++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/services/billing.py b/services/billing.py index dec7f4a9bf..e6bb18aa81 100644 --- a/services/billing.py +++ b/services/billing.py @@ -158,7 +158,9 @@ def delete_subscription(self, owner): stripe.SubscriptionSchedule.release(subscription_schedule_id) stripe.Subscription.modify( - owner.stripe_subscription_id, cancel_at_period_end=True, prorate=False + owner.stripe_subscription_id, + cancel_at_period_end=True, + proration_behavior="none", ) @_log_stripe_error diff --git a/services/tests/test_billing.py b/services/tests/test_billing.py index 3b49695e88..abb7e0e9de 100644 --- a/services/tests/test_billing.py +++ b/services/tests/test_billing.py @@ -284,7 +284,9 @@ def test_delete_subscription_without_schedule_modifies_subscription_to_delete_at retrieve_subscription_mock.return_value = MockSubscription(subscription_params) self.stripe.delete_subscription(owner) modify_mock.assert_called_once_with( - stripe_subscription_id, cancel_at_period_end=True, prorate=False + stripe_subscription_id, + cancel_at_period_end=True, + proration_behavior="none", ) owner.refresh_from_db() assert owner.stripe_subscription_id == stripe_subscription_id @@ -320,7 +322,9 @@ def test_delete_subscription_with_schedule_releases_schedule_and_cancels_subscri self.stripe.delete_subscription(owner) schedule_release_mock.assert_called_once_with(stripe_schedule_id) modify_mock.assert_called_once_with( - stripe_subscription_id, cancel_at_period_end=True, prorate=False + stripe_subscription_id, + cancel_at_period_end=True, + proration_behavior="none", ) owner.refresh_from_db() assert owner.stripe_subscription_id == stripe_subscription_id From 18722b4a03674ec8cb937bc85377fcd0239025a2 Mon Sep 17 00:00:00 2001 From: ajay-sentry <159853603+ajay-sentry@users.noreply.github.com> Date: Fri, 17 May 2024 10:44:27 -0700 Subject: [PATCH 06/11] dev: Upgrade stripe version to latest and add typings (#569) --- .../tests/views/test_account_viewset.py | 2 +- .../tests/views/test_invoice_viewset.py | 2 +- billing/tests/test_views.py | 57 +++--- billing/views.py | 184 ++++++++++-------- codecov/settings_dev.py | 9 + codecov/settings_prod.py | 11 ++ codecov/settings_staging.py | 8 + plan/service.py | 4 +- plan/tests/test_plan.py | 26 +++ requirements.in | 2 +- requirements.txt | 2 +- services/billing.py | 45 +++-- services/decorators.py | 2 +- services/tests/test_billing.py | 66 +++---- 14 files changed, 259 insertions(+), 161 deletions(-) diff --git a/api/internal/tests/views/test_account_viewset.py b/api/internal/tests/views/test_account_viewset.py index 1356263812..398e4ad574 100644 --- a/api/internal/tests/views/test_account_viewset.py +++ b/api/internal/tests/views/test_account_viewset.py @@ -8,7 +8,7 @@ from rest_framework import status from rest_framework.reverse import reverse from rest_framework.test import APITestCase -from stripe.error import StripeError +from stripe import StripeError from api.internal.tests.test_utils import GetAdminProviderAdapter from codecov_auth.models import Service diff --git a/api/internal/tests/views/test_invoice_viewset.py b/api/internal/tests/views/test_invoice_viewset.py index 3b4d0c4d50..c9aa8fce8b 100644 --- a/api/internal/tests/views/test_invoice_viewset.py +++ b/api/internal/tests/views/test_invoice_viewset.py @@ -5,7 +5,7 @@ from rest_framework import status from rest_framework.reverse import reverse from rest_framework.test import APITestCase -from stripe.error import InvalidRequestError, StripeError +from stripe import InvalidRequestError, StripeError from api.internal.tests.test_utils import GetAdminProviderAdapter from codecov_auth.tests.factories import OwnerFactory diff --git a/billing/tests/test_views.py b/billing/tests/test_views.py index 52d6061ff5..a1e877846e 100644 --- a/billing/tests/test_views.py +++ b/billing/tests/test_views.py @@ -2,6 +2,7 @@ from datetime import datetime, timedelta from unittest.mock import patch +from pytest import raises import stripe from django.conf import settings from freezegun import freeze_time @@ -18,18 +19,12 @@ class MockSubscriptionPlan(object): def __init__(self, params): - self.name = params["new_plan"] - - -class MockOboOrg(object): - def __init__(self, owner): - self.obo_organization = owner.ownerid - self.obo = 15 + self.id = params["new_plan"] class MockSubscription(object): def __init__(self, owner, params): - self.metadata = MockOboOrg(owner) + self.metadata = {"obo_organization": owner.ownerid, "obo": 15} self.plan = MockSubscriptionPlan(params) self.quantity = params["new_quantity"] self.customer = "cus_123" @@ -38,7 +33,7 @@ def __init__(self, owner, params): "data": [ { "quantity": params["new_quantity"], - "plan": {"name": params["new_plan"]}, + "plan": {"id": params["new_plan"]}, } ] } @@ -53,7 +48,7 @@ def setUp(self): stripe_customer_id="20f0", stripe_subscription_id="3p00" ) - def _send_event(self, payload): + def _send_event(self, payload, errorSig=None): timestamp = time.time_ns() request = APIRequestFactory().post( @@ -63,7 +58,8 @@ def _send_event(self, payload): return self.client.post( reverse("stripe-webhook"), **{ - StripeHTTPHeaders.SIGNATURE: "t={},v1={}".format( + StripeHTTPHeaders.SIGNATURE: errorSig + or "t={},v1={}".format( timestamp, stripe.WebhookSignature._compute_signature( "{}.{}".format(timestamp, request.body.decode("utf-8")), @@ -75,6 +71,16 @@ def _send_event(self, payload): format="json", ) + def test_invalid_event_signature(self): + response = self._send_event( + payload={ + "type": "blah", + "data": {}, + }, + errorSig="lol", + ) + assert response.status_code == status.HTTP_400_BAD_REQUEST + def test_invoice_payment_succeeded_sets_owner_delinquent_false(self): self.owner.deliquent = True self.owner.save() @@ -178,14 +184,14 @@ def test_customer_subscription_created_does_nothing_if_no_plan_id(self): self.owner.stripe_customer_id = None self.owner.save() - response = self._send_event( + self._send_event( payload={ "type": "customer.subscription.created", "data": { "object": { "id": "FOEKDCDEQ", "customer": "sdo050493", - "plan": {"id": None, "name": "users-inappy"}, + "plan": {"id": None}, "metadata": {"obo_organization": self.owner.ownerid}, "quantity": 20, } @@ -211,7 +217,7 @@ def test_customer_subscription_created_does_nothing_if_plan_not_paid_user_plan( "object": { "id": "FOEKDCDEQ", "customer": "sdo050493", - "plan": {"id": "fieown4", "name": "users-free"}, + "plan": {"id": "?"}, "metadata": {"obo_organization": self.owner.ownerid}, "quantity": 20, } @@ -240,7 +246,7 @@ def test_customer_subscription_created_sets_plan_info(self): "object": { "id": stripe_subscription_id, "customer": stripe_customer_id, - "plan": {"id": "fieown4", "name": plan_name}, + "plan": {"id": "plan_H6P16wij3lUuxg"}, "metadata": {"obo_organization": self.owner.ownerid}, "quantity": quantity, "status": "active", @@ -274,7 +280,7 @@ def test_customer_subscription_created_can_trigger_trial_expiration( "object": { "id": stripe_subscription_id, "customer": stripe_customer_id, - "plan": {"id": "fieown4", "name": plan_name}, + "plan": {"id": "plan_H6P16wij3lUuxg"}, "metadata": {"obo_organization": self.owner.ownerid}, "quantity": quantity, "default_payment_method": "blabla", @@ -301,7 +307,7 @@ def test_customer_subscription_updated_does_not_change_subscription_if_not_paid_ "object": { "id": self.owner.stripe_subscription_id, "customer": self.owner.stripe_customer_id, - "plan": {"id": "fieown4", "name": "users-free"}, + "plan": {"id": "?"}, "metadata": {"obo_organization": self.owner.ownerid}, "quantity": 20, "status": "active", @@ -334,7 +340,7 @@ def test_customer_subscription_updated_does_not_change_subscription_if_there_is_ "object": { "id": self.owner.stripe_subscription_id, "customer": self.owner.stripe_customer_id, - "plan": {"id": "fieown4", "name": "users-free"}, + "plan": {"id": "?"}, "metadata": {"obo_organization": self.owner.ownerid}, "quantity": 20, "status": "active", @@ -372,7 +378,9 @@ def test_customer_subscription_updated_sets_free_and_deactivates_all_repos_if_in "object": { "id": self.owner.stripe_subscription_id, "customer": self.owner.stripe_customer_id, - "plan": {"id": "fieown4", "name": "users-pr-inappy"}, + "plan": { + "id": "plan_H6P16wij3lUuxg", + }, "metadata": {"obo_organization": self.owner.ownerid}, "quantity": 20, "status": "incomplete_expired", @@ -402,14 +410,14 @@ def test_customer_subscription_updated_sets_fields_on_success(self, upm_mock): plan_name = "users-pr-inappy" quantity = 20 - response = self._send_event( + self._send_event( payload={ "type": "customer.subscription.updated", "data": { "object": { "id": self.owner.stripe_subscription_id, "customer": self.owner.stripe_customer_id, - "plan": {"id": "fieown4", "name": plan_name}, + "plan": {"id": "plan_H6P16wij3lUuxg"}, "metadata": {"obo_organization": self.owner.ownerid}, "quantity": quantity, "status": "active", @@ -435,7 +443,8 @@ def test_subscription_schedule_released_updates_owner_with_existing_subscription self.owner.save() self.new_params = { - "new_plan": "users-pr-inappm", + "new_plan": "plan_H6P3KZXwmAbqPS", + "new_id": "plan_H6P3KZXwmAbqPS", "new_quantity": 7, "subscription_id": None, } @@ -456,7 +465,7 @@ def test_subscription_schedule_released_updates_owner_with_existing_subscription ) self.owner.refresh_from_db() - assert self.owner.plan == self.new_params["new_plan"] + assert self.owner.plan == settings.STRIPE_PLAN_VALS[self.new_params["new_plan"]] assert self.owner.plan_user_count == self.new_params["new_quantity"] @patch("services.billing.stripe.Subscription.retrieve") @@ -471,7 +480,7 @@ def test_subscription_schedule_created_logs_a_new_schedule( self.owner.save() self.params = { - "new_plan": "users-pr-inappm", + "new_plan": "plan_H6P3KZXwmAbqPS", "new_quantity": 7, "subscription_id": subscription_id, } diff --git a/billing/views.py b/billing/views.py index 400dcf55e8..0243c33abb 100644 --- a/billing/views.py +++ b/billing/views.py @@ -8,7 +8,6 @@ from rest_framework.views import APIView from codecov_auth.models import Owner -from plan.constants import PAID_PLANS from plan.service import PlanService from services.billing import BillingService @@ -16,7 +15,7 @@ if settings.STRIPE_API_KEY: stripe.api_key = settings.STRIPE_API_KEY - stripe.api_version = "2023-10-16" + stripe.api_version = "2024-04-10" log = logging.getLogger(__name__) @@ -24,15 +23,15 @@ class StripeWebhookHandler(APIView): permission_classes = [AllowAny] - def _log_updated(self, updated): + def _log_updated(self, updated) -> None: if updated >= 1: log.info(f"Successfully updated info for {updated} customer(s)") else: log.warning("Could not find customer") - def invoice_payment_succeeded(self, invoice): + def invoice_payment_succeeded(self, invoice: stripe.Invoice) -> None: log.info( - "Setting delinquency status False", + "Invoice Payment Succeeded - Setting delinquency status False", extra=dict( stripe_customer_id=invoice.customer, stripe_subscription_id=invoice.subscription, @@ -48,9 +47,9 @@ def invoice_payment_succeeded(self, invoice): self._log_updated(1) - def invoice_payment_failed(self, invoice): + def invoice_payment_failed(self, invoice: stripe.Invoice) -> None: log.info( - "Setting delinquency status True", + "Invoice Payment Failed - Setting Deliquency status True", extra=dict( stripe_customer_id=invoice.customer, stripe_subscription_id=invoice.subscription, @@ -62,9 +61,9 @@ def invoice_payment_failed(self, invoice): ).update(delinquent=True) self._log_updated(updated) - def customer_subscription_deleted(self, subscription): + def customer_subscription_deleted(self, subscription: stripe.Subscription) -> None: log.info( - "Setting free plan and deactivating repos for stripe customer", + "Customer Subscription Deleted - Setting free plan and deactivating repos for stripe customer", extra=dict( stripe_subscription_id=subscription.id, stripe_customer_id=subscription.customer, @@ -80,22 +79,29 @@ def customer_subscription_deleted(self, subscription): self._log_updated(1) - def subscription_schedule_created(self, schedule): + def subscription_schedule_created( + self, schedule: stripe.SubscriptionSchedule + ) -> None: subscription = stripe.Subscription.retrieve(schedule["subscription"]) + sub_item_plan_id = subscription.plan.id + plan_name = settings.STRIPE_PLAN_VALS[sub_item_plan_id] log.info( - f"Schedule created for customer " - f"with -- plan: {subscription.plan.name}, quantity {subscription.quantity}", + "Schedule created for customer", extra=dict( stripe_customer_id=subscription.customer, stripe_subscription_id=subscription.id, - ownerid=subscription.metadata.obo_organization, + ownerid=subscription.metadata["obo_organization"], + plan=plan_name, + quantity=subscription.quantity, ), ) - def subscription_schedule_updated(self, schedule): + def subscription_schedule_updated( + self, schedule: stripe.SubscriptionSchedule + ) -> None: if schedule["subscription"]: subscription = stripe.Subscription.retrieve(schedule["subscription"]) - scheduled_phase = schedule["phases"][1] + scheduled_phase = schedule["phases"][-1] scheduled_plan = scheduled_phase["items"][0] plan_id = scheduled_plan["plan"] stripe_plan_dict = settings.STRIPE_PLAN_IDS @@ -104,83 +110,105 @@ def subscription_schedule_updated(self, schedule): ] quantity = scheduled_plan["quantity"] log.info( - f"Schedule updated for customer " - f"with -- plan: {plan_name}, quantity {quantity}", + "Schedule updated for customer", extra=dict( stripe_customer_id=subscription.customer, stripe_subscription_id=subscription.id, - ownerid=subscription.metadata.obo_organization, + ownerid=subscription.metadata["obo_organization"], + plan=plan_name, + quantity=quantity, ), ) - def subscription_schedule_released(self, schedule): + def subscription_schedule_released(self, schedule: stripe.Subscription) -> None: subscription = stripe.Subscription.retrieve(schedule["released_subscription"]) - owner = Owner.objects.get(ownerid=subscription.metadata.obo_organization) - requesting_user_id = subscription.metadata.obo + owner = Owner.objects.get(ownerid=subscription.metadata["obo_organization"]) + requesting_user_id = subscription.metadata["obo"] plan_service = PlanService(current_org=owner) - plan_service = PlanService(current_org=owner) - plan_service.update_plan( - name=subscription.plan.name, user_count=subscription.quantity - ) + sub_item_plan_id = subscription.plan.id + plan_name = settings.STRIPE_PLAN_VALS[sub_item_plan_id] + plan_service.update_plan(name=plan_name, user_count=subscription.quantity) log.info( - f"Stripe subscription modified successfully for owner {owner.ownerid} by user #{requesting_user_id}", - extra=dict(ownerid=owner.ownerid, requesting_user_id=requesting_user_id), + "Successfully updated customer plan info", + extra=dict( + stripe_subscription_id=subscription.id, + stripe_customer_id=subscription.customer, + plan=plan_name, + quantity=subscription.quantity, + ownerid=owner.ownerid, + requesting_user_id=requesting_user_id, + ), ) - def customer_created(self, customer): + def customer_created(self, customer: stripe.Customer) -> None: # Based on what stripe doesn't gives us (an ownerid!) # in this event we cannot reliably create a customer, # so we're just logging that we created the event and # relying on customer.subscription.created to handle sub creation log.info("Customer created", extra=dict(stripe_customer_id=customer.id)) - def customer_subscription_created(self, subscription): - if not subscription.plan.id: + def customer_subscription_created(self, subscription: stripe.Subscription) -> None: + sub_item_plan_id = subscription.plan.id + + if not sub_item_plan_id: log.warning( - "Subscription created missing plan id, exiting", + "Subscription created, but missing plan_id", extra=dict( stripe_customer_id=subscription.customer, - ownerid=subscription.metadata.obo_organization, + ownerid=subscription.metadata["obo_organization"], + subscription_plan=subscription.plan, ), ) return - if subscription.plan.name not in PAID_PLANS: + if sub_item_plan_id not in settings.STRIPE_PLAN_VALS: log.warning( - f"Subscription creation requested for invalid plan " - f"'{subscription.plan.name}' -- doing nothing", + "Subscription creation requested for invalid plan", extra=dict( stripe_customer_id=subscription.customer, - ownerid=subscription.metadata.obo_organization, + ownerid=subscription.metadata["obo_organization"], + plan_id=sub_item_plan_id, ), ) return + plan_name = settings.STRIPE_PLAN_VALS[sub_item_plan_id] + log.info( - f"Subscription created for customer " - f"with -- plan: {subscription.plan.name}, quantity {subscription.quantity}", + "Subscription created for customer", extra=dict( stripe_customer_id=subscription.customer, stripe_subscription_id=subscription.id, - ownerid=subscription.metadata.obo_organization, + ownerid=subscription.metadata["obo_organization"], + plan=plan_name, + quantity=subscription.quantity, ), ) - owner = Owner.objects.get(ownerid=subscription.metadata.obo_organization) + owner = Owner.objects.get(ownerid=subscription.metadata["obo_organization"]) owner.stripe_subscription_id = subscription.id owner.stripe_customer_id = subscription.customer owner.save() plan_service = PlanService(current_org=owner) plan_service.expire_trial_when_upgrading() - plan_service.update_plan( - name=subscription.plan.name, user_count=subscription.quantity + + plan_service.update_plan(name=plan_name, user_count=subscription.quantity) + + log.info( + "Successfully updated customer plan info", + extra=dict( + stripe_subscription_id=subscription.id, + stripe_customer_id=subscription.customer, + plan=plan_name, + quantity=subscription.quantity, + ), ) self._log_updated(1) - def customer_subscription_updated(self, subscription): + def customer_subscription_updated(self, subscription: stripe.Subscription) -> None: owner: Owner = Owner.objects.get( stripe_subscription_id=subscription.id, stripe_customer_id=subscription.customer, @@ -201,36 +229,45 @@ def customer_subscription_updated(self, subscription): if not subscription_schedule_id: if subscription.status == "incomplete_expired": log.info( - "Subscription updated with status change " - "to 'incomplete_expired' -- cancelling to free", - extra=dict(stripe_subscription_id=subscription.id), + "Subscription status updated to incomplete_expired, cancelling to free", + extra=dict( + stripe_subscription_id=subscription.id, + stripe_customer_id=subscription.customer, + ), ) plan_service.set_default_plan_data() # TODO: think of how to create services for different objects/classes to delegate responsibilities that are not # from the owner owner.repository_set.update(active=False, activated=False) return - # TODO: we can delete this if statement if we confirm there aren't any PAID_PLANS out there - if subscription.plan.name not in PAID_PLANS: - log.warning( - f"Subscription update requested with invalid plan " - f"{subscription.plan.name} -- doing nothing", - extra=dict(stripe_subscription_id=subscription.id), + + sub_item_plan_id = subscription.plan.id + if sub_item_plan_id not in settings.STRIPE_PLAN_VALS: + log.error( + "Subscription update requested with invalid plan", + extra=dict( + stripe_subscription_id=subscription.id, + stripe_customer_id=subscription.customer, + plan_id=sub_item_plan_id, + ), ) return - log.info( - f"Subscription updated with -- " - f"plan: {subscription.plan.name}, quantity: {subscription.quantity}", - extra=dict(stripe_subscription_id=subscription.id), - ) + plan_name = settings.STRIPE_PLAN_VALS[sub_item_plan_id] + + plan_service.update_plan(name=plan_name, user_count=subscription.quantity) - plan_service.update_plan( - name=subscription.plan.name, user_count=subscription.quantity + log.info( + "Successfully updated customer subscription", + extra=dict( + stripe_subscription_id=subscription.id, + stripe_customer_id=subscription.customer, + plan=plan_name, + quantity=subscription.quantity, + ), ) - log.info("Successfully updated info for 1 customer") - def customer_updated(self, customer): + def customer_updated(self, customer: stripe.Customer) -> None: new_default_payment_method = customer["invoice_settings"][ "default_payment_method" ] @@ -247,30 +284,23 @@ def customer_updated(self, customer): subscription["id"], default_payment_method=new_default_payment_method ) - def checkout_session_completed(self, checkout_session): + def checkout_session_completed( + self, checkout_session: stripe.checkout.Session + ) -> None: log.info( "Checkout session completed", - extra=dict(ownerid=checkout_session.client_reference_id), + extra=dict( + ownerid=checkout_session.client_reference_id, + stripe_customer_id=checkout_session.customer, + ), ) owner = Owner.objects.get(ownerid=checkout_session.client_reference_id) owner.stripe_customer_id = checkout_session.customer owner.save() - # Segment - segment_checkout_session_details = {"plan": None, "userid_type": "org"} - try: - segment_checkout_session_details["plan"] = checkout_session.display_items[ - 0 - ]["plan"]["name"] - except: - log.warning( - "Could not find plan in checkout.session.completed event", - extra=dict(ownerid=checkout_session.client_reference_id), - ) - self._log_updated(1) - def post(self, request, *args, **kwargs): + def post(self, request, *args, **kwargs) -> Response: if settings.STRIPE_ENDPOINT_SECRET is None: log.critical( "Stripe endpoint secret improperly configured -- webhooks will not be processed." @@ -281,7 +311,7 @@ def post(self, request, *args, **kwargs): self.request.META.get(StripeHTTPHeaders.SIGNATURE), settings.STRIPE_ENDPOINT_SECRET, ) - except stripe.error.SignatureVerificationError as e: + except stripe.SignatureVerificationError as e: log.warning(f"Stripe webhook event received with invalid signature -- {e}") return Response("Invalid signature", status=status.HTTP_400_BAD_REQUEST) if self.event.type not in StripeWebhookEvents.subscribed_events: diff --git a/codecov/settings_dev.py b/codecov/settings_dev.py index 7d29ae2aa2..ce8e1c2a3d 100644 --- a/codecov/settings_dev.py +++ b/codecov/settings_dev.py @@ -29,6 +29,15 @@ "users-teamy": "price_1OCM2cGlVGuVgOrkMWUFjPFz", } +STRIPE_PLAN_VALS = { + "plan_H6P3KZXwmAbqPS": "users-pr-inappm", + "plan_H6P16wij3lUuxg": "users-pr-inappy", + "price_1Mj1kYGlVGuVgOrk7jucaZAa": "users-sentrym", + "price_1Mj1mMGlVGuVgOrkC0ORc6iW": "users-sentryy", + "price_1OCM0gGlVGuVgOrkWDYEBtSL": "users-teamm", + "price_1OCM2cGlVGuVgOrkMWUFjPFz": "users-teamy", +} + CORS_ALLOW_CREDENTIALS = True CODECOV_URL = "localhost" diff --git a/codecov/settings_prod.py b/codecov/settings_prod.py index 8050006770..6b82842220 100644 --- a/codecov/settings_prod.py +++ b/codecov/settings_prod.py @@ -26,6 +26,17 @@ "users-teamy": "price_1NrlXiGlVGuVgOrkgMTw5yno", } +STRIPE_PLAN_VALS = { + "price_1Gv2B8GlVGuVgOrkFnLunCgc": "users-pr-inappm", + "price_1Gv2COGlVGuVgOrkuOYVLIj7": "users-pr-inappy", + "price_1MlY9yGlVGuVgOrkHluurBtJ": "users-sentrym", + "price_1MlYAYGlVGuVgOrke9SdbBUn": "users-sentryy", + "price_1LmjzwGlVGuVgOrkIwlM46EU": "users-enterprisey", + "price_1LmjypGlVGuVgOrkzKtNqhwW": "users-enterprisem", + "price_1NqPKdGlVGuVgOrkm9OFvtz8": "users-teamm", + "price_1NrlXiGlVGuVgOrkgMTw5yno": "users-teamy", +} + CORS_ALLOW_HEADERS += ["sentry-trace", "baggage"] CORS_ALLOW_CREDENTIALS = True CODECOV_URL = get_config("setup", "codecov_url", default="https://codecov.io") diff --git a/codecov/settings_staging.py b/codecov/settings_staging.py index 1f6dd50424..95317aa670 100644 --- a/codecov/settings_staging.py +++ b/codecov/settings_staging.py @@ -26,6 +26,14 @@ "users-teamm": "price_1OCM0gGlVGuVgOrkWDYEBtSL", "users-teamy": "price_1OCM2cGlVGuVgOrkMWUFjPFz", } +STRIPE_PLAN_VALS = { + "plan_H6P3KZXwmAbqPS": "users-pr-inappm", + "plan_H6P16wij3lUuxg": "users-pr-inappy", + "price_1Mj1kYGlVGuVgOrk7jucaZAa": "users-sentrym", + "price_1Mj1mMGlVGuVgOrkC0ORc6iW": "users-sentryy", + "price_1OCM0gGlVGuVgOrkWDYEBtSL": "users-teamm", + "price_1OCM2cGlVGuVgOrkMWUFjPFz": "users-teamy", +} CORS_ALLOW_HEADERS += ["sentry-trace", "baggage"] CORS_ALLOWED_ORIGIN_REGEXES = [ diff --git a/plan/service.py b/plan/service.py index f1692c33ea..672aff3f83 100644 --- a/plan/service.py +++ b/plan/service.py @@ -44,9 +44,11 @@ def __init__(self, current_org: Owner): else: self.plan_data = USER_PLAN_REPRESENTATIONS[self.current_org.plan] - def update_plan(self, name: PlanName, user_count: int) -> None: + def update_plan(self, name, user_count: int | None) -> None: if name not in USER_PLAN_REPRESENTATIONS: raise ValueError("Unsupported plan") + if not user_count: + raise ValueError("Quantity Needed") self.current_org.plan = name self.current_org.plan_user_count = user_count self.plan_data = USER_PLAN_REPRESENTATIONS[self.current_org.plan] diff --git a/plan/tests/test_plan.py b/plan/tests/test_plan.py index 93db49aa80..e4d75b73ab 100644 --- a/plan/tests/test_plan.py +++ b/plan/tests/test_plan.py @@ -3,6 +3,7 @@ from django.test import TestCase from freezegun import freeze_time +from pytest import raises from codecov.commands.exceptions import ValidationError from codecov_auth.tests.factories import OwnerFactory @@ -337,6 +338,31 @@ def test_plan_service_has_no_seats_left(self): assert plan_service.has_seats_left == False + def test_plan_service_update_plan_invalid_name(self): + current_org = OwnerFactory(plan=PlanName.BASIC_PLAN_NAME.value) + plan_service = PlanService(current_org=current_org) + + with raises(ValueError, match="Unsupported plan"): + plan_service.update_plan(name="blah", user_count=1) + + def test_plan_service_update_plan_invalid_user_count(self): + current_org = OwnerFactory(plan=PlanName.BASIC_PLAN_NAME.value) + plan_service = PlanService(current_org=current_org) + + with raises(ValueError, match="Quantity Needed"): + plan_service.update_plan( + name=PlanName.BASIC_PLAN_NAME.value, user_count=None + ) + + def test_plan_service_update_plan_succeeds(self): + current_org = OwnerFactory(plan=PlanName.BASIC_PLAN_NAME.value) + plan_service = PlanService(current_org=current_org) + + plan_service.update_plan(name=PlanName.TEAM_MONTHLY.value, user_count=8) + + assert current_org.plan == PlanName.TEAM_MONTHLY.value + assert current_org.plan_user_count == 8 + class AvailablePlansBeforeTrial(TestCase): """ diff --git a/requirements.in b/requirements.in index f0a23e495f..09c749a670 100644 --- a/requirements.in +++ b/requirements.in @@ -48,7 +48,7 @@ sentry-sdk>=1.40.0 sentry-sdk[celery] setproctitle simplejson -stripe +stripe>=9.6.0 urllib3>=1.26.17 vcrpy whitenoise diff --git a/requirements.txt b/requirements.txt index 93a63eef62..e6ae0146dc 100644 --- a/requirements.txt +++ b/requirements.txt @@ -443,7 +443,7 @@ starlette==0.36.2 # via ariadne statsd==3.3.0 # via shared -stripe==8.9.0 +stripe==9.6.0 # via -r requirements.in text-unidecode==1.3 # via faker diff --git a/services/billing.py b/services/billing.py index e6bb18aa81..38a902b4c8 100644 --- a/services/billing.py +++ b/services/billing.py @@ -22,14 +22,14 @@ if settings.STRIPE_API_KEY: stripe.api_key = settings.STRIPE_API_KEY - stripe.api_version = "2023-10-16" + stripe.api_version = "2024-04-10" def _log_stripe_error(method): def catch_and_raise(*args, **kwargs): try: return method(*args, **kwargs) - except stripe.error.StripeError as e: + except stripe.StripeError as e: log.warning(e.user_message) raise @@ -91,7 +91,7 @@ def __init__(self, requesting_user): self.requesting_user = requesting_user - def _get_checkout_session_and_subscription_metadata(self, owner): + def _get_checkout_session_and_subscription_metadata(self, owner: Owner): return { "service": owner.service, "obo_organization": owner.ownerid, @@ -108,7 +108,7 @@ def get_invoice(self, owner, invoice_id): ) try: invoice = stripe.Invoice.retrieve(invoice_id) - except stripe.error.InvalidRequestError as e: + except stripe.InvalidRequestError as e: log.info(f"invoice {invoice_id} not found for owner {owner.ownerid}") return None if invoice["customer"] != owner.stripe_customer_id: @@ -127,7 +127,7 @@ def filter_invoices_by_total(self, invoice): return invoice @_log_stripe_error - def list_filtered_invoices(self, owner, limit=10): + def list_filtered_invoices(self, owner: Owner, limit=10): log.info(f"Fetching invoices from Stripe for ownerid {owner.ownerid}") if owner.stripe_customer_id is None: log.info("stripe_customer_id is None, not fetching invoices") @@ -142,7 +142,7 @@ def list_filtered_invoices(self, owner, limit=10): return list(invoices_filtered_by_status_and_total) @_log_stripe_error - def delete_subscription(self, owner): + def delete_subscription(self, owner: Owner): subscription = stripe.Subscription.retrieve(owner.stripe_subscription_id) subscription_schedule_id = subscription.schedule @@ -164,7 +164,7 @@ def delete_subscription(self, owner): ) @_log_stripe_error - def get_subscription(self, owner): + def get_subscription(self, owner: Owner): if not owner.stripe_subscription_id: return None return stripe.Subscription.retrieve( @@ -177,7 +177,7 @@ def get_subscription(self, owner): ) @_log_stripe_error - def get_schedule(self, owner): + def get_schedule(self, owner: Owner): if not owner.stripe_subscription_id: return None @@ -252,7 +252,7 @@ def modify_subscription(self, owner, desired_plan): ) def _modify_subscription_schedule( - self, owner, subscription, subscription_schedule_id, desired_plan + self, owner: Owner, subscription, subscription_schedule_id, desired_plan ): current_subscription_start_date = subscription["current_period_start"] current_subscription_end_date = subscription["current_period_end"] @@ -297,7 +297,7 @@ def _is_upgrading_seats(self, owner: Owner, desired_plan: dict) -> bool: """ Returns `True` if purchasing more seats. """ - return ( + return bool( owner.plan_user_count and owner.plan_user_count < desired_plan["quantity"] ) @@ -308,7 +308,7 @@ def _is_extending_term(self, owner: Owner, desired_plan: dict) -> bool: current_plan_info = USER_PLAN_REPRESENTATIONS.get(owner.plan) desired_plan_info = USER_PLAN_REPRESENTATIONS.get(desired_plan["value"]) - return ( + return bool( current_plan_info and current_plan_info.billing_rate == PlanBillingRate.MONTHLY.value and desired_plan_info @@ -339,7 +339,7 @@ def _is_similar_plan(self, owner: Owner, desired_plan: dict) -> bool: elif owner.plan in TEAM_PLANS and desired_plan["value"] not in TEAM_PLANS: return True - return is_same_term and is_same_seats + return bool(is_same_term and is_same_seats) def _get_proration_params(self, owner: Owner, desired_plan: dict) -> str: if ( @@ -361,7 +361,10 @@ def _get_success_and_cancel_url(self, owner): @_log_stripe_error def create_checkout_session(self, owner: Owner, desired_plan): success_url, cancel_url = self._get_success_and_cancel_url(owner) - log.info("Creating Stripe Checkout Session for owner: {owner.ownerid}") + log.info( + "Creating Stripe Checkout Session for owner", + extra=dict(owner_id=owner.ownerid), + ) if not owner.stripe_customer_id: customer_email = owner.email @@ -374,19 +377,19 @@ def create_checkout_session(self, owner: Owner, desired_plan): billing_address_collection="required", payment_method_types=["card"], payment_method_collection="if_required", - client_reference_id=owner.ownerid, + client_reference_id=str(owner.ownerid), success_url=success_url, cancel_url=cancel_url, customer=customer, customer_email=customer_email, + mode="subscription", + line_items=[ + { + "price": settings.STRIPE_PLAN_IDS[desired_plan["value"]], + "quantity": desired_plan["quantity"], + } + ], subscription_data={ - "items": [ - { - "plan": settings.STRIPE_PLAN_IDS[desired_plan["value"]], - "quantity": desired_plan["quantity"], - } - ], - "payment_behavior": "allow_incomplete", "metadata": self._get_checkout_session_and_subscription_metadata(owner), }, ) diff --git a/services/decorators.py b/services/decorators.py index dfdcf63b8f..47244ae2f9 100644 --- a/services/decorators.py +++ b/services/decorators.py @@ -30,7 +30,7 @@ def stripe_safe(method): def exec_method(*args, **kwargs): try: return method(*args, **kwargs) - except stripe.error.StripeError as e: + except stripe.StripeError as e: exception = APIException(detail=e.user_message) exception.status_code = e.http_status raise exception diff --git a/services/tests/test_billing.py b/services/tests/test_billing.py index abb7e0e9de..68865c5e2b 100644 --- a/services/tests/test_billing.py +++ b/services/tests/test_billing.py @@ -3,7 +3,7 @@ from django.conf import settings from django.test import TestCase -from stripe.error import InvalidRequestError +from stripe import InvalidRequestError from codecov_auth.models import Service from codecov_auth.tests.factories import OwnerFactory @@ -1008,19 +1008,19 @@ def test_create_checkout_session_with_email_and_no_stripe_customer_id( billing_address_collection="required", payment_method_types=["card"], payment_method_collection="if_required", - client_reference_id=owner.ownerid, + client_reference_id=str(owner.ownerid), customer_email=owner.email, customer=None, success_url=f"{settings.CODECOV_DASHBOARD_URL}/plan/gh/{owner.username}?success", cancel_url=f"{settings.CODECOV_DASHBOARD_URL}/plan/gh/{owner.username}?cancel", + mode="subscription", + line_items=[ + { + "price": settings.STRIPE_PLAN_IDS[desired_plan["value"]], + "quantity": desired_quantity, + } + ], subscription_data={ - "items": [ - { - "plan": settings.STRIPE_PLAN_IDS[desired_plan["value"]], - "quantity": desired_quantity, - } - ], - "payment_behavior": "allow_incomplete", "metadata": { "service": owner.service, "obo_organization": owner.ownerid, @@ -1057,19 +1057,19 @@ def test_create_checkout_session_with_no_email_and_no_stripe_customer_id( billing_address_collection="required", payment_method_types=["card"], payment_method_collection="if_required", - client_reference_id=owner.ownerid, + client_reference_id=str(owner.ownerid), customer_email=owner.email, customer=None, success_url=f"{settings.CODECOV_DASHBOARD_URL}/plan/gh/{owner.username}?success", cancel_url=f"{settings.CODECOV_DASHBOARD_URL}/plan/gh/{owner.username}?cancel", + mode="subscription", + line_items=[ + { + "price": settings.STRIPE_PLAN_IDS[desired_plan["value"]], + "quantity": desired_quantity, + } + ], subscription_data={ - "items": [ - { - "plan": settings.STRIPE_PLAN_IDS[desired_plan["value"]], - "quantity": desired_quantity, - } - ], - "payment_behavior": "allow_incomplete", "metadata": { "service": owner.service, "obo_organization": owner.ownerid, @@ -1106,19 +1106,19 @@ def test_create_checkout_session_with_stripe_customer_id_and_no_email( billing_address_collection="required", payment_method_types=["card"], payment_method_collection="if_required", - client_reference_id=owner.ownerid, + client_reference_id=str(owner.ownerid), customer=owner.stripe_customer_id, customer_email=None, success_url=f"{settings.CODECOV_DASHBOARD_URL}/plan/gh/{owner.username}?success", cancel_url=f"{settings.CODECOV_DASHBOARD_URL}/plan/gh/{owner.username}?cancel", + mode="subscription", + line_items=[ + { + "price": settings.STRIPE_PLAN_IDS[desired_plan["value"]], + "quantity": desired_quantity, + } + ], subscription_data={ - "items": [ - { - "plan": settings.STRIPE_PLAN_IDS[desired_plan["value"]], - "quantity": desired_quantity, - } - ], - "payment_behavior": "allow_incomplete", "metadata": { "service": owner.service, "obo_organization": owner.ownerid, @@ -1155,19 +1155,19 @@ def test_create_checkout_session_with_stripe_customer_id_and_email( billing_address_collection="required", payment_method_types=["card"], payment_method_collection="if_required", - client_reference_id=owner.ownerid, + client_reference_id=str(owner.ownerid), customer=owner.stripe_customer_id, customer_email=None, success_url=f"{settings.CODECOV_DASHBOARD_URL}/plan/gh/{owner.username}?success", cancel_url=f"{settings.CODECOV_DASHBOARD_URL}/plan/gh/{owner.username}?cancel", + mode="subscription", + line_items=[ + { + "price": settings.STRIPE_PLAN_IDS[desired_plan["value"]], + "quantity": desired_quantity, + } + ], subscription_data={ - "items": [ - { - "plan": settings.STRIPE_PLAN_IDS[desired_plan["value"]], - "quantity": desired_quantity, - } - ], - "payment_behavior": "allow_incomplete", "metadata": { "service": owner.service, "obo_organization": owner.ownerid, From 7fe50b42fe0ada3bff64ebc0ee8f09119b1c9854 Mon Sep 17 00:00:00 2001 From: ajay-sentry <159853603+ajay-sentry@users.noreply.github.com> Date: Mon, 20 May 2024 09:39:34 -0700 Subject: [PATCH 07/11] dev: add isort rules for ruff and fix (#571) --- api/public/v2/compare/views.py | 2 +- api/shared/permissions.py | 15 ++++++++------- billing/tests/test_views.py | 2 +- .../tests/test_get_is_current_user_an_admin.py | 2 +- graphql_api/types/config/config.py | 2 +- ruff.toml | 6 ++++-- upload/helpers.py | 1 + upload/views/bundle_analysis.py | 3 ++- upload/views/commits.py | 1 + upload/views/empty_upload.py | 5 +++-- upload/views/reports.py | 1 + upload/views/upload_completion.py | 1 + 12 files changed, 25 insertions(+), 16 deletions(-) diff --git a/api/public/v2/compare/views.py b/api/public/v2/compare/views.py index 296e1a5ddd..518e64b3e2 100644 --- a/api/public/v2/compare/views.py +++ b/api/public/v2/compare/views.py @@ -1,6 +1,6 @@ -from distutils.util import strtobool from inspect import Parameter +from distutils.util import strtobool from drf_spectacular.types import OpenApiTypes from drf_spectacular.utils import OpenApiParameter, extend_schema from rest_framework import mixins diff --git a/api/shared/permissions.py b/api/shared/permissions.py index bcbd98104e..3c6cd7a242 100644 --- a/api/shared/permissions.py +++ b/api/shared/permissions.py @@ -1,18 +1,19 @@ import logging - from typing import Any, Tuple + from asgiref.sync import async_to_sync from django.conf import settings -from django.http import Http404 -from rest_framework.permissions import SAFE_METHODS # ['GET', 'HEAD', 'OPTIONS'] -from rest_framework.permissions import BasePermission -from django.http import HttpRequest +from django.http import Http404, HttpRequest +from rest_framework.permissions import ( + SAFE_METHODS, # ['GET', 'HEAD', 'OPTIONS'] + BasePermission, +) -from codecov_auth.models import Owner -from core.models import Repository import services.self_hosted as self_hosted from api.shared.mixins import InternalPermissionsMixin, SuperPermissionsMixin from api.shared.repo.repository_accessors import RepoAccessors +from codecov_auth.models import Owner +from core.models import Repository from services.activation import try_auto_activate from services.decorators import torngit_safe from services.repo_providers import get_generic_adapter_params, get_provider diff --git a/billing/tests/test_views.py b/billing/tests/test_views.py index a1e877846e..3274dee1ef 100644 --- a/billing/tests/test_views.py +++ b/billing/tests/test_views.py @@ -2,10 +2,10 @@ from datetime import datetime, timedelta from unittest.mock import patch -from pytest import raises import stripe from django.conf import settings from freezegun import freeze_time +from pytest import raises from rest_framework import status from rest_framework.reverse import reverse from rest_framework.test import APIRequestFactory, APITestCase diff --git a/codecov_auth/commands/owner/interactors/tests/test_get_is_current_user_an_admin.py b/codecov_auth/commands/owner/interactors/tests/test_get_is_current_user_an_admin.py index 56467eedb0..b0e23cc1d7 100644 --- a/codecov_auth/commands/owner/interactors/tests/test_get_is_current_user_an_admin.py +++ b/codecov_auth/commands/owner/interactors/tests/test_get_is_current_user_an_admin.py @@ -1,8 +1,8 @@ -from distutils.util import execute from unittest.mock import patch import pytest from asgiref.sync import async_to_sync +from distutils.util import execute from django.test import TransactionTestCase, override_settings from codecov_auth.tests.factories import GetAdminProviderAdapter, OwnerFactory diff --git a/graphql_api/types/config/config.py b/graphql_api/types/config/config.py index 65efe38abe..e11ba7ef34 100644 --- a/graphql_api/types/config/config.py +++ b/graphql_api/types/config/config.py @@ -1,7 +1,7 @@ -from distutils.util import strtobool from typing import List from ariadne import ObjectType +from distutils.util import strtobool from django.conf import settings import services.self_hosted as self_hosted diff --git a/ruff.toml b/ruff.toml index 77bf06f7da..b11c13ad5e 100644 --- a/ruff.toml +++ b/ruff.toml @@ -36,11 +36,13 @@ indent-width = 4 target-version = "py312" [lint] -# Currently only enabled F541: https://docs.astral.sh/ruff/rules/ -select = ["F541"] +# Currently only enabled for F541 and I: https://docs.astral.sh/ruff/rules/ +select = ["F541", "I"] ignore = [] # Allow fix for all enabled rules (when `--fix`) is provided. +# The preferred method (for now) w.r.t. fixable rules is to manually update the makefile +# with --fix and re-run 'make lint' fixable = ["ALL"] unfixable = [] diff --git a/upload/helpers.py b/upload/helpers.py index 83ce35b28b..4ea26302d7 100644 --- a/upload/helpers.py +++ b/upload/helpers.py @@ -37,6 +37,7 @@ from utils.config import get_config from utils.encryption import encryptor from utils.github import get_github_integration_token + from .constants import ci, global_upload_token_providers is_pull_noted_in_branch = re.compile(r".*(pull|pr)\/(\d+).*") diff --git a/upload/views/bundle_analysis.py b/upload/views/bundle_analysis.py index 41e995c9e3..85ed9c8b8a 100644 --- a/upload/views/bundle_analysis.py +++ b/upload/views/bundle_analysis.py @@ -6,8 +6,9 @@ from rest_framework.permissions import BasePermission from rest_framework.response import Response from rest_framework.views import APIView -from shared.bundle_analysis.storage import StoragePaths, get_bucket_name from sentry_sdk import metrics as sentry_metrics +from shared.bundle_analysis.storage import StoragePaths, get_bucket_name + from codecov_auth.authentication.repo_auth import ( GitHubOIDCTokenAuthentication, OrgLevelTokenAuthentication, diff --git a/upload/views/commits.py b/upload/views/commits.py index fe9eafdda5..bc96cf68a3 100644 --- a/upload/views/commits.py +++ b/upload/views/commits.py @@ -3,6 +3,7 @@ from rest_framework.exceptions import ValidationError from rest_framework.generics import ListCreateAPIView from sentry_sdk import metrics as sentry_metrics + from codecov_auth.authentication.repo_auth import ( GitHubOIDCTokenAuthentication, GlobalTokenAuthentication, diff --git a/upload/views/empty_upload.py b/upload/views/empty_upload.py index ef77308447..0432b9f3e8 100644 --- a/upload/views/empty_upload.py +++ b/upload/views/empty_upload.py @@ -8,8 +8,9 @@ from rest_framework.exceptions import NotFound from rest_framework.generics import CreateAPIView from rest_framework.response import Response -from shared.torngit.exceptions import TorngitClientError, TorngitClientGeneralError from sentry_sdk import metrics as sentry_metrics +from shared.torngit.exceptions import TorngitClientError, TorngitClientGeneralError + from codecov_auth.authentication.repo_auth import ( GitHubOIDCTokenAuthentication, GlobalTokenAuthentication, @@ -21,8 +22,8 @@ from services.task import TaskService from services.yaml import final_commit_yaml from upload.helpers import ( - try_to_get_best_possible_bot_token, generate_upload_sentry_metrics_tags, + try_to_get_best_possible_bot_token, ) from upload.views.base import GetterMixin from upload.views.uploads import CanDoCoverageUploadsPermission diff --git a/upload/views/reports.py b/upload/views/reports.py index 39f78e28f9..90027d7b0a 100644 --- a/upload/views/reports.py +++ b/upload/views/reports.py @@ -4,6 +4,7 @@ from rest_framework.exceptions import ValidationError from rest_framework.generics import CreateAPIView, ListCreateAPIView, RetrieveAPIView from sentry_sdk import metrics as sentry_metrics + from codecov_auth.authentication.repo_auth import ( GitHubOIDCTokenAuthentication, GlobalTokenAuthentication, diff --git a/upload/views/upload_completion.py b/upload/views/upload_completion.py index 7a6071a2b7..49d1d7aed5 100644 --- a/upload/views/upload_completion.py +++ b/upload/views/upload_completion.py @@ -4,6 +4,7 @@ from rest_framework.generics import CreateAPIView from rest_framework.response import Response from sentry_sdk import metrics as sentry_metrics + from codecov_auth.authentication.repo_auth import ( GitHubOIDCTokenAuthentication, GlobalTokenAuthentication, From 62f64f3530508091d0453cbcbf60188cd9e9f745 Mon Sep 17 00:00:00 2001 From: JerrySentry <142266253+JerrySentry@users.noreply.github.com> Date: Tue, 21 May 2024 13:41:10 -0400 Subject: [PATCH 08/11] Lower log severity level to remove Sentry noise (#576) --- core/commands/commit/interactors/get_file_content.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/commands/commit/interactors/get_file_content.py b/core/commands/commit/interactors/get_file_content.py index 30300b3728..d8d3a8a105 100644 --- a/core/commands/commit/interactors/get_file_content.py +++ b/core/commands/commit/interactors/get_file_content.py @@ -21,7 +21,7 @@ async def get_file_from_service(self, commit, path): return content.get("content").decode("utf-8") # TODO raise this to the API so we can handle it. except Exception: - log.exception( + log.info( "GetFileContentInteractor - exception raised", extra=dict(commitid=commit.commitid), ) From 1979f833bcbafc92be306ad4af4c36ac9adf2d4b Mon Sep 17 00:00:00 2001 From: Rohit Vinnakota <148245014+rohitvinnakota-codecov@users.noreply.github.com> Date: Wed, 22 May 2024 11:33:25 -0600 Subject: [PATCH 09/11] Add encode secret string resolver (#568) --- .../interactors/encode_secret_string.py | 27 +++++++++++++ .../tests/test_encode_secret_string.py | 34 ++++++++++++++++ core/commands/repository/interactors/utils.py | 6 +++ core/commands/repository/repository.py | 8 ++++ .../test_repository_encoded_secret_string.py | 40 +++++++++++++++++++ .../types/repository/repository.graphql | 5 +++ graphql_api/types/repository/repository.py | 14 ++++++- 7 files changed, 132 insertions(+), 2 deletions(-) create mode 100644 core/commands/repository/interactors/encode_secret_string.py create mode 100644 core/commands/repository/interactors/tests/test_encode_secret_string.py create mode 100644 core/commands/repository/interactors/utils.py create mode 100644 graphql_api/tests/test_repository_encoded_secret_string.py diff --git a/core/commands/repository/interactors/encode_secret_string.py b/core/commands/repository/interactors/encode_secret_string.py new file mode 100644 index 0000000000..d7f1b2bde4 --- /dev/null +++ b/core/commands/repository/interactors/encode_secret_string.py @@ -0,0 +1,27 @@ +from dataclasses import dataclass + +from codecov.commands.base import BaseInteractor +from codecov.commands.exceptions import Unauthenticated, ValidationError +from codecov.db import sync_to_async +from codecov_auth.models import Owner +from core.commands.repository.interactors.utils import encode_secret_string +from core.models import Repository + + +class EncodeSecretStringInteractor(BaseInteractor): + @sync_to_async + def execute(self, owner: Owner, repo: Repository, value: str) -> str: + if not self.current_user.is_authenticated: + raise Unauthenticated() + if not repo: + raise ValidationError("Repo not found") + + to_encode = "/".join( + ( + owner.service, + owner.service_id, + repo.service_id, + value, + ) + ) + return encode_secret_string(to_encode) diff --git a/core/commands/repository/interactors/tests/test_encode_secret_string.py b/core/commands/repository/interactors/tests/test_encode_secret_string.py new file mode 100644 index 0000000000..97c02c492b --- /dev/null +++ b/core/commands/repository/interactors/tests/test_encode_secret_string.py @@ -0,0 +1,34 @@ +from unittest.mock import patch + +import pytest +from asgiref.sync import async_to_sync +from django.contrib.auth.models import AnonymousUser +from django.test import TransactionTestCase +from shared.encryption.yaml_secret import yaml_secret_encryptor + +from codecov.commands.exceptions import Unauthenticated, ValidationError +from core.tests.factories import OwnerFactory, RepositoryFactory + +from ..encode_secret_string import EncodeSecretStringInteractor + + +class EncodeSecretStringInteractorTest(TransactionTestCase): + @async_to_sync + def execute(self, owner, repo, value): + return EncodeSecretStringInteractor(owner, "github").execute(owner, repo, value) + + def test_encode_secret_string(self): + owner = OwnerFactory() + repo = RepositoryFactory(author=owner, name="repo-1") + res = self.execute(owner, repo=repo, value="token-1") + check_encryptor = yaml_secret_encryptor + assert "token-1" in check_encryptor.decode(res[7:]) + + def test_validation_error_when_repo_not_found(self): + owner = OwnerFactory() + with pytest.raises(ValidationError): + self.execute(owner, repo=None, value="token-1") + + def test_user_is_not_authenticated(self): + with pytest.raises(Unauthenticated) as e: + self.execute(None, repo=None, value="test") diff --git a/core/commands/repository/interactors/utils.py b/core/commands/repository/interactors/utils.py new file mode 100644 index 0000000000..2f81abefb9 --- /dev/null +++ b/core/commands/repository/interactors/utils.py @@ -0,0 +1,6 @@ +from shared.encryption.yaml_secret import yaml_secret_encryptor + + +def encode_secret_string(value) -> str: + encryptor = yaml_secret_encryptor + return "secret:%s" % encryptor.encode(value).decode() diff --git a/core/commands/repository/repository.py b/core/commands/repository/repository.py index 0287c56d8e..42587e608a 100644 --- a/core/commands/repository/repository.py +++ b/core/commands/repository/repository.py @@ -1,7 +1,10 @@ from codecov.commands.base import BaseCommand +from codecov_auth.models import Owner +from core.models import Repository from timeseries.models import MeasurementName from .interactors.activate_measurements import ActivateMeasurementsInteractor +from .interactors.encode_secret_string import EncodeSecretStringInteractor from .interactors.fetch_repository import FetchRepositoryInteractor from .interactors.get_repository_token import GetRepositoryTokenInteractor from .interactors.get_upload_token import GetUploadTokenInteractor @@ -33,3 +36,8 @@ def activate_measurements( return self.get_interactor(ActivateMeasurementsInteractor).execute( repo_name, owner_name, measurement_type ) + + def encode_secret_string(self, owner: Owner, repo: Repository, value: str): + return self.get_interactor(EncodeSecretStringInteractor).execute( + owner, repo, value + ) diff --git a/graphql_api/tests/test_repository_encoded_secret_string.py b/graphql_api/tests/test_repository_encoded_secret_string.py new file mode 100644 index 0000000000..fd3180433f --- /dev/null +++ b/graphql_api/tests/test_repository_encoded_secret_string.py @@ -0,0 +1,40 @@ +from django.test import TransactionTestCase +from shared.encryption.yaml_secret import yaml_secret_encryptor + +from codecov_auth.tests.factories import OwnerFactory +from core.tests.factories import RepositoryFactory + +from .helper import GraphQLTestHelper + + +class TestEncodedString(TransactionTestCase, GraphQLTestHelper): + def _request(self, variables=None): + query = f""" + query EncodedSecretString($value: String!) {{ + owner(username: "{self.org.username}") {{ + repository(name: "{self.repo.name}") {{ + ... on Repository {{ + encodedSecretString(value: $value) {{ + value + }} + }} + }} + }} + }} + """ + data = self.gql_request(query, owner=self.owner, variables=variables) + return data["owner"]["repository"]["encodedSecretString"]["value"] + + def setUp(self): + self.org = OwnerFactory(username="test-org") + self.repo = RepositoryFactory( + name="test-repo", + author=self.org, + private=True, + ) + self.owner = OwnerFactory(permission=[self.repo.pk]) + + def test_encoded_secret_string(self): + res = self._request(variables={"value": "token-1"}) + check_encryptor = yaml_secret_encryptor + assert "token-1" in check_encryptor.decode(res[7:]) diff --git a/graphql_api/types/repository/repository.graphql b/graphql_api/types/repository/repository.graphql index 945c4d687f..c4393ab7a5 100644 --- a/graphql_api/types/repository/repository.graphql +++ b/graphql_api/types/repository/repository.graphql @@ -81,6 +81,7 @@ type Repository { orderingDirection: OrderingDirection ): [ComponentMeasurements!]! componentsYaml(termId: String): [ComponentsYaml]! + encodedSecretString(value: String!): EncodedSecretString! } type PullConnection { @@ -116,4 +117,8 @@ type BranchEdge { node: Branch! } +type EncodedSecretString { + value: String! +} + union RepositoryResult = Repository | NotFoundError | OwnerNotActivatedError diff --git a/graphql_api/types/repository/repository.py b/graphql_api/types/repository/repository.py index a71dfb75d5..878276b129 100644 --- a/graphql_api/types/repository/repository.py +++ b/graphql_api/types/repository/repository.py @@ -1,5 +1,5 @@ -from datetime import datetime, timedelta -from typing import Any, Iterable, List, Mapping, Optional +from datetime import datetime +from typing import Iterable, List, Mapping, Optional import yaml from ariadne import ObjectType, UnionType, convert_kwargs_to_snake_case @@ -529,3 +529,13 @@ def resolve_is_first_pull_request(repository: Repository, info) -> bool: return not first_pr.compared_to return False + + +@repository_bindable.field("encodedSecretString") +@sync_to_async +def resolve_encoded_secret_string( + repository: Repository, info: GraphQLResolveInfo, value: str +) -> dict[str, str]: + command = info.context["executor"].get_command("repository") + owner = info.context["request"].current_owner + return {"value": command.encode_secret_string(owner, repository, value)} From e1af7126487c27bd15f05245657d2b8d7a922de8 Mon Sep 17 00:00:00 2001 From: ajay-sentry <159853603+ajay-sentry@users.noreply.github.com> Date: Wed, 22 May 2024 12:15:17 -0700 Subject: [PATCH 10/11] dev: Enable most F rules for Ruff (#574) --- codecov/settings_base.py | 3 ++- .../pull/interactors/tests/test_fetch_pull_requests.py | 1 - graphql_api/tests/test_owner.py | 2 +- graphql_api/tests/test_owner_measurements.py | 1 - graphql_api/tests/test_plan.py | 1 - graphql_api/tests/test_plan_representation.py | 1 - graphql_api/tests/test_repository_measurements.py | 1 - graphql_api/types/__init__.py | 2 +- graphql_api/types/commit/commit.py | 2 +- graphql_api/types/mutation/mutation.py | 2 -- ruff.toml | 4 ++-- services/yaml.py | 1 - timeseries/tests/test_helpers.py | 1 - upload/tests/test_upload.py | 2 -- 14 files changed, 7 insertions(+), 17 deletions(-) diff --git a/codecov/settings_base.py b/codecov/settings_base.py index cea8a0659e..bf6ae419e4 100644 --- a/codecov/settings_base.py +++ b/codecov/settings_base.py @@ -1,6 +1,7 @@ import os from urllib.parse import urlparse +import django_prometheus import sentry_sdk from corsheaders.defaults import default_headers from sentry_sdk.integrations.celery import CeleryIntegration @@ -252,7 +253,7 @@ } # See https://django-postgres-extra.readthedocs.io/en/master/settings.html -POSTGRES_EXTRA_DB_BACKEND_BASE: "django_prometheus.db.backends.postgresql" +POSTGRES_EXTRA_DB_BACKEND_BASE: "django_prometheus.db.backends.postgresql" # type: ignore # Allows to use the pgpartition command PSQLEXTRA_PARTITIONING_MANAGER = ( diff --git a/core/commands/pull/interactors/tests/test_fetch_pull_requests.py b/core/commands/pull/interactors/tests/test_fetch_pull_requests.py index 89122fcf2b..6e58ab46a8 100644 --- a/core/commands/pull/interactors/tests/test_fetch_pull_requests.py +++ b/core/commands/pull/interactors/tests/test_fetch_pull_requests.py @@ -3,7 +3,6 @@ from django.contrib.auth.models import AnonymousUser from django.test import TransactionTestCase -from api.internal import pull from core.models import PullStates from core.tests.factories import OwnerFactory, PullFactory, RepositoryFactory from reports.tests.factories import UploadFactory diff --git a/graphql_api/tests/test_owner.py b/graphql_api/tests/test_owner.py index 8d70b36e65..995f0a12cc 100644 --- a/graphql_api/tests/test_owner.py +++ b/graphql_api/tests/test_owner.py @@ -16,7 +16,7 @@ OwnerFactory, UserFactory, ) -from core.tests.factories import CommitFactory, OwnerFactory, RepositoryFactory +from core.tests.factories import CommitFactory, RepositoryFactory from plan.constants import PlanName, TrialStatus from reports.tests.factories import CommitReportFactory, UploadFactory diff --git a/graphql_api/tests/test_owner_measurements.py b/graphql_api/tests/test_owner_measurements.py index 14956243ec..ffb08bbf45 100644 --- a/graphql_api/tests/test_owner_measurements.py +++ b/graphql_api/tests/test_owner_measurements.py @@ -2,7 +2,6 @@ from unittest.mock import patch from django.test import TransactionTestCase, override_settings -from django.utils import timezone from codecov_auth.tests.factories import OwnerFactory from core.tests.factories import RepositoryFactory diff --git a/graphql_api/tests/test_plan.py b/graphql_api/tests/test_plan.py index c21544fc70..3a586afa29 100644 --- a/graphql_api/tests/test_plan.py +++ b/graphql_api/tests/test_plan.py @@ -5,7 +5,6 @@ from freezegun import freeze_time from codecov_auth.tests.factories import OwnerFactory -from core.tests.factories import OwnerFactory from plan.constants import PlanName, TrialStatus from .helper import GraphQLTestHelper diff --git a/graphql_api/tests/test_plan_representation.py b/graphql_api/tests/test_plan_representation.py index 2500d2bad0..98a5eaef7c 100644 --- a/graphql_api/tests/test_plan_representation.py +++ b/graphql_api/tests/test_plan_representation.py @@ -5,7 +5,6 @@ from freezegun import freeze_time from codecov_auth.tests.factories import OwnerFactory -from core.tests.factories import OwnerFactory from plan.constants import PlanName, TrialStatus from .helper import GraphQLTestHelper diff --git a/graphql_api/tests/test_repository_measurements.py b/graphql_api/tests/test_repository_measurements.py index 3ecff3e527..315d192648 100644 --- a/graphql_api/tests/test_repository_measurements.py +++ b/graphql_api/tests/test_repository_measurements.py @@ -2,7 +2,6 @@ from unittest.mock import patch from django.test import TransactionTestCase, override_settings -from django.utils import timezone from codecov_auth.tests.factories import OwnerFactory from core.tests.factories import RepositoryFactory diff --git a/graphql_api/types/__init__.py b/graphql_api/types/__init__.py index 8a61aeae48..ee49b82941 100644 --- a/graphql_api/types/__init__.py +++ b/graphql_api/types/__init__.py @@ -24,7 +24,7 @@ from .component_comparison import component_comparison, component_comparison_bindable from .config import config, config_bindable from .coverage_totals import coverage_totals, coverage_totals_bindable -from .enums import enum_types, enums +from .enums import enum_types from .file import commit_file, file_bindable from .flag import flag, flag_bindable from .flag_comparison import flag_comparison, flag_comparison_bindable diff --git a/graphql_api/types/commit/commit.py b/graphql_api/types/commit/commit.py index 0df09601c0..fc00b3ff6f 100644 --- a/graphql_api/types/commit/commit.py +++ b/graphql_api/types/commit/commit.py @@ -31,7 +31,7 @@ MissingHeadReport, ) from graphql_api.types.enums import OrderingDirection, PathContentDisplayType -from graphql_api.types.errors import MissingCoverage, MissingHeadReport, UnknownPath +from graphql_api.types.errors import MissingCoverage, UnknownPath from graphql_api.types.errors.errors import UnknownFlags from services.bundle_analysis import BundleAnalysisComparison, BundleAnalysisReport from services.comparison import Comparison, ComparisonReport diff --git a/graphql_api/types/mutation/mutation.py b/graphql_api/types/mutation/mutation.py index 2b2c85380e..f8977ff142 100644 --- a/graphql_api/types/mutation/mutation.py +++ b/graphql_api/types/mutation/mutation.py @@ -1,7 +1,5 @@ from ariadne import MutationType -from graphql_api.types.mutation.start_trial.start_trial import resolve_start_trial - from .activate_measurements import ( error_activate_measurements, resolve_activate_measurements, diff --git a/ruff.toml b/ruff.toml index b11c13ad5e..96b5da4538 100644 --- a/ruff.toml +++ b/ruff.toml @@ -37,8 +37,8 @@ target-version = "py312" [lint] # Currently only enabled for F541 and I: https://docs.astral.sh/ruff/rules/ -select = ["F541", "I"] -ignore = [] +select = ["F", "I"] +ignore = ["F841", "F401", "F405", "F403"] # Allow fix for all enabled rules (when `--fix`) is provided. # The preferred method (for now) w.r.t. fixable rules is to manually update the makefile diff --git a/services/yaml.py b/services/yaml.py index 8ffad7f6f2..3862f6727e 100644 --- a/services/yaml.py +++ b/services/yaml.py @@ -4,7 +4,6 @@ from asgiref.sync import async_to_sync from shared.yaml import UserYaml, fetch_current_yaml_from_provider_via_reference -from shared.yaml.user_yaml import UserYaml from shared.yaml.validation import validate_yaml from yaml import safe_load diff --git a/timeseries/tests/test_helpers.py b/timeseries/tests/test_helpers.py index 804080f8c7..66509cac1c 100644 --- a/timeseries/tests/test_helpers.py +++ b/timeseries/tests/test_helpers.py @@ -4,7 +4,6 @@ import pytest from django.conf import settings from django.test import TransactionTestCase -from django.utils import timezone from freezegun import freeze_time from freezegun.api import FakeDatetime from shared.reports.resources import Report, ReportFile, ReportLine diff --git a/upload/tests/test_upload.py b/upload/tests/test_upload.py index 933ea423f2..9edadcdae3 100644 --- a/upload/tests/test_upload.py +++ b/upload/tests/test_upload.py @@ -1065,7 +1065,6 @@ async def get_commit(self, commit, token): "reportid": "dec1f00b-1883-40d0-afd6-6dcb876510be", "redis_key": "upload/b521e55/dec1f00b-1883-40d0-afd6-6dcb876510be/plain", "url": None, - "branch": None, "job": None, } @@ -1143,7 +1142,6 @@ async def get_commit(self, commit, token): "reportid": "dec1f00b-1883-40d0-afd6-6dcb876510be", "redis_key": "upload/b521e55/dec1f00b-1883-40d0-afd6-6dcb876510be/plain", "url": None, - "branch": None, "job": None, } From ad629f2a03f0c6aa18e96a43c4c5338a2e758893 Mon Sep 17 00:00:00 2001 From: Rohit Vinnakota <148245014+rohitvinnakota-codecov@users.noreply.github.com> Date: Wed, 22 May 2024 14:24:41 -0600 Subject: [PATCH 11/11] Add update self hosted settings mutation (#559) --- .../tests/test_update_self_hosted_settings.py | 61 ++++++++++++++++ .../update_self_hosted_settings.py | 38 ++++++++++ codecov_auth/commands/owner/owner.py | 4 ++ .../test_update_self_hosted_settings.py | 70 +++++++++++++++++++ graphql_api/tests/test_config.py | 20 ++++++ graphql_api/types/config/config.graphql | 1 + graphql_api/types/config/config.py | 11 ++- graphql_api/types/mutation/__init__.py | 2 + graphql_api/types/mutation/mutation.graphql | 1 + graphql_api/types/mutation/mutation.py | 7 ++ .../update_self_hosted_settings/__init__.py | 10 +++ .../update_self_hosted_settings.graphql | 9 +++ .../update_self_hosted_settings.py | 19 +++++ 13 files changed, 252 insertions(+), 1 deletion(-) create mode 100644 codecov_auth/commands/owner/interactors/tests/test_update_self_hosted_settings.py create mode 100644 codecov_auth/commands/owner/interactors/update_self_hosted_settings.py create mode 100644 graphql_api/tests/mutation/test_update_self_hosted_settings.py create mode 100644 graphql_api/types/mutation/update_self_hosted_settings/__init__.py create mode 100644 graphql_api/types/mutation/update_self_hosted_settings/update_self_hosted_settings.graphql create mode 100644 graphql_api/types/mutation/update_self_hosted_settings/update_self_hosted_settings.py diff --git a/codecov_auth/commands/owner/interactors/tests/test_update_self_hosted_settings.py b/codecov_auth/commands/owner/interactors/tests/test_update_self_hosted_settings.py new file mode 100644 index 0000000000..436c8ebf15 --- /dev/null +++ b/codecov_auth/commands/owner/interactors/tests/test_update_self_hosted_settings.py @@ -0,0 +1,61 @@ +from unittest.mock import patch + +import pytest +from asgiref.sync import async_to_sync +from django.contrib.auth.models import AnonymousUser +from django.test import TransactionTestCase, override_settings + +from codecov.commands.exceptions import Unauthenticated, ValidationError +from codecov_auth.commands.owner.interactors.update_self_hosted_settings import ( + UpdateSelfHostedSettingsInteractor, +) +from codecov_auth.tests.factories import OwnerFactory + + +class UpdateSelfHostedSettingsInteractorTest(TransactionTestCase): + @async_to_sync + def execute( + self, + current_user, + input={ + "shouldAutoActivate": None, + }, + ): + return UpdateSelfHostedSettingsInteractor(None, "github", current_user).execute( + input=input, + ) + + @override_settings(IS_ENTERPRISE=True) + def test_update_self_hosted_settings_when_auto_activate_is_true(self): + owner = OwnerFactory(plan_auto_activate=False) + self.execute(current_user=owner, input={"shouldAutoActivate": True}) + owner.refresh_from_db() + assert owner.plan_auto_activate == True + + @override_settings(IS_ENTERPRISE=True) + def test_update_self_hosted_settings_when_auto_activate_is_false(self): + owner = OwnerFactory(plan_auto_activate=True) + self.execute(current_user=owner, input={"shouldAutoActivate": False}) + owner.refresh_from_db() + assert owner.plan_auto_activate == False + + @override_settings(IS_ENTERPRISE=False) + def test_validation_error_when_not_self_hosted_instance(self): + owner = OwnerFactory(plan_auto_activate=True) + with pytest.raises(ValidationError): + self.execute( + current_user=owner, + input={ + "shouldAutoActivate": False, + }, + ) + + @override_settings(IS_ENTERPRISE=True) + def test_user_is_not_authenticated(self): + with pytest.raises(Unauthenticated) as e: + self.execute( + current_user=AnonymousUser(), + input={ + "shouldAutoActivate": False, + }, + ) diff --git a/codecov_auth/commands/owner/interactors/update_self_hosted_settings.py b/codecov_auth/commands/owner/interactors/update_self_hosted_settings.py new file mode 100644 index 0000000000..080cfe6d72 --- /dev/null +++ b/codecov_auth/commands/owner/interactors/update_self_hosted_settings.py @@ -0,0 +1,38 @@ +from dataclasses import dataclass + +from django.conf import settings + +import services.self_hosted as self_hosted +from codecov.commands.base import BaseInteractor +from codecov.commands.exceptions import Unauthenticated, ValidationError +from codecov.db import sync_to_async +from services.refresh import RefreshService + + +@dataclass +class UpdateSelfHostedSettingsInput: + auto_activate_members: bool = False + + +class UpdateSelfHostedSettingsInteractor(BaseInteractor): + def validate(self) -> None: + if not self.current_user.is_authenticated: + raise Unauthenticated() + + if not settings.IS_ENTERPRISE: + raise ValidationError( + "enable_autoactivation and disable_autoactivation are only available in self-hosted environments" + ) + + @sync_to_async + def execute(self, input: UpdateSelfHostedSettingsInput) -> None: + self.validate() + typed_input = UpdateSelfHostedSettingsInput( + auto_activate_members=input.get("shouldAutoActivate"), + ) + + should_auto_activate = typed_input.auto_activate_members + if should_auto_activate: + self_hosted.enable_autoactivation() + else: + self_hosted.disable_autoactivation() diff --git a/codecov_auth/commands/owner/owner.py b/codecov_auth/commands/owner/owner.py index 4c8bac2f26..af866ddcb1 100644 --- a/codecov_auth/commands/owner/owner.py +++ b/codecov_auth/commands/owner/owner.py @@ -18,6 +18,7 @@ from .interactors.trigger_sync import TriggerSyncInteractor from .interactors.update_default_organization import UpdateDefaultOrganizationInteractor from .interactors.update_profile import UpdateProfileInteractor +from .interactors.update_self_hosted_settings import UpdateSelfHostedSettingsInteractor class OwnerCommands(BaseCommand): @@ -82,3 +83,6 @@ def cancel_trial(self, org_username: str) -> None: return self.get_interactor(CancelTrialInteractor).execute( org_username=org_username ) + + def update_self_hosted_settings(self, input) -> None: + return self.get_interactor(UpdateSelfHostedSettingsInteractor).execute(input) diff --git a/graphql_api/tests/mutation/test_update_self_hosted_settings.py b/graphql_api/tests/mutation/test_update_self_hosted_settings.py new file mode 100644 index 0000000000..065b2fe32a --- /dev/null +++ b/graphql_api/tests/mutation/test_update_self_hosted_settings.py @@ -0,0 +1,70 @@ +import pytest +from django.test import TransactionTestCase, override_settings + +from codecov.commands.exceptions import ValidationError +from codecov_auth.tests.factories import OwnerFactory +from graphql_api.tests.helper import GraphQLTestHelper + +query = """ + mutation($input: UpdateSelfHostedSettingsInput!) { + updateSelfHostedSettings(input: $input) { + error { + __typename + ... on ResolverError { + message + } + } + } + } +""" + + +class UpdateSelfHostedSettingsTest(GraphQLTestHelper, TransactionTestCase): + def _request(self, owner=None): + return self.gql_request( + query, + variables={"input": {"shouldAutoActivate": True}}, + owner=owner, + ) + + def _request_deactivate(self, owner=None): + return self.gql_request( + query, + variables={"input": {"shouldAutoActivate": False}}, + owner=owner, + ) + + @override_settings(IS_ENTERPRISE=True) + def test_unauthenticated(self): + assert self._request() == { + "updateSelfHostedSettings": { + "error": { + "__typename": "UnauthenticatedError", + "message": "You are not authenticated", + } + } + } + + @override_settings(IS_ENTERPRISE=True) + def test_authenticated_enable_autoactivation(self): + owner = OwnerFactory() + assert self._request(owner=owner) == {"updateSelfHostedSettings": None} + + @override_settings(IS_ENTERPRISE=True) + def test_authenticate_disable_autoactivation(self): + owner = OwnerFactory() + assert self._request_deactivate(owner=owner) == { + "updateSelfHostedSettings": None + } + + @override_settings(IS_ENTERPRISE=False) + def test_invalid_settings(self): + owner = OwnerFactory() + assert self._request(owner=owner) == { + "updateSelfHostedSettings": { + "error": { + "__typename": "ValidationError", + "message": "enable_autoactivation and disable_autoactivation are only available in self-hosted environments", + } + } + } diff --git a/graphql_api/tests/test_config.py b/graphql_api/tests/test_config.py index 21beddb89b..f0ca6cbf57 100644 --- a/graphql_api/tests/test_config.py +++ b/graphql_api/tests/test_config.py @@ -68,6 +68,26 @@ def test_seats_used_self_hosted(self, activated_owners): }, } + def test_plan_auto_activate(self): + data = self.gql_request("query { config { planAutoActivate }}") + assert data == { + "config": { + "planAutoActivate": None, + }, + } + + @override_settings(IS_ENTERPRISE=True) + @patch("services.self_hosted.is_autoactivation_enabled") + def test_plan_auto_activate_self_hosted(self, is_autoactivation_enabled): + is_autoactivation_enabled.return_value = True + + data = self.gql_request("query { config { planAutoActivate }}") + assert data == { + "config": { + "planAutoActivate": True, + }, + } + def test_seats_limit(self): data = self.gql_request("query { config { seatsLimit }}") assert data == { diff --git a/graphql_api/types/config/config.graphql b/graphql_api/types/config/config.graphql index a07badf73e..e4d393f33b 100644 --- a/graphql_api/types/config/config.graphql +++ b/graphql_api/types/config/config.graphql @@ -1,5 +1,6 @@ type Config { loginProviders: [LoginProvider!]! + planAutoActivate: Boolean seatsUsed: Int seatsLimit: Int isTimescaleEnabled: Boolean! diff --git a/graphql_api/types/config/config.py b/graphql_api/types/config/config.py index e11ba7ef34..a743f025ca 100644 --- a/graphql_api/types/config/config.py +++ b/graphql_api/types/config/config.py @@ -1,8 +1,9 @@ -from typing import List +from typing import List, Optional from ariadne import ObjectType from distutils.util import strtobool from django.conf import settings +from graphql.type.definition import GraphQLResolveInfo import services.self_hosted as self_hosted from codecov.db import sync_to_async @@ -65,6 +66,14 @@ def resolve_sync_providers(_, info) -> List[str]: return sync_providers +@config_bindable.field("planAutoActivate") +def resolve_plan_auto_activate(_, info: GraphQLResolveInfo) -> Optional[bool]: + if not settings.IS_ENTERPRISE: + return None + + return self_hosted.is_autoactivation_enabled() + + @config_bindable.field("seatsUsed") @sync_to_async def resolve_seats_used(_, info): diff --git a/graphql_api/types/mutation/__init__.py b/graphql_api/types/mutation/__init__.py index 91803bb6b0..f80c070257 100644 --- a/graphql_api/types/mutation/__init__.py +++ b/graphql_api/types/mutation/__init__.py @@ -19,6 +19,7 @@ from .sync_with_git_provider import gql_sync_with_git_provider from .update_default_organization import gql_update_default_organization from .update_profile import gql_update_profile +from .update_self_hosted_settings import gql_update_self_hosted_settings mutation = ariadne_load_local_graphql(__file__, "mutation.graphql") mutation = mutation + gql_create_api_token @@ -39,3 +40,4 @@ mutation = mutation + gql_start_trial mutation = mutation + gql_cancel_trial mutation = mutation + gql_delete_component_measurements +mutation = mutation + gql_update_self_hosted_settings diff --git a/graphql_api/types/mutation/mutation.graphql b/graphql_api/types/mutation/mutation.graphql index 8269eafc45..fe48f67a9b 100644 --- a/graphql_api/types/mutation/mutation.graphql +++ b/graphql_api/types/mutation/mutation.graphql @@ -25,4 +25,5 @@ type Mutation { saveSentryState(input: SaveSentryStateInput!): SaveSentryStatePayload saveTermsAgreement(input: SaveTermsAgreementInput!): SaveTermsAgreementPayload deleteComponentMeasurements(input: DeleteComponentMeasurementsInput!): DeleteComponentMeasurementsPayload + updateSelfHostedSettings(input: UpdateSelfHostedSettingsInput!): UpdateSelfHostedSettingsPayload } diff --git a/graphql_api/types/mutation/mutation.py b/graphql_api/types/mutation/mutation.py index f8977ff142..ba86474534 100644 --- a/graphql_api/types/mutation/mutation.py +++ b/graphql_api/types/mutation/mutation.py @@ -39,6 +39,10 @@ resolve_update_default_organization, ) from .update_profile import error_update_profile, resolve_update_profile +from .update_self_hosted_settings import ( + error_update_self_hosted_settings, + resolve_update_self_hosted_settings, +) mutation_bindable = MutationType() @@ -67,6 +71,8 @@ mutation_bindable.field("deleteComponentMeasurements")( resolve_delete_component_measurements ) +mutation_bindable.field("updateSelfHostedSettings")(resolve_update_self_hosted_settings) + mutation_resolvers = [ mutation_bindable, @@ -88,4 +94,5 @@ error_save_terms_agreement, error_start_trial, error_cancel_trial, + error_update_self_hosted_settings, ] diff --git a/graphql_api/types/mutation/update_self_hosted_settings/__init__.py b/graphql_api/types/mutation/update_self_hosted_settings/__init__.py new file mode 100644 index 0000000000..c0cd5030f9 --- /dev/null +++ b/graphql_api/types/mutation/update_self_hosted_settings/__init__.py @@ -0,0 +1,10 @@ +from graphql_api.helpers.ariadne import ariadne_load_local_graphql + +from .update_self_hosted_settings import ( + error_update_self_hosted_settings, + resolve_update_self_hosted_settings, +) + +gql_update_self_hosted_settings = ariadne_load_local_graphql( + __file__, "update_self_hosted_settings.graphql" +) diff --git a/graphql_api/types/mutation/update_self_hosted_settings/update_self_hosted_settings.graphql b/graphql_api/types/mutation/update_self_hosted_settings/update_self_hosted_settings.graphql new file mode 100644 index 0000000000..e184f904b1 --- /dev/null +++ b/graphql_api/types/mutation/update_self_hosted_settings/update_self_hosted_settings.graphql @@ -0,0 +1,9 @@ +union UpdateSelfHostedSettingsError = UnauthenticatedError | ValidationError + +type UpdateSelfHostedSettingsPayload { + error: UpdateSelfHostedSettingsError +} + +input UpdateSelfHostedSettingsInput { + shouldAutoActivate: Boolean! +} diff --git a/graphql_api/types/mutation/update_self_hosted_settings/update_self_hosted_settings.py b/graphql_api/types/mutation/update_self_hosted_settings/update_self_hosted_settings.py new file mode 100644 index 0000000000..eaf23404b4 --- /dev/null +++ b/graphql_api/types/mutation/update_self_hosted_settings/update_self_hosted_settings.py @@ -0,0 +1,19 @@ +from ariadne import UnionType + +from codecov_auth.commands.owner import OwnerCommands +from graphql_api.helpers.mutation import ( + require_authenticated, + resolve_union_error_type, + wrap_error_handling_mutation, +) + + +@wrap_error_handling_mutation +@require_authenticated +async def resolve_update_self_hosted_settings(_, info, input): + command: OwnerCommands = info.context["executor"].get_command("owner") + return await command.update_self_hosted_settings(input) + + +error_update_self_hosted_settings = UnionType("UpdateSelfHostedSettingsError") +error_update_self_hosted_settings.type_resolver(resolve_union_error_type)