From 107fdbdfde1f780daeeaf0ddd6ae9c1e46bbdd47 Mon Sep 17 00:00:00 2001 From: Juan Denis Date: Tue, 18 Aug 2026 22:06:35 -0400 Subject: [PATCH] refactor(auth): remove the last inline role checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Plan 76 milestone A, the third row. 23 handlers decided admin access inline, all in two files, in two shapes. 18 were the mid-route gate this milestone already built a door for: current_user_id = get_jwt_identity() user = User.query.get(current_user_id) if user.role != 'admin': return jsonify({'error': 'Admin access required'}), 403 which is four lines to say require_admin_user(), and carried the two-line identity lookup with it. Migrating them closes both rows at once: the identity census drops 87 -> 69 and the controller-boundary baseline loses 18 persistence crossings (528 -> 510), with no new entries. It also fixes a latent 500: where the check omitted the `not user or` guard, a token naming a deleted user hit `None.role` instead of answering 403. The other 5 are `not user.is_admin and app.user_id != user.id` — admin OR owner, which is a different question. Those keep their shape and only converge the truth source from `role != 'admin'` to `is_admin`; routing them through can_access_app would silently widen access, since that also honours resource grants. The migration initially left `current_user_id` referenced but unbound in four places. Two were caught by tests; the other two (create_manual_app, upload_app_archive) have no coverage and were caught by an AST scan for names used-but-never-assigned. Worth remembering that a mechanical removal needs the scan, not just the suite. Gate: -k "system or apps or app_ or host_snapshot or manual_app or upload", 304 passed / 13 skipped; all seven ratchets green (52 passed). Co-Authored-By: Claude Opus 5 (1M context) --- backend/app/api/apps.py | 39 +++----- backend/app/api/system.py | 91 ++++--------------- backend/tests/IDENTITY_DOOR_CEILING | 2 +- .../api_controller_boundary_baseline.json | 20 +--- 4 files changed, 33 insertions(+), 119 deletions(-) diff --git a/backend/app/api/apps.py b/backend/app/api/apps.py index 8f67289b..7a9d115c 100644 --- a/backend/app/api/apps.py +++ b/backend/app/api/apps.py @@ -43,7 +43,7 @@ from app.services.upload_service import ( get_current_version, ) from app import paths -from app.middleware.rbac import get_current_user +from app.middleware.rbac import get_current_user, require_admin_user apps_bp = Blueprint('apps', __name__) @@ -505,7 +505,7 @@ def set_app_workspace(app_id): if not app: return jsonify({'error': 'Application not found'}), 404 # Use user.id (int) for comparisons — get_jwt_identity() is the stringified token id. - if user.role != 'admin' and app.user_id != user.id: + if not user.is_admin and app.user_id != user.id: return jsonify({'error': 'Access denied'}), 403 target = (request.get_json() or {}).get('workspace_id') @@ -515,7 +515,7 @@ def set_app_workspace(app_id): ws = Workspace.query.get(target) if not ws: return jsonify({'error': 'Workspace not found'}), 404 - if user.role != 'admin' and WorkspaceService.get_user_role(ws.id, user.id) is None: + if not user.is_admin and WorkspaceService.get_user_role(ws.id, user.id) is None: return jsonify({'error': 'Not a member of the target workspace'}), 403 # Role reconciliation (#33): a 'viewer' member can't move resources into it. if not WorkspaceService.can_write_in_workspace(user, ws.id): @@ -728,7 +728,7 @@ def list_app_grants(app_id): app = Application.query_active().filter_by(id=app_id).first() if not app: return jsonify({'error': 'Application not found'}), 404 - if user.role != 'admin' and app.user_id != user.id: + if not user.is_admin and app.user_id != user.id: return jsonify({'error': 'Access denied'}), 403 grants = ResourceGrantService.list_for_resource('application', app.id) return jsonify({'grants': [g.to_dict() for g in grants]}), 200 @@ -743,7 +743,7 @@ def grant_app_access(app_id): app = Application.query_active().filter_by(id=app_id).first() if not app: return jsonify({'error': 'Application not found'}), 404 - if user.role != 'admin' and app.user_id != user.id: + if not user.is_admin and app.user_id != user.id: return jsonify({'error': 'Access denied'}), 403 data = request.get_json() or {} grantee_id = data.get('user_id') @@ -771,7 +771,7 @@ def revoke_app_access(app_id, grant_id): app = Application.query_active().filter_by(id=app_id).first() if not app: return jsonify({'error': 'Application not found'}), 404 - if user.role != 'admin' and app.user_id != user.id: + if not user.is_admin and app.user_id != user.id: return jsonify({'error': 'Access denied'}), 403 ok = ResourceGrantService.revoke(grant_id, resource_type='application', resource_id=app.id) return jsonify({'success': ok}), (200 if ok else 404) @@ -781,10 +781,7 @@ def revoke_app_access(app_id, grant_id): @jwt_required() def create_app_from_repository(): """Create a new application by cloning a Git repository.""" - current_user_id = get_jwt_identity() - user = User.query.get(current_user_id) - if not user or user.role != 'admin': - return jsonify({'error': 'Admin access required'}), 403 + user = require_admin_user() data = request.get_json() or {} name = _service_slug(data.get('name')) @@ -810,7 +807,7 @@ def create_app_from_repository(): if source_connection_id: try: source_repo = SourceConnectionService.get_authenticated_clone_url( - user_id=current_user_id, + user_id=user.id, connection_id=int(source_connection_id), full_name=repository_full_name, ) @@ -915,7 +912,7 @@ def create_app_from_repository(): app_type=resolved_app_type, status='stopped', root_path=app_path, - user_id=current_user_id, + user_id=user.id, port=port, buildpack_type=buildpack_type, buildpack_plan=json.dumps(buildpack_plan) if buildpack_plan else None, @@ -958,7 +955,7 @@ def create_app_from_repository(): try: from app.services.manifest_persistence_service import ManifestPersistenceService manifest_summary = ManifestPersistenceService.apply_import( - app, manifest, user_id=current_user_id, + app, manifest, user_id=user.id, source_repo=deploy_repo_url, source_ref=branch or 'main', ) except Exception: @@ -973,7 +970,7 @@ def create_app_from_repository(): try: from app.services.deployment_job_service import DeploymentJobService enqueue_result = DeploymentJobService.enqueue_app_deploy( - app, user_id=current_user_id, trigger='install') + app, user_id=user.id, trigger='install') if enqueue_result.get('success'): deploy_job_id = enqueue_result.get('job_id') else: @@ -1083,10 +1080,7 @@ def create_app(): @jwt_required() def create_manual_app(): """Register an app that already exists on the server (manual/local).""" - current_user_id = get_jwt_identity() - user = User.query.get(current_user_id) - if not user or user.role != 'admin': - return jsonify({'error': 'Admin access required'}), 403 + user = require_admin_user() data = request.get_json() or {} name = _service_slug(data.get('name')) @@ -1150,7 +1144,7 @@ def create_manual_app(): systemd_unit=systemd_unit, managed_by=managed_by, ingress_plane=_resolve_ingress_plane(data, app_type, managed_by), - user_id=current_user_id, + user_id=user.id, workspace_id=ws_id, project_id=project_id, environment_id=environment_id, @@ -1172,10 +1166,7 @@ def create_manual_app(): @jwt_required() def upload_app_archive(): """Create or update an app by uploading a zip archive.""" - current_user_id = get_jwt_identity() - user = User.query.get(current_user_id) - if not user or user.role != 'admin': - return jsonify({'error': 'Admin access required'}), 403 + user = require_admin_user() if 'file' not in request.files: return jsonify({'error': 'No file provided'}), 400 @@ -1281,7 +1272,7 @@ def upload_app_archive(): ), version=new_version, upload_path=upload_archive_path, - user_id=current_user_id, + user_id=user.id, workspace_id=ws_id, project_id=project_id, environment_id=environment_id, diff --git a/backend/app/api/system.py b/backend/app/api/system.py index 204eb295..5bb78015 100644 --- a/backend/app/api/system.py +++ b/backend/app/api/system.py @@ -11,6 +11,7 @@ from app.services import install_profile_service from app.services.site_domain_service import SiteDomainService from app.utils.domain import is_valid_canonical_domain from app.utils.version import get_panel_version, get_install_dir +from app.middleware.rbac import require_admin_user # Cache for update check (to avoid hitting GitHub API too often) _update_cache = { @@ -103,11 +104,7 @@ def check_update(): @system_bp.route('/metrics', methods=['GET']) @jwt_required() def get_metrics(): - current_user_id = get_jwt_identity() - user = User.query.get(current_user_id) - - if user.role != 'admin': - return jsonify({'error': 'Admin access required'}), 403 + require_admin_user() metrics = SystemService.get_all_metrics() return jsonify(metrics), 200 @@ -117,11 +114,7 @@ def get_metrics(): @jwt_required() def get_system_info(): """Get system information (hostname, IP, kernel, CPU info).""" - current_user_id = get_jwt_identity() - user = User.query.get(current_user_id) - - if user.role != 'admin': - return jsonify({'error': 'Admin access required'}), 403 + require_admin_user() return jsonify(SystemService.get_system_info()), 200 @@ -129,11 +122,7 @@ def get_system_info(): @system_bp.route('/cpu', methods=['GET']) @jwt_required() def get_cpu_metrics(): - current_user_id = get_jwt_identity() - user = User.query.get(current_user_id) - - if user.role != 'admin': - return jsonify({'error': 'Admin access required'}), 403 + require_admin_user() return jsonify(SystemService.get_cpu_metrics()), 200 @@ -141,11 +130,7 @@ def get_cpu_metrics(): @system_bp.route('/memory', methods=['GET']) @jwt_required() def get_memory_metrics(): - current_user_id = get_jwt_identity() - user = User.query.get(current_user_id) - - if user.role != 'admin': - return jsonify({'error': 'Admin access required'}), 403 + require_admin_user() return jsonify(SystemService.get_memory_metrics()), 200 @@ -153,11 +138,7 @@ def get_memory_metrics(): @system_bp.route('/disk', methods=['GET']) @jwt_required() def get_disk_metrics(): - current_user_id = get_jwt_identity() - user = User.query.get(current_user_id) - - if user.role != 'admin': - return jsonify({'error': 'Admin access required'}), 403 + require_admin_user() return jsonify(SystemService.get_disk_metrics()), 200 @@ -165,11 +146,7 @@ def get_disk_metrics(): @system_bp.route('/network', methods=['GET']) @jwt_required() def get_network_metrics(): - current_user_id = get_jwt_identity() - user = User.query.get(current_user_id) - - if user.role != 'admin': - return jsonify({'error': 'Admin access required'}), 403 + require_admin_user() return jsonify(SystemService.get_network_metrics()), 200 @@ -272,11 +249,7 @@ def get_resource_tier(): Returns tier, specs, and feature permissions based on server resources. Results are cached for 1 hour unless refresh=true is passed. """ - current_user_id = get_jwt_identity() - user = User.query.get(current_user_id) - - if user.role != 'admin': - return jsonify({'error': 'Admin access required'}), 403 + require_admin_user() force_refresh = request.args.get('refresh', 'false').lower() == 'true' tier_info = ResourceTierService.get_tier_info(force_refresh=force_refresh) @@ -293,11 +266,7 @@ def get_host_snapshot(): filesystem the panel actually writes to, which mounts are missing from /etc/fstab, and what changed since the previous boot. """ - current_user_id = get_jwt_identity() - user = User.query.get(current_user_id) - - if not user or user.role != 'admin': - return jsonify({'error': 'Admin access required'}), 403 + require_admin_user() from app.services import host_snapshot_service return jsonify(host_snapshot_service.current_state()), 200 @@ -307,11 +276,7 @@ def get_host_snapshot(): @jwt_required() def get_host_snapshot_history(): """Recent host snapshots, newest first.""" - current_user_id = get_jwt_identity() - user = User.query.get(current_user_id) - - if not user or user.role != 'admin': - return jsonify({'error': 'Admin access required'}), 403 + require_admin_user() from app.models.host_snapshot import HostSnapshot @@ -337,11 +302,7 @@ def get_capacity(): /resource-tier remains for callers that only want the tier label; this one additionally answers "what did we install" and "can this box host apps". """ - current_user_id = get_jwt_identity() - user = User.query.get(current_user_id) - - if not user or user.role != 'admin': - return jsonify({'error': 'Admin access required'}), 403 + require_admin_user() force_refresh = request.args.get('refresh', 'false').lower() == 'true' tier_info = ResourceTierService.get_tier_info(force_refresh=force_refresh) @@ -360,11 +321,7 @@ def get_capacity(): @jwt_required() def set_capacity_profile(): """Record an operator's profile change made after install.""" - current_user_id = get_jwt_identity() - user = User.query.get(current_user_id) - - if not user or user.role != 'admin': - return jsonify({'error': 'Admin access required'}), 403 + user = require_admin_user() data = request.get_json() or {} profile = data.get('profile') @@ -388,11 +345,7 @@ def get_server_time(): @jwt_required() def get_timezones(): """Get list of available timezones.""" - current_user_id = get_jwt_identity() - user = User.query.get(current_user_id) - - if user.role != 'admin': - return jsonify({'error': 'Admin access required'}), 403 + require_admin_user() timezones = SystemService.get_available_timezones() return jsonify({'timezones': timezones}), 200 @@ -402,11 +355,7 @@ def get_timezones(): @jwt_required() def set_timezone(): """Set server timezone (admin only).""" - current_user_id = get_jwt_identity() - user = User.query.get(current_user_id) - - if user.role != 'admin': - return jsonify({'error': 'Admin access required'}), 403 + require_admin_user() data = request.get_json() if not data or 'timezone' not in data: @@ -423,11 +372,7 @@ def set_timezone(): @system_bp.route('/processes', methods=['GET']) @jwt_required() def get_processes(): - current_user_id = get_jwt_identity() - user = User.query.get(current_user_id) - - if user.role != 'admin': - return jsonify({'error': 'Admin access required'}), 403 + require_admin_user() processes = [] for proc in psutil.process_iter(['pid', 'name', 'cpu_percent', 'memory_percent', 'status']): @@ -455,11 +400,7 @@ def get_processes(): @system_bp.route('/services', methods=['GET']) @jwt_required() def get_services(): - current_user_id = get_jwt_identity() - user = User.query.get(current_user_id) - - if user.role != 'admin': - return jsonify({'error': 'Admin access required'}), 403 + require_admin_user() # Common services to check service_names = ['nginx', 'mysql', 'postgresql', 'redis', 'docker', 'php-fpm'] diff --git a/backend/tests/IDENTITY_DOOR_CEILING b/backend/tests/IDENTITY_DOOR_CEILING index 84df3526..b5489e5e 100644 --- a/backend/tests/IDENTITY_DOOR_CEILING +++ b/backend/tests/IDENTITY_DOOR_CEILING @@ -1 +1 @@ -87 +69 diff --git a/backend/tests/api_controller_boundary_baseline.json b/backend/tests/api_controller_boundary_baseline.json index 59e3296b..249669a2 100644 --- a/backend/tests/api_controller_boundary_baseline.json +++ b/backend/tests/api_controller_boundary_baseline.json @@ -3,7 +3,7 @@ "policy": "Exact reviewed inventory of direct persistence, subprocess, and filesystem mutation calls in Flask API controllers. New calls are forbidden; remove entries as controllers delegate to services.", "counts": { "filesystem": 11, - "persistence": 517 + "persistence": 499 }, "owners": { "api/admin.py": { @@ -272,14 +272,12 @@ "api/apps.py::create_app_from_repository::filesystem::shutil.rmtree(app_path, ignore_errors=True)::1", "api/apps.py::create_app_from_repository::persistence::Application.query.filter_by(name=name, server_id=None).first()::1", "api/apps.py::create_app_from_repository::persistence::Application.query.get(app.id)::1", - "api/apps.py::create_app_from_repository::persistence::User.query.get(current_user_id)::1", "api/apps.py::create_app_from_repository::persistence::db.session.add(app)::1", "api/apps.py::create_app_from_repository::persistence::db.session.commit()::1", "api/apps.py::create_app_from_repository::persistence::db.session.commit()::2", "api/apps.py::create_app_from_repository::persistence::db.session.delete(existing_app)::1", "api/apps.py::create_app_from_repository::persistence::db.session.rollback()::1", "api/apps.py::create_manual_app::persistence::Application.query.filter_by(name=name).first()::1", - "api/apps.py::create_manual_app::persistence::User.query.get(current_user_id)::1", "api/apps.py::create_manual_app::persistence::db.session.add(app)::1", "api/apps.py::create_manual_app::persistence::db.session.commit()::1", "api/apps.py::delete_app::persistence::Application.query_active().filter_by(id=app_id).first()::1", @@ -367,7 +365,6 @@ "api/apps.py::upload_app_archive::filesystem::os.remove(zippath)::4", "api/apps.py::upload_app_archive::persistence::Application.query.filter_by(name=name).first()::1", "api/apps.py::upload_app_archive::persistence::Application.query_active().filter_by(name=name).first()::1", - "api/apps.py::upload_app_archive::persistence::User.query.get(current_user_id)::1", "api/apps.py::upload_app_archive::persistence::db.session.add(app)::1", "api/apps.py::upload_app_archive::persistence::db.session.commit()::1", "api/apps.py::upload_app_archive::persistence::db.session.commit()::2", @@ -687,23 +684,8 @@ "api/survey.py::run_survey::persistence::Server.query.get(server_id)::1", "api/survey.py::set_management_mode::persistence::Server.query.get(server_id)::1", "api/survey.py::set_management_mode::persistence::db.session.commit()::1", - "api/system.py::get_capacity::persistence::User.query.get(current_user_id)::1", - "api/system.py::get_cpu_metrics::persistence::User.query.get(current_user_id)::1", - "api/system.py::get_disk_metrics::persistence::User.query.get(current_user_id)::1", - "api/system.py::get_host_snapshot::persistence::User.query.get(current_user_id)::1", "api/system.py::get_host_snapshot_history::persistence::HostSnapshot.query.order_by(HostSnapshot.captured_at.desc(), HostSnapshot.id.desc()).limit(limit).all()::1", - "api/system.py::get_host_snapshot_history::persistence::User.query.get(current_user_id)::1", - "api/system.py::get_memory_metrics::persistence::User.query.get(current_user_id)::1", - "api/system.py::get_metrics::persistence::User.query.get(current_user_id)::1", - "api/system.py::get_network_metrics::persistence::User.query.get(current_user_id)::1", - "api/system.py::get_processes::persistence::User.query.get(current_user_id)::1", - "api/system.py::get_resource_tier::persistence::User.query.get(current_user_id)::1", - "api/system.py::get_services::persistence::User.query.get(current_user_id)::1", - "api/system.py::get_system_info::persistence::User.query.get(current_user_id)::1", "api/system.py::get_system_notices::persistence::User.query.get(current_user_id)::1", - "api/system.py::get_timezones::persistence::User.query.get(current_user_id)::1", - "api/system.py::set_capacity_profile::persistence::User.query.get(current_user_id)::1", - "api/system.py::set_timezone::persistence::User.query.get(current_user_id)::1", "api/telemetry.py::list_event_types::persistence::db.session.query(SystemEvent.event_type)::1", "api/telemetry.py::list_sources::persistence::db.session.query(SystemEvent.source)::1", "api/templates.py::check_app_update::persistence::Application.query_active().filter_by(id=app_id).first()::1",