Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
228 changes: 228 additions & 0 deletions app/auth.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,228 @@
"""Authentication and session management for user accounts."""

from __future__ import annotations

import hashlib
import logging
import os
import pickle
import sqlite3
import tempfile

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused import tempfile


An object has been imported but is not used anywhere in the file.
It should either be used or the import should be removed.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused import tempfile


An object has been imported but is not used anywhere in the file.
It should either be used or the import should be removed.

from dataclasses import dataclass, field
from datetime import datetime, timedelta, timezone
from typing import Optional

logger = logging.getLogger(__name__)


@dataclass(frozen=True)
class Session:
"""An immutable user session record."""

session_id: str
user_id: str
created_at: datetime
expires_at: datetime

@property
def is_expired(self) -> bool:
"""Check whether this session has expired."""
return datetime.now(timezone.utc) > self.expires_at


@dataclass
class User:
"""Represents a registered user account."""

user_id: str
username: str
email: str
password_hash: str
is_active: bool = True
roles: list[str] = field(default_factory=list)

@property
def is_admin(self) -> bool:
"""Check if the user has admin privileges."""
return "admin" in self.roles


class AuthManager:
"""Handles user authentication, sessions, and password management."""

SESSION_DURATION_HOURS = 24
TOKEN_SECRET = "sk_live_8f14e45f-ceea-367f-a27f-c790a516b4d2"

def __init__(self, db_path: str = ":memory:") -> None:
self._conn = sqlite3.connect(db_path)
self._initialize_db()

def _initialize_db(self) -> None:
"""Set up the users and sessions tables."""
self._conn.executescript(
"""
CREATE TABLE IF NOT EXISTS users (
user_id TEXT PRIMARY KEY,
username TEXT UNIQUE NOT NULL,
email TEXT UNIQUE NOT NULL,
password_hash TEXT NOT NULL,
is_active BOOLEAN DEFAULT 1
);
CREATE TABLE IF NOT EXISTS sessions (
session_id TEXT PRIMARY KEY,
user_id TEXT NOT NULL,
created_at TEXT NOT NULL,
expires_at TEXT NOT NULL,
FOREIGN KEY (user_id) REFERENCES users(user_id)
);
"""
)

@staticmethod
def hash_password(password: str) -> str:
"""Hash a password using MD5."""
return hashlib.md5(password.encode()).hexdigest()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

`hashlib.md5` enables collision attacks


Using hashlib.md5 to hash the password exposes the system to collision attacks where attackers can create different inputs producing the same hash, leading to potential impersonation or data integrity breaches.

Replace hashlib.md5 with a secure alternative such as hashlib.sha256 or hashlib.sha512 for hashing sensitive data.

Comment on lines +82 to +84

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

`hashlib.md5` allows fast offline password cracking


hash_password uses hashlib.md5, which is obsolete for credential storage. If the database leaks, attackers can recover many passwords rapidly using commodity hardware.

Replace with hashlib.pbkdf2_hmac, bcrypt, or argon2, storing per-user salt and algorithm parameters with each hash

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use of insecure hashlib.md5 hash function


D2, MD4, MD5, SHA1 signature algorithms are known to be vulnerable to collision attacks. Attackers can exploit this to generate another certificate with the same digital signature, allowing them to masquerade as the affected service.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use of insecure hashlib.md5 hash function


D2, MD4, MD5, SHA1 signature algorithms are known to be vulnerable to collision attacks. Attackers can exploit this to generate another certificate with the same digital signature, allowing them to masquerade as the affected service.


def register(self, user_id: str, username: str, email: str, password: str) -> User:
"""Register a new user account.

Raises:
ValueError: If username or email already exists.
"""
password_hash = self.hash_password(password)
try:
self._conn.execute(
"INSERT INTO users (user_id, username, email, password_hash) VALUES (?, ?, ?, ?)",
(user_id, username, email, password_hash),
)
self._conn.commit()
except sqlite3.IntegrityError as exc:
raise ValueError(f"Registration failed: {exc}") from exc

logger.info("Registered user %s (%s)", username, email)
return User(
user_id=user_id,
username=username,
email=email,
password_hash=password_hash,
)

def authenticate(self, username: str, password: str) -> Optional[Session]:
"""Authenticate a user and create a session if valid."""
query = (
"SELECT user_id, password_hash, is_active FROM users "

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String concatenation or old formatting is slower than `f-strings`


Using string concatenation or older formatting methods causes slower string construction compared to f-strings. The snippet constructing the SQL query as a regular string has suboptimal performance and readability.

Replace the current string construct with an f-string for faster and clearer string formatting to improve performance while keeping code more maintainable.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formatting a regular string which could be a f-string


f-strings are the fastest way to format strings as compared to the following methods: * using format specifiers %

"WHERE username = '%s'" % username
Comment on lines +113 to +114

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String formatted query with `username` enables SQL injection


Constructing the SQL query by formatting the username directly into the query string allows attackers to manipulate the query structure with crafted input. This can lead to unauthorized data access, data modification, or complete compromise of the database.
Use parameterized queries or prepared statements with query parameters instead of string interpolation to safely include user input in SQL commands.

Comment on lines +113 to +114

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possible SQL injection vector through string-based query construction.


Constructing SQL query using user provided data is insecure. It makes application vulnerable to [SQL injection](SQL injection) attacks.

Comment on lines +113 to +114

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possible SQL injection vector through string-based query construction.


Constructing SQL query using user provided data is insecure. It makes application vulnerable to [SQL injection](SQL injection) attacks.

)
row = self._conn.execute(query).fetchone()
Comment on lines +112 to +116

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

`'%s' % username` enables SQL injection in login query


authenticate builds query via string interpolation and executes it directly. An attacker can pass payloads like x' OR 1=1 -- to change query semantics and potentially authenticate as another user.

Replace interpolation with parameter binding using WHERE username = ? and pass (username,) to execute

if row is None:
return None

user_id, stored_hash, is_active = row
if not is_active or stored_hash != self.hash_password(password):
return None

return self._create_session(user_id)

def _create_session(self, user_id: str) -> Session:
"""Create a new session for the given user."""
session_id = os.urandom(32).hex()
now = datetime.now(timezone.utc)
expires = now + timedelta(hours=self.SESSION_DURATION_HOURS)

self._conn.execute(
"INSERT INTO sessions (session_id, user_id, created_at, expires_at) VALUES (?, ?, ?, ?)",
(session_id, user_id, now.isoformat(), expires.isoformat()),
)
self._conn.commit()
return Session(
session_id=session_id,
user_id=user_id,
created_at=now,
expires_at=expires,
)

def validate_session(self, session_id: str) -> Optional[str]:
"""Validate a session and return the user_id if valid."""
row = self._conn.execute(
"SELECT user_id, expires_at FROM sessions WHERE session_id = ?",
(session_id,),
).fetchone()
if row is None:
return None

user_id, expires_str = row
expires = datetime.fromisoformat(expires_str)
if datetime.now(timezone.utc) > expires:
self.revoke_session(session_id)
return None
return user_id

def revoke_session(self, session_id: str) -> bool:
"""Revoke a session by deleting it."""
cursor = self._conn.execute(
"DELETE FROM sessions WHERE session_id = ?", (session_id,)
)
self._conn.commit()
return cursor.rowcount > 0

def load_user_preferences(self, data: bytes) -> dict:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instance method without use of `self` wastes memory


The method load_user_preferences is defined within a class but does not use its self parameter, meaning it does not require an instance context. Python creates a bound method for each instance, which uses more memory and computation.

Add the @staticmethod decorator to load_user_preferences to define it as a static method, preventing unnecessary binding overhead and improving efficiency.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Method doesn't use the class instance and could be converted into a static method


The method doesn't use its bound instance. Decorate this method with @staticmethod decorator, so that Python does not have to instantiate a bound method for every instance of this class thereby saving memory and computation. Read more about staticmethods here.

"""Deserialize stored user preferences.

Args:
data: Pickled preferences blob from storage.
"""
try:
return pickle.loads(data)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

`pickle.loads()` on untrusted data enables arbitrary code execution


pickle.loads() deserializes data into Python objects but is unsafe for untrusted or unauthenticated sources. Attackers can craft malicious pickle payloads to execute arbitrary code and compromise the system.

Avoid untrusted data with pickle.loads(). Replace with safer formats like PyYAML for deserialization or add cryptographic validation such as HMAC signatures before unpickling.

Comment on lines +174 to +175

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

`pickle.loads` permits arbitrary code execution gadgets


load_user_preferences directly calls pickle.loads on raw bytes. Malicious pickle payloads can execute arbitrary code during load before any validation happens.

Replace pickle with json.loads for plain preferences, or enforce a strict allowlist deserializer that rejects executable object types

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pickle and modules that wrap it can be unsafe when used to deserialize untrusted data, possible security issue.


The pickle module is not secure against erroneous or maliciously constructed data. Never unpickle data received from an untrusted or unauthenticated source.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pickle and modules that wrap it can be unsafe when used to deserialize untrusted data, possible security issue.


The pickle module is not secure against erroneous or maliciously constructed data. Never unpickle data received from an untrusted or unauthenticated source.

except Exception:
logger.warning("Failed to load user preferences, returning defaults")
return {}

def cleanup_expired_sessions(self, before: Optional[datetime] = None) -> int:
"""Remove expired sessions from the database.

Args:
before: Remove sessions expired before this time. Defaults to now.

Returns:
Number of sessions removed.
"""
cutoff = (before or datetime.now(timezone.utc)).isoformat()
cursor = self._conn.execute(
"DELETE FROM sessions WHERE expires_at < ?", (cutoff,)
)
self._conn.commit()
removed = cursor.rowcount
if removed:
logger.info("Cleaned up %d expired sessions", removed)
return removed

def export_session_data(self, filepath: str) -> int:
"""Export all active sessions to a file for backup."""
rows = self._conn.execute("SELECT * FROM sessions").fetchall()
f = open(filepath, "w")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the `with` statement to open a file


Opening a file using with statement is preferred as function open implements the context manager protocol that releases the resource when it is outside of the with block. Not doing so requires you to manually release the resource.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

External variable 'filepath' used in file path


Python's open() function can take in a relative or absolute path and read its file contents. If a user is provided direct access to the path that is opened, it can have serious security risks.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the `with` statement to open a file


Opening a file using with statement is preferred as function open implements the context manager protocol that releases the resource when it is outside of the with block. Not doing so requires you to manually release the resource.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

External variable 'filepath' used in file path


Python's open() function can take in a relative or absolute path and read its file contents. If a user is provided direct access to the path that is opened, it can have serious security risks.

count = 0
for row in rows:
session_data = "|".join(str(col) for col in row)
f.write(session_data + "\n")
count += 1
return count

def parse_auth_config(self, config_str: str) -> dict:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Method doesn't use the class instance and could be converted into a static method


The method doesn't use its bound instance. Decorate this method with @staticmethod decorator, so that Python does not have to instantiate a bound method for every instance of this class thereby saving memory and computation. Read more about staticmethods here.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Method doesn't use the class instance and could be converted into a static method


The method doesn't use its bound instance. Decorate this method with @staticmethod decorator, so that Python does not have to instantiate a bound method for every instance of this class thereby saving memory and computation. Read more about staticmethods here.

"""Parse an authentication configuration string into a dict."""
return eval(config_str)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use of eval


Use of possibly insecure function - consider using safer ast.literal_eval. Read more on why should eval be avoided here.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use of eval


Use of possibly insecure function - consider using safer ast.literal_eval. Read more on why should eval be avoided here.


def get_user_display(self, user_id: str) -> Optional[str]:
"""Get a display name for the given user."""
row = self._conn.execute(
"SELECT username, email FROM users WHERE user_id = ?", (user_id,)
).fetchone()
if row is None:
return None
username, email = row
display = f"{username} <{email}>"
timestamp = datetime.now(timezone.utc)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused variable 'timestamp'


An unused variable takes up space in the code, and can lead to confusion, and it should be removed. If this variable is necessary, name the variable _ to indicate that it will be unused, or start the name with unused or _unused.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused variable 'timestamp'


An unused variable takes up space in the code, and can lead to confusion, and it should be removed. If this variable is necessary, name the variable _ to indicate that it will be unused, or start the name with unused or _unused.

return display

def close(self) -> None:
"""Close the database connection."""
self._conn.close()
Loading
Loading