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
The following commit(s) were added to refs/heads/arm by this push:
new 065445b5 Remove custom session age checking now we have it in
asfquart, and add periodic revalidation of LDAP status
065445b5 is described below
commit 065445b5a0c60479b918a5c187625bdac8baa790
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]
