Merge branch 'nora/expire-cookies' into staging

Author: nora-shap

.coveragerc

@@ -205,7 +205,7 @@ test_env.install_cli:
 
 test_env.container_prepare:
 	apt-get -y install git build-essential netcat-traditional
-	git config --global --add safe.directory /app || true
+	git config --global --add safe.directory /app/apps/codecov-api || true
 
 test_env.container_check_db:
 	while ! nc -vz postgres 5432; do sleep 1; echo "waiting for postgres"; done

Makefile

@@ -1,3 +1,4 @@
+import datetime
 import logging
 from datetime import timedelta
 from typing import Optional, Sequence
@@ -92,11 +93,13 @@ def impersonate_owner(self, request, queryset):
 
     # this cookie is read by the `ImpersonationMiddleware` and
     # will reset `request.current_owner` to the impersonated owner
+    max_age = datetime.timedelta(minutes=3)
     response.set_cookie(
         "staff_user",
         owner.ownerid,
         domain=settings.COOKIES_DOMAIN,
         samesite=settings.COOKIE_SAME_SITE,
+        max_age=max_age,
     )
     History.log(
         Owner.objects.get(ownerid=owner.ownerid),

codecov_auth/admin.py

@@ -8,7 +8,7 @@ services:
       - redis
       - timescale
     volumes:
-      - ./:/app
+      - ./:/app/apps/codecov-api
       - ./docker/test.yml:/config/codecov.yml
     entrypoint:
       - ./dev.sh

docker-compose.yml

@@ -6,8 +6,8 @@ ARG BERGLAS_VERSION=2.0.6
 FROM us-docker.pkg.dev/berglas/berglas/berglas:$BERGLAS_VERSION as berglas
 
 FROM $REQUIREMENTS_IMAGE as app
-COPY . /app
-WORKDIR /app
+COPY . /app/apps/codecov-api
+WORKDIR /app/apps/codecov-api
 RUN pip install setuptools==71.1.0
 RUN python manage.py collectstatic --no-input
 
@@ -25,27 +25,27 @@ RUN pip uninstall -y typing
 ENV DJANGO_SETTINGS_MODULE="codecov.settings_enterprise"
 # Remove the settings dev and enterprise files.
 # These should *never* make it to enterprise.
-RUN rm /app/codecov/settings_dev.py && \
-    rm /app/codecov/settings_prod.py && \
-    rm /app/codecov/settings_test.py && \
-    rm /app/codecov/settings_staging.py && \
-    rm /app/dev.sh && \
-    rm /app/migrate.sh && \
-    rm /app/prod.sh && \
-    rm /app/staging.sh && \
-    rm /app/production.yml && \
-    rm /app/development.yml
+RUN rm /app/apps/codecov-api/codecov/settings_dev.py && \
+    rm /app/apps/codecov-api/codecov/settings_prod.py && \
+    rm /app/apps/codecov-api/codecov/settings_test.py && \
+    rm /app/apps/codecov-api/codecov/settings_staging.py && \
+    rm /app/apps/codecov-api/dev.sh && \
+    rm /app/apps/codecov-api/migrate.sh && \
+    rm /app/apps/codecov-api/prod.sh && \
+    rm /app/apps/codecov-api/staging.sh && \
+    rm /app/apps/codecov-api/production.yml && \
+    rm /app/apps/codecov-api/development.yml
 # Remove unneeded folders
-RUN rm -rf /app/.github
-RUN rm -rf /app/.circleci
+RUN rm -rf /app/apps/codecov-api/.github
+RUN rm -rf /app/apps/codecov-api/.circleci
 # Create the codecov user to run the container as
 RUN addgroup --system application \
     && adduser --system codecov --ingroup application --home /home/codecov
 RUN mkdir -p /config && chown codecov:application /config
 # copy the enterprise settings module.
-WORKDIR /app
+WORKDIR /app/apps/codecov-api
 RUN chmod +x enterprise.sh && \
-    chown codecov:application /app
+    chown codecov:application /app/apps/codecov-api
 ARG RELEASE_VERSION
 ENV RUN_ENV="ENTERPRISE"
 ENV RELEASE_VERSION=$RELEASE_VERSION

docker/Dockerfile

@@ -252,10 +252,6 @@ def resolve_has_different_number_of_head_and_base_reports(
     if "comparison" not in info.context:
         return False
     comparison: Comparison = info.context["comparison"]
-    try:
-        comparison.validate()
-    except Exception:
-        return False
     return comparison.has_different_number_of_head_and_base_sessions
 
 

graphql_api/types/comparison/comparison.py

@@ -728,14 +728,41 @@ def head_report(self):
             pass
         return report
 
+    @cached_property
+    def head_report_without_applied_diff(self):
+        """
+        This is a variant to the head_report property without having an applied diff.
+        This is created because applying the diff calls the provider, which adds a
+        diff_totals key to the head_report object as well as adjusting the diff_total
+        values in each session. This variant should only be used if you are not using
+        any diff related data, as it saves an unnecessary request to the provider otherwise.
+        """
+        try:
+            report = report_service.build_report_from_commit(self.head_commit)
+        except minio.error.S3Error as e:
+            if e.code == "NoSuchKey":
+                raise MissingComparisonReport("Missing head report")
+            else:
+                raise e
+
+        return report
+
     @cached_property
     def has_different_number_of_head_and_base_sessions(self):
-        log.info("has_different_number_of_head_and_base_sessions - Start")
-        head_sessions = self.head_report.sessions
-        base_sessions = self.base_report.sessions
-        log.info(
-            f"has_different_number_of_head_and_base_sessions - Retrieved sessions - head {len(head_sessions)} / base {len(base_sessions)}"
-        )
+        """
+        This method checks if the head and the base have different number of sessions.
+        It makes use of the head_report_without_applied_diff property instead of the
+        head_report one as it doesn't need diff related data for this computation (see
+        the description of that property above for more context).
+        This method should be replaced with a direct call to the report_uploads table instead,
+        but leaving the implementation the same for now for consistency.
+        """
+        try:
+            head_sessions = self.head_report_without_applied_diff.sessions
+            base_sessions = self.base_report.sessions
+        except Exception:
+            return False
+
         # We're treating this case as false since considering CFF's complicates the logic
         if self._has_cff_sessions(head_sessions) or self._has_cff_sessions(
             base_sessions
@@ -746,12 +773,9 @@ def has_different_number_of_head_and_base_sessions(self):
     # I feel this method should belong to the API Report class, but we're thinking of getting rid of that class soon
     # In truth, this should be in the shared.Report class
     def _has_cff_sessions(self, sessions) -> bool:
-        log.info(f"_has_cff_sessions - sessions count {len(sessions)}")
         for session in sessions.values():
             if session.session_type.value == "carriedforward":
-                log.info("_has_cff_sessions - Found carriedforward")
                 return True
-        log.info("_has_cff_sessions - No carriedforward")
         return False
 
     @property

services/comparison.py

@@ -925,13 +925,17 @@ def test_head_and_base_reports_have_cff_sessions(
         fc = self.comparison.has_different_number_of_head_and_base_sessions
         assert fc == False
 
+    @patch(
+        "services.comparison.Comparison.head_report_without_applied_diff",
+        new_callable=PropertyMock,
+    )
     def test_head_and_base_reports_have_different_number_of_reports(
-        self, base_report_mock, head_report_mock, _
+        self, head_report_no_diff_mock, base_report_mock, head_report_mock, _
     ):
         # Only relevant files keys to the session object
         head_report_sessions = {"0": {"st": "uploaded"}, "1": {"st": "uploaded"}}
         head_report = SerializableReport(sessions=head_report_sessions)
-        head_report_mock.return_value = head_report
+        head_report_no_diff_mock.return_value = head_report
         base_report_sessions = {"0": {"st": "uploaded"}}
         base_report = SerializableReport(sessions=base_report_sessions)
         base_report_mock.return_value = base_report