From cbd17336582afdc830a9b6838eb4a3251a134b46 Mon Sep 17 00:00:00 2001 From: Sean Hammond Date: Wed, 22 May 2024 18:04:59 +0100 Subject: [PATCH] Prevent deleted users from authenticating Prevent users who've been marked as deleted (but still exist in the DB, for now) from authenticating with auth clients, cookies, `HTTP_X_FORWARDED_USER` headers, or bearer tokens. --- h/security/policy/_basic_http_auth.py | 2 +- h/security/policy/_cookie.py | 2 +- h/security/policy/_remote_user.py | 2 +- h/security/policy/bearer_token.py | 2 +- tests/common/fixtures/services.py | 8 ++++++-- tests/unit/h/security/policy/_basic_http_auth_test.py | 7 +++++++ tests/unit/h/security/policy/_cookie_test.py | 7 +++++++ tests/unit/h/security/policy/_remote_user_test.py | 8 ++++++++ tests/unit/h/security/policy/bearer_token_test.py | 7 +++++++ 9 files changed, 39 insertions(+), 6 deletions(-) diff --git a/h/security/policy/_basic_http_auth.py b/h/security/policy/_basic_http_auth.py index d504bf4a70d..d8fa1bb0e0b 100644 --- a/h/security/policy/_basic_http_auth.py +++ b/h/security/policy/_basic_http_auth.py @@ -76,7 +76,7 @@ def identity(self, request): return None # If you forward a user it must exist and match your authority - if not user or user.authority != auth_client.authority: + if (not user) or user.deleted or (user.authority != auth_client.authority): return None return Identity.from_models(auth_client=auth_client, user=user) diff --git a/h/security/policy/_cookie.py b/h/security/policy/_cookie.py index a805b409388..564420c540a 100644 --- a/h/security/policy/_cookie.py +++ b/h/security/policy/_cookie.py @@ -17,7 +17,7 @@ def identity(self, request): self._add_vary_by_cookie(request) user = request.find_service(AuthCookieService).verify_cookie() - if not user: + if (not user) or user.deleted: return None return Identity.from_models(user=user) diff --git a/h/security/policy/_remote_user.py b/h/security/policy/_remote_user.py index 0ff7e90d372..4366f89454b 100644 --- a/h/security/policy/_remote_user.py +++ b/h/security/policy/_remote_user.py @@ -16,7 +16,7 @@ def identity(self, request): return None user = request.find_service(name="user").fetch(user_id) - if user is None: + if user is None or user.deleted: return None return Identity.from_models(user=user) diff --git a/h/security/policy/bearer_token.py b/h/security/policy/bearer_token.py index 1240203d29d..5ae0c38ac67 100644 --- a/h/security/policy/bearer_token.py +++ b/h/security/policy/bearer_token.py @@ -57,7 +57,7 @@ def _load_identity(self, request): return None user = request.find_service(name="user").fetch(token.userid) - if user is None: + if user is None or user.deleted: return None return Identity.from_models(user=user) diff --git a/tests/common/fixtures/services.py b/tests/common/fixtures/services.py index 52557684538..6f180fbc9f3 100644 --- a/tests/common/fixtures/services.py +++ b/tests/common/fixtures/services.py @@ -138,7 +138,9 @@ def annotation_metadata_service(mock_service): @pytest.fixture def auth_cookie_service(mock_service): - return mock_service(AuthCookieService) + auth_cookie_service = mock_service(AuthCookieService) + auth_cookie_service.verify_cookie.return_value.deleted = False + return auth_cookie_service @pytest.fixture @@ -279,7 +281,9 @@ def user_password_service(mock_service): @pytest.fixture def user_service(mock_service): - return mock_service(UserService, name="user") + user_service = mock_service(UserService, name="user") + user_service.fetch.return_value.deleted = False + return user_service @pytest.fixture diff --git a/tests/unit/h/security/policy/_basic_http_auth_test.py b/tests/unit/h/security/policy/_basic_http_auth_test.py index 9afb473ea56..f21854875e8 100644 --- a/tests/unit/h/security/policy/_basic_http_auth_test.py +++ b/tests/unit/h/security/policy/_basic_http_auth_test.py @@ -101,6 +101,13 @@ def test_identify_returns_None_if_forwarded_user_is_not_found( assert AuthClientPolicy().identity(pyramid_request) is None + def test_identify_returns_None_if_forwarded_user_is_marked_as_deleted( + self, user_service, pyramid_request + ): + user_service.fetch.return_value.deleted = True + + assert AuthClientPolicy().identity(pyramid_request) is None + def test_identify_returns_None_if_forwarded_userid_is_invalid( self, user_service, pyramid_request ): diff --git a/tests/unit/h/security/policy/_cookie_test.py b/tests/unit/h/security/policy/_cookie_test.py index 9125dba1446..76644066324 100644 --- a/tests/unit/h/security/policy/_cookie_test.py +++ b/tests/unit/h/security/policy/_cookie_test.py @@ -17,6 +17,13 @@ def test_identity(self, pyramid_request, auth_cookie_service): user=auth_cookie_service.verify_cookie.return_value ) + def test_identity_when_user_marked_as_deleted( + self, pyramid_request, auth_cookie_service + ): + auth_cookie_service.verify_cookie.return_value.deleted = True + + assert CookiePolicy().identity(pyramid_request) is None + def test_identity_with_no_cookie(self, pyramid_request, auth_cookie_service): auth_cookie_service.verify_cookie.return_value = None diff --git a/tests/unit/h/security/policy/_remote_user_test.py b/tests/unit/h/security/policy/_remote_user_test.py index 3bfa1dec68c..5d301323c58 100644 --- a/tests/unit/h/security/policy/_remote_user_test.py +++ b/tests/unit/h/security/policy/_remote_user_test.py @@ -26,3 +26,11 @@ def test_identity_returns_None_for_no_user(self, pyramid_request, user_service): user_service.fetch.return_value = None assert RemoteUserPolicy().identity(pyramid_request) is None + + def test_identity_returns_None_for_user_marked_as_deleted( + self, pyramid_request, user_service + ): + pyramid_request.environ["HTTP_X_FORWARDED_USER"] = sentinel.forwarded_user + user_service.fetch.return_value.deleted = True + + assert RemoteUserPolicy().identity(pyramid_request) is None diff --git a/tests/unit/h/security/policy/bearer_token_test.py b/tests/unit/h/security/policy/bearer_token_test.py index a0364592ab0..40ab759f0db 100644 --- a/tests/unit/h/security/policy/bearer_token_test.py +++ b/tests/unit/h/security/policy/bearer_token_test.py @@ -57,3 +57,10 @@ def test_identity_returns_None_for_invalid_users( user_service.fetch.return_value = None assert BearerTokenPolicy().identity(pyramid_request) is None + + def test_identity_returns_None_for_user_marked_as_deleted( + self, pyramid_request, user_service + ): + user_service.fetch.return_value.deleted = True + + assert BearerTokenPolicy().identity(pyramid_request) is None