[ENG-8317] Fix GV session/cookie issue which breaks Zotero/OAuth1 addons

.gitignore

@@ -2,5 +2,9 @@
 db.sqlite3
 __pycache__
 .venv
-
+.idea/
+.DS_Store
+.DS_Store?
+*.swp
+*~
 addon_service/static/gravyvalet_code_docs/

addon_service/tests/test_by_type/test_authorized_link_account.py

@@ -6,6 +6,7 @@
 
 from django.test import TestCase
 from django.urls import reverse
+from itsdangerous import Signer
 from rest_framework.test import APITestCase
 
 from addon_service import models as db
@@ -24,6 +25,7 @@
     AccessTokenCredentials,
     UsernamePasswordCredentials,
 )
+from app import settings
 
 
 MOCK_CREDENTIALS = {
@@ -101,7 +103,9 @@ def setUpTestData(cls):
 
     def setUp(self):
         super().setUp()
-        self.client.cookies["osf"] = self._user.user_uri
+        self.client.cookies[settings.OSF_AUTH_COOKIE_NAME] = (
+            Signer(settings.OSF_AUTH_COOKIE_SECRET).sign(self._user.user_uri).decode()
+        )
         self._mock_osf = MockOSF()
         self._mock_osf.configure_assumed_caller(self._user.user_uri)
         self.enterContext(self._mock_osf.mocking())
@@ -343,7 +347,9 @@ def setUpTestData(cls):
         cls._external_service = _factories.ExternalLinkOAuth2ServiceFactory()
 
     def setUp(self):
-        self.client.cookies["osf"] = self._user.user_uri
+        self.client.cookies[settings.OSF_AUTH_COOKIE_NAME] = (
+            Signer(settings.OSF_AUTH_COOKIE_SECRET).sign(self._user.user_uri).decode()
+        )
 
         self._mock_osf = MockOSF()
         self._mock_osf.configure_assumed_caller(self._user.user_uri)

addon_service/tests/test_by_type/test_configured_link_addon.py

@@ -7,6 +7,7 @@
 from django.core.exceptions import ValidationError
 from django.test import TestCase
 from django.urls import reverse
+from itsdangerous import Signer
 from rest_framework.test import APITestCase
 
 from addon_service import models as db
@@ -19,6 +20,7 @@
 )
 from addon_toolkit import AddonCapabilities
 from addon_toolkit.interfaces.link import SupportedResourceTypes
+from app import settings
 
 
 def mock_target_url(self):
@@ -49,7 +51,9 @@ def setUpTestData(cls):
 
     def setUp(self):
         super().setUp()
-        self.client.cookies["osf"] = self._user.user_uri
+        self.client.cookies[settings.OSF_AUTH_COOKIE_NAME] = (
+            Signer(settings.OSF_AUTH_COOKIE_SECRET).sign(self._user.user_uri).decode()
+        )
         self._mock_osf = MockOSF()
         self._mock_osf.configure_user_role(
             self._user.user_uri, self._resource.resource_uri, "admin"
@@ -255,7 +259,9 @@ def setUpTestData(cls):
 
     def setUp(self):
         super().setUp()
-        self.client.cookies["osf"] = self._user.user_uri
+        self.client.cookies[settings.OSF_AUTH_COOKIE_NAME] = (
+            Signer(settings.OSF_AUTH_COOKIE_SECRET).sign(self._user.user_uri).decode()
+        )
         self._mock_osf = MockOSF()
         self._mock_osf.configure_user_role(
             self._user.user_uri, self._resource.resource_uri, "admin"

addon_service/tests/test_by_type/test_configured_link_addon_null_values.py

@@ -4,6 +4,7 @@
 
 from django.test import TestCase
 from django.urls import reverse
+from itsdangerous import Signer
 from rest_framework.test import APITestCase
 
 from addon_service import models as db
@@ -14,6 +15,7 @@
 from addon_service.tests._helpers import MockOSF
 from addon_toolkit import AddonCapabilities
 from addon_toolkit.interfaces.link import SupportedResourceTypes
+from app import settings
 
 
 def mock_target_url(self):
@@ -68,7 +70,9 @@ def setUpTestData(cls):
 
     def setUp(self):
         super().setUp()
-        self.client.cookies["osf"] = self._user.user_uri
+        self.client.cookies[settings.OSF_AUTH_COOKIE_NAME] = (
+            Signer(settings.OSF_AUTH_COOKIE_SECRET).sign(self._user.user_uri).decode()
+        )
         self._mock_osf = MockOSF()
         self._mock_osf.configure_user_role(
             self._user.user_uri, self._resource.resource_uri, "admin"

addon_service/tests/test_by_type/test_configured_storage_addon.py

@@ -5,6 +5,7 @@
 from django.test import TestCase
 from django.urls import reverse
 from django.utils import timezone
+from itsdangerous import Signer
 from rest_framework.test import APITestCase
 
 from addon_service.common import hmac as hmac_utils
@@ -26,7 +27,9 @@ def set_auth_header(self, auth_type):
             credentials = base64.b64encode(b"admin:password").decode()
             self.client.credentials(HTTP_AUTHORIZATION=f"Basic {credentials}")
         elif auth_type == "session":
-            self.client.cookies[settings.OSF_AUTH_COOKIE_NAME] = "some auth"
+            self.client.cookies[settings.OSF_AUTH_COOKIE_NAME] = (
+                Signer(settings.OSF_AUTH_COOKIE_SECRET).sign("some auth").decode()
+            )
         elif auth_type == "no_auth":
             self.client.cookies.clear()
             self.client.credentials()
@@ -210,7 +213,9 @@ def test_get_waterbutler_credentials(self):
     def test_get_waterbutler_credentials__error__no_headers(self):
         # credentials request requires HMAC-signed headers
         # Cookie + OSF-side permissions will not suffice
-        self.client.cookies[settings.OSF_AUTH_COOKIE_NAME] = "some auth"
+        self.client.cookies[settings.OSF_AUTH_COOKIE_NAME] = (
+            Signer(settings.OSF_AUTH_COOKIE_SECRET).sign("some_auth").decode()
+        )
         _mock_osf = MockOSF()
         _mock_osf.configure_user_role(
             self._user.user_uri, self._configured_storage_addon.resource_uri, "admin"

addon_service/tests/test_by_type/test_enhanced_resource_types.py

@@ -1,13 +1,15 @@
 from http import HTTPStatus
 
 from django.test import TestCase
+from itsdangerous import Signer
 from rest_framework.test import APITestCase
 
 from addon_service.common.credentials_formats import CredentialsFormats
 from addon_service.common.enum_serializers import EnumNameMultipleChoiceField
 from addon_service.tests import _factories
 from addon_service.tests._helpers import MockOSF
 from addon_toolkit.interfaces.link import SupportedResourceTypes
+from app import settings
 
 
 class TestResourceTypesSorting(APITestCase):
@@ -24,7 +26,9 @@ def setUpTestData(cls):
 
     def setUp(self):
         super().setUp()
-        self.client.cookies["osf"] = self._user.user_uri
+        self.client.cookies[settings.OSF_AUTH_COOKIE_NAME] = (
+            Signer(settings.OSF_AUTH_COOKIE_SECRET).sign(self._user.user_uri).decode()
+        )
         self._mock_osf = MockOSF()
         self._mock_osf.configure_assumed_caller(self._user.user_uri)
         self.enterContext(self._mock_osf.mocking())

addon_service/tests/test_by_type/test_null_resource_link_addon.py

@@ -5,13 +5,15 @@
 from django.core.exceptions import ValidationError
 from django.test import TestCase
 from django.urls import reverse
+from itsdangerous import Signer
 from rest_framework.test import APITestCase
 
 from addon_service.configured_addon.link.models import is_supported_resource_type
 from addon_service.configured_addon.link.serializers import VerifiedLinkSerializer
 from addon_service.tests import _factories
 from addon_service.tests._helpers import MockOSF
 from addon_toolkit.interfaces.link import SupportedResourceTypes
+from app import settings
 
 
 def mock_target_url(self):
@@ -238,7 +240,9 @@ def setUpTestData(cls):
 
     def setUp(self):
         super().setUp()
-        self.client.cookies["osf"] = self._user.user_uri
+        self.client.cookies[settings.OSF_AUTH_COOKIE_NAME] = (
+            Signer(settings.OSF_AUTH_COOKIE_SECRET).sign(self._user.user_uri).decode()
+        )
         self._mock_osf = MockOSF()
         self._mock_osf.configure_user_role(
             self._user.user_uri, self._resource.resource_uri, "admin"

addon_service/tests/test_by_type/test_session_sharing.py

@@ -30,7 +30,7 @@ def test_process_request_with_valid_cookie(self):
         signed_cookie = signer.sign(session_key)
 
         request = self.factory.get("/")
-        request.COOKIES = {settings.SESSION_COOKIE_NAME: signed_cookie}
+        request.COOKIES = {settings.SESSION_COOKIE_NAME: signed_cookie.decode()}
 
         result = self.middleware.process_request(request)
 
@@ -73,7 +73,7 @@ def test_session_store_instantiation(self, mock_session_store):
         signed_cookie = signer.sign(session_key)
 
         request = self.factory.get("/")
-        request.COOKIES = {settings.SESSION_COOKIE_NAME: signed_cookie}
+        request.COOKIES = {settings.SESSION_COOKIE_NAME: signed_cookie.decode()}
 
         self.middleware.process_request(request)
 
@@ -119,7 +119,7 @@ def test_api_access_with_shared_session(self):
         signer = itsdangerous.Signer(settings.OSF_AUTH_COOKIE_SECRET)
         signed_cookie = signer.sign(session_key)
 
-        self.client.cookies[settings.OSF_AUTH_COOKIE_NAME] = signed_cookie
+        self.client.cookies[settings.OSF_AUTH_COOKIE_NAME] = signed_cookie.decode()
 
         session_store = SessionStore(session_key=session_key)
         session_store["user_reference_uri"] = self._user.user_uri
@@ -135,7 +135,7 @@ def test_session_persistence_across_requests(self):
         signer = itsdangerous.Signer(settings.OSF_AUTH_COOKIE_SECRET)
         signed_cookie = signer.sign(session_key)
 
-        self.client.cookies[settings.OSF_AUTH_COOKIE_NAME] = signed_cookie
+        self.client.cookies[settings.OSF_AUTH_COOKIE_NAME] = signed_cookie.decode()
 
         session_store = SessionStore(session_key=session_key)
         session_store["user_reference_uri"] = self._user.user_uri
@@ -160,7 +160,7 @@ def test_invalid_session_handling(self):
         signer = itsdangerous.Signer(settings.OSF_AUTH_COOKIE_SECRET)
         signed_cookie = signer.sign(invalid_session_key)
 
-        self.client.cookies[settings.OSF_AUTH_COOKIE_NAME] = signed_cookie
+        self.client.cookies[settings.OSF_AUTH_COOKIE_NAME] = signed_cookie.decode()
 
         url = f"/v1/resource-references/{self._resource.pk}/"
         response = self.client.get(url)
@@ -213,7 +213,7 @@ def test_session_key_encoding(self, mock_ensure_str):
         signed_cookie = signer.sign(session_key)
 
         request = RequestFactory().get("/")
-        request.COOKIES = {settings.SESSION_COOKIE_NAME: signed_cookie}
+        request.COOKIES = {settings.SESSION_COOKIE_NAME: signed_cookie.decode()}
 
         middleware = UnsignCookieSessionMiddleware(Mock())
         middleware.process_request(request)

app/env.py

@@ -46,9 +46,16 @@
 OSF_HMAC_EXPIRATION_SECONDS = int(os.environ.get("OSF_HMAC_EXPIRATION_SECONDS", 110))
 OSF_BASE_URL = os.environ.get("OSF_BASE_URL", "https://osf.example")
 OSF_API_BASE_URL = os.environ.get("OSF_API_BASE_URL", "https://api.osf.example")
-OSF_AUTH_COOKIE_NAME = os.environ.get("OSF_AUTH_COOKIE_NAME", "osf_staging")
+OSF_AUTH_COOKIE_NAME = os.environ.get("OSF_AUTH_COOKIE_NAME", "osf")
 OSF_AUTH_COOKIE_SECRET = os.environ.get("OSF_AUTH_COOKIE_SECRET", "CHANGEME")
-SESSION_COOKIE_DOMAIN = os.environ.get("SESSION_COOKIE_DOMAIN")
+SESSION_COOKIE_DOMAIN = os.environ.get("SESSION_COOKIE_DOMAIN", None)
+SESSION_COOKIE_SECURE = os.environ.get(
+    "SESSION_COOKIE_SECURE", True
+)  # Change to False for local dev
+SESSION_COOKIE_HTTPONLY = os.environ.get("SESSION_COOKIE_HTTPONLY", True)
+SESSION_COOKIE_SAMESITE = os.environ.get(
+    "SESSION_COOKIE_SAMESITE", "None"
+)  # Change to "Lax" for local dev
 
 ###
 # amqp/celery

app/middleware.py

@@ -2,7 +2,10 @@
 
 import itsdangerous
 from django.conf import settings
+from django.contrib.sessions.backends.base import UpdateError
+from django.contrib.sessions.exceptions import SessionInterrupted
 from django.contrib.sessions.middleware import SessionMiddleware
+from django.utils.cache import patch_vary_headers
 
 
 SessionStore = import_module(settings.SESSION_ENGINE).SessionStore
@@ -16,9 +19,8 @@ def ensure_str(value):
 
 class UnsignCookieSessionMiddleware(SessionMiddleware):
     """
-    Overrides the process_request hook of SessionMiddleware
-    to retrieve the session key for finding the correct session
-    by unsigning the cookie value using server secret
+    Overrides the process_request hook of SessionMiddleware to retrieve the session key for finding/setting the
+    correct session by unsigning/signing the cookie value using server secret.
     """
 
     def process_request(self, request):
@@ -29,7 +31,54 @@ def process_request(self, request):
                     itsdangerous.Signer(settings.OSF_AUTH_COOKIE_SECRET).unsign(cookie)
                 )
             except itsdangerous.BadSignature:
+                request.session = SessionStore
                 return None
             request.session = SessionStore(session_key=session_key)
         else:
             request.session = SessionStore()
+
+    def process_response(self, request, response):
+        """
+        If `request.session` was modified, or if the configuration is to save the session every time, save the changes
+        and set a session cookie. This is port from `SessionMiddleware.process_response` with the following changes:
+        1) Sign cookie value using server secret.
+        2) Don't delete cookie.
+        3) Don't set `Max-Age` or `Expires`
+        """
+        try:
+            accessed = request.session.accessed
+            modified = request.session.modified
+            empty = request.session.is_empty()
+        except AttributeError:
+            return response
+
+        if accessed:
+            patch_vary_headers(response, ("Cookie",))
+
+        # GV only accesses or modifies OSF cookie, but does not delete it. OSF handles the creation and deletion.
+        if settings.SESSION_COOKIE_NAME in request.COOKIES and empty:
+            return response
+
+        if (modified or settings.SESSION_SAVE_EVERY_REQUEST) and not empty:
+            if response.status_code < 500:
+                try:
+                    request.session.save()
+                except UpdateError:
+                    raise SessionInterrupted(
+                        "The request's session was deleted before the request completed. "
+                        "The user may have logged out in a concurrent request, for example."
+                    )
+                response.set_cookie(
+                    settings.SESSION_COOKIE_NAME,
+                    ensure_str(
+                        itsdangerous.Signer(settings.OSF_AUTH_COOKIE_SECRET).sign(
+                            request.session.session_key
+                        )
+                    ),
+                    domain=settings.SESSION_COOKIE_DOMAIN,
+                    secure=settings.SESSION_COOKIE_SECURE,
+                    httponly=settings.SESSION_COOKIE_HTTPONLY,
+                    samesite=settings.SESSION_COOKIE_SAMESITE,
+                )
+
+        return response

app/settings.py

@@ -69,6 +69,9 @@
 SESSION_ENGINE = "django.contrib.sessions.backends.cache"
 SESSION_COOKIE_NAME = env.OSF_AUTH_COOKIE_NAME
 SESSION_COOKIE_DOMAIN = env.SESSION_COOKIE_DOMAIN
+SESSION_COOKIE_SECURE = env.SESSION_COOKIE_SECURE
+SESSION_COOKIE_HTTPONLY = env.SESSION_COOKIE_HTTPONLY
+SESSION_COOKIE_SAMESITE = env.SESSION_COOKIE_SAMESITE
 OSF_AUTH_COOKIE_SECRET = env.OSF_AUTH_COOKIE_SECRET
 REDIS_HOST = env.REDIS_HOST
 
@@ -115,7 +118,6 @@
     "django.middleware.security.SecurityMiddleware",
     "django.middleware.common.CommonMiddleware",
     "django.middleware.csrf.CsrfViewMiddleware",
-    "django.contrib.sessions.middleware.SessionMiddleware",
     "app.middleware.UnsignCookieSessionMiddleware",
     "django.contrib.auth.middleware.AuthenticationMiddleware",
     "django.contrib.messages.middleware.MessageMiddleware",