mirror of
https://github.com/jhd3197/ServerKit.git
synced 2026-10-29 08:07:37 +09:00
refactor(auth): remove the last inline role checks
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) <noreply@anthropic.com>
This commit is contained in:
1 parent
b182deaff9
commit
107fdbdfde
4 files changed
+33
-119
No files matched your search
+15
-24
@@ -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,
|
||||
|
||||
+16
-75
@@ -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']
|
||||
|
||||
@@ -1 +1 @@
|
||||
87
|
||||
69
|
||||
@@ -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",
|
||||
|
||||
Reference in new issue
Block a user