From 7a8e07108d9d1268658182f54a4002de5f44e9f9 Mon Sep 17 00:00:00 2001 From: Yuhuai Liu Date: Fri, 7 Feb 2025 20:40:03 -0500 Subject: [PATCH 1/4] enable sharing sessions between GV and OSF --- addon_service/authentication.py | 1 - addon_service/common/osf.py | 7 +------ app/env.py | 3 +++ app/middleware.py | 35 +++++++++++++++++++++++++++++++++ app/settings.py | 22 +++++++++++++++++++-- poetry.lock | 30 +++++++++++++++++++++++++++- pyproject.toml | 2 ++ 7 files changed, 90 insertions(+), 10 deletions(-) create mode 100644 app/middleware.py diff --git a/addon_service/authentication.py b/addon_service/authentication.py index 82d2cbb6..e9dbe28d 100644 --- a/addon_service/authentication.py +++ b/addon_service/authentication.py @@ -12,7 +12,6 @@ def authenticate(self, request: DrfRequest): _user_uri = osf.get_osf_user_uri(request) if _user_uri: UserReference.objects.get_or_create(user_uri=_user_uri) - request.session["user_reference_uri"] = _user_uri return True, None return None # unauthenticated diff --git a/addon_service/common/osf.py b/addon_service/common/osf.py index 10a7582b..c9fe319f 100644 --- a/addon_service/common/osf.py +++ b/addon_service/common/osf.py @@ -58,12 +58,7 @@ async def get_osf_user_uri(request: django_http.HttpRequest) -> str | None: _auth_headers = _get_osf_auth_headers(request) if not _auth_headers: return None - _client = await get_singleton_client_session() - async with _client.get(_osfapi_me_url(), headers=_auth_headers) as _response: - if HTTPStatus(_response.status).is_client_error: - return None - _response_content = await _response.json() - return _iri_from_osfapi_resource(_response_content["data"]) + return request.session["user_reference_uri"] @async_to_sync diff --git a/app/env.py b/app/env.py index 25d60e49..f7e417f4 100644 --- a/app/env.py +++ b/app/env.py @@ -36,6 +36,8 @@ OSFDB_CONN_MAX_AGE = os.environ.get("OSFDB_CONN_MAX_AGE", 0) OSFDB_SSLMODE = os.environ.get("OSFDB_SSLMODE", "prefer") +REDIS_HOST = os.environ.get("REDIS_HOST", "redis://192.168.168.167:6379") + ### # for interacting with osf @@ -46,6 +48,7 @@ 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_SECRET = os.environ.get("OSF_AUTH_COOKIE_SECRET", "CHANGEME") ### # amqp/celery diff --git a/app/middleware.py b/app/middleware.py new file mode 100644 index 00000000..927a868d --- /dev/null +++ b/app/middleware.py @@ -0,0 +1,35 @@ +from importlib import import_module + +import itsdangerous +from django.conf import settings +from django.contrib.sessions.middleware import SessionMiddleware + + +SessionStore = import_module(settings.SESSION_ENGINE).SessionStore + + +def ensure_str(value): + if isinstance(value, bytes): + return value.decode() + return 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 + """ + + def process_request(self, request): + cookie = request.COOKIES.get(settings.SESSION_COOKIE_NAME) + if cookie: + try: + session_key = ensure_str( + itsdangerous.Signer(settings.OSF_AUTH_COOKIE_SECRET).unsign(cookie) + ) + except itsdangerous.BadSignature: + return None + request.session = SessionStore(session_key=session_key) + else: + request.session = SessionStore() diff --git a/app/settings.py b/app/settings.py index 0241d565..f4c85a83 100644 --- a/app/settings.py +++ b/app/settings.py @@ -65,7 +65,24 @@ OSF_BASE_URL = env.OSF_BASE_URL.rstrip("/") OSF_API_BASE_URL = env.OSF_API_BASE_URL.rstrip("/") ALLOWED_RESOURCE_URI_PREFIXES = {OSF_BASE_URL} -SESSION_COOKIE_DOMAIN = env.SESSION_COOKIE_DOMAIN +# SESSION_COOKIE_DOMAIN = env.SESSION_COOKIE_DOMAIN +SESSION_ENGINE = "django.contrib.sessions.backends.cache" +SESSION_COOKIE_NAME = env.OSF_AUTH_COOKIE_NAME +OSF_AUTH_COOKIE_SECRET = env.OSF_AUTH_COOKIE_SECRET +REDIS_HOST = env.REDIS_HOST + +CACHES = { + "default": { + "BACKEND": "django.core.cache.backends.redis.RedisCache", + "LOCATION": REDIS_HOST, + # 'OPTIONS': { + # 'CLIENT_CLASS': 'django_redis.client.DefaultClient', + # 'CONNECTION_POOL_KWARGS': { + # 'max_connections': 100, + # }, + # }, + }, +} if DEBUG: # allow for local osf shenanigans @@ -101,9 +118,10 @@ MIDDLEWARE = [ "corsheaders.middleware.CorsMiddleware", "django.middleware.security.SecurityMiddleware", - "django.contrib.sessions.middleware.SessionMiddleware", "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", "django.middleware.clickjacking.XFrameOptionsMiddleware", diff --git a/poetry.lock b/poetry.lock index f35a593a..b0020063 100644 --- a/poetry.lock +++ b/poetry.lock @@ -1138,6 +1138,18 @@ files = [ [package.extras] colors = ["colorama (>=0.4.6)"] +[[package]] +name = "itsdangerous" +version = "2.2.0" +description = "Safely pass data to untrusted environments and back." +optional = false +python-versions = ">=3.8" +groups = ["main"] +files = [ + {file = "itsdangerous-2.2.0-py3-none-any.whl", hash = "sha256:c6242fc49e35958c8b15141343aa660db5fc54d4f13a1db01a3f5891b98700ef"}, + {file = "itsdangerous-2.2.0.tar.gz", hash = "sha256:e0050c0b7da1eea53ffaf149c0cfbb5c6e2e2b69c4bef22c81fa6eb73e5f6173"}, +] + [[package]] name = "jinja2" version = "3.1.4" @@ -1803,6 +1815,22 @@ files = [ {file = "pyyaml-6.0.2.tar.gz", hash = "sha256:d584d9ec91ad65861cc08d42e834324ef890a082e591037abe114850ff7bbc3e"}, ] +[[package]] +name = "redis" +version = "5.2.1" +description = "Python client for Redis database and key-value store" +optional = false +python-versions = ">=3.8" +groups = ["main"] +files = [ + {file = "redis-5.2.1-py3-none-any.whl", hash = "sha256:ee7e1056b9aea0f04c6c2ed59452947f34c4940ee025f5dd83e6a6418b6989e4"}, + {file = "redis-5.2.1.tar.gz", hash = "sha256:16f2e22dff21d5125e8481515e386711a34cbec50f0e44413dd7d9c060a54e0f"}, +] + +[package.extras] +hiredis = ["hiredis (>=3.0.0)"] +ocsp = ["cryptography (>=36.0.1)", "pyopenssl (==23.2.1)", "requests (>=2.31.0)"] + [[package]] name = "referencing" version = "0.35.1" @@ -2437,4 +2465,4 @@ testing = ["coverage (>=5.0.3)", "zope.event", "zope.testing"] [metadata] lock-version = "2.1" python-versions = "^3.12" -content-hash = "0a5d1b9ae431553e15534618f985ea8f476c492bbf617b5be6b0c1fde45b5d0c" +content-hash = "9f3d39b29f00131619e3d005ac8687d5c6189ef2b3e95dc7c3adb18714202b27" diff --git a/pyproject.toml b/pyproject.toml index a4181f49..c2e03fc7 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -26,6 +26,8 @@ boa-api = "^0.1.14" django-silk = "^5.3.2" django-celery-beat = "^2.7.0" tqdm = "^4.67.1" +itsdangerous = "^2.2.0" +redis = "^5.2.1" [tool.poetry.group.dev.dependencies] From a8476313695f60441b5ee2fd454cbc50a0651cc7 Mon Sep 17 00:00:00 2001 From: Oleh Paduchak Date: Mon, 10 Feb 2025 14:47:20 +0200 Subject: [PATCH 2/4] fixed shared session issues and hmac auth --- addon_service/common/osf.py | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/addon_service/common/osf.py b/addon_service/common/osf.py index c9fe319f..b39008bf 100644 --- a/addon_service/common/osf.py +++ b/addon_service/common/osf.py @@ -48,17 +48,31 @@ def for_capabilities(capabilities: AddonCapabilities) -> "OSFPermission": async def get_osf_user_uri(request: django_http.HttpRequest) -> str | None: """get a uri identifying the user making this request""" try: - return _get_hmac_verified_user_iri(request) + uri = _get_hmac_verified_user_iri(request) + request.session["user_reference_uri"] = uri + return uri except hmac_utils.RejectedHmac as e: _logger.critical(f"rejected hmac signature!?\n\tpath:{request.path}") raise PermissionDenied(e) except hmac_utils.NotUsingHmac: pass # the only acceptable hmac-related error is not using hmac at all # not hmac -- ask osf + + if uri := request.session.get("user_reference_uri"): + return uri + _auth_headers = _get_osf_auth_headers(request) if not _auth_headers: return None - return request.session["user_reference_uri"] + # we must handle the case when old, already created sessions must somehow set user_reference_uri + # TODO: delete after all existing osf sessions have been revoked/expired + _client = await get_singleton_client_session() + async with _client.get(_osfapi_me_url(), headers=_auth_headers) as _response: + if HTTPStatus(_response.status).is_client_error: + return None + _response_content = await _response.json() + uri = _iri_from_osfapi_resource(_response_content["data"]) + request.session["user_reference_uri"] = uri @async_to_sync From d144c8e2ef208b8b78b5c8e1e7b7ddb12bcfbcd5 Mon Sep 17 00:00:00 2001 From: Oleh Paduchak Date: Mon, 10 Feb 2025 17:57:34 +0200 Subject: [PATCH 3/4] fixed tests --- .github/workflows/run_gravyvalet_tests.yml | 4 ++ addon_service/authentication.py | 2 + addon_service/common/osf.py | 7 +-- addon_service/tests/_helpers.py | 4 +- .../test_authorized_storage_account.py | 49 ++++++++++--------- 5 files changed, 36 insertions(+), 30 deletions(-) diff --git a/.github/workflows/run_gravyvalet_tests.yml b/.github/workflows/run_gravyvalet_tests.yml index 180f92e5..60652ea3 100644 --- a/.github/workflows/run_gravyvalet_tests.yml +++ b/.github/workflows/run_gravyvalet_tests.yml @@ -14,6 +14,10 @@ jobs: postgres-version: ['15'] runs-on: ubuntu-latest services: + redis: + image: redis:latest + ports: + - 6379:6379 postgres: image: postgres:${{ matrix.postgres-version }} env: diff --git a/addon_service/authentication.py b/addon_service/authentication.py index e9dbe28d..47525e45 100644 --- a/addon_service/authentication.py +++ b/addon_service/authentication.py @@ -12,6 +12,8 @@ def authenticate(self, request: DrfRequest): _user_uri = osf.get_osf_user_uri(request) if _user_uri: UserReference.objects.get_or_create(user_uri=_user_uri) + if not request.session.get("user_reference_uri"): + request.session["user_reference_uri"] = _user_uri return True, None return None # unauthenticated diff --git a/addon_service/common/osf.py b/addon_service/common/osf.py index b39008bf..3b646e3e 100644 --- a/addon_service/common/osf.py +++ b/addon_service/common/osf.py @@ -48,9 +48,7 @@ def for_capabilities(capabilities: AddonCapabilities) -> "OSFPermission": async def get_osf_user_uri(request: django_http.HttpRequest) -> str | None: """get a uri identifying the user making this request""" try: - uri = _get_hmac_verified_user_iri(request) - request.session["user_reference_uri"] = uri - return uri + return _get_hmac_verified_user_iri(request) except hmac_utils.RejectedHmac as e: _logger.critical(f"rejected hmac signature!?\n\tpath:{request.path}") raise PermissionDenied(e) @@ -71,8 +69,7 @@ async def get_osf_user_uri(request: django_http.HttpRequest) -> str | None: if HTTPStatus(_response.status).is_client_error: return None _response_content = await _response.json() - uri = _iri_from_osfapi_resource(_response_content["data"]) - request.session["user_reference_uri"] = uri + return _iri_from_osfapi_resource(_response_content["data"]) @async_to_sync diff --git a/addon_service/tests/_helpers.py b/addon_service/tests/_helpers.py index 4a036338..b4377923 100644 --- a/addon_service/tests/_helpers.py +++ b/addon_service/tests/_helpers.py @@ -34,7 +34,7 @@ class MockOSF: - _configured_caller_uri: str | None = None + _configured_caller_uri: str | None = "" _permissions: dict[str, dict[str, str | bool]] def __init__(self, permissions=None): @@ -81,7 +81,7 @@ def configure_resource_visibility(self, resource_uri, *, public=True): self._permissions[resource_uri]["public"] = public def _get_assumed_caller(self, cookies=None): - if self._configured_caller_uri: + if self._configured_caller_uri or self._configured_caller_uri is None: return self._configured_caller_uri if cookies is not None: return cookies.get(settings.USER_REFERENCE_COOKIE) diff --git a/addon_service/tests/test_by_type/test_authorized_storage_account.py b/addon_service/tests/test_by_type/test_authorized_storage_account.py index c7c995ca..0e254b52 100644 --- a/addon_service/tests/test_by_type/test_authorized_storage_account.py +++ b/addon_service/tests/test_by_type/test_authorized_storage_account.py @@ -150,6 +150,7 @@ def test_post(self): def test_post__sets_credentials(self): for creds_format in NON_OAUTH_FORMATS: + self.setUp() external_service = _factories.ExternalStorageServiceFactory() external_service.int_credentials_format = creds_format.value external_service.save() @@ -200,29 +201,31 @@ def tet_post__does_not_set_auth_url(self): self.assertNotIn("auth_url", _resp.data) - def test_post__api_base_url__success(self): - for service_type in [ - ServiceTypes.HOSTED, - ServiceTypes.PUBLIC | ServiceTypes.HOSTED, - ]: - with self.subTest(service_type=service_type): - service = _factories.ExternalStorageOAuth2ServiceFactory( - service_type=service_type - ) - _resp = self.client.post( - reverse("authorized-storage-accounts-list"), - _make_post_payload( - external_service=service, api_root="https://api.my.service/" - ), - format="vnd.api+json", - ) - with self.subTest("Creation succeeds"): - self.assertEqual(_resp.status_code, HTTPStatus.CREATED) - with self.subTest("api_base_url set on account"): - account = db.AuthorizedStorageAccount.objects.get( - id=_resp.data["id"] - ) - self.assertTrue(account._api_base_url) + def test_post__api_base_url_hosted__success(self): + self._test_post__api_base_url__success(ServiceTypes.HOSTED) + + def test_post__api_base_url_hybrid__success(self): + self._test_post__api_base_url__success( + ServiceTypes.PUBLIC | ServiceTypes.HOSTED + ) + + def _test_post__api_base_url__success(self, service_type): + with self.subTest(service_type=service_type): + service = _factories.ExternalStorageOAuth2ServiceFactory( + service_type=service_type + ) + _resp = self.client.post( + reverse("authorized-storage-accounts-list"), + _make_post_payload( + external_service=service, api_root="https://api.my.service/" + ), + format="vnd.api+json", + ) + with self.subTest("Creation succeeds"): + self.assertEqual(_resp.status_code, HTTPStatus.CREATED) + with self.subTest("api_base_url set on account"): + account = db.AuthorizedStorageAccount.objects.get(id=_resp.data["id"]) + self.assertTrue(account._api_base_url) def test_post__api_base_url__invalid__required(self): service = _factories.ExternalStorageOAuth2ServiceFactory( From e190fb6d8b1a39d413eedea97c4a84c00f5db6b8 Mon Sep 17 00:00:00 2001 From: Oleh Paduchak Date: Mon, 10 Feb 2025 18:03:40 +0200 Subject: [PATCH 4/4] fixed redis_url --- .github/workflows/run_gravyvalet_tests.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/run_gravyvalet_tests.yml b/.github/workflows/run_gravyvalet_tests.yml index 60652ea3..372428c5 100644 --- a/.github/workflows/run_gravyvalet_tests.yml +++ b/.github/workflows/run_gravyvalet_tests.yml @@ -62,3 +62,4 @@ jobs: POSTGRES_DB: gravyvalettest POSTGRES_USER: postgres SECRET_KEY: oh-so-secret + REDIS_HOST: redis://localhost:6379