diff --git a/app/__init__.py b/app/__init__.py index 7835dad..66f5629 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -112,7 +112,10 @@ def create_app(config_name=None): @login_manager.user_loader def load_user(user_id): - return User.query.get(int(user_id)) + # db.session.get() is the SQLAlchemy 2.x successor to the deprecated + # Model.query.get(). Both hit the identity map first (no SQL if the + # object is already in session), then fall back to a SELECT by PK. + return db.session.get(User, int(user_id)) # ── Context processors ──────────────────────────────────────────────────── @app.context_processor diff --git a/app/models.py b/app/models.py index bd5a7ba..70bfd7e 100644 --- a/app/models.py +++ b/app/models.py @@ -124,11 +124,34 @@ class Ticket(db.Model): history = db.relationship('TicketHistory', backref='ticket', lazy='dynamic', cascade='all, delete-orphan') def generate_ticket_number(self): - """Generate unique ticket number like TKT-20240101-0001""" + """Generate a unique ticket number like TKT-20240101-0001. + + Concurrency safety + ------------------ + The naive approach — query the last ticket, increment its sequence, + then insert — has a TOCTOU race: two concurrent requests can both read + the same "last" ticket and generate the same next number, causing an + IntegrityError at commit time. + + We eliminate the race by appending FOR UPDATE to the SELECT. This + acquires a row-level write lock on the last matching ticket for the + duration of the current transaction, serialising concurrent callers + through the database rather than through application code. The lock is + released automatically when the transaction commits or rolls back. + + MySQL / MariaDB: with_for_update() emits SELECT … FOR UPDATE. + SQLite (dev/test): SELECT … FOR UPDATE is silently ignored, which is + acceptable because SQLite's connection-level locking already prevents + concurrent writes in practice. + """ date_str = datetime.utcnow().strftime('%Y%m%d') - last = Ticket.query.filter( - Ticket.ticket_number.like(f'TKT-{date_str}-%') - ).order_by(Ticket.id.desc()).first() + last = ( + Ticket.query + .filter(Ticket.ticket_number.like(f'TKT-{date_str}-%')) + .order_by(Ticket.id.desc()) + .with_for_update() # acquires a write lock; serialises concurrent callers + .first() + ) if last: seq = int(last.ticket_number.split('-')[-1]) + 1 else: diff --git a/app/routes/admin.py b/app/routes/admin.py index 4c73fac..24a413d 100644 --- a/app/routes/admin.py +++ b/app/routes/admin.py @@ -11,6 +11,7 @@ from app import db from app.models import (User, Ticket, Comment, ActivityLog, KnowledgeBase, KBAttachment, UserRole, TicketStatus) from app.services.log_service import log_action +from app.services.validation_service import validate_password, validate_file admin_bp = Blueprint('admin', __name__, url_prefix='/admin') logger = logging.getLogger(__name__) @@ -73,20 +74,196 @@ def it_required(f): @login_required @it_required def index(): + from sqlalchemy import func, case + + now = datetime.utcnow() + day7 = now - timedelta(days=7) + day30 = now - timedelta(days=30) + + # ── Headline stats ──────────────────────────────────────────────────────── stats = { - 'total_tickets' : Ticket.query.count(), - 'open' : Ticket.query.filter_by(status=TicketStatus.OPEN).count(), - 'in_progress' : Ticket.query.filter_by(status=TicketStatus.IN_PROGRESS).count(), - 'resolved' : Ticket.query.filter_by(status=TicketStatus.RESOLVED).count(), - 'closed' : Ticket.query.filter_by(status=TicketStatus.CLOSED).count(), - 'total_users' : User.query.filter_by(is_active=True).count(), - 'employees' : User.query.filter_by(role=UserRole.EMPLOYEE, is_active=True).count(), - 'it_staff' : User.query.filter( - User.role.in_([UserRole.IT_STAFF, UserRole.ADMIN]), - User.is_active == True).count(), + 'total_tickets' : Ticket.query.count(), + 'open' : Ticket.query.filter_by(status=TicketStatus.OPEN).count(), + 'in_progress' : Ticket.query.filter_by(status=TicketStatus.IN_PROGRESS).count(), + 'resolved' : Ticket.query.filter_by(status=TicketStatus.RESOLVED).count(), + 'closed' : Ticket.query.filter_by(status=TicketStatus.CLOSED).count(), + 'total_users' : User.query.filter_by(is_active=True).count(), + 'employees' : User.query.filter_by(role=UserRole.EMPLOYEE, is_active=True).count(), + 'it_staff' : User.query.filter( + User.role.in_([UserRole.IT_STAFF, UserRole.ADMIN]), + User.is_active == True).count(), } - recent_logs = ActivityLog.query.order_by(ActivityLog.created_at.desc()).limit(20).all() - return render_template('admin/index.html', stats=stats, recent_logs=recent_logs) + + # ── IT performance metrics ──────────────────────────────────────────────── + # Avg resolution time (hours) — resolved tickets only + resolved_tickets = Ticket.query.filter( + Ticket.resolved_at.isnot(None), + Ticket.created_at.isnot(None), + ).all() + if resolved_tickets: + total_hours = sum( + (t.resolved_at - t.created_at).total_seconds() / 3600 + for t in resolved_tickets + ) + avg_resolution_hours = round(total_hours / len(resolved_tickets), 1) + else: + avg_resolution_hours = None + + # Resolved last 7 / 30 days + resolved_7d = Ticket.query.filter(Ticket.resolved_at >= day7).count() + resolved_30d = Ticket.query.filter(Ticket.resolved_at >= day30).count() + + # New tickets last 7 / 30 days + new_7d = Ticket.query.filter(Ticket.created_at >= day7).count() + new_30d = Ticket.query.filter(Ticket.created_at >= day30).count() + + # Unassigned open tickets + unassigned = Ticket.query.filter( + Ticket.assigned_to_id.is_(None), + Ticket.status.in_([TicketStatus.OPEN, TicketStatus.IN_PROGRESS]), + ).count() + + # SLA at-risk: open/in-progress tickets older than 3 days + sla_breach = Ticket.query.filter( + Ticket.status.in_([TicketStatus.OPEN, TicketStatus.IN_PROGRESS]), + Ticket.created_at < now - timedelta(days=3), + ).count() + + # Open tickets by priority + priority_breakdown = {} + for priority in ('critical', 'high', 'medium', 'low'): + priority_breakdown[priority] = Ticket.query.filter( + Ticket.priority == priority, + Ticket.status.in_([TicketStatus.OPEN, TicketStatus.IN_PROGRESS, TicketStatus.PENDING]), + ).count() + + # Open tickets by category (top 6) + from app.models import TicketCategory + category_breakdown = [] + for cat in (TicketCategory.SOFTWARE, TicketCategory.HARDWARE, TicketCategory.NETWORK, + TicketCategory.ACCESS, TicketCategory.EMAIL, TicketCategory.OTHER): + count = Ticket.query.filter( + Ticket.category == cat, + Ticket.status.in_([TicketStatus.OPEN, TicketStatus.IN_PROGRESS]), + ).count() + category_breakdown.append((cat.replace('_', ' ').title(), count)) + category_breakdown.sort(key=lambda x: x[1], reverse=True) + + # ── Staff performance ───────────────────────────────────────────────────── + # Replaced N×5 per-member query loop with two aggregated SQL queries so + # that the dashboard cost is constant regardless of IT team size. + # + # Query 1: grouped counts per assignee (open workload, totals, urgent). + # Query 2: resolution timestamps for avg-hours calculation (one row per + # resolved ticket, grouped in Python to avoid TIMESTAMPDIFF + # dialect differences between MySQL and SQLite). + it_members = User.query.filter( + User.role.in_([UserRole.IT_STAFF, UserRole.ADMIN]), + User.is_active == True, + ).order_by(User.full_name).all() + + it_member_ids = [m.id for m in it_members] + + # ── Aggregated count query ──────────────────────────────────────────────── + # Produces one row per assignee with all needed counts computed in SQL. + count_rows = ( + db.session.query( + Ticket.assigned_to_id, + func.sum(case( + (Ticket.status.in_([TicketStatus.OPEN, TicketStatus.IN_PROGRESS, TicketStatus.PENDING]), 1), + else_=0, + )).label('assigned_open'), + func.sum(case( + (Ticket.status.in_([TicketStatus.RESOLVED, TicketStatus.CLOSED]), 1), + else_=0, + )).label('resolved_total'), + func.sum(case( + (Ticket.resolved_at >= day30, 1), + else_=0, + )).label('resolved_30d'), + func.sum(case( + ( + (Ticket.priority.in_(['critical', 'high'])) & + (Ticket.status.in_([TicketStatus.OPEN, TicketStatus.IN_PROGRESS])), + 1, + ), + else_=0, + )).label('urgent_open'), + ) + .filter(Ticket.assigned_to_id.in_(it_member_ids)) + .group_by(Ticket.assigned_to_id) + .all() + ) + counts_by_id = {row.assigned_to_id: row for row in count_rows} + + # ── Resolution-time query ───────────────────────────────────────────────── + # Fetches only the two timestamp columns needed for avg-hours per assignee. + # Using Python arithmetic avoids TIMESTAMPDIFF / strftime dialect issues. + res_rows = ( + db.session.query( + Ticket.assigned_to_id, + Ticket.created_at, + Ticket.resolved_at, + ) + .filter( + Ticket.assigned_to_id.in_(it_member_ids), + Ticket.resolved_at.isnot(None), + Ticket.created_at.isnot(None), + ) + .all() + ) + # Group timestamps by assignee in Python for the avg-hours calculation. + res_by_id: dict[int, list[float]] = {} + for row in res_rows: + hours = (row.resolved_at - row.created_at).total_seconds() / 3600 + res_by_id.setdefault(row.assigned_to_id, []).append(hours) + + # ── Assemble staff_stats from pre-fetched data ──────────────────────────── + staff_stats = [] + for member in it_members: + row = counts_by_id.get(member.id) + hours = res_by_id.get(member.id, []) + staff_stats.append({ + 'name' : member.full_name, + 'assigned_open' : int(row.assigned_open) if row else 0, + 'resolved_total': int(row.resolved_total) if row else 0, + 'resolved_30d' : int(row.resolved_30d) if row else 0, + 'avg_hours' : round(sum(hours) / len(hours), 1) if hours else None, + 'urgent_open' : int(row.urgent_open) if row else 0, + }) + + # Sort by open workload desc + staff_stats.sort(key=lambda x: x['assigned_open'], reverse=True) + + # ── 5 most urgent open tickets ──────────────────────────────────────────── + urgent_tickets = Ticket.query.filter( + Ticket.status.in_([TicketStatus.OPEN, TicketStatus.IN_PROGRESS]), + ).order_by( + # critical first, then high, then by age + case( + (Ticket.priority == 'critical', 0), + (Ticket.priority == 'high', 1), + (Ticket.priority == 'medium', 2), + else_=3, + ), + Ticket.created_at.asc(), + ).limit(8).all() + + return render_template('admin/index.html', + stats = stats, + avg_resolution_hours= avg_resolution_hours, + resolved_7d = resolved_7d, + resolved_30d = resolved_30d, + new_7d = new_7d, + new_30d = new_30d, + unassigned = unassigned, + sla_breach = sla_breach, + priority_breakdown = priority_breakdown, + category_breakdown = category_breakdown, + staff_stats = staff_stats, + urgent_tickets = urgent_tickets, + now = now, + ) # ─── User Management ───────────────────────────────────────────────────────── @@ -127,12 +304,9 @@ def create_user(): flash('That username is already taken.', 'danger') return render_template('admin/create_user.html', roles=_roles(), form=request.form) - if password != confirm: - flash('Passwords do not match.', 'danger') - return render_template('admin/create_user.html', roles=_roles(), form=request.form) - - if len(password) < 8: - flash('Password must be at least 8 characters.', 'danger') + pw_error = validate_password(password, confirm) + if pw_error: + flash(pw_error, 'danger') return render_template('admin/create_user.html', roles=_roles(), form=request.form) # ── Create ──────────────────────────────────────────────────────────── @@ -147,6 +321,10 @@ def create_user(): ) user.set_password(password) db.session.add(user) + # Flush to obtain user.id from the DB sequence before logging. + # Without this flush, user.id is None and the activity log entry + # records entity_id=None, making the log entry unlinkable. + db.session.flush() log_action(current_user.id, 'admin_user_create', 'user', user.id, f'email={email} role={role}') db.session.commit() @@ -172,6 +350,11 @@ def edit_user(user_id): user.is_active = bool(request.form.get('is_active')) new_pw = request.form.get('new_password', '') if new_pw: + confirm_pw = request.form.get('confirm_password', '') + pw_error = validate_password(new_pw, confirm_pw) + if pw_error: + flash(pw_error, 'danger') + return render_template('admin/edit_user.html', user=user, roles=_roles()) user.set_password(new_pw) logger.info(f'[ADMIN PASSWORD RESET] target_user_id={user.id} by admin_id={current_user.id}') log_action(current_user.id, 'admin_user_edit', 'user', user.id, @@ -314,10 +497,13 @@ def kb_upload_image(): if not f or not f.filename: return jsonify({'error': 'No file provided'}), 400 - ext = f.filename.rsplit('.', 1)[-1].lower() if '.' in f.filename else '' - if ext not in {'png', 'jpg', 'jpeg', 'gif', 'webp'}: - return jsonify({'error': 'Only image files are accepted (PNG, JPG, GIF, WEBP)'}), 400 + _image_exts = {'png', 'jpg', 'jpeg', 'gif', 'webp'} + file_error = validate_file(f, _image_exts) + if file_error: + logger.warning(f'[KB IMAGE UPLOAD REJECTED] {file_error} filename="{f.filename}" user_id={current_user.id}') + return jsonify({'error': file_error}), 400 + ext = f.filename.rsplit('.', 1)[-1].lower() stored_name = f"{uuid.uuid4().hex}.{ext}" upload_dir = current_app.config['UPLOAD_FOLDER'] os.makedirs(upload_dir, exist_ok=True) @@ -329,10 +515,20 @@ def kb_upload_image(): # ── File-serve route (images embedded in articles + attachment downloads) ───── -@admin_bp.route('/kb/files/') +@admin_bp.route('/kb/files/') @login_required def kb_serve_file(stored_name): - """Serve a KB attachment file. Login required — no public access.""" + """Serve a KB attachment file. Login required — no public access. + + Security note + ------------- + The converter is used deliberately instead of . + The converter permits forward slashes in the captured segment, + which would allow a crafted URL like /kb/files/../../etc/passwd to + traverse outside the upload directory. disallows slashes, + restricting the value to a flat filename — matching the UUID-based + stored_name format (e.g. 'a1b2c3d4e5f6....png') used by all upload helpers. + """ upload_dir = current_app.config['UPLOAD_FOLDER'] return send_from_directory(upload_dir, stored_name) @@ -400,7 +596,11 @@ def kb_new(): db.session.flush() for f in request.files.getlist('attachments'): - if f and f.filename and _kb_allowed(f.filename): + if f and f.filename: + file_error = validate_file(f, _KB_ALLOWED_EXT) + if file_error: + logger.warning(f'[KB NEW UPLOAD REJECTED] {file_error} filename="{f.filename}" user_id={current_user.id}') + continue att = _save_kb_file(f, article.id) db.session.add(att) @@ -433,7 +633,11 @@ def kb_edit(article_id): article.is_published= bool(request.form.get('is_published')) and not bool(request.form.get('_save_as_draft')) for f in request.files.getlist('attachments'): - if f and f.filename and _kb_allowed(f.filename): + if f and f.filename: + file_error = validate_file(f, _KB_ALLOWED_EXT) + if file_error: + logger.warning(f'[KB EDIT UPLOAD REJECTED] {file_error} filename="{f.filename}" user_id={current_user.id}') + continue att = _save_kb_file(f, article.id) db.session.add(att) diff --git a/app/routes/auth.py b/app/routes/auth.py index 48f2d5c..1c83207 100644 --- a/app/routes/auth.py +++ b/app/routes/auth.py @@ -6,6 +6,7 @@ from flask_login import login_user, logout_user, login_required, current_user from app import db, limiter from app.models import User, UserRole from app.services.log_service import log_action +from app.services.validation_service import validate_password auth_bp = Blueprint('auth', __name__, url_prefix='/auth') logger = logging.getLogger(__name__) @@ -75,26 +76,30 @@ def register(): flash('Email already registered.', 'danger') elif User.query.filter_by(username=username).first(): flash('Username already taken.', 'danger') - elif password != confirm: - flash('Passwords do not match.', 'danger') - elif len(password) < 8: - flash('Password must be at least 8 characters.', 'danger') else: - user = User( - email = email, - username = username, - full_name = full_name, - department = department, - phone = phone, - role = UserRole.EMPLOYEE, - ) - user.set_password(password) - db.session.add(user) - log_action(user.id, 'user_register', 'user', user.id, f'email={email}') - db.session.commit() - logger.info(f'[AUTH REGISTER] user_id={user.id} email={email}') - flash('Account created! You may now log in.', 'success') - return redirect(url_for('auth.login')) + pw_error = validate_password(password, confirm) + if pw_error: + flash(pw_error, 'danger') + else: + user = User( + email = email, + username = username, + full_name = full_name, + department = department, + phone = phone, + role = UserRole.EMPLOYEE, + ) + user.set_password(password) + db.session.add(user) + # Flush to obtain user.id from the DB sequence before logging. + # Without this flush, user.id is None and the activity log entry + # records entity_id=None, making the log entry unlinkable. + db.session.flush() + log_action(user.id, 'user_register', 'user', user.id, f'email={email}') + db.session.commit() + logger.info(f'[AUTH REGISTER] user_id={user.id} email={email}') + flash('Account created! You may now log in.', 'success') + return redirect(url_for('auth.login')) return render_template('auth/register.html') @@ -128,11 +133,9 @@ def profile(): current_user.web_notif = web_notif if new_pw: - if new_pw != confirm_pw: - flash('Passwords do not match.', 'danger') - return render_template('auth/profile.html') - if len(new_pw) < 8: - flash('Password must be at least 8 characters.', 'danger') + pw_error = validate_password(new_pw, confirm_pw) + if pw_error: + flash(pw_error, 'danger') return render_template('auth/profile.html') current_user.set_password(new_pw) logger.info(f'[AUTH PASSWORD CHANGE] user_id={current_user.id}') diff --git a/app/routes/chatbot.py b/app/routes/chatbot.py index 028fd53..ba998b3 100644 --- a/app/routes/chatbot.py +++ b/app/routes/chatbot.py @@ -3,7 +3,7 @@ import logging import requests from flask import Blueprint, request, jsonify, current_app from flask_login import login_required, current_user -from app import db +from app import db, limiter from app.models import Ticket, TicketStatus, TicketPriority, TicketCategory from app.services.notification_service import notify_new_ticket from app.services.log_service import log_action @@ -79,6 +79,7 @@ def _call_groq(api_key, history, user_msg): @chatbot_bp.route('/message', methods=['POST']) @login_required +@limiter.limit('20 per minute; 100 per hour') def chat(): data = request.get_json(force=True) history = data.get('history', []) # [{role, content}, ...] @@ -146,6 +147,10 @@ def chat(): ) ticket.ticket_number = ticket.generate_ticket_number() db.session.add(ticket) + # Flush to obtain ticket.id from the DB sequence before logging. + # Without this flush, ticket.id is None and the activity log + # entry records entity_id=None, making the log entry unlinkable. + db.session.flush() log_action(current_user.id, 'ticket_create_chatbot', 'ticket', ticket.id, f'ticket_number={ticket.ticket_number} ai_generated=True') db.session.commit() diff --git a/app/routes/tickets.py b/app/routes/tickets.py index 42a5d70..22cb85a 100644 --- a/app/routes/tickets.py +++ b/app/routes/tickets.py @@ -15,17 +15,16 @@ from app.services.notification_service import ( notify_comment_added, notify_assignment, ) from app.services.log_service import log_action, log_ticket_history +from app.services.validation_service import validate_file tickets_bp = Blueprint('tickets', __name__) logger = logging.getLogger(__name__) +# Allowed extensions for ticket and comment attachments. +# validate_file() uses this set for both extension and magic-byte checks. ALLOWED_EXT = {'png', 'jpg', 'jpeg', 'gif', 'pdf', 'doc', 'docx', 'txt', 'zip', 'log'} -def allowed_file(filename): - return '.' in filename and filename.rsplit('.', 1)[1].lower() in ALLOWED_EXT - - def save_attachment(file, ticket_id=None, comment_id=None, uploader_id=None): filename = secure_filename(file.filename) ext = filename.rsplit('.', 1)[1].lower() if '.' in filename else '' @@ -114,7 +113,11 @@ def create_ticket(): # Handle file uploads for f in request.files.getlist('attachments'): - if f and f.filename and allowed_file(f.filename): + if f and f.filename: + file_error = validate_file(f, ALLOWED_EXT) + if file_error: + logger.warning(f'[TICKET UPLOAD REJECTED] {file_error} filename="{f.filename}" user_id={current_user.id}') + continue save_attachment(f, ticket_id=ticket.id, uploader_id=current_user.id) log_action(current_user.id, 'ticket_create', 'ticket', ticket.id, @@ -193,7 +196,11 @@ def ticket_detail(ticket_id): db.session.flush() for f in request.files.getlist('attachments'): - if f and f.filename and allowed_file(f.filename): + if f and f.filename: + file_error = validate_file(f, ALLOWED_EXT) + if file_error: + logger.warning(f'[COMMENT UPLOAD REJECTED] {file_error} filename="{f.filename}" user_id={current_user.id}') + continue save_attachment(f, ticket_id=ticket.id, comment_id=comment.id, uploader_id=current_user.id) @@ -266,7 +273,7 @@ def update_ticket(ticket_id): def _user_label(uid): if uid is None: return 'Unassigned' - u = User.query.get(uid) + u = db.session.get(User, uid) return u.full_name if u else f'User #{uid}' ticket.assigned_to_id = new_assigned @@ -275,7 +282,7 @@ def update_ticket(ticket_id): _user_label(new_assigned), current_user.id) changes.append(f'assigned_to: {old_assigned} → {new_assigned}') - notify_assignment(ticket, current_user) + # notify_assignment is called AFTER commit below — see Fix #13. ticket.internal_notes = internal_notes ticket.resolution_notes = resolution @@ -291,6 +298,13 @@ def update_ticket(ticket_id): db.session.commit() logger.info(f'[TICKET UPDATE] ticket_id={ticket.id} changes={changes} by user_id={current_user.id}') + # Both notification calls are placed after commit so that create_notification's + # independent commit never races against an uncommitted ticket state. If the + # parent commit above had failed, neither notification would be sent — which + # is the correct behaviour (no notification for a change that did not persist). + if new_assigned != old_assigned: + notify_assignment(ticket, current_user) + if new_status != old_status: notify_status_change(ticket, old_status, current_user) @@ -322,6 +336,18 @@ def delete_comment(comment_id): @login_required def download_attachment(att_id): att = Attachment.query.get_or_404(att_id) + # Authorization: employees may only download attachments belonging to + # their own tickets. IT staff have unrestricted access across all tickets. + # att.ticket_id is the authoritative link — comment attachments also carry + # the parent ticket_id, so this check covers both ticket and comment files. + if not current_user.is_it_staff: + ticket = Ticket.query.get_or_404(att.ticket_id) + if ticket.created_by_id != current_user.id: + logger.warning( + f'[ATTACHMENT ACCESS DENIED] att_id={att_id} ticket_id={att.ticket_id} ' + f'user_id={current_user.id}' + ) + abort(403) upload_dir = current_app.config['UPLOAD_FOLDER'] return send_from_directory(upload_dir, att.stored_name, as_attachment=True, download_name=att.filename) @@ -385,8 +411,20 @@ def knowledge_base(): @login_required def kb_article(article_id): article = KnowledgeBase.query.get_or_404(article_id) - article.view_count += 1 + # Increment view_count atomically at the SQL level. A Python-level + # read-modify-write (article.view_count += 1) is not safe under concurrent + # requests: two simultaneous reads both see the same value and one + # increment is silently lost. The SQL expression KnowledgeBase.view_count + 1 + # delegates the addition to the database, which serialises it correctly. + from sqlalchemy import update as sa_update + db.session.execute( + sa_update(KnowledgeBase) + .where(KnowledgeBase.id == article_id) + .values(view_count=KnowledgeBase.view_count + 1) + ) db.session.commit() + # Re-fetch so the template receives the post-increment value. + db.session.refresh(article) return render_template('tickets/kb_article.html', article=article) diff --git a/app/services/log_service.py b/app/services/log_service.py index e1afa0a..76a6717 100644 --- a/app/services/log_service.py +++ b/app/services/log_service.py @@ -54,8 +54,16 @@ def log_action(user_id, action, entity_type=None, entity_id=None, details=None): own business objects. This ensures the log entry is only persisted when the parent operation succeeds — an independent commit here would leave orphaned log entries for operations that were subsequently rolled back. + + Error isolation + --------------- + On failure, only the log entry itself is expelled from the session via + expunge(). db.session.rollback() is intentionally NOT called here because + that would wipe the entire session — silently undoing the parent business + operation (ticket creation, user update, etc.) that triggered this log call. """ ip = _get_real_ip() + entry = None try: entry = ActivityLog( @@ -74,8 +82,14 @@ def log_action(user_id, action, entity_type=None, entity_id=None, details=None): f'user_id={user_id} ip={ip}' ) except Exception as exc: - db.session.rollback() - logger.error(f'[ACTIVITY LOG ERROR] {exc}') + # Expunge only the failed log entry — do NOT roll back the full session, + # as that would undo the parent operation that called this function. + if entry is not None: + try: + db.session.expunge(entry) + except Exception: + pass + logger.error(f'[ACTIVITY LOG ERROR] action={action} entity={entity_type}:{entity_id} error={exc}') def log_ticket_history(ticket, field_name, old_value, new_value, changed_by_id): @@ -85,8 +99,15 @@ def log_ticket_history(ticket, field_name, old_value, new_value, changed_by_id): ---------------- Like log_action, this function does NOT commit — the caller is responsible for committing the session after all field changes have been recorded. + + Error isolation + --------------- + On failure, only the failed history entry is expelled from the session. + db.session.rollback() is intentionally NOT called here — that would undo + the parent ticket update that triggered this history recording. """ from app.models import TicketHistory + entry = None try: entry = TicketHistory( ticket_id = ticket.id, @@ -102,5 +123,11 @@ def log_ticket_history(ticket, field_name, old_value, new_value, changed_by_id): f'"{old_value}" -> "{new_value}" by user_id={changed_by_id}' ) except Exception as exc: - db.session.rollback() - logger.error(f'[TICKET HISTORY ERROR] {exc}') \ No newline at end of file + # Expunge only the failed history entry — do NOT roll back the full + # session, as that would undo the parent ticket update. + if entry is not None: + try: + db.session.expunge(entry) + except Exception: + pass + logger.error(f'[TICKET HISTORY ERROR] ticket_id={ticket.id} field={field_name} error={exc}') \ No newline at end of file diff --git a/app/services/notification_service.py b/app/services/notification_service.py index 7dd0e06..0f64395 100644 --- a/app/services/notification_service.py +++ b/app/services/notification_service.py @@ -251,7 +251,7 @@ def notify_comment_added(comment): ticket_id = ticket.id, link = f'/tickets/{ticket.id}#comment-{comment.id}', ) - user = User.query.get(user_id) + user = db.session.get(User, user_id) if user and user.email_notif and not is_internal: html = render_template_string(_STATUS_UPDATE_EMAIL, ticket_number = ticket.ticket_number, diff --git a/app/services/validation_service.py b/app/services/validation_service.py new file mode 100644 index 0000000..cf27b28 --- /dev/null +++ b/app/services/validation_service.py @@ -0,0 +1,179 @@ +""" +Shared validation helpers used across multiple route modules. + +Centralising these here prevents logic divergence that arises from +copy-pasting validation code into auth.py, admin.py, and future modules. +""" + +import logging + +logger = logging.getLogger(__name__) + + +# ─── Password Validation ────────────────────────────────────────────────────── + +def validate_password(password: str, confirm: str) -> str | None: + """Validate a new password and its confirmation field. + + Returns an error message string if validation fails, or None if the + password is acceptable. Callers should flash the returned message with + the 'danger' category and return early. + + Rules + ----- + - Password and confirmation must match. + - Minimum length: 8 characters. + + Parameters + ---------- + password : str – the candidate password (plain text) + confirm : str – the confirmation field value + """ + if password != confirm: + return 'Passwords do not match.' + if len(password) < 8: + return 'Password must be at least 8 characters.' + return None + + +# ─── File MIME-Type Validation ──────────────────────────────────────────────── + +# Magic-byte signatures for every extension in ALLOWED_EXT / _KB_ALLOWED_EXT. +# Format: extension → list of (offset, bytes) tuples that must ALL be present. +# Using raw magic bytes avoids a dependency on python-magic / libmagic while +# still catching the "renamed shell.php → shell.pdf" class of attack. +# +# References: https://en.wikipedia.org/wiki/List_of_file_signatures +_MAGIC: dict[str, list[tuple[int, bytes]]] = { + 'png' : [(0, b'\x89PNG\r\n\x1a\n')], + 'jpg' : [(0, b'\xff\xd8\xff')], + 'jpeg': [(0, b'\xff\xd8\xff')], + 'gif' : [(0, b'GIF87a'), (0, b'GIF89a')], # either signature is valid + 'webp': [(0, b'RIFF'), (8, b'WEBP')], + 'pdf' : [(0, b'%PDF')], + 'zip' : [(0, b'PK\x03\x04')], + # Office Open XML (.docx, .xlsx, .pptx) are ZIP archives internally + 'docx': [(0, b'PK\x03\x04')], + 'xlsx': [(0, b'PK\x03\x04')], + 'pptx': [(0, b'PK\x03\x04')], + # Legacy OLE2 compound document (.doc, .xls, .ppt) + 'doc' : [(0, b'\xd0\xcf\x11\xe0\xa1\xb1\x1a\xe1')], + 'xls' : [(0, b'\xd0\xcf\x11\xe0\xa1\xb1\x1a\xe1')], + 'ppt' : [(0, b'\xd0\xcf\x11\xe0\xa1\xb1\x1a\xe1')], + # Plain-text formats — no reliable magic bytes; skip byte check + 'txt' : [], + 'csv' : [], + 'log' : [], +} + +# Maximum number of bytes to read for magic-byte inspection. +# 12 bytes covers all signatures above (WEBP needs offset 8 + 4 = 12). +_MAGIC_READ_BYTES = 12 + +# Per-file size cap (bytes). Mirrors MAX_CONTENT_LENGTH so a single +# large file cannot saturate the total-request budget on its own. +_MAX_FILE_BYTES = 16 * 1024 * 1024 # 16 MB + + +def validate_file(file, allowed_extensions: set[str]) -> str | None: + """Validate an uploaded file object by extension, size, and magic bytes. + + Returns an error message string if validation fails, or None on success. + The file stream seek position is reset to 0 after inspection so callers + can still read or save the file normally. + + Parameters + ---------- + file : werkzeug FileStorage object + allowed_extensions : set of lowercase extension strings (without leading dot) + + Checks performed + ---------------- + 1. Extension is in the allowed set. + 2. File does not exceed the per-file size cap (_MAX_FILE_BYTES). + 3. Magic bytes in the file content match the declared extension, where + a known signature exists. Plain-text types (txt, csv, log) are + accepted on extension alone since they have no reliable magic bytes. + """ + filename = file.filename or '' + if '.' not in filename: + return 'File has no extension.' + + ext = filename.rsplit('.', 1)[1].lower() + + # ── 1. Extension check ──────────────────────────────────────────────────── + if ext not in allowed_extensions: + return f'File type ".{ext}" is not permitted.' + + # ── 2. Size check ───────────────────────────────────────────────────────── + # Read the file in chunks to measure size without loading it all into RAM, + # then seek back to the start so the caller can still save it. + file.stream.seek(0, 2) # seek to end + file_size = file.stream.tell() # position == size + file.stream.seek(0) # rewind + + if file_size > _MAX_FILE_BYTES: + mb = _MAX_FILE_BYTES // (1024 * 1024) + return f'File exceeds the {mb} MB per-file size limit.' + + # ── 3. Magic-byte check ─────────────────────────────────────────────────── + signatures = _MAGIC.get(ext) + if signatures is None: + # Extension is allowed but has no entry in _MAGIC — treat as safe. + # This handles future extensions added to ALLOWED_EXT without a + # corresponding _MAGIC entry; log a warning for visibility. + logger.warning(f'[FILE VALIDATION] No magic signature defined for ext="{ext}"; skipping byte check.') + return None + + if not signatures: + # Explicit empty list means "no magic bytes available for this type" + # (txt, csv, log) — accepted on extension alone. + return None + + header = file.stream.read(_MAGIC_READ_BYTES) + file.stream.seek(0) # rewind after inspection + + # For extensions with multiple valid signatures (e.g. GIF87a / GIF89a) + # the file is valid if ANY of the listed signatures matches. + matched = False + for sigs in _group_by_alternative(signatures): + # Each alternative is a list of (offset, bytes) pairs that must ALL match. + if all(header[offset:offset + len(magic)] == magic for offset, magic in sigs): + matched = True + break + + if not matched: + logger.warning( + f'[FILE VALIDATION] Magic-byte mismatch: filename="{filename}" ext="{ext}" ' + f'header={header.hex()}' + ) + return f'File content does not match its declared type (.{ext}).' + + return None + + +def _group_by_alternative( + signatures: list[tuple[int, bytes]], +) -> list[list[tuple[int, bytes]]]: + """Split a flat signature list into per-alternative groups. + + For most extensions there is a single signature, so this returns + [[sig1, sig2, ...]]. For GIF (two valid headers) it returns + [[gif87_sig], [gif89_sig]] so the caller can treat each inner list as + a complete match candidate. + + The rule: each entry with offset=0 starts a new alternative group. + Entries with offset>0 are appended to the current group (they are + additional constraints on the same file type, e.g. WEBP needs both + offset-0 'RIFF' and offset-8 'WEBP'). + """ + groups: list[list[tuple[int, bytes]]] = [] + for offset, magic in signatures: + if offset == 0: + groups.append([(offset, magic)]) + else: + if groups: + groups[-1].append((offset, magic)) + else: + groups.append([(offset, magic)]) + return groups if groups else [[]] diff --git a/app/templates/admin/index.html b/app/templates/admin/index.html index a332045..6b1fbe0 100644 --- a/app/templates/admin/index.html +++ b/app/templates/admin/index.html @@ -3,15 +3,16 @@ {% block page_title %}IT Operations Overview{% endblock %} {% block content %} - + +{# ── Headline stat cards ───────────────────────────────────────────────────── #}
{% for label, value, icon, color, bg in [ - ('Total Tickets', stats.total_tickets, 'bi-collection', 'var(--text)', 'rgba(255,255,255,.05)'), - ('Open', stats.open, 'bi-circle', 'var(--info)', 'rgba(96,165,250,.1)'), - ('In Progress', stats.in_progress, 'bi-arrow-repeat', 'var(--accent3)', 'rgba(0,180,216,.1)'), - ('Resolved', stats.resolved, 'bi-check-circle', 'var(--success)', 'rgba(45,212,191,.1)'), - ('Closed', stats.closed, 'bi-archive', 'var(--muted)', 'rgba(112,112,160,.1)'), - ('Active Users', stats.total_users, 'bi-people', 'var(--accent2)', 'rgba(123,94,167,.1)'), + ('Total Tickets', stats.total_tickets, 'bi-collection', 'var(--text)', 'rgba(255,255,255,.05)'), + ('Open', stats.open, 'bi-circle', 'var(--info)', 'rgba(96,165,250,.1)'), + ('In Progress', stats.in_progress, 'bi-arrow-repeat','var(--accent3)','rgba(0,180,216,.1)'), + ('Resolved', stats.resolved, 'bi-check-circle','var(--success)','rgba(45,212,191,.1)'), + ('Closed', stats.closed, 'bi-archive', 'var(--muted)', 'rgba(112,112,160,.1)'), + ('Active Users', stats.total_users, 'bi-people', 'var(--accent2)','rgba(123,94,167,.1)'), ] %}
@@ -25,44 +26,236 @@ {% endfor %}
- -
-
- Recent Activity - {% if current_user.is_admin %} - Full log → - {% endif %} +{# ── Row 1: Performance KPIs + Priority breakdown ─────────────────────────── #} +
+ + {# KPI cards #} +
+
+
IT Performance — Last 30 Days
+
+
+ + {# Avg resolution time #} +
+
+
+ {% if avg_resolution_hours is none %}— + {% elif avg_resolution_hours >= 24 %}{{ (avg_resolution_hours / 24)|round(1) }}d + {% else %}{{ avg_resolution_hours }}h{% endif %} +
+
Avg Resolution Time
+
+
+ + {# Resolved last 7d #} +
+
+
{{ resolved_7d }}
+
Resolved (7 days)
+
+
+ + {# New last 7d #} +
+
+
{{ new_7d }}
+
New (7 days)
+
+
+ + {# Resolution rate 30d #} +
+ {% set rate = ((resolved_30d / new_30d * 100)|int) if new_30d > 0 else 0 %} +
+
+ {{ rate }}% +
+
Resolution Rate (30d)
+
+
+ + {# Unassigned #} +
+ +
+
+ {{ unassigned }} +
+
Unassigned Open
+
+ +
+ + {# SLA at-risk #} +
+
+
+ {{ sla_breach }} +
+
SLA At-Risk (>3 days)
+
+
+ + {# Resolved 30d #} +
+
+
{{ resolved_30d }}
+
Resolved (30 days)
+
+
+ + {# New 30d #} +
+
+
{{ new_30d }}
+
New (30 days)
+
+
+ +
+
+
-
- - - - - - - - - - - - {% for log in recent_logs %} - - - - - - - + + {# Priority breakdown #} +
+
+
Open by Priority
+
+ {% set total_open = priority_breakdown.values()|sum %} + {% for priority, count in [ + ('critical', priority_breakdown.critical), + ('high', priority_breakdown.high), + ('medium', priority_breakdown.medium), + ('low', priority_breakdown.low), + ] %} +
+
+ {{ priority.upper() }} + {{ count }} +
+
+
+
+
{% endfor %} -
-
ActionUserEntityDetailsTime
- - {{ log.action }} - - - {% if log.user %}{{ log.user.full_name }}{% else %}System{% endif %} - {{ log.entity_type or '—' }} {% if log.entity_id %}#{{ log.entity_id }}{% endif %}{{ log.details or '—' }}{{ log.created_at.strftime('%b %d %H:%M') }}
+ +
+
Open by Category
+
+ {% for cat, count in category_breakdown %} + {% if count > 0 %} +
+ {{ cat }} + {{ count }} +
+ {% endif %} + {% endfor %} +
+
+
-{% endblock %} + +{# ── Row 2: Staff performance + Urgent queue ──────────────────────────────── #} +
+ + {# Staff performance table #} +
+
+
IT Staff Performance
+
+ {% if staff_stats %} + + + + + + + + + + + + + {% for s in staff_stats %} + + + + + + + + + {% endfor %} + +
Staff MemberOpenUrgentResolved (30d)Avg TimeTotal Resolved
{{ s.name }} + + {{ s.assigned_open }} + + + {% if s.urgent_open > 0 %} + {{ s.urgent_open }} + {% else %} + + {% endif %} + {{ s.resolved_30d }} + {% if s.avg_hours is none %}— + {% elif s.avg_hours >= 24 %}{{ (s.avg_hours / 24)|round(1) }}d + {% else %}{{ s.avg_hours }}h{% endif %} + {{ s.resolved_total }}
+ {% else %} +
No IT staff found.
+ {% endif %} +
+
+
+ + {# Urgent open tickets queue #} +
+
+
+ Needs Attention + View all → +
+
+ {% if urgent_tickets %} + {% for t in urgent_tickets %} + + {{ t.priority.upper() }} +
+
{{ t.title }}
+
+ {{ t.ticket_number }} · {{ t.creator.full_name.split()[0] }} · + {% set age = ((now - t.created_at).total_seconds() / 3600)|int %} + {% if age >= 24 %}{{ (age / 24)|int }}d ago{% else %}{{ age }}h ago{% endif %} +
+
+ {% if t.assignee %} + {{ t.assignee.full_name.split()[0] }} + {% else %} + Unassigned + {% endif %} + + {% endfor %} + {% else %} +
+ + All caught up — no urgent open tickets. +
+ {% endif %} +
+
+
+ +
+{% endblock %} \ No newline at end of file