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 403efc3c9dd51ab785993488e1a29906ab73257d Author: Alastair McFarlane <[email protected]> AuthorDate: Wed Apr 1 11:12:17 2026 +0100 Replace ALLOW_TESTS with dedicated Test config mode. Add additional runtime safety checks. --- BUILD.md | 2 +- DEVELOPMENT.md | 2 +- Makefile | 4 +- atr/admin/__init__.py | 25 +++----- atr/config.py | 121 +++++++++++++++++++++++++++---------- atr/db/interaction.py | 2 +- atr/docs/authorization-security.md | 5 +- atr/docs/running-the-server.md | 4 +- atr/docs/storage-interface.md | 2 +- atr/get/download.py | 2 +- atr/get/projects.py | 6 +- atr/get/published.py | 4 +- atr/get/test.py | 21 +++++-- atr/ldap.py | 2 +- atr/post/test.py | 10 +++ atr/principal.py | 4 +- atr/server.py | 18 +++--- atr/storage/writers/keys.py | 4 +- atr/storage/writers/mail.py | 3 +- atr/storage/writers/release.py | 4 +- atr/tabulate.py | 5 +- atr/user.py | 8 +-- atr/util.py | 14 +---- atr/worker.py | 13 ++-- docker-compose.yml | 2 +- tests/docker-compose.yml | 4 +- tests/unit/test_ldap.py | 12 ++-- tests/unit/test_user.py | 11 ++-- tests/unit/test_vote.py | 3 +- 29 files changed, 185 insertions(+), 132 deletions(-) diff --git a/BUILD.md b/BUILD.md index c0ed22b0..85c7a4b0 100644 --- a/BUILD.md +++ b/BUILD.md @@ -58,7 +58,7 @@ docker compose up --build The compose configuration: - Mounts `atr/` for live code changes -- Enables test mode (`ALLOW_TESTS=1`) +- Enables test mode (`TESTS=1`) - Exposes port 8080 ## Documentation build diff --git a/DEVELOPMENT.md b/DEVELOPMENT.md index 28a50fa8..99ce1ec8 100644 --- a/DEVELOPMENT.md +++ b/DEVELOPMENT.md @@ -62,7 +62,7 @@ Visit [`https://127.0.0.1:8080/`](https://127.0.0.1:8080/) and accept the self-s The container: -- Runs in test mode (`ALLOW_TESTS=1`) with mock authentication +- Runs in test mode (`TESTS=1`) with mock authentication - Mounts `atr/` for live code changes without rebuilding - Auto-reloads when files change diff --git a/Makefile b/Makefile index 374c5182..5804516d 100644 --- a/Makefile +++ b/Makefile @@ -100,7 +100,7 @@ run-alpine: -v "$$PWD/$(STATE_DIR):/opt/atr/state" \ -v "$$PWD/$(STATE_DIR)/hypercorn/secrets/localhost.apache.org+2-key.pem:/opt/atr/state/hypercorn/secrets/key.pem" \ -v "$$PWD/$(STATE_DIR)/hypercorn/secrets/localhost.apache.org+2.pem:/opt/atr/state/hypercorn/secrets/cert.pem" \ - -e APP_HOST=localhost.apache.org:8080 -e ALLOW_TESTS=1 \ + -e APP_HOST=localhost.apache.org:8080 -e TESTS=1 \ -e SSH_HOST=0.0.0.0 -e BIND=0.0.0.0:8080 \ tooling-trusted-release @@ -121,7 +121,7 @@ serve: serve-local: @scripts/check-certs @scripts/check-perms - APP_HOST=localhost.apache.org:8080 DISABLE_CHECK_CACHE=1 ALLOW_TESTS=1 \ + APP_HOST=localhost.apache.org:8080 DISABLE_CHECK_CACHE=1 TESTS=1 \ SSH_HOST=127.0.0.1 uv run --frozen hypercorn --bind $(BIND) \ --keyfile hypercorn/secrets/localhost.apache.org+2-key.pem \ --certfile hypercorn/secrets/localhost.apache.org+2.pem \ diff --git a/atr/admin/__init__.py b/atr/admin/__init__.py index cca450a1..c584918d 100644 --- a/atr/admin/__init__.py +++ b/atr/admin/__init__.py @@ -449,7 +449,7 @@ async def delete_test_openpgp_keys_get( Display the form to delete test user OpenPGP keys. """ - if not config.get().ALLOW_TESTS: + if not config.is_test_mode(): return quart.abort(404) rendered_form = form.render( @@ -469,7 +469,7 @@ async def delete_test_openpgp_keys_post( Delete all test user OpenPGP keys and their links. """ - if not config.get().ALLOW_TESTS: + if not config.is_test_mode(): return quart.abort(404) test_uid = "test" @@ -683,7 +683,7 @@ async def logs(_session: web.Committer, _logs: Literal["logs"]) -> web.QuartResp """ URL: GET /logs """ - _require_debug_and_allow_tests() + _require_non_production_mode() recent_logs = log.get_recent_logs() if recent_logs is None: raise base.ASFQuartException("Debug logging not initialised", errorcode=404) @@ -1129,10 +1129,7 @@ async def validate_jwt_get(_session: web.Committer, _validate_jwt: Literal["vali """ URL: GET /validate-jwt """ - try: - _require_debug_and_allow_tests() - except base.ASFQuartException: - return quart.abort(404) + _require_non_production_mode() rendered_form = form.render( model_cls=ValidateJwtForm, submit_label="Validate JWT", @@ -1148,11 +1145,7 @@ async def validate_jwt_post( """ URL: POST /validate-jwt """ - try: - _require_debug_and_allow_tests() - except base.ASFQuartException: - return quart.abort(404) - + _require_non_production_mode() token = validate_form.token result: dict[str, Any] = {"token_length": len(token), "valid": False} @@ -1352,11 +1345,9 @@ async def _get_filesystem_dirs_unfinished(filesystem_dirs: list[str]) -> None: filesystem_dirs.append(version_dir_path) -def _require_debug_and_allow_tests() -> None: - conf = config.get() - debug_and_allow_tests = (config.get_mode() == config.Mode.Debug) and conf.ALLOW_TESTS - if not debug_and_allow_tests: - raise base.ASFQuartException("Not available without ALLOW_TESTS", errorcode=403) +def _require_non_production_mode() -> None: + if config.is_production_mode(): + quart.abort(404) async def _rotate_jwt_key_page(rendered_form: htm.Element) -> str: diff --git a/atr/config.py b/atr/config.py index 2bd57128..cca1f89a 100644 --- a/atr/config.py +++ b/atr/config.py @@ -68,7 +68,6 @@ def _config_secrets_get( class AppConfig: 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) APP_HOST = decouple.config("APP_HOST", default="127.0.0.1") @@ -141,6 +140,7 @@ class DebugConfig(AppConfig): class Mode(enum.Enum): Debug = "Debug" + Test = "Test" Production = "Production" Profiling = "Profiling" @@ -158,37 +158,96 @@ class ProfilingConfig(AppConfig): USE_BLOCKBUSTER = True +class TestConfig(DebugConfig): + pass + + # Load all possible configurations _CONFIG_DICT: Final = { Mode.Debug: DebugConfig, + Mode.Test: TestConfig, Mode.Production: ProductionConfig, Mode.Profiling: ProfilingConfig, } def get() -> type[AppConfig]: - try: - config = _CONFIG_DICT[get_mode()] - except KeyError: - exit("Error: Invalid mode. Expected values Debug, Production, or Profiling.") + return _CONFIG_DICT[get_mode()] + + +def get_mode() -> Mode: + global _global_mode + + profiling = decouple.config("PROFILING", default=False, cast=bool) + production = decouple.config("PRODUCTION", default=False, cast=bool) + test = decouple.config("TESTS", default=False, cast=bool) + + # Make sure we don't set more than one - which would fall back into whichever is first in the next conditional + # This prevents accidental production in test mode, for example + enabled = [name for name, val in [("PROFILING", profiling), ("PRODUCTION", production), ("TESTS", test)] if val] + if len(enabled) > 1: + exit(f"Only one mode flag may be set, but got: {', '.join(enabled)}") + + if _global_mode is None: + if profiling: + _global_mode = Mode.Profiling + elif production: + _global_mode = Mode.Production + elif test: + _global_mode = Mode.Test + else: + _global_mode = Mode.Debug + + return _global_mode + + +def is_dev_environment() -> bool: + conf = get() + for development_host in ("127.0.0.1", "atr", "atr-dev", "localhost.apache.org"): + if (conf.APP_HOST == development_host) or conf.APP_HOST.startswith(f"{development_host}:"): + return True + return False - if config.ALLOW_TESTS and (get_mode() != Mode.Debug): - raise RuntimeError("ALLOW_TESTS can only be enabled in Debug mode") + +def is_ldap_configured() -> bool: + conf = get() + return bool(conf.LDAP_BIND_DN and conf.LDAP_BIND_PASSWORD) + + +def is_production_mode() -> bool: + return get_mode() == Mode.Production + + +def is_test_mode() -> bool: + return get_mode() == Mode.Test + + +def validate() -> None: + """ + Runs validity and safety checks to ensure configuration is consistent and secure: + + Path checks - absolute and relative paths are set correctly + Debug mode can only be set on a development URL (127.0.0.1, atr, etc.) + LDAP must be configured in production + Cannot set additional admins at runtime in production + Dev URLs cannot be set in production mode + """ + conf = get() absolute_paths = [ - (config.PROJECT_ROOT, "PROJECT_ROOT"), - (config.STATE_DIR, "STATE_DIR"), - (config.DOWNLOADS_STORAGE_DIR, "DOWNLOADS_STORAGE_DIR"), - (config.FINISHED_STORAGE_DIR, "FINISHED_STORAGE_DIR"), - (config.UNFINISHED_STORAGE_DIR, "UNFINISHED_STORAGE_DIR"), - (config.SVN_STORAGE_DIR, "SVN_STORAGE_DIR"), - (config.ARCHIVES_STORAGE_DIR, "ARCHIVES_STORAGE_DIR"), - (config.ATTESTABLE_STORAGE_DIR, "ATTESTABLE_STORAGE_DIR"), - (config.STORAGE_AUDIT_LOG_FILE, "STORAGE_AUDIT_LOG_FILE"), - (config.PERFORMANCE_LOG_FILE, "PERFORMANCE_LOG_FILE"), + (conf.PROJECT_ROOT, "PROJECT_ROOT"), + (conf.STATE_DIR, "STATE_DIR"), + (conf.DOWNLOADS_STORAGE_DIR, "DOWNLOADS_STORAGE_DIR"), + (conf.FINISHED_STORAGE_DIR, "FINISHED_STORAGE_DIR"), + (conf.UNFINISHED_STORAGE_DIR, "UNFINISHED_STORAGE_DIR"), + (conf.SVN_STORAGE_DIR, "SVN_STORAGE_DIR"), + (conf.ARCHIVES_STORAGE_DIR, "ARCHIVES_STORAGE_DIR"), + (conf.ATTESTABLE_STORAGE_DIR, "ATTESTABLE_STORAGE_DIR"), + (conf.STORAGE_AUDIT_LOG_FILE, "STORAGE_AUDIT_LOG_FILE"), + (conf.PERFORMANCE_LOG_FILE, "PERFORMANCE_LOG_FILE"), ] relative_paths = [ - (config.SQLITE_DB_PATH, "SQLITE_DB_PATH"), + (conf.SQLITE_DB_PATH, "SQLITE_DB_PATH"), ] for path, name in absolute_paths: @@ -198,18 +257,14 @@ def get() -> type[AppConfig]: if path.startswith("/"): raise RuntimeError(f"{name} must be a relative path") - return config - - -def get_mode() -> Mode: - global _global_mode - - if _global_mode is None: - if decouple.config("PROFILING", default=False, cast=bool): - _global_mode = Mode.Profiling - elif decouple.config("PRODUCTION", default=False, cast=bool): - _global_mode = Mode.Production - else: - _global_mode = Mode.Debug - - return _global_mode + if (not is_dev_environment()) and (get_mode() == Mode.Debug): + raise RuntimeError("Debug mode can only be set in development environment") + + # Production-specific guards + if is_production_mode(): + if not (conf.LDAP_BIND_DN and conf.LDAP_BIND_PASSWORD): + raise RuntimeError("LDAP bind credentials must be configured in production mode") + if conf.ADMIN_USERS_ADDITIONAL or conf.TOOLING_USERS_ADDITIONAL: + raise RuntimeError("Cannot manually configure additional users in production") + if is_dev_environment(): + raise RuntimeError("Production mode cannot use a development APP_HOST") diff --git a/atr/db/interaction.py b/atr/db/interaction.py index d38e7258..55f3eaa4 100644 --- a/atr/db/interaction.py +++ b/atr/db/interaction.py @@ -555,7 +555,7 @@ async def validate_trusted_jwt(publisher: str, jwt: str) -> tuple[github.Trusted def vote_duration_bypass() -> bool: - return (config.get_mode() == config.Mode.Debug) or config.get().ALLOW_TESTS + return not config.is_production_mode() def vote_end_get(latest_vote_task: sql.Task | None) -> datetime.datetime | None: diff --git a/atr/docs/authorization-security.md b/atr/docs/authorization-security.md index fcb51ff3..213b2d9e 100644 --- a/atr/docs/authorization-security.md +++ b/atr/docs/authorization-security.md @@ -211,14 +211,15 @@ The cache is per-user and in-memory. It does not persist across server restarts. ### Test mode -When `ALLOW_TESTS` is enabled in the configuration, a special "test" user and "test" committee are available. **This should never be enabled in production.** The security implications are significant: +When running in test mode (env == `TESTS`), a special "test" user and "test" committee are available. **This should never be enabled in production.** The security implications are significant: 1. All authenticated users (not just the test user) are granted membership in the "test" committee and project [`principal`](/ref/atr/principal.py). 2. Authorization checks in the storage layer are completely skipped for the test committee [`release`](/ref/atr/storage/writers/release.py). 3. Rate limiting is disabled [`server`](/ref/atr/server.py). 4. A hardcoded "test" user bypasses LDAP verification. -If `ALLOW_TESTS` is accidentally left enabled in production, every authenticated user gains unauthorized access to the test committee and its resources. This flag is intended for use only in development and test environments where `DEBUG_MODE` is also set. +If this is accidentally left enabled in production, every authenticated user gains unauthorized access to the test committee and its resources. This flag is intended for use only in development and test environments where `DEBUG_MODE` is also set. +As such, on starting the server in production mode (env == `PRODUCTION`), a safety check will run to ensure certain sensitive values are not misconfigured. ## Implementation references diff --git a/atr/docs/running-the-server.md b/atr/docs/running-the-server.md index e0a0371f..a696b998 100644 --- a/atr/docs/running-the-server.md +++ b/atr/docs/running-the-server.md @@ -67,8 +67,8 @@ ATR serves on multiple hosts, but we recommend using `localhost.apache.org` cons ### Environment variables -* `ADMIN_USERS_ADDITIONAL` : Enable additional users as admins -* `ALLOW_TESTS=1`: Enable test mode with mock authentication +* `ADMIN_USERS_ADDITIONAL` : Enable additional users as admins (inoperative in production) +* `TESTS=1`: Enable test mode with mock authentication * `APP_HOST`: Hostname for the application * `BIND`: Address and port to bind (default: `127.0.0.1:8080`) * `LDAP_BIND_DN`: LDAP bind DN for rsync writes diff --git a/atr/docs/storage-interface.md b/atr/docs/storage-interface.md index 2acb0b99..5f1b6a47 100644 --- a/atr/docs/storage-interface.md +++ b/atr/docs/storage-interface.md @@ -26,7 +26,7 @@ The storage interface recognizes several permission levels: general public (unau The storage interface does not make it impossible to bypass authorization, because you can always import `db` directly and write to the database. But it makes bypassing authorization an explicit choice that requires deliberate action, and it makes the safer path the easier path. This is a pragmatic approach to security: we cannot prevent all mistakes, but we can make it harder to make them accidentally. -**Note:** When `ALLOW_TESTS` is enabled, authorization checks in the storage layer are completely skipped for the test committee [`release`](/ref/atr/storage/writers/release.py). This is an intentional exception for development and test environments only. See [Authorization security](authorization-security#test-mode) for the full security implications of this flag. +**Note:** In Test mode, authorization checks in the storage layer are completely skipped for the test committee [`release`](/ref/atr/storage/writers/release.py). This is an intentional exception for development and test environments only. See [Authorization security](authorization-security#test-mode) for the full security implications of this flag. ## How do we read from storage? diff --git a/atr/get/download.py b/atr/get/download.py index a2105024..88915530 100644 --- a/atr/get/download.py +++ b/atr/get/download.py @@ -124,7 +124,7 @@ async def sh_selected( content = await f.read() download_urls_selected = util.as_url(urls_selected, project_key=str(project_key), version_key=str(version_key)) download_path = util.as_url(path, project_key=str(project_key), version_key=str(version_key), file_path="") - curl_options = "--insecure" if util.is_dev_environment() else "--proto =https --tlsv1.2" + curl_options = "--insecure" if config.is_dev_environment() else "--proto =https --tlsv1.2" content = content.replace("[CURL_EXTRA]", curl_options) content = content.replace("[URL_OF_URLS]", f"https://{app_host}{download_urls_selected}";) content = content.replace("[URLS_PREFIX]", f"https://{app_host}{download_path}";) diff --git a/atr/get/projects.py b/atr/get/projects.py index 099fc218..094d9636 100644 --- a/atr/get/projects.py +++ b/atr/get/projects.py @@ -128,15 +128,15 @@ async def select(session: web.Committer, _project_select: Literal["project/selec if session.uid: async with db.session() as data: # TODO: Move this filtering logic somewhere else - # The ALLOW_TESTS line allows test projects to be shown - conf = config.get() + # Test mode allows test projects to be shown + test_mode = config.is_test_mode() all_projects = await data.project(status=sql.ProjectStatus.ACTIVE, _committee=True).all() user_projects = [ p for p in all_projects if p.committee and ( - (conf.ALLOW_TESTS and (p.committee.key == "test")) + (test_mode and (p.committee.key == "test")) or (session.uid in p.committee.committee_members) or (session.uid in p.committee.committers) or (session.uid in p.committee.release_managers) diff --git a/atr/get/published.py b/atr/get/published.py index 4c0d7281..151b658a 100644 --- a/atr/get/published.py +++ b/atr/get/published.py @@ -41,7 +41,7 @@ async def path(session: web.Committer, _published: Literal["published"], file_pa # This route is for debugging # When developing locally, there is no proxy to view the downloads directory # Therefore this path acts as a way to check the contents of that directory - if not config.get().ALLOW_TESTS: + if not config.is_test_mode(): return quart.abort(404) return await _path(session, str(file_path)) @@ -51,7 +51,7 @@ async def root(session: web.Committer, _published: Literal["published/"]) -> web """ URL: /published/ """ - if not config.get().ALLOW_TESTS: + if not config.is_test_mode(): return quart.abort(404) return await _path(session, "") diff --git a/atr/get/test.py b/atr/get/test.py index d030478f..822d150b 100644 --- a/atr/get/test.py +++ b/atr/get/test.py @@ -48,6 +48,9 @@ async def test_empty(_session: web.Public, _test_empty: Literal["test/empty"]) - """ URL: /test/empty """ + if config.is_production_mode(): + return quart.abort(404) + empty_form = form.render( model_cls=form.Empty, submit_label="Submit empty form", @@ -68,7 +71,9 @@ async def test_login(_session: web.Public, _test_login: Literal["test/login"]) - """ URL: /test/login """ - if not config.get().ALLOW_TESTS: + # Some test routes work anywhere outside of production + # but test logins should be Test mode only + if not config.is_test_mode(): return quart.abort(404) session_data = atr.models.session.CookieData( @@ -93,7 +98,7 @@ async def test_login_banned( """ URL: /test/login-banned """ - if not config.get().ALLOW_TESTS: + if not config.is_test_mode(): return quart.abort(404) session_data = atr.models.session.CookieData( @@ -121,7 +126,7 @@ async def test_merge( """ URL: /test/merge/<project_key>/<version_key> """ - if not config.get().ALLOW_TESTS: + if config.is_production_mode(): return quart.abort(404) async with storage.write(session) as write_n: @@ -176,6 +181,9 @@ async def test_multiple(_session: web.Public, _test_multiple: Literal["test/mult """ URL: /test/multiple """ + if config.is_production_mode(): + return quart.abort(404) + apple_form = form.render( model_cls=shared.test.AppleForm, submit_label="Order apples", @@ -207,7 +215,7 @@ async def test_recheck_session( Reset the last_account_check to epoch so the next request triggers a re-check. """ - if not config.get().ALLOW_TESTS: + if not config.is_test_mode(): return quart.abort(404) import asfquart.session as asfquart_session @@ -226,6 +234,9 @@ async def test_single(session: web.Public, _test_single: Literal["test/single"]) """ URL: /test/single """ + if config.is_production_mode(): + return quart.abort(404) + import htpy vote_widget = htpy.div(class_="btn-group", role="group")[ @@ -263,7 +274,7 @@ async def test_vote( """ URL: /test/vote/<category>/<project_key>/<version_key> """ - if not config.get().ALLOW_TESTS: + if config.is_production_mode(): return quart.abort(404) category_map = { diff --git a/atr/ldap.py b/atr/ldap.py index 11f9f557..73d53ffe 100644 --- a/atr/ldap.py +++ b/atr/ldap.py @@ -249,7 +249,7 @@ 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 config.is_test_mode(): if asf_uid == "test": return True if asf_uid == "test-banned": diff --git a/atr/post/test.py b/atr/post/test.py index 4d66d859..fe045c46 100644 --- a/atr/post/test.py +++ b/atr/post/test.py @@ -19,6 +19,7 @@ from typing import Literal import quart import atr.blueprints.post as post +import atr.config as config import atr.form as form import atr.get as get import atr.log as log @@ -33,6 +34,9 @@ async def test_empty( """ URL: /test/empty """ + if config.is_production_mode(): + return quart.abort(404) + msg = "Empty form submitted successfully" log.info(msg) await quart.flash(msg, "success") @@ -46,6 +50,9 @@ async def test_multiple( """ URL: /test/multiple """ + if config.is_production_mode(): + return quart.abort(404) + match multiple_form: case shared.test.AppleForm() as apple: msg = f"Apple order received: variety={apple.variety}, quantity={apple.quantity}, organic={apple.organic}" @@ -67,6 +74,9 @@ async def test_single( """ URL: /test/single """ + if config.is_production_mode(): + return quart.abort(404) + file_names = [f.filename for f in single_form.files] if single_form.files else [] compatibility_names = [f.value for f in single_form.compatibility] if single_form.compatibility else [] if (single_form.message == "Forbidden message!") and (session is not None): diff --git a/atr/principal.py b/atr/principal.py index 55a77f33..dec4b3df 100644 --- a/atr/principal.py +++ b/atr/principal.py @@ -280,7 +280,7 @@ class AuthoriserLDAP: if not self.__cache.outdated(asf_uid): return - if config.get().ALLOW_TESTS and (asf_uid == "test"): + if config.is_test_mode() and (asf_uid == "test"): # The test user does not exist in LDAP, so we hardcode their data committees = frozenset({"test"}) projects = frozenset({"test"}) @@ -411,7 +411,7 @@ def _augment_test_membership( committees: frozenset[str], projects: frozenset[str], ) -> tuple[frozenset[str], frozenset[str]]: - if config.get().ALLOW_TESTS: + if config.is_test_mode(): committees = committees.union({"test"}) projects = projects.union({"test"}) return committees, projects diff --git a/atr/server.py b/atr/server.py index ab441290..786ccb4e 100644 --- a/atr/server.py +++ b/atr/server.py @@ -156,7 +156,7 @@ def _app_create_base(app_config: type[config.AppConfig]) -> base.QuartApp: app.cfg["MAX_SESSION_AGE"] = app.config.get("MAX_SESSION_AGE", 0) app.secret_key = asfquart_secret_key - if not util.is_dev_environment(): + if not config.is_dev_environment(): app.asgi_app = proxy_fix.ProxyFixMiddleware(app.asgi_app, mode="legacy", trusted_hops=1) return app @@ -270,7 +270,7 @@ def _app_setup_context(app: base.QuartApp) -> None: "is_admin_fn": user.is_admin, "is_viewing_as_admin_fn": util.is_user_viewing_as_admin, "is_committee_member_fn": user.is_committee_member, - "is_test_mode": config.get().ALLOW_TESTS, + "is_test_mode": config.is_test_mode(), "post": post, "static_url": util.static_url, "topnav_unfinished_releases": topnav_unfinished_releases, @@ -361,7 +361,7 @@ def _app_setup_logging(app: base.QuartApp, config_mode: config.Mode, app_config: # Output handler: pretty console for dev (Debug and Allow Tests), JSON for non-dev (Docker, etc.) output_handler = logging.StreamHandler(sys.stderr) - use_json_output = app_config.LOG_JSON or (not util.is_dev_environment()) + use_json_output = app_config.LOG_JSON or (not config.is_dev_environment()) if use_json_output: # JSON output should include rendered exceptions output_handler.setFormatter(loggers.create_json_formatter(shared_processors)) @@ -372,7 +372,7 @@ def _app_setup_logging(app: base.QuartApp, config_mode: config.Mode, app_config: log_queue: queue.Queue[logging.LogRecord] = queue.Queue(-1) handlers: list[logging.Handler] = [output_handler] - if util.is_dev_environment(): + if config.is_dev_environment(): handlers.append(log.create_debug_handler()) listener = logging.handlers.QueueListener(log_queue, *handlers, respect_handler_level=True) @@ -432,7 +432,7 @@ def _app_setup_rate_limits(app: base.QuartApp, conf: type[config.AppConfig]): return f"user:{session.uid}" return f"ip:{quart.request.remote_addr}" - if not conf.ALLOW_TESTS: + if not config.is_test_mode(): rate_limiter.RateLimiter( app, default_limits=[ @@ -595,8 +595,6 @@ def _create_app(app_config: type[config.AppConfig]) -> base.QuartApp: if os.sep != "/": raise RuntimeError('ATR requires a POSIX compatible filesystem where os.sep is "/"') config_mode = config.get_mode() - if (not util.is_dev_environment()) and (config_mode == config.Mode.Debug): - raise RuntimeError("Debug mode can only be set in development environment") hot_reload = _is_hot_reload() _validate_config(app_config, hot_reload) _migrate_state(app_config.STATE_DIR, hot_reload) @@ -727,7 +725,7 @@ async def _initialise_pubsub(conf: type[config.AppConfig], app: base.QuartApp): async def _initialise_test_environment(conf: type[config.AppConfig]) -> None: - if not conf.ALLOW_TESTS: + if not config.is_test_mode(): return async with db.session() as data: @@ -934,7 +932,7 @@ def _register_routes(app: base.QuartApp) -> None: # noqa: C901 exc_info = (type(error), error, error.__traceback__) log.error("Unhandled exception", exc_info=exc_info) - if util.is_dev_environment(): + if config.is_dev_environment(): return await template.render( "error.html", error=str(error), @@ -1022,6 +1020,8 @@ def _set_file_permissions_to_read_only() -> None: def _validate_config(app_config: type[config.AppConfig], hot_reload: bool) -> None: + config.validate() + # Custom configuration for the database path is no longer supported configured_path = app_config.SQLITE_DB_PATH if configured_path != "database/atr.db": diff --git a/atr/storage/writers/keys.py b/atr/storage/writers/keys.py index 1caca975..3a29ae1a 100644 --- a/atr/storage/writers/keys.py +++ b/atr/storage/writers/keys.py @@ -185,7 +185,7 @@ class FoundationCommitter(GeneralPublic): async def test_user_delete_all(self, test_uid: str) -> outcome.Outcome[int]: """Delete all OpenPGP keys and their links for a test user.""" - if not config.get().ALLOW_TESTS: + if not config.is_test_mode(): return outcome.Error(storage.AccessError("Test key deletion not enabled")) try: @@ -341,7 +341,7 @@ and was published by the committee.\ if uids == test_key_uids: # Allow the test key - if config.get().ALLOW_TESTS and (self.__asf_uid == "test"): + if config.is_test_mode() and (self.__asf_uid == "test"): # TODO: "test" is already an admin user # But we want to narrow that down to only actions like this # TODO: Add include_test: bool to user.is_admin? diff --git a/atr/storage/writers/mail.py b/atr/storage/writers/mail.py index 729d1276..78dcc699 100644 --- a/atr/storage/writers/mail.py +++ b/atr/storage/writers/mail.py @@ -17,6 +17,7 @@ from __future__ import annotations +import atr.config as config import atr.db as db import atr.log as log import atr.mail as mail @@ -53,7 +54,7 @@ class FoundationCommitter(GeneralPublic): self.__asf_uid = asf_uid async def send(self, message: mail.Message, category: mail.MailFooterCategory) -> tuple[str, list[str]]: - is_dev = util.is_dev_environment() + is_dev = config.is_dev_environment() if is_dev: log.info(f"Dev environment detected, not sending email to {message.email_to}") diff --git a/atr/storage/writers/release.py b/atr/storage/writers/release.py index 4668b6c7..b1402a55 100644 --- a/atr/storage/writers/release.py +++ b/atr/storage/writers/release.py @@ -142,7 +142,7 @@ class CommitteeParticipant(FoundationCommitter): # In test mode, delete the counter for test committee releases # This allows revision numbers to be reused in testing committee = release.project.committee - is_test_release = config.get().ALLOW_TESTS and (committee is not None) and (committee.key == "test") + is_test_release = config.is_test_mode() and (committee is not None) and (committee.key == "test") if is_test_release: counter_delete_stmt = sqlmodel.delete(sql.RevisionCounter).where( via(sql.RevisionCounter.release_key) == release_key @@ -445,7 +445,7 @@ class CommitteeParticipant(FoundationCommitter): if not project: raise storage.AccessError(f"Project {project_key} not found") - tests_allowed = config.get().ALLOW_TESTS + tests_allowed = config.is_test_mode() committee = project.committee is_test_committee = tests_allowed and (committee is not None) and (committee.key == "test") should_skip_auth = is_test_committee diff --git a/atr/tabulate.py b/atr/tabulate.py index e1b7e2fd..3a279c4f 100644 --- a/atr/tabulate.py +++ b/atr/tabulate.py @@ -18,6 +18,7 @@ import time from collections.abc import Generator +import atr.config as config import atr.db as db import atr.log as log import atr.models as models @@ -29,7 +30,7 @@ MAX_THREAD_MESSAGES = 10000 async def vote_committee(thread_id: str, release: sql.Release) -> sql.Committee | None: committee = release.project.committee - if util.is_dev_environment(): + if config.is_dev_environment(): message_count = 0 async for _mid, msg in util.thread_messages(thread_id): message_count += 1 @@ -450,7 +451,7 @@ def _vote_resolution_votes( async def _vote_status(asf_uid: str, list_raw: str, committee: sql.Committee | None) -> models.tabulate.VoteStatus: status = models.tabulate.VoteStatus.UNKNOWN - if util.is_dev_environment(): + if config.is_dev_environment(): committee_label = list_raw.split(".apache.org", 1)[0].split(".", 1)[-1] async with db.session() as data: committee = await data.committee(key=committee_label).get() diff --git a/atr/user.py b/atr/user.py index f4fa210b..56e27115 100644 --- a/atr/user.py +++ b/atr/user.py @@ -42,7 +42,7 @@ async def candidate_drafts(uid: str, user_projects: list[sql.Project] | None = N def is_admin(user_id: str | None) -> bool: if user_id is None: return False - if config.get().ALLOW_TESTS and (user_id == "test"): + if config.is_test_mode() and (user_id == "test"): return True if util.is_user_session_downgraded(): return False @@ -54,7 +54,7 @@ def is_admin(user_id: str | None) -> bool: async def is_admin_async(user_id: str | None) -> bool: if user_id is None: return False - if config.get().ALLOW_TESTS and (user_id == "test"): + if config.is_test_mode() and (user_id == "test"): return True if util.is_user_session_downgraded(): return False @@ -86,9 +86,9 @@ async def projects(uid: str, committee_only: bool = False, super_project: bool = if p.committee is None: continue - # Allow access to test project when ALLOW_TESTS is enabled + # Allow access to test project in Test mode # This means that the Test project will show in the user interface for everyone - if config.get().ALLOW_TESTS and (p.committee.key == "test"): + if config.is_test_mode() and (p.committee.key == "test"): user_projects.append(p) continue diff --git a/atr/util.py b/atr/util.py index dd90de83..b716a98c 100644 --- a/atr/util.py +++ b/atr/util.py @@ -594,14 +594,6 @@ def intersect_algs(policy: dict[str, Any], policy_key: str, supported: set[bytes return [a for a in algs if isinstance(a, str) and (a.encode("ascii") in supported)] -def is_dev_environment() -> bool: - conf = config.get() - for development_host in ("127.0.0.1", "atr", "atr-dev", "localhost.apache.org"): - if (conf.APP_HOST == development_host) or conf.APP_HOST.startswith(f"{development_host}:"): - return True - return False - - async def is_dir_resolve(path: pathlib.Path) -> pathlib.Path | None: try: resolved_path = await asyncio.to_thread(path.resolve) @@ -623,10 +615,6 @@ def is_disallowed_dotfile(segment: str) -> bool: return True -def is_ldap_configured() -> bool: - return ldap.get_bind_credentials() is not None - - def is_user_session_downgraded() -> bool: """Check whether a user session is downgraded from active admin privileges.""" try: @@ -1024,7 +1012,7 @@ async def task_archive_url(task_mid: str, recipient: str | None = None) -> str | if "@" not in task_mid: return None - if is_dev_environment() and (task_mid in DEV_THREAD_URLS): + if config.is_dev_environment() and (task_mid in DEV_THREAD_URLS): return DEV_THREAD_URLS[task_mid] recipient_address = recipient or USER_TESTS_ADDRESS diff --git a/atr/worker.py b/atr/worker.py index 41ee0843..494cdabf 100644 --- a/atr/worker.py +++ b/atr/worker.py @@ -43,7 +43,6 @@ import atr.models.sql as sql import atr.tasks as tasks import atr.tasks.checks as checks import atr.tasks.task as task -import atr.util as util # Resource limits, 5 minutes and 3GB _CPU_LIMIT_SECONDS: Final = 300 @@ -230,8 +229,6 @@ async def _task_process(task_id: int, task_type: str, task_args: list[str] | dic """Process a claimed task.""" import atr.config as config - conf = config.get() - log.info(f"Processing task {task_id} ({task_type}) with raw args {task_args}") try: task_type_member = sql.TaskType(task_type) @@ -242,11 +239,11 @@ async def _task_process(task_id: int, task_type: str, task_args: list[str] | dic task_results: results.Results | None try: - # In any of these three cases, we will skip the LDAP check - is_system = asf_uid == "system" - is_test = conf.ALLOW_TESTS and (asf_uid == "test") - is_dev_without_ldap = util.is_dev_environment() and (not util.is_ldap_configured()) - if not (is_system or is_test or is_dev_without_ldap): + if ( + asf_uid != "system" + and not (config.is_test_mode() and asf_uid == "test") + and (config.is_production_mode() or config.is_ldap_configured()) + ): user_account = await ldap.account_lookup(asf_uid) if (user_account is None) or ldap.is_banned(user_account): raise RuntimeError(f"Account '{asf_uid}' is banned or does not exist") diff --git a/docker-compose.yml b/docker-compose.yml index 29792500..cac6f478 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -4,7 +4,7 @@ services: context: . dockerfile: Dockerfile.alpine environment: - - ALLOW_TESTS=1 + - TESTS=1 - APP_HOST=atr:8080 - BIND=0.0.0.0:8080 - SSH_HOST=0.0.0.0 diff --git a/tests/docker-compose.yml b/tests/docker-compose.yml index 8a2d4000..cea72af1 100644 --- a/tests/docker-compose.yml +++ b/tests/docker-compose.yml @@ -4,7 +4,7 @@ services: context: .. dockerfile: Dockerfile.alpine environment: - - ALLOW_TESTS=1 + - TESTS=1 - APP_HOST=atr:8080 - BIND=0.0.0.0:8080 - SSH_HOST=0.0.0.0 @@ -22,7 +22,7 @@ services: context: .. dockerfile: Dockerfile.alpine environment: - - ALLOW_TESTS=1 + - TESTS=1 - APP_HOST=atr-dev:8080 - BIND=0.0.0.0:8080 - SSH_HOST=0.0.0.0 diff --git a/tests/unit/test_ldap.py b/tests/unit/test_ldap.py index a1260cee..d4cf2bc9 100644 --- a/tests/unit/test_ldap.py +++ b/tests/unit/test_ldap.py @@ -39,12 +39,10 @@ class MockConfig: 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 @@ -126,21 +124,21 @@ async def test_is_active_returns_true_when_ldap_not_configured(monkeypatch: "Mon @pytest.mark.asyncio 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)) + monkeypatch.setattr("atr.config.is_test_mode", lambda: True) assert await ldap.is_active("test") is True @pytest.mark.asyncio 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)) + monkeypatch.setattr("atr.config.is_test_mode", lambda: True) assert await ldap.is_active("test-banned") is False @pytest.mark.asyncio 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.config.is_test_mode", lambda: True) monkeypatch.setattr("atr.ldap.account_lookup", mock.AsyncMock(return_value=None)) assert await ldap.is_active("ghost") is False @@ -149,7 +147,7 @@ async def test_is_active_returns_false_when_account_not_found(monkeypatch: "Monk 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.config.is_test_mode", lambda: True) monkeypatch.setattr("atr.ldap.account_lookup", mock.AsyncMock(return_value=account)) assert await ldap.is_active("alice") is True @@ -160,7 +158,7 @@ async def test_is_active_returns_false_for_banned_account(monkeypatch: "MonkeyPa {"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.config.is_test_mode", lambda: True) monkeypatch.setattr("atr.ldap.account_lookup", mock.AsyncMock(return_value=account)) assert await ldap.is_active("bad") is False diff --git a/tests/unit/test_user.py b/tests/unit/test_user.py index 23c93ce2..a05931bc 100644 --- a/tests/unit/test_user.py +++ b/tests/unit/test_user.py @@ -19,6 +19,7 @@ from typing import TYPE_CHECKING import pytest +import atr.config as config import atr.user as user if TYPE_CHECKING: @@ -31,8 +32,7 @@ class MockApp: class MockConfig: - def __init__(self, allow_tests: bool = False, admin_users_additional: str = ""): - self.ALLOW_TESTS = allow_tests + def __init__(self, admin_users_additional: str = ""): self.ADMIN_USERS_ADDITIONAL = admin_users_additional @@ -61,7 +61,7 @@ async def test_is_admin_async_returns_true_for_cached_admin(mock_app: MockApp, m @pytest.mark.asyncio 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)) + monkeypatch.setattr("atr.config.get_mode", lambda: config.Mode.Test) mock_app.extensions["admins"] = frozenset() assert await user.is_admin_async("test") is True @@ -74,7 +74,8 @@ def test_is_admin_returns_false_for_none(mock_app: MockApp, monkeypatch: "Monkey 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)) + monkeypatch.setattr("atr.config.get_mode", lambda: config.Mode.Debug) + monkeypatch.setattr("atr.config.get", lambda: MockConfig()) mock_app.extensions["admins"] = frozenset() assert user.is_admin("test") is False @@ -102,6 +103,6 @@ def test_is_admin_returns_true_for_cached_admin(mock_app: MockApp, monkeypatch: 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)) + monkeypatch.setattr("atr.config.get_mode", lambda: config.Mode.Test) mock_app.extensions["admins"] = frozenset() assert user.is_admin("test") is True diff --git a/tests/unit/test_vote.py b/tests/unit/test_vote.py index f204c9ed..595d9c94 100644 --- a/tests/unit/test_vote.py +++ b/tests/unit/test_vote.py @@ -30,8 +30,7 @@ class MockApp: class MockConfig: - def __init__(self, allow_tests: bool = False, admin_users_additional: str = "") -> None: - self.ALLOW_TESTS = allow_tests + def __init__(self, admin_users_additional: str = "") -> None: self.ADMIN_USERS_ADDITIONAL = admin_users_additional --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
