This is an automated email from the ASF dual-hosted git repository. arm pushed a commit to branch arm in repository https://gitbox.apache.org/repos/asf/tooling-trusted-releases.git
commit 2ea02fe8245a328b7c4639976f5e8ad3c2ffe5ab Author: Alastair McFarlane <[email protected]> AuthorDate: Tue Mar 31 14:29:10 2026 +0100 Remove custom session age checking now we have it in asfquart, and add periodic revalidation of LDAP status --- atr/config.py | 3 ++- atr/get/test.py | 48 +++++++++++++++++++++++++++++++++ atr/ldap.py | 7 +++-- atr/server.py | 47 +++++++++++++++----------------- playwright/test.py | 48 ++++++++++++++++++++++++++++----- tests/unit/test_ldap.py | 72 +++++++++++++++++++++++++++++++++++++++++++++++-- 6 files changed, 189 insertions(+), 36 deletions(-) diff --git a/atr/config.py b/atr/config.py index c144cb91..2bd57128 100644 --- a/atr/config.py +++ b/atr/config.py @@ -67,7 +67,7 @@ def _config_secrets_get( class AppConfig: - ABSOLUTE_SESSION_MAX_SECONDS = decouple.config("ABSOLUTE_SESSION_MAX_SECONDS", default=60 * 60 * 72, cast=int) + ACCOUNT_CHECK_INTERVAL = decouple.config("ACCOUNT_CHECK_INTERVAL", default=300, cast=int) ALLOW_TESTS = decouple.config("ALLOW_TESTS", default=False, cast=bool) ATR_STATUS = decouple.config("ATR_STATUS", default="ALPHA", cast=str) DISABLE_CHECK_CACHE = decouple.config("DISABLE_CHECK_CACHE", default=False, cast=bool) @@ -81,6 +81,7 @@ class AppConfig: LOG_LEVEL = decouple.config("LOG_LEVEL", default="INFO", cast=lambda x: x.upper()) LOG_JSON = decouple.config("LOG_JSON", default=False, cast=bool) LOG_PUBLIC_KEY = _config_secrets("LOG_PUBLIC_KEY", STATE_DIR, default=None, cast=str) + MAX_SESSION_AGE = decouple.config("MAX_SESSION_AGE", default=60 * 60 * 72, cast=int) PUBSUB_URL = _config_secrets("PUBSUB_URL", STATE_DIR, default=None, cast=str) PUBSUB_USER = _config_secrets("PUBSUB_USER", STATE_DIR, default=None, cast=str) PUBSUB_PASSWORD = _config_secrets("PUBSUB_PASSWORD", STATE_DIR, default=None, cast=str) diff --git a/atr/get/test.py b/atr/get/test.py index f1ccba41..3c3a5346 100644 --- a/atr/get/test.py +++ b/atr/get/test.py @@ -86,6 +86,54 @@ async def test_login(_session: web.Public, _test_login: Literal["test/login"]) - return await web.redirect(root.index) [email protected] +async def test_login_banned( + _session: web.Public, _test_login_banned: Literal["test/login-banned"] +) -> web.WerkzeugResponse: + """ + URL: /test/login-banned + """ + if not config.get().ALLOW_TESTS: + return quart.abort(404) + + session_data = atr.models.session.CookieData( + uid="test-banned", + fullname="Banned Test User", + pmcs=[], + projects=[], + isMember=False, + isChair=False, + roleaccount=False, + metadata={}, + ) + + util.write_quart_session_cookie(session_data) + return await web.redirect(root.index) + + [email protected] +async def test_recheck_session( + _session: web.Public, _test_recheck_session: Literal["test/recheck-session"] +) -> web.WerkzeugResponse: + """ + URL: /test/recheck-session + + Reset the last_account_check to epoch so the next request triggers a re-check. + """ + if not config.get().ALLOW_TESTS: + return quart.abort(404) + + import asfquart.session as asfquart_session + + existing = await asfquart_session.read() + if existing is None: + raise base.ASFQuartException("No session to recheck", errorcode=400) + + existing["metadata"]["last_account_check"] = 0 + asfquart_session.write(existing) + return await web.redirect(root.index) + + @get.typed async def test_merge( session: web.Committer, diff --git a/atr/ldap.py b/atr/ldap.py index 4de9a17a..11f9f557 100644 --- a/atr/ldap.py +++ b/atr/ldap.py @@ -249,10 +249,13 @@ async def handle_update(payload: dict): async def is_active(asf_uid: str) -> bool: import atr.config as config + if config.get().ALLOW_TESTS: + if asf_uid == "test": + return True + if asf_uid == "test-banned": + return False if get_bind_credentials() is None: return True - if config.get().ALLOW_TESTS and (asf_uid == "test"): - return True account = await account_lookup(asf_uid) if account is None: return False diff --git a/atr/server.py b/atr/server.py index 865d1043..ab441290 100644 --- a/atr/server.py +++ b/atr/server.py @@ -28,6 +28,7 @@ import pathlib import queue import stat import sys +import time import urllib.parse import uuid from collections.abc import Iterable @@ -54,6 +55,7 @@ import atr.db.interaction as interaction import atr.filters as filters import atr.form as form import atr.jwtoken as jwtoken +import atr.ldap as ldap import atr.log as log import atr.manager as manager import atr.models.sql as sql @@ -151,6 +153,7 @@ def _app_create_base(app_config: type[config.AppConfig]) -> base.QuartApp: # Our AppConfig.SECRET_KEY is None since we no longer support that setting asfquart_secret_key = app.secret_key app.config.from_object(app_config) + app.cfg["MAX_SESSION_AGE"] = app.config.get("MAX_SESSION_AGE", 0) app.secret_key = asfquart_secret_key if not util.is_dev_environment(): @@ -451,40 +454,34 @@ def _app_setup_request_lifecycle(app: base.QuartApp) -> None: await _reset_request_log_context() @app.before_request - async def validate_session_lifetime() -> None: - """Enforce absolute maximum session lifetime per ASVS 7.3.2.""" + async def validate_session() -> None: + """ + Check account is still active and augment cookie with additional information + Note - absolute session max lifetime (MAX_SESSION_AGE) is handled by asfquart + """ session = await asfquart.session.read() - if session is None: + if session is None or session.uid is None: return conf = config.get() - max_lifetime_seconds = conf.ABSOLUTE_SESSION_MAX_SECONDS - max_lifetime = datetime.timedelta(seconds=max_lifetime_seconds) + account_check_interval = conf.ACCOUNT_CHECK_INTERVAL - # Check if session has a creation timestamp in metadata - created_at_str = session.metadata.get("created_at") + # Check if session has a check timestamp in metadata + last_check = session.metadata.get("last_account_check") + current_time = time.time() - if created_at_str is None: - # First time seeing this session, record creation time - session.metadata["created_at"] = datetime.datetime.now(datetime.UTC).isoformat() + if last_check is None or (current_time - last_check > account_check_interval): + # First time checking this session, record time + session.metadata["last_account_check"] = current_time + if not await ldap.is_active(str(session.uid)): + asfquart.session.clear() + raise base.ASFQuartException("Session expired", errorcode=401) + + if last_check is None: + # If this was the first time we saw the session, make sure it contains the right data pmcs = util.cookie_pmcs_or_session_pmcs(session) session_data = util.session_cookie_data_from_client(session, pmcs) util.write_quart_session_cookie(session_data) - return - - # Parse the creation timestamp and check session age - try: - created_at = datetime.datetime.fromisoformat(created_at_str) - except (ValueError, TypeError): - # Invalid timestamp, treat as expired - asfquart.session.clear() - raise base.ASFQuartException("Session expired", errorcode=401) - - session_age = datetime.datetime.now(datetime.UTC) - created_at - - if session_age > max_lifetime: - asfquart.session.clear() - raise base.ASFQuartException("Session expired", errorcode=401) @app.after_request async def log_request(response: quart.Response) -> quart.Response: diff --git a/playwright/test.py b/playwright/test.py index 38bfd5eb..1ba110e6 100755 --- a/playwright/test.py +++ b/playwright/test.py @@ -142,8 +142,9 @@ def lifecycle_03_add_file(page: Page, credentials: Credentials, version_key: str file_input_locator = page.locator('input[name="file_data"]') expect(file_input_locator).to_be_visible() - logging.info("Setting the input file to /run/tests/example.txt") - file_input_locator.set_input_files("/run/tests/example.txt") + example_txt = os.path.join(os.getcwd(), "example.txt") + logging.info(f"Setting the input file to {example_txt}") + file_input_locator.set_input_files(example_txt) logging.info("Locating and activating the add files button") submit_button_locator = page.get_by_role("button", name="Add files") @@ -557,6 +558,10 @@ def test_all(page: Page, credentials: Credentials, skip_slow: bool) -> None: test_checks_06_targz, test_checks_07_cache, ] + tests["session"] = [ + test_session_01_banned_user_is_rejected, + test_session_02_recheck_allows_active_user, + ] # Order between our tests must be preserved # Insertion order is reliable since Python 3.6 @@ -758,8 +763,9 @@ def test_checks_07_cache(page: Page, credentials: Credentials) -> None: file_input_locator = page.locator('input[name="file_data"]') expect(file_input_locator).to_be_visible() - logging.info("Setting the input file to /run/tests/example.txt") - file_input_locator.set_input_files("/run/tests/example.txt") + example_txt = os.path.join(os.getcwd(), "example.txt") + logging.info(f"Setting the input file to {example_txt}") + file_input_locator.set_input_files(example_txt) logging.info("Locating and activating the add files button") submit_button_locator = page.get_by_role("button", name="Add files") @@ -793,7 +799,7 @@ def test_checks_07_cache(page: Page, credentials: Credentials) -> None: def test_openpgp_01_upload(page: Page, credentials: Credentials) -> None: - for key_path in glob.glob("/run/tests/*.asc"): + for key_path in glob.glob(os.path.join(os.getcwd(), "*.asc")): key_fingerprint_lower = os.path.basename(key_path).split(".")[0].lower() key_fingerprint_upper = key_fingerprint_lower.upper() break @@ -1040,6 +1046,36 @@ def test_projects_03_add_project(page: Page, credentials: Credentials) -> None: logging.info("Project title confirmed on view page") +def test_session_01_banned_user_is_rejected(page: Page, credentials: Credentials) -> None: + logging.info("Navigating to test login as banned user") + go_to_path(page, "/test/login-banned", wait=False) + wait_for_path(page, "/") + + logging.info("Verifying that the session expired error is shown") + error_locator = page.locator("h2.text-danger-emphasis:has-text('Session expired')") + expect(error_locator).to_be_visible() + logging.info("Session expired error confirmed for banned user") + + logging.info("Clearing cookies to recover from banned session") + page.context.clear_cookies() + + logging.info("Re-logging in as normal test user") + go_to_path(page, "/test/login", wait=False) + wait_for_path(page, "/") + logging.info("Banned user rejection test completed successfully") + + +def test_session_02_recheck_allows_active_user(page: Page, credentials: Credentials) -> None: + logging.info("Navigating to recheck-session to reset last_account_check to epoch") + go_to_path(page, "/test/recheck-session", wait=False) + wait_for_path(page, "/") + + logging.info("Verifying that active user passes the re-check (no error heading)") + error_locator = page.locator("h2.text-danger-emphasis") + expect(error_locator).not_to_be_visible() + logging.info("Active user recheck test completed successfully") + + def test_ssh_01_add_key(page: Page, credentials: Credentials) -> None: logging.info("Starting SSH key addition test") go_to_path(page, "/keys") @@ -1103,7 +1139,7 @@ def test_ssh_02_rsync_upload(page: Page, credentials: Credentials) -> None: project_key = TEST_PROJECT version_key = "0.2" source_dir_rel = f"apache-{project_key}-{version_key}" - source_dir_abs = f"/run/tests/{source_dir_rel}" + source_dir_abs = os.path.join(os.getcwd(), source_dir_rel) file1 = f"apache-{project_key}-{version_key}.tar.gz" file2 = f"{file1}.sha512" diff --git a/tests/unit/test_ldap.py b/tests/unit/test_ldap.py index 15f88441..a1260cee 100644 --- a/tests/unit/test_ldap.py +++ b/tests/unit/test_ldap.py @@ -16,6 +16,7 @@ # under the License. import pathlib +import unittest.mock as mock from typing import TYPE_CHECKING import pytest @@ -33,10 +34,17 @@ class MockApp: class MockConfig: - def __init__(self, state_dir: pathlib.Path, ldap_bind_dn: str | None, ldap_bind_password: str | None): - self.STATE_DIR = str(state_dir) + def __init__( + self, + state_dir: pathlib.Path | None = None, + ldap_bind_dn: str | None = None, + ldap_bind_password: str | None = None, + allow_tests: bool = False, + ): + self.STATE_DIR = str(state_dir) if state_dir else "" self.LDAP_BIND_DN = ldap_bind_dn self.LDAP_BIND_PASSWORD = ldap_bind_password + self.ALLOW_TESTS = allow_tests @pytest.fixture @@ -109,6 +117,66 @@ async def test_fetch_admin_users_returns_reasonable_count(ldap_configured: bool) assert len(admins) < 100 [email protected] +async def test_is_active_returns_true_when_ldap_not_configured(monkeypatch: "MonkeyPatch"): + monkeypatch.setattr("atr.ldap.get_bind_credentials", lambda: None) + assert await ldap.is_active("anyone") is True + + [email protected] +async def test_is_active_returns_true_for_test_user_when_tests_allowed(monkeypatch: "MonkeyPatch"): + monkeypatch.setattr("atr.ldap.get_bind_credentials", lambda: ("dn", "pw")) + monkeypatch.setattr("atr.config.get", lambda: MockConfig(allow_tests=True)) + assert await ldap.is_active("test") is True + + [email protected] +async def test_is_active_returns_false_for_test_banned_user_when_tests_allowed(monkeypatch: "MonkeyPatch"): + monkeypatch.setattr("atr.ldap.get_bind_credentials", lambda: ("dn", "pw")) + monkeypatch.setattr("atr.config.get", lambda: MockConfig(allow_tests=True)) + assert await ldap.is_active("test-banned") is False + + [email protected] +async def test_is_active_returns_false_when_account_not_found(monkeypatch: "MonkeyPatch"): + monkeypatch.setattr("atr.ldap.get_bind_credentials", lambda: ("dn", "pw")) + monkeypatch.setattr("atr.config.get", lambda: MockConfig(allow_tests=False)) + monkeypatch.setattr("atr.ldap.account_lookup", mock.AsyncMock(return_value=None)) + assert await ldap.is_active("ghost") is False + + [email protected] +async def test_is_active_returns_true_for_active_account(monkeypatch: "MonkeyPatch"): + account = ldap.Result(dn="uid=alice,ou=people,dc=apache,dc=org", uid=["alice"]) + monkeypatch.setattr("atr.ldap.get_bind_credentials", lambda: ("dn", "pw")) + monkeypatch.setattr("atr.config.get", lambda: MockConfig(allow_tests=False)) + monkeypatch.setattr("atr.ldap.account_lookup", mock.AsyncMock(return_value=account)) + assert await ldap.is_active("alice") is True + + [email protected] +async def test_is_active_returns_false_for_banned_account(monkeypatch: "MonkeyPatch"): + account = ldap.Result.model_validate( + {"dn": "uid=bad,ou=people,dc=apache,dc=org", "uid": ["bad"], "asf-banned": ["yes"]} + ) + monkeypatch.setattr("atr.ldap.get_bind_credentials", lambda: ("dn", "pw")) + monkeypatch.setattr("atr.config.get", lambda: MockConfig(allow_tests=False)) + monkeypatch.setattr("atr.ldap.account_lookup", mock.AsyncMock(return_value=account)) + assert await ldap.is_active("bad") is False + + +def test_is_banned_returns_false_for_account_without_flag(): + account = ldap.Result(dn="uid=alice,ou=people,dc=apache,dc=org", uid=["alice"]) + assert ldap.is_banned(account) is False + + +def test_is_banned_returns_true_for_account_with_flag(): + account = ldap.Result.model_validate( + {"dn": "uid=bad,ou=people,dc=apache,dc=org", "uid": ["bad"], "asf-banned": ["yes"]} + ) + assert ldap.is_banned(account) is True + + def _skip_if_unavailable(ldap_configured: bool) -> None: if not ldap_configured: pytest.skip("LDAP not configured") --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
