This is an automated email from the ASF dual-hosted git repository.
sbp pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tooling-trusted-releases.git
The following commit(s) were added to refs/heads/main by this push:
new d885221 Use the LDAP admins cache when checking whether the user is
an admin
d885221 is described below
commit d8852212d123674e8dcaa62e9b6f48b757c86086
Author: Sean B. Palmer <[email protected]>
AuthorDate: Fri Jan 23 16:43:18 2026 +0000
Use the LDAP admins cache when checking whether the user is an admin
---
atr/blueprints/admin.py | 2 +-
atr/cache.py | 15 +++++++
atr/config.py | 15 -------
atr/tasks/checks/paths.py | 2 +-
atr/user.py | 34 ++++++++++-----
tests/unit/test_cache.py | 32 ++++++++++++++
tests/unit/test_user.py | 107 ++++++++++++++++++++++++++++++++++++++++++++++
7 files changed, 179 insertions(+), 28 deletions(-)
diff --git a/atr/blueprints/admin.py b/atr/blueprints/admin.py
index ca58b77..a6f93ae 100644
--- a/atr/blueprints/admin.py
+++ b/atr/blueprints/admin.py
@@ -140,7 +140,7 @@ async def _check_admin_access() -> None:
if web_session is None:
raise base.ASFQuartException("Not authenticated", errorcode=401)
- if web_session.uid not in user.get_admin_users():
+ if not user.is_admin(web_session.uid):
raise base.ASFQuartException("You are not authorized to access the
admin interface", errorcode=403)
quart.g.session = web.Committer(web_session)
diff --git a/atr/cache.py b/atr/cache.py
index fef42c5..4feb61d 100644
--- a/atr/cache.py
+++ b/atr/cache.py
@@ -38,6 +38,21 @@ class AdminsCache(schema.Strict):
admins: frozenset[str] = schema.description("Set of admin user IDs from
LDAP")
+def admins_get() -> frozenset[str]:
+ app = asfquart.APP
+ return app.extensions.get("admins", frozenset())
+
+
+async def admins_get_async() -> frozenset[str]:
+ try:
+ return admins_get()
+ except RuntimeError:
+ cache_data = await admins_read_from_file()
+ if cache_data is None:
+ return frozenset()
+ return cache_data.admins
+
+
async def admins_read_from_file() -> AdminsCache | None:
cache_path = _admins_path()
if not cache_path.exists():
diff --git a/atr/config.py b/atr/config.py
index 0fceba1..b8090c5 100644
--- a/atr/config.py
+++ b/atr/config.py
@@ -120,22 +120,7 @@ class AppConfig:
SESSION_COOKIE_SAMESITE = "Strict"
SESSION_COOKIE_NAME = "__Host-session"
- # FIXME: retrieve the list of admin users from LDAP or oath session /
isRoot
ADMIN_USERS_ADDITIONAL = decouple.config("ADMIN_USERS_ADDITIONAL",
default="", cast=str)
- ADMIN_USERS = frozenset(
- {
- "cwells",
- "dfoulks",
- "fluxo",
- "gmcdonald",
- "humbedooh",
- "sbp",
- "akm",
- "arm",
- "wave",
- }
- | set(ADMIN_USERS_ADDITIONAL.split(",") if ADMIN_USERS_ADDITIONAL else
[])
- )
class DebugConfig(AppConfig):
diff --git a/atr/tasks/checks/paths.py b/atr/tasks/checks/paths.py
index 8f82e9c..b1a5a62 100644
--- a/atr/tasks/checks/paths.py
+++ b/atr/tasks/checks/paths.py
@@ -179,7 +179,7 @@ async def _check_path_process_single(
relative_path_str = str(relative_path)
# For debugging and testing
- if user.is_admin(asf_uid) and (full_path.name ==
"deliberately_slow_ATR_task_filename.txt"):
+ if (await user.is_admin_async(asf_uid)) and (full_path.name ==
"deliberately_slow_ATR_task_filename.txt"):
await asyncio.sleep(20)
errors: list[str] = []
diff --git a/atr/user.py b/atr/user.py
index cfb1c1d..e57e678 100644
--- a/atr/user.py
+++ b/atr/user.py
@@ -19,6 +19,7 @@
import functools
+import atr.cache as cache
import atr.config as config
import atr.db as db
import atr.models.sql as sql
@@ -37,21 +38,24 @@ async def candidate_drafts(uid: str, user_projects:
list[sql.Project] | None = N
return user_candidate_drafts
[email protected]
-def get_admin_users() -> set[str]:
- admin_users = set(config.get().ADMIN_USERS)
- if config.get().ALLOW_TESTS:
- # TODO: Just for debugging, but ideally we would do this in a targeted
way
- # We need this, for example, for deleting releases
- admin_users.add("test")
- return admin_users
+def is_admin(user_id: str | None) -> bool:
+ if user_id is None:
+ return False
+ if config.get().ALLOW_TESTS and (user_id == "test"):
+ return True
+ if user_id in _get_additional_admin_users():
+ return True
+ return user_id in cache.admins_get()
-def is_admin(user_id: str | None) -> bool:
- """Check whether a user is an admin."""
+async def is_admin_async(user_id: str | None) -> bool:
if user_id is None:
return False
- return user_id in get_admin_users()
+ if config.get().ALLOW_TESTS and (user_id == "test"):
+ return True
+ if user_id in _get_additional_admin_users():
+ return True
+ return user_id in await cache.admins_get_async()
def is_committee_member(committee: sql.Committee | None, uid: str) -> bool:
@@ -90,3 +94,11 @@ async def projects(uid: str, committee_only: bool = False,
super_project: bool =
if (uid in p.committee.committee_members) or (uid in
p.committee.committers):
user_projects.append(p)
return user_projects
+
+
[email protected]
+def _get_additional_admin_users() -> frozenset[str]:
+ additional = config.get().ADMIN_USERS_ADDITIONAL
+ if not additional:
+ return frozenset()
+ return frozenset(additional.split(","))
diff --git a/tests/unit/test_cache.py b/tests/unit/test_cache.py
index a8f962f..dec2adc 100644
--- a/tests/unit/test_cache.py
+++ b/tests/unit/test_cache.py
@@ -94,6 +94,38 @@ def test_admins_cache_validates_with_good_data():
assert data.admins == frozenset({"alice", "bob"})
[email protected]
+async def test_admins_get_async_falls_back_to_file_without_app_context(
+ state_dir: pathlib.Path, monkeypatch: "MonkeyPatch"
+):
+ def raise_runtime_error() -> frozenset[str]:
+ raise RuntimeError("No app context")
+
+ monkeypatch.setattr("atr.cache.admins_get", raise_runtime_error)
+
+ await cache.admins_save_to_file(frozenset({"file_alice", "file_bob"}))
+ result = await cache.admins_get_async()
+ assert result == frozenset({"file_alice", "file_bob"})
+
+
[email protected]
+async def test_admins_get_async_uses_extensions_when_available(mock_app:
_MockApp):
+ mock_app.extensions["admins"] = frozenset({"async_alice"})
+ result = await cache.admins_get_async()
+ assert result == frozenset({"async_alice"})
+
+
+def test_admins_get_returns_empty_frozenset_when_not_set(mock_app: _MockApp):
+ result = cache.admins_get()
+ assert result == frozenset()
+
+
+def test_admins_get_returns_frozenset_from_extensions(mock_app: _MockApp):
+ mock_app.extensions["admins"] = frozenset({"alice", "bob"})
+ result = cache.admins_get()
+ assert result == frozenset({"alice", "bob"})
+
+
@pytest.mark.asyncio
async def test_admins_read_from_file_returns_none_for_invalid_json(state_dir:
pathlib.Path):
cache_path = state_dir / "cache" / "admins.json"
diff --git a/tests/unit/test_user.py b/tests/unit/test_user.py
new file mode 100644
index 0000000..268b22e
--- /dev/null
+++ b/tests/unit/test_user.py
@@ -0,0 +1,107 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from typing import TYPE_CHECKING
+
+import pytest
+
+import atr.user as user
+
+if TYPE_CHECKING:
+ from pytest import MonkeyPatch
+
+
+class _MockApp:
+ def __init__(self):
+ self.extensions: dict[str, object] = {}
+
+
+class _MockConfig:
+ def __init__(self, allow_tests: bool = False, admin_users_additional: str
= ""):
+ self.ALLOW_TESTS = allow_tests
+ self.ADMIN_USERS_ADDITIONAL = admin_users_additional
+
+
[email protected]
+def mock_app(monkeypatch: "MonkeyPatch") -> _MockApp:
+ app = _MockApp()
+ monkeypatch.setattr("asfquart.APP", app)
+ return app
+
+
[email protected]
+async def test_is_admin_async_returns_false_for_none(mock_app: _MockApp,
monkeypatch: "MonkeyPatch"):
+ monkeypatch.setattr("atr.config.get", lambda: _MockConfig())
+ mock_app.extensions["admins"] = frozenset()
+ assert await user.is_admin_async(None) is False
+
+
[email protected]
+async def test_is_admin_async_returns_true_for_cached_admin(mock_app:
_MockApp, monkeypatch: "MonkeyPatch"):
+ user._get_additional_admin_users.cache_clear()
+ monkeypatch.setattr("atr.config.get", lambda: _MockConfig())
+ mock_app.extensions["admins"] = frozenset({"async_admin"})
+ assert await user.is_admin_async("async_admin") is True
+
+
[email protected]
+async def test_is_admin_async_returns_true_for_test_user(mock_app: _MockApp,
monkeypatch: "MonkeyPatch"):
+ user._get_additional_admin_users.cache_clear()
+ monkeypatch.setattr("atr.config.get", lambda:
_MockConfig(allow_tests=True))
+ mock_app.extensions["admins"] = frozenset()
+ assert await user.is_admin_async("test") is True
+
+
+def test_is_admin_returns_false_for_none(mock_app: _MockApp, monkeypatch:
"MonkeyPatch"):
+ monkeypatch.setattr("atr.config.get", lambda: _MockConfig())
+ mock_app.extensions["admins"] = frozenset()
+ assert user.is_admin(None) is False
+
+
+def test_is_admin_returns_false_for_test_user_when_not_allowed(mock_app:
_MockApp, monkeypatch: "MonkeyPatch"):
+ user._get_additional_admin_users.cache_clear()
+ monkeypatch.setattr("atr.config.get", lambda:
_MockConfig(allow_tests=False))
+ mock_app.extensions["admins"] = frozenset()
+ assert user.is_admin("test") is False
+
+
+def test_is_admin_returns_false_for_unknown_user(mock_app: _MockApp,
monkeypatch: "MonkeyPatch"):
+ monkeypatch.setattr("atr.config.get", lambda: _MockConfig())
+ mock_app.extensions["admins"] = frozenset({"alice", "bob"})
+ assert user.is_admin("nobody") is False
+
+
+def test_is_admin_returns_true_for_additional_admin(mock_app: _MockApp,
monkeypatch: "MonkeyPatch"):
+ user._get_additional_admin_users.cache_clear()
+ monkeypatch.setattr("atr.config.get", lambda:
_MockConfig(admin_users_additional="alice,bob"))
+ mock_app.extensions["admins"] = frozenset()
+ assert user.is_admin("alice") is True
+ assert user.is_admin("bob") is True
+
+
+def test_is_admin_returns_true_for_cached_admin(mock_app: _MockApp,
monkeypatch: "MonkeyPatch"):
+ user._get_additional_admin_users.cache_clear()
+ monkeypatch.setattr("atr.config.get", lambda: _MockConfig())
+ mock_app.extensions["admins"] = frozenset({"cached_admin"})
+ assert user.is_admin("cached_admin") is True
+
+
+def test_is_admin_returns_true_for_test_user_when_allowed(mock_app: _MockApp,
monkeypatch: "MonkeyPatch"):
+ user._get_additional_admin_users.cache_clear()
+ monkeypatch.setattr("atr.config.get", lambda:
_MockConfig(allow_tests=True))
+ mock_app.extensions["admins"] = frozenset()
+ assert user.is_admin("test") is True
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
