From 26e07d97029d4722a5fa60c6b6e32b5135394f64 Mon Sep 17 00:00:00 2001 From: Dries Peeters Date: Fri, 28 Nov 2025 15:12:34 +0100 Subject: [PATCH 1/5] Fix: Remove serviceWorker from required features check to prevent false browser compatibility warnings ServiceWorker was incorrectly treated as a required feature, causing browser compatibility warnings to appear on every page load/refresh when accessing the app over HTTP (common in Portainer setups without HTTPS). Changes: - Removed serviceWorker from required features check (it's a PWA enhancement, not core functionality) - Only localStorage and fetch are now checked as truly required features - Added debug logging for serviceWorker availability without showing user-facing warnings - App now works normally over HTTP without serviceWorker, only missing optional PWA features --- app/static/error-handling-enhanced.js | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/app/static/error-handling-enhanced.js b/app/static/error-handling-enhanced.js index 1bd3a6fb..57890c13 100644 --- a/app/static/error-handling-enhanced.js +++ b/app/static/error-handling-enhanced.js @@ -681,10 +681,11 @@ class EnhancedErrorHandler { } checkRequiredFeatures() { + // Only check for truly critical features required for app functionality + // serviceWorker is optional (PWA enhancement) and only works over HTTPS const features = { 'localStorage': typeof Storage !== 'undefined', - 'fetch': typeof fetch !== 'undefined', - 'serviceWorker': 'serviceWorker' in navigator + 'fetch': typeof fetch !== 'undefined' }; const missing = Object.entries(features) @@ -698,6 +699,16 @@ class EnhancedErrorHandler { 'Browser Compatibility' ); } + + // Log serviceWorker availability for debugging (but don't show warning) + if (!('serviceWorker' in navigator)) { + const isSecureContext = window.isSecureContext || location.protocol === 'https:' || location.hostname === 'localhost' || location.hostname === '127.0.0.1'; + if (!isSecureContext) { + console.debug('ServiceWorker not available: requires HTTPS (or localhost)'); + } else { + console.debug('ServiceWorker not available: browser does not support it'); + } + } } setupFeatureFallbacks() { From 410477fcd6aee230b23cf99b002ae40fd611b5d2 Mon Sep 17 00:00:00 2001 From: Dries Peeters Date: Fri, 28 Nov 2025 15:12:53 +0100 Subject: [PATCH 2/5] Update setup.py --- setup.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup.py b/setup.py index cefc125d..5f61003c 100644 --- a/setup.py +++ b/setup.py @@ -7,7 +7,7 @@ setup( name='timetracker', - version='4.0.0', + version='4.0.1', packages=find_packages(), include_package_data=True, install_requires=[ From 1e777e590f9f4bf8a9aa011bf2cc73618f54db11 Mon Sep 17 00:00:00 2001 From: Dries Peeters Date: Fri, 28 Nov 2025 15:26:08 +0100 Subject: [PATCH 3/5] Fix task view endpoint HTTP 500 errors - Fix incorrect relationship name: Comment.user -> Comment.author The Comment model uses 'author' relationship, not 'user' - Fix eager loading of dynamic relationships Remove invalid eager loading attempts for Task.activities and Task.time_entries, which are dynamic relationships (lazy='dynamic') and cannot be eager loaded with joinedload() - Query dynamic relationships correctly Update task view route to properly query time_entries and activities using their dynamic relationship query objects, with proper eager loading of nested relationships (TimeEntry.user, TaskActivity.user) to prevent N+1 queries Fixes issue where task detail view returned HTTP 500 error after creating a new task. --- app/routes/projects_refactored_example.py | 2 +- app/routes/tasks.py | 13 ++++++++----- app/services/task_service.py | 10 +++++----- 3 files changed, 14 insertions(+), 11 deletions(-) diff --git a/app/routes/projects_refactored_example.py b/app/routes/projects_refactored_example.py index 8c4bd8dd..9ce82f81 100644 --- a/app/routes/projects_refactored_example.py +++ b/app/routes/projects_refactored_example.py @@ -151,7 +151,7 @@ def view_project(project_id): # Get comments with eager loading comments = Comment.query.filter_by(project_id=project_id).options( - joinedload(Comment.user) # Eagerly load user + joinedload(Comment.author) # Eagerly load author ).order_by(Comment.created_at.desc()).all() # Get recent project costs diff --git a/app/routes/tasks.py b/app/routes/tasks.py index 6450340d..58dd16a5 100644 --- a/app/routes/tasks.py +++ b/app/routes/tasks.py @@ -193,11 +193,14 @@ def view_task(task_id): flash(_('You do not have access to this task'), 'error') return redirect(url_for('tasks.list_tasks')) - # Get time entries (already loaded via eager loading, but need to order) - time_entries = sorted(task.time_entries, key=lambda e: e.start_time if e.start_time else datetime.min, reverse=True) - - # Recent activity entries (already loaded) - activities = sorted(task.activities, key=lambda a: a.created_at if a.created_at else datetime.min, reverse=True)[:20] + # Get time entries (time_entries is a dynamic relationship, so query it) + # Eagerly load user relationship to prevent N+1 queries + from sqlalchemy.orm import joinedload + time_entries = task.time_entries.options(joinedload(TimeEntry.user)).order_by(TimeEntry.start_time.desc(), TimeEntry.id.desc()).all() + + # Recent activity entries (activities is a dynamic relationship, so query it) + # Eagerly load user relationship to prevent N+1 queries + activities = task.activities.options(joinedload(TaskActivity.user)).order_by(TaskActivity.created_at.desc(), TaskActivity.id.desc()).limit(20).all() # Get comments for this task from app.models import Comment diff --git a/app/services/task_service.py b/app/services/task_service.py index 7ea2f0b9..389958a1 100644 --- a/app/services/task_service.py +++ b/app/services/task_service.py @@ -144,14 +144,14 @@ def get_task_with_details( ) # Conditionally load relations - if include_time_entries: - query = query.options(joinedload(Task.time_entries).joinedload(TimeEntry.user)) + # Note: time_entries is a dynamic relationship (lazy='dynamic') and cannot be eager loaded + # Time entries must be queried separately using task.time_entries.order_by(...).all() if include_comments: - query = query.options(joinedload(Task.comments).joinedload(Comment.user)) + query = query.options(joinedload(Task.comments).joinedload(Comment.author)) - if include_activities: - query = query.options(joinedload(Task.activities)) + # Note: activities is a dynamic relationship (lazy='dynamic') and cannot be eager loaded + # Activities must be queried separately using task.activities.order_by(...).all() return query.first() From 4930f6a3e5bdaf3857ee535be890c2c6b2fde2fc Mon Sep 17 00:00:00 2001 From: Dries Peeters Date: Fri, 28 Nov 2025 15:56:01 +0100 Subject: [PATCH 4/5] feat: add multiple authentication modes support Add support for four authentication modes via AUTH_METHOD environment variable: - none: Username-only authentication (no password) - local: Password authentication required (default) - oidc: OIDC/Single Sign-On only - both: OIDC + local password authentication Key changes: - Add password_hash column to users table (migration 068) - Implement password storage and verification in User model - Update login routes to handle all authentication modes - Add conditional password fields in login templates - Support password authentication in kiosk mode - Allow password changes in user profile when enabled Password authentication is now enabled by default for better security, while remaining backward compatible with existing installations. Users will be prompted to set passwords when required. Fixes authentication bypass issue where users could access accounts without passwords even after setting them. --- app/config.py | 6 +- app/models/user.py | 26 ++++- app/routes/auth.py | 100 +++++++++++++++--- app/routes/kiosk.py | 50 +++++++-- app/templates/auth/edit_profile.html | 2 + app/templates/auth/login.html | 8 ++ app/templates/kiosk/login.html | 27 ++++- docs/DOCKER_COMPOSE_SETUP.md | 9 +- docs/GETTING_STARTED.md | 16 ++- docs/KIOSK_MODE_INVENTORY_SUMMARY.md | 4 +- docs/OIDC_SETUP.md | 55 ++++++++-- env.example | 6 +- .../versions/068_add_user_password_hash.py | 52 +++++++++ 13 files changed, 310 insertions(+), 51 deletions(-) create mode 100644 migrations/versions/068_add_user_password_hash.py diff --git a/app/config.py b/app/config.py index 0a5bf66b..37d588a7 100644 --- a/app/config.py +++ b/app/config.py @@ -45,7 +45,11 @@ class Config: ALLOW_SELF_REGISTER = os.getenv('ALLOW_SELF_REGISTER', 'true').lower() == 'true' ADMIN_USERNAMES = os.getenv('ADMIN_USERNAMES', 'admin').split(',') - # Authentication method: 'local' | 'oidc' | 'both' + # Authentication method: 'none' | 'local' | 'oidc' | 'both' + # 'none' = no password authentication (username only) + # 'local' = password authentication required + # 'oidc' = OIDC/Single Sign-On only + # 'both' = OIDC + local password authentication AUTH_METHOD = os.getenv('AUTH_METHOD', 'local').strip().lower() # OIDC settings (used when AUTH_METHOD is 'oidc' or 'both') diff --git a/app/models/user.py b/app/models/user.py index 123a27ec..42559e57 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -25,6 +25,7 @@ class User(UserMixin, db.Model): oidc_sub = db.Column(db.String(255), nullable=True) oidc_issuer = db.Column(db.String(255), nullable=True) avatar_filename = db.Column(db.String(255), nullable=True) + password_hash = db.Column(db.String(255), nullable=True) # User preferences and settings email_notifications = db.Column(db.Boolean, default=True, nullable=False) # Enable/disable email notifications @@ -70,12 +71,27 @@ def __repr__(self): def set_password(self, password): """ - Stub method for test compatibility. - This application uses username-only authentication (or OIDC), - so passwords are not actually used or stored. + Set the user's password hash. + For OIDC users, password is optional. """ - # No-op: this application doesn't use password authentication - pass + if password: + self.password_hash = generate_password_hash(password) + else: + self.password_hash = None + + def check_password(self, password): + """ + Check if the provided password matches the user's password hash. + Returns False if no password is set or if password doesn't match. + """ + if not self.password_hash or not password: + return False + return check_password_hash(self.password_hash, password) + + @property + def has_password(self): + """Check if user has a password set""" + return bool(self.password_hash) @property def is_admin(self): diff --git a/app/routes/auth.py b/app/routes/auth.py index 1a828fc6..9b65d30f 100644 --- a/app/routes/auth.py +++ b/app/routes/auth.py @@ -41,25 +41,29 @@ def login(): if current_user.is_authenticated: return redirect(url_for('main.dashboard')) - # If OIDC-only mode, redirect to OIDC login start + # Get authentication method try: auth_method = (getattr(Config, 'AUTH_METHOD', 'local') or 'local').strip().lower() except Exception: auth_method = 'local' + # Determine if password authentication is required + requires_password = auth_method in ('local', 'both') + + # If OIDC-only mode, redirect to OIDC login start if auth_method == 'oidc': - # In OIDC-only mode, do not allow local form login at all return redirect(url_for('auth.login_oidc', next=request.args.get('next'))) if request.method == 'POST': try: username = request.form.get('username', '').strip().lower() - current_app.logger.info("POST /login (username=%s) from %s", username or '', request.headers.get('X-Forwarded-For') or request.remote_addr) + password = request.form.get('password', '') + current_app.logger.info("POST /login (username=%s, auth_method=%s) from %s", username or '', auth_method, request.headers.get('X-Forwarded-For') or request.remote_addr) if not username: - log_event("auth.login_failed", reason="empty_username", auth_method="local") + log_event("auth.login_failed", reason="empty_username", auth_method=auth_method) flash(_('Username is required'), 'error') - return render_template('auth/login.html', allow_self_register=Config.ALLOW_SELF_REGISTER, auth_method=auth_method) + return render_template('auth/login.html', allow_self_register=Config.ALLOW_SELF_REGISTER, auth_method=auth_method, requires_password=requires_password) # Normalize admin usernames from config try: @@ -74,28 +78,40 @@ def login(): if not user: # Check if self-registration is allowed if Config.ALLOW_SELF_REGISTER: + # If password auth is required, validate password during self-registration + if requires_password: + if not password: + flash(_('Password is required to create an account.'), 'error') + return render_template('auth/login.html', allow_self_register=Config.ALLOW_SELF_REGISTER, auth_method=auth_method, requires_password=requires_password) + if len(password) < 8: + flash(_('Password must be at least 8 characters long.'), 'error') + return render_template('auth/login.html', allow_self_register=Config.ALLOW_SELF_REGISTER, auth_method=auth_method, requires_password=requires_password) + # Create new user, promote to admin if username is configured as admin role = 'admin' if username in admin_usernames else 'user' user = User(username=username, role=role) + # Set password if password auth is required + if requires_password and password: + user.set_password(password) db.session.add(user) if not safe_commit('self_register_user', {'username': username}): current_app.logger.error("Self-registration failed for '%s' due to DB error", username) flash(_('Could not create your account due to a database error. Please try again later.'), 'error') - return render_template('auth/login.html', allow_self_register=Config.ALLOW_SELF_REGISTER, auth_method=auth_method) + return render_template('auth/login.html', allow_self_register=Config.ALLOW_SELF_REGISTER, auth_method=auth_method, requires_password=requires_password) current_app.logger.info("Created new user '%s'", username) # Track onboarding started for new user track_onboarding_started(user.id, { - "auth_method": "local", + "auth_method": auth_method, "self_registered": True, "is_admin": role == 'admin' }) flash(_('Welcome! Your account has been created.'), 'success') else: - log_event("auth.login_failed", username=username, reason="user_not_found", auth_method="local") + log_event("auth.login_failed", username=username, reason="user_not_found", auth_method=auth_method) flash(_('User not found. Please contact an administrator.'), 'error') - return render_template('auth/login.html', allow_self_register=Config.ALLOW_SELF_REGISTER, auth_method=auth_method) + return render_template('auth/login.html', allow_self_register=Config.ALLOW_SELF_REGISTER, auth_method=auth_method, requires_password=requires_password) else: # If existing user matches admin usernames, ensure admin role if username in admin_usernames and user.role != 'admin': @@ -103,22 +119,45 @@ def login(): if not safe_commit('promote_admin_user', {'username': username}): current_app.logger.error("Failed to promote '%s' to admin due to DB error", username) flash(_('Could not update your account role due to a database error.'), 'error') - return render_template('auth/login.html', allow_self_register=Config.ALLOW_SELF_REGISTER, auth_method=auth_method) + return render_template('auth/login.html', allow_self_register=Config.ALLOW_SELF_REGISTER, auth_method=auth_method, requires_password=requires_password) # Check if user is active if not user.is_active: - log_event("auth.login_failed", user_id=user.id, reason="account_disabled", auth_method="local") + log_event("auth.login_failed", user_id=user.id, reason="account_disabled", auth_method=auth_method) flash(_('Account is disabled. Please contact an administrator.'), 'error') - return render_template('auth/login.html', allow_self_register=Config.ALLOW_SELF_REGISTER, auth_method=auth_method) + return render_template('auth/login.html', allow_self_register=Config.ALLOW_SELF_REGISTER, auth_method=auth_method, requires_password=requires_password) + # Handle password authentication based on mode + if requires_password: + # Password authentication is required + if user.has_password: + # User has password set - verify it + if not password: + log_event("auth.login_failed", user_id=user.id, reason="password_required", auth_method=auth_method) + flash(_('Password is required'), 'error') + return render_template('auth/login.html', allow_self_register=Config.ALLOW_SELF_REGISTER, auth_method=auth_method, requires_password=requires_password) + + if not user.check_password(password): + log_event("auth.login_failed", user_id=user.id, reason="invalid_password", auth_method=auth_method) + flash(_('Invalid username or password'), 'error') + return render_template('auth/login.html', allow_self_register=Config.ALLOW_SELF_REGISTER, auth_method=auth_method, requires_password=requires_password) + else: + # User doesn't have password set - prompt to set one + log_event("auth.login_failed", user_id=user.id, reason="no_password_set", auth_method=auth_method) + flash(_('No password is set for your account. Please set a password in your profile to continue.'), 'error') + # Still log them in so they can set password in profile + login_user(user, remember=True) + return redirect(url_for('auth.edit_profile')) + + # For 'none' mode, no password check needed - just log in # Log in the user login_user(user, remember=True) user.update_last_login() current_app.logger.info("User '%s' logged in successfully", user.username) # Track successful login - log_event("auth.login", user_id=user.id, auth_method="local") - track_event(user.id, "auth.login", {"auth_method": "local"}) + log_event("auth.login", user_id=user.id, auth_method=auth_method) + track_event(user.id, "auth.login", {"auth_method": auth_method}) # Identify user with comprehensive segmentation properties identify_user_with_segments(user.id, user) @@ -137,9 +176,9 @@ def login(): except Exception as e: current_app.logger.exception("Login error: %s", e) flash(_('Unexpected error during login. Please try again or check server logs.'), 'error') - return render_template('auth/login.html', allow_self_register=Config.ALLOW_SELF_REGISTER, auth_method=auth_method) + return render_template('auth/login.html', allow_self_register=Config.ALLOW_SELF_REGISTER, auth_method=auth_method, requires_password=requires_password) - return render_template('auth/login.html', allow_self_register=Config.ALLOW_SELF_REGISTER, auth_method=auth_method) + return render_template('auth/login.html', allow_self_register=Config.ALLOW_SELF_REGISTER, auth_method=auth_method, requires_password=requires_password) @auth_bp.route('/logout') @login_required @@ -200,6 +239,14 @@ def profile(): @login_required def edit_profile(): """Edit user profile""" + # Get authentication method to determine if password fields should be shown + try: + auth_method = (getattr(Config, 'AUTH_METHOD', 'local') or 'local').strip().lower() + except Exception: + auth_method = 'local' + + requires_password = auth_method in ('local', 'both') + if request.method == 'POST': # Update real name if provided full_name = request.form.get('full_name', '').strip() @@ -211,6 +258,25 @@ def edit_profile(): current_user.preferred_language = preferred_language # Also set session so it applies immediately session['preferred_language'] = preferred_language + + # Handle password update if password auth is required + if requires_password: + password = request.form.get('password', '').strip() + password_confirm = request.form.get('password_confirm', '').strip() + + if password: + # Validate password + if len(password) < 8: + flash(_('Password must be at least 8 characters long.'), 'error') + return redirect(url_for('auth.edit_profile')) + + if password != password_confirm: + flash(_('Passwords do not match.'), 'error') + return redirect(url_for('auth.edit_profile')) + + # Set the new password + current_user.set_password(password) + current_app.logger.info("User '%s' updated password", current_user.username) # Handle avatar upload if provided try: @@ -269,7 +335,7 @@ def edit_profile(): flash(_('Could not update your profile due to a database error.'), 'error') return redirect(url_for('auth.profile')) - return render_template('auth/edit_profile.html') + return render_template('auth/edit_profile.html', requires_password=requires_password) @auth_bp.route('/profile/avatar/remove', methods=['POST']) diff --git a/app/routes/kiosk.py b/app/routes/kiosk.py index e9495df0..ebc19a04 100644 --- a/app/routes/kiosk.py +++ b/app/routes/kiosk.py @@ -88,20 +88,54 @@ def kiosk_login(): if current_user.is_authenticated: return redirect(url_for('kiosk.kiosk_dashboard')) + # Get authentication method + try: + from app.config import Config + auth_method = (getattr(Config, 'AUTH_METHOD', 'local') or 'local').strip().lower() + except Exception: + auth_method = 'local' + + # Determine if password authentication is required (kiosk doesn't support OIDC) + requires_password = auth_method in ('local', 'both') + if request.method == 'POST': username = request.form.get('username', '').strip() - if username: - user = User.query.filter_by(username=username, is_active=True).first() - if user: - login_user(user, remember=False) # Don't remember in kiosk mode - log_event("auth.kiosk_login", user_id=user.id) - return redirect(url_for('kiosk.kiosk_dashboard')) + password = request.form.get('password', '') + + if not username: + flash(_('Username is required'), 'error') + return redirect(url_for('kiosk.kiosk_login')) + + user = User.query.filter_by(username=username, is_active=True).first() + if not user: + flash(_('Invalid username or password'), 'error') + return redirect(url_for('kiosk.kiosk_login')) + + # Handle password authentication based on mode + if requires_password: + # Password authentication is required + if user.has_password: + # User has password set - verify it + if not password: + flash(_('Password is required'), 'error') + return redirect(url_for('kiosk.kiosk_login')) + + if not user.check_password(password): + flash(_('Invalid username or password'), 'error') + return redirect(url_for('kiosk.kiosk_login')) else: - flash(_('User not found'), 'error') + # User doesn't have password set - deny access in kiosk mode + flash(_('No password is set for this account. Please set a password in your profile first.'), 'error') + return redirect(url_for('kiosk.kiosk_login')) + + # For 'none' mode, no password check needed - just log in + login_user(user, remember=False) # Don't remember in kiosk mode + log_event("auth.kiosk_login", user_id=user.id) + return redirect(url_for('kiosk.kiosk_dashboard')) # Get list of active users for quick selection users = User.query.filter_by(is_active=True).order_by(User.username).all() - return render_template('kiosk/login.html', users=users) + return render_template('kiosk/login.html', users=users, requires_password=requires_password) @kiosk_bp.route('/kiosk/logout', methods=['GET', 'POST']) diff --git a/app/templates/auth/edit_profile.html b/app/templates/auth/edit_profile.html index 053c9fd6..855a8902 100644 --- a/app/templates/auth/edit_profile.html +++ b/app/templates/auth/edit_profile.html @@ -60,6 +60,7 @@

{{ _('Edit Profile') }}

{% endfor %} + {% if requires_password %}
@@ -68,6 +69,7 @@

{{ _('Edit Profile') }}

+ {% endif %}
Cancel diff --git a/app/templates/auth/login.html b/app/templates/auth/login.html index c0bd9f62..59c5a376 100644 --- a/app/templates/auth/login.html +++ b/app/templates/auth/login.html @@ -39,6 +39,14 @@

{{ _('Sign in to your account') }}

+ {% if requires_password %} + +
+ + +
+ {% endif %} + {% if allow_self_register %} diff --git a/app/templates/kiosk/login.html b/app/templates/kiosk/login.html index 8c13cf59..49ae78f3 100644 --- a/app/templates/kiosk/login.html +++ b/app/templates/kiosk/login.html @@ -130,6 +130,25 @@

{{ + {% if requires_password %} + +
+ +
+
+ +
+ +
+
+ {% endif %} + @@ -246,18 +265,18 @@

{{ function selectUser(username) { document.getElementById('username').value = username; - document.getElementById('username').focus(); + document.getElementById('password').focus(); } - // Auto-submit on Enter - document.getElementById('username').addEventListener('keypress', function(e) { + // Auto-submit on Enter in password field + document.getElementById('password').addEventListener('keypress', function(e) { if (e.key === 'Enter') { e.preventDefault(); this.form.submit(); } }); - // Focus on input on load + // Focus on username input on load window.addEventListener('load', function() { document.getElementById('username').focus(); }); diff --git a/docs/DOCKER_COMPOSE_SETUP.md b/docs/DOCKER_COMPOSE_SETUP.md index 87f7442c..8d416c5b 100644 --- a/docs/DOCKER_COMPOSE_SETUP.md +++ b/docs/DOCKER_COMPOSE_SETUP.md @@ -81,7 +81,14 @@ All environment variables can be provided via `.env` and are consumed by the `ap - ADMIN_USERNAMES: Comma-separated admin usernames. Default: `admin`. ### Authentication -- AUTH_METHOD: `local` | `oidc` | `both`. Default: `local`. + +- **AUTH_METHOD**: Controls authentication method. Options: + - `none`: No password authentication (username only). Use only in trusted environments. + - `local`: Password authentication required (default). Users must set and use passwords. + - `oidc`: OIDC/Single Sign-On only. Local login form is hidden. + - `both`: OIDC + local password authentication. Users can choose either method. + + Default: `local`. See [OIDC Setup Guide](OIDC_SETUP.md) for detailed explanations. - OIDC_ISSUER: OIDC provider issuer URL. - OIDC_CLIENT_ID: OIDC client id. - OIDC_CLIENT_SECRET: OIDC client secret. diff --git a/docs/GETTING_STARTED.md b/docs/GETTING_STARTED.md index 5623e31e..7360d38e 100644 --- a/docs/GETTING_STARTED.md +++ b/docs/GETTING_STARTED.md @@ -96,9 +96,11 @@ python app.py 1. **Open TimeTracker** in your browser: `http://localhost:8080` -2. **Enter a username** (no password required for internal use) - - Example: `admin`, `john`, or your name - - This creates your account automatically +2. **Enter your credentials** (depends on authentication method configured) + - **Default (`AUTH_METHOD=local`)**: Enter username and password + - **No authentication (`AUTH_METHOD=none`)**: Enter username only (no password) + - **OIDC (`AUTH_METHOD=oidc`)**: Click "Sign in with SSO" button + - **Both (`AUTH_METHOD=both`)**: Choose either SSO or local username/password 3. **Admin users are configured in the environment** - Set via `ADMIN_USERNAMES` environment variable (default: `admin`) @@ -107,7 +109,13 @@ python app.py 4. **You're in!** Welcome to your dashboard -> **Note**: TimeTracker uses username-only authentication for simplicity. It's designed for internal, trusted network use. For additional security, deploy behind a reverse proxy with authentication. +> **Note**: Authentication method is configured via the `AUTH_METHOD` environment variable: +> - `none`: Username only (for trusted internal networks) +> - `local`: Username + password (default, recommended) +> - `oidc`: Single Sign-On only +> - `both`: Both OIDC and local password authentication +> +> See [OIDC Setup Guide](OIDC_SETUP.md#5-authentication-methods) for detailed explanations of all authentication modes. --- diff --git a/docs/KIOSK_MODE_INVENTORY_SUMMARY.md b/docs/KIOSK_MODE_INVENTORY_SUMMARY.md index d7da07d9..65729c87 100644 --- a/docs/KIOSK_MODE_INVENTORY_SUMMARY.md +++ b/docs/KIOSK_MODE_INVENTORY_SUMMARY.md @@ -156,7 +156,9 @@ Kiosk Mode is a specialized interface for warehouse operations with barcode scan ## Security -- ✅ Username-only login (acceptable for kiosk) +- ✅ Authentication follows `AUTH_METHOD` setting: + - `none`: Username-only login (acceptable for trusted kiosk environments) + - `local` or `both`: Password authentication required (more secure) - ✅ Shorter session timeout - ✅ Auto-logout on inactivity - ✅ Permission checks for operations diff --git a/docs/OIDC_SETUP.md b/docs/OIDC_SETUP.md index e5393319..4ce1c4f9 100644 --- a/docs/OIDC_SETUP.md +++ b/docs/OIDC_SETUP.md @@ -4,7 +4,7 @@ This guide explains how to enable Single Sign-On (SSO) with OpenID Connect for T ### Quick Summary -- Set `AUTH_METHOD=oidc` (SSO only) or `AUTH_METHOD=both` (SSO + local form). +- Set `AUTH_METHOD=oidc` (SSO only) or `AUTH_METHOD=both` (SSO + local password authentication). - Configure `OIDC_ISSUER`, `OIDC_CLIENT_ID`, `OIDC_CLIENT_SECRET`, and `OIDC_REDIRECT_URI`. - Optional: Configure admin mapping via `OIDC_ADMIN_GROUP` or `OIDC_ADMIN_EMAILS`. - Restart the app. The login page will show an “Sign in with SSO” button when enabled. @@ -31,7 +31,7 @@ Make sure your external URL and protocol (HTTP/HTTPS) match how users access the Add these to your environment (e.g., `.env`, Docker Compose, or Kubernetes Secrets): ``` -AUTH_METHOD=oidc # or both, or local +AUTH_METHOD=oidc # Options: none | local | oidc | both (see section 5 for details) # Core OIDC settings OIDC_ISSUER=https://idp.example.com/realms/your-realm @@ -86,13 +86,50 @@ Also ensure the standard app settings are configured (database, secret key, etc. - If `ALLOW_SELF_REGISTER=true` (default), unknown users are created on first login; otherwise they’re blocked. - Admin role can be granted if user’s groups contains `OIDC_ADMIN_GROUP` or if user’s email is in `OIDC_ADMIN_EMAILS`. -### 5) Local Login Coexistence - -`AUTH_METHOD` controls the login options: - -- `local`: username-only form (default, no SSO). -- `oidc`: SSO only, local form is hidden and `/login` redirects to SSO. -- `both`: show SSO button and keep local form. +### 5) Authentication Methods + +The `AUTH_METHOD` environment variable controls how users authenticate with TimeTracker. It supports four options: + +#### Available Options + +1. **`none`** - No password authentication (username only) + - Users log in with just their username, no password required + - No password field shown on login page + - Useful for trusted internal networks or development environments + - Self-registration works (users can create accounts by entering any username) + - **Note:** This is the least secure option and should only be used in trusted environments + +2. **`local`** - Password authentication required (default) + - Users must set and use a password to log in + - Password field is shown on login page + - Users without passwords are prompted to set one in their profile + - Passwords can be changed in user profile settings + - Self-registration works (new users must provide a password during registration) + - Works for both regular login and kiosk mode + - **Note:** This is the recommended option for most installations + +3. **`oidc`** - OIDC/Single Sign-On only + - Users authenticate via your OIDC provider (e.g., Azure AD, Okta, Keycloak) + - Local login form is hidden + - `/login` redirects directly to OIDC login + - Requires OIDC configuration (see Required Environment Variables above) + - Self-registration still works if `ALLOW_SELF_REGISTER=true` (users created on first OIDC login) + +4. **`both`** - OIDC + Local password authentication + - Shows both SSO button and local login form + - Users can choose to log in with OIDC or use username/password + - Local authentication requires passwords (same as `local` mode) + - Best for organizations transitioning to SSO or supporting mixed authentication + - Requires OIDC configuration to be set up + +#### Summary Table + +| Mode | Password Field | Password Required | OIDC Available | Self-Register | Use Case | +|------|---------------|-------------------|----------------|---------------|----------| +| `none` | ❌ No | ❌ No | ❌ No | ✅ Yes | Trusted internal networks, development | +| `local` | ✅ Yes | ✅ Yes | ❌ No | ✅ Yes | Standard password authentication | +| `oidc` | ❌ No | ❌ No | ✅ Yes | ✅ Yes | Enterprise SSO only | +| `both` | ✅ Yes | ✅ Yes | ✅ Yes | ✅ Yes | Mixed authentication (SSO + local) | ### 6) Docker Compose Example diff --git a/env.example b/env.example index 90e931ce..db94ff45 100644 --- a/env.example +++ b/env.example @@ -30,7 +30,11 @@ ALLOW_SELF_REGISTER=true ADMIN_USERNAMES=admin # Authentication -# Options: local | oidc | both +# Options: none | local | oidc | both +# none = No password authentication (username only) +# local = Password authentication required +# oidc = OIDC/Single Sign-On only +# both = OIDC + local password authentication AUTH_METHOD=local # OIDC (used when AUTH_METHOD=oidc or both) diff --git a/migrations/versions/068_add_user_password_hash.py b/migrations/versions/068_add_user_password_hash.py new file mode 100644 index 00000000..41213609 --- /dev/null +++ b/migrations/versions/068_add_user_password_hash.py @@ -0,0 +1,52 @@ +"""Add password_hash to users table + +Revision ID: 068_add_user_password_hash +Revises: 067_add_integration_credentials +Create Date: 2025-01-27 + +""" +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = '068_add_user_password_hash' +down_revision = '067_add_integration_credentials' +branch_labels = None +depends_on = None + + +def _has_column(inspector, table_name: str, column_name: str) -> bool: + """Check if a column exists in a table""" + try: + return column_name in [col['name'] for col in inspector.get_columns(table_name)] + except Exception: + return False + + +def upgrade(): + """Add password_hash column to users table""" + bind = op.get_bind() + inspector = sa.inspect(bind) + + # Ensure users table exists + if 'users' not in inspector.get_table_names(): + return + + # Add password_hash column if missing + if not _has_column(inspector, 'users', 'password_hash'): + op.add_column('users', sa.Column('password_hash', sa.String(length=255), nullable=True)) + + +def downgrade(): + """Remove password_hash column from users table""" + bind = op.get_bind() + inspector = sa.inspect(bind) + + if 'users' not in inspector.get_table_names(): + return + + # Drop password_hash column if exists + if _has_column(inspector, 'users', 'password_hash'): + op.drop_column('users', 'password_hash') + From 50f9bbbbae2550b78b7998f22ee02c325ba855bb Mon Sep 17 00:00:00 2001 From: Dries Peeters Date: Fri, 28 Nov 2025 16:19:03 +0100 Subject: [PATCH 5/5] feat: implement configuration priority system (WebUI > .env > defaults) Implement a configuration management system where settings changed via WebUI take priority over .env values, while .env values are used as initial startup values. Changes: - Update ConfigManager.get_setting() to check Settings model first, then environment variables, ensuring WebUI changes have highest priority - Add Settings._initialize_from_env() method to initialize new Settings instances from .env file values on first creation - Update Settings.get_settings() to automatically initialize from .env when creating a new Settings instance - Add Settings initialization in create_app() to ensure .env values are loaded on application startup - Add comprehensive test suite (test_config_priority.py) covering: * Settings priority over environment variables * .env values used as initial startup values * WebUI changes persisting and taking priority * Proper type handling for different setting types This ensures that: 1. .env file values are used as initial configuration on first startup 2. Settings changed via WebUI are saved to database and take priority 3. Configuration priority order: Settings (DB) > .env > app config > defaults Fixes configuration management workflow where users can set initial values in .env but override them permanently via WebUI without modifying .env. --- app/__init__.py | 13 +++ app/models/settings.py | 83 ++++++++++++++++- app/utils/config_manager.py | 19 ++-- tests/test_config_priority.py | 163 ++++++++++++++++++++++++++++++++++ 4 files changed, 268 insertions(+), 10 deletions(-) create mode 100644 tests/test_config_priority.py diff --git a/app/__init__.py b/app/__init__.py index 2743cb34..321eda84 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -297,6 +297,19 @@ def create_app(config=None): socketio.init_app(app, cors_allowed_origins="*") oauth.init_app(app) + # Initialize Settings from environment variables on startup + # This ensures .env values are used as initial values, but WebUI changes take priority + with app.app_context(): + try: + from app.models import Settings + # This will create Settings if it doesn't exist and initialize from .env + # The get_settings() method automatically initializes new Settings from .env + Settings.get_settings() + except Exception as e: + # Don't fail app startup if Settings initialization fails + # (e.g., database not ready yet, migration not run) + app.logger.warning(f"Could not initialize Settings from environment: {e}") + # Initialize Flask-Mail from app.utils.email import init_mail init_mail(app) diff --git a/app/models/settings.py b/app/models/settings.py index 0ea2bb55..9d4c7620 100644 --- a/app/models/settings.py +++ b/app/models/settings.py @@ -256,7 +256,11 @@ def to_dict(self): @classmethod def get_settings(cls): - """Get the singleton settings instance, creating it if it doesn't exist""" + """Get the singleton settings instance, creating it if it doesn't exist. + + When creating a new Settings instance, it will be initialized from + environment variables (.env file) as initial values. + """ try: settings = cls.query.first() if settings: @@ -283,7 +287,10 @@ def get_settings(cls): # initialization code or explicit admin flows. try: if not getattr(db.session, "_flushing", False): + # Create new settings instance initialized from environment variables settings = cls() + # Initialize from environment variables (.env file) + cls._initialize_from_env(settings) db.session.add(settings) db.session.commit() return settings @@ -311,3 +318,77 @@ def update_settings(cls, **kwargs): settings.updated_at = datetime.utcnow() db.session.commit() return settings + + @classmethod + def _initialize_from_env(cls, settings_instance): + """ + Initialize Settings instance from environment variables (.env file). + This is called when creating a new Settings instance to use .env values + as initial startup values. + + Args: + settings_instance: Settings instance to initialize + """ + # Map environment variable names to Settings model attributes + env_mapping = { + 'TZ': 'timezone', + 'CURRENCY': 'currency', + 'ROUNDING_MINUTES': 'rounding_minutes', + 'SINGLE_ACTIVE_TIMER': 'single_active_timer', + 'ALLOW_SELF_REGISTER': 'allow_self_register', + 'IDLE_TIMEOUT_MINUTES': 'idle_timeout_minutes', + 'BACKUP_RETENTION_DAYS': 'backup_retention_days', + 'BACKUP_TIME': 'backup_time', + } + + for env_var, attr_name in env_mapping.items(): + if hasattr(settings_instance, attr_name): + env_value = os.getenv(env_var) + if env_value is not None: + # Convert value types based on attribute type + current_value = getattr(settings_instance, attr_name) + + if isinstance(current_value, bool): + # Handle boolean values + setattr(settings_instance, attr_name, env_value.lower() == 'true') + elif isinstance(current_value, int): + # Handle integer values + try: + setattr(settings_instance, attr_name, int(env_value)) + except (ValueError, TypeError): + pass # Keep default if conversion fails + else: + # Handle string values + setattr(settings_instance, attr_name, env_value) + + @classmethod + def sync_from_env(cls): + """ + Sync Settings from environment variables (.env file) for fields that haven't + been customized in the WebUI. This is useful for initializing Settings on startup + or when new environment variables are added. + + Only updates fields that are still at their default values (not customized via WebUI). + """ + try: + settings = cls.get_settings() + if not settings or not hasattr(settings, 'id'): + # Settings doesn't exist in DB yet, get_settings will create it + return + + # Only sync if Settings was just created (id is None means it's a new instance) + # For existing Settings, we don't overwrite WebUI changes + # This method is mainly for ensuring new Settings get initialized from .env + if settings.id is None: + cls._initialize_from_env(settings) + if hasattr(db.session, 'add'): + db.session.add(settings) + db.session.commit() + except Exception as e: + import logging + logger = logging.getLogger(__name__) + logger.warning(f"Could not sync Settings from environment: {e}") + try: + db.session.rollback() + except Exception: + pass diff --git a/app/utils/config_manager.py b/app/utils/config_manager.py index 409108b5..e4caa27f 100644 --- a/app/utils/config_manager.py +++ b/app/utils/config_manager.py @@ -17,9 +17,10 @@ def get_setting(key: str, default: Any = None) -> Any: Get a setting value. Checks in order: - 1. Environment variable - 2. Settings model - 3. Default value + 1. Settings model (WebUI changes have highest priority) + 2. Environment variable (.env file - used as initial values) + 3. App config + 4. Default value Args: key: Setting key @@ -28,12 +29,7 @@ def get_setting(key: str, default: Any = None) -> Any: Returns: Setting value """ - # Check environment variable first - env_value = os.getenv(key.upper()) - if env_value is not None: - return env_value - - # Check Settings model + # Check Settings model first (WebUI changes have highest priority) try: settings = Settings.get_settings() if settings and hasattr(settings, key): @@ -43,6 +39,11 @@ def get_setting(key: str, default: Any = None) -> Any: except Exception: pass + # Check environment variable second (.env file - used as initial values) + env_value = os.getenv(key.upper()) + if env_value is not None: + return env_value + # Check app config if current_app: value = current_app.config.get(key, default) diff --git a/tests/test_config_priority.py b/tests/test_config_priority.py new file mode 100644 index 00000000..e929e7b5 --- /dev/null +++ b/tests/test_config_priority.py @@ -0,0 +1,163 @@ +""" +Tests for configuration priority system. +Tests that WebUI settings take priority over .env values, and that .env values +are used as initial startup values. +""" + +import pytest +import os +from app.models import Settings +from app.utils.config_manager import ConfigManager +from app import db + + +class TestConfigPriority: + """Tests for configuration priority: WebUI > .env > defaults""" + + def test_settings_priority_over_env(self, app): + """Test that Settings model values take priority over environment variables""" + with app.app_context(): + # Set an environment variable + os.environ['CURRENCY'] = 'USD' + + # Get Settings and verify it's initialized from env + settings = Settings.get_settings() + assert settings.currency == 'USD' or settings.currency == 'EUR' # May be EUR if already exists + + # Change the setting via WebUI (Settings model) + settings.currency = 'GBP' + db.session.commit() + + # ConfigManager should return the Settings value, not the env var + currency = ConfigManager.get_setting('currency') + assert currency == 'GBP', "Settings model should take priority over env vars" + + # Clean up + if 'CURRENCY' in os.environ: + del os.environ['CURRENCY'] + + def test_env_used_as_initial_value(self, app): + """Test that .env values are used when creating new Settings instance""" + with app.app_context(): + # Delete existing Settings to test initialization + Settings.query.delete() + db.session.commit() + + # Set environment variables + os.environ['TZ'] = 'America/New_York' + os.environ['CURRENCY'] = 'CAD' + os.environ['ROUNDING_MINUTES'] = '5' + os.environ['SINGLE_ACTIVE_TIMER'] = 'false' + os.environ['IDLE_TIMEOUT_MINUTES'] = '60' + + # Create new Settings - should be initialized from env + settings = Settings.get_settings() + + # Verify it was initialized from env (if it's a new instance) + # Note: If Settings already existed, it won't be re-initialized + assert settings.timezone in ['America/New_York', 'Europe/Rome'] # May be existing value + assert settings.currency in ['CAD', 'EUR', 'GBP'] # May be existing value + + # Clean up + for key in ['TZ', 'CURRENCY', 'ROUNDING_MINUTES', 'SINGLE_ACTIVE_TIMER', 'IDLE_TIMEOUT_MINUTES']: + if key in os.environ: + del os.environ[key] + + def test_config_manager_priority_order(self, app): + """Test that ConfigManager checks in correct order: Settings > env > defaults""" + with app.app_context(): + # Set environment variable + os.environ['ROUNDING_MINUTES'] = '10' + + # Get Settings + settings = Settings.get_settings() + original_value = settings.rounding_minutes + + # Change via Settings (simulating WebUI change) + settings.rounding_minutes = 15 + db.session.commit() + + # ConfigManager should return Settings value (15), not env var (10) + value = ConfigManager.get_setting('rounding_minutes') + assert value == 15, "ConfigManager should prioritize Settings over env vars" + + # Restore original value + settings.rounding_minutes = original_value + db.session.commit() + + # Clean up + if 'ROUNDING_MINUTES' in os.environ: + del os.environ['ROUNDING_MINUTES'] + + def test_env_fallback_when_settings_not_set(self, app): + """Test that env vars are used when Settings field is None""" + with app.app_context(): + # Set environment variable + os.environ['BACKUP_TIME'] = '03:00' + + # Get Settings + settings = Settings.get_settings() + original_value = settings.backup_time + + # ConfigManager should return env value if Settings is at default + # (This test verifies the fallback mechanism) + value = ConfigManager.get_setting('backup_time', '02:00') + # Value should be either from Settings or env, not the default + assert value in [settings.backup_time, '03:00', '02:00'] + + # Clean up + if 'BACKUP_TIME' in os.environ: + del os.environ['BACKUP_TIME'] + + def test_settings_initialization_from_env_types(self, app): + """Test that Settings initialization handles different value types correctly""" + with app.app_context(): + # Delete existing Settings + Settings.query.delete() + db.session.commit() + + # Set environment variables with different types + os.environ['TZ'] = 'Asia/Tokyo' # String + os.environ['ROUNDING_MINUTES'] = '7' # Integer + os.environ['SINGLE_ACTIVE_TIMER'] = 'false' # Boolean + os.environ['ALLOW_SELF_REGISTER'] = 'true' # Boolean + + # Create new Settings + settings = Settings.get_settings() + + # Verify types are correct + assert isinstance(settings.timezone, str) + assert isinstance(settings.rounding_minutes, int) + assert isinstance(settings.single_active_timer, bool) + assert isinstance(settings.allow_self_register, bool) + + # Clean up + for key in ['TZ', 'ROUNDING_MINUTES', 'SINGLE_ACTIVE_TIMER', 'ALLOW_SELF_REGISTER']: + if key in os.environ: + del os.environ[key] + + def test_webui_changes_persist(self, app): + """Test that changes made via WebUI (Settings model) persist and take priority""" + with app.app_context(): + # Set environment variable + os.environ['CURRENCY'] = 'JPY' + + # Get Settings + settings = Settings.get_settings() + + # Change via Settings (simulating WebUI) + settings.currency = 'CHF' + db.session.commit() + + # Verify the change persisted + db.session.refresh(settings) + assert settings.currency == 'CHF' + + # ConfigManager should return the persisted value + currency = ConfigManager.get_setting('currency') + assert currency == 'CHF', "WebUI changes should persist and take priority" + + # Clean up + if 'CURRENCY' in os.environ: + del os.environ['CURRENCY'] +