Merge pull request 'Fix #147: Benutzerverwaltung nur noch für Admins zugänglich' (#180) from fds2610-bot/Steuerkram:main into main
Reviewed-on: https://codeberg.org/fds2610/Steuerkram/pulls/180
This commit is contained in:
fds2610
commit
e109f0cb7c
12 changed files with 259 additions and 13 deletions
18
Changes.md
18
Changes.md
|
|
@ -2,6 +2,24 @@
|
|||
|
||||
---
|
||||
|
||||
## 2026-08-09 – Regressionstest: Reflektiertes XSS über Kontonummer-Freitextfeld (#149)
|
||||
|
||||
- `BookingController::store()` schreibt eine nicht auflösbare Freitext-Kontonummer (`kategorie_kontonummer`/`zahlungs_kontonummer`) ungefiltert in `$errors[...]`. Die Ausgabestelle in `src/Views/belege/detail.php:419` escaped Schlüssel und Wert bereits seit Fix #132 (Commit 6489819) konsequent mit `$h()` – der in #149 gemeldete Bug war zum Zeitpunkt der Meldung durch die dortige Änderung bereits geschlossen, nur ohne eigene Testabdeckung für diesen speziellen Eingabepfad.
|
||||
- **`tests/84_BookingStoreReflectedXssTest.php`** (neu): Rendert `belege/detail.php` mit einem `<script>`-Payload in `formErrors['kategorie_konto_id']` (wie ihn `store()` bei nicht gefundener Kontonummer erzeugt) und stellt sicher, dass der Payload nur escaped in der Ausgabe erscheint.
|
||||
|
||||
## 2026-08-09 – Fix: Fehlende Autorisierung in der Benutzerverwaltung (#147)
|
||||
|
||||
- `users` hatte kein Rollen-Feld – `Auth::require()` prüfte für alle `/useradmin`-Routen (`GET/POST /useradmin`, `.../{id}`, `.../{id}/delete`, `.../{id}/toggle`) nur eine gültige Session, keine besondere Berechtigung. Jeder eingeloggte Benutzer (auch einer, der nur für die Belegerfassung angelegt wurde) konnte damit beliebige Konten anlegen, das Passwort jedes anderen Benutzers – inklusive des ursprünglichen Admin-Accounts – setzen sowie andere Benutzer löschen oder deaktivieren (Privilege Escalation, CWE-862/CWE-269).
|
||||
- **`migrations/093_users_role.sql`** (neu): `users.role ENUM('admin','user') NOT NULL DEFAULT 'user'`. Auf Bestandsinstallationen wird der älteste Benutzer (`MIN(id)`) automatisch zu `admin`, damit die Benutzerverwaltung nach dem Update nicht für alle gesperrt ist.
|
||||
- **`src/Models/User.php`:** `create()` akzeptiert jetzt eine optionale Rolle (Default `user`), neue Methode `setRole()`; `all()`/`find()`/`findByUsername()` liefern das Rollen-Feld mit.
|
||||
- **`src/Auth.php`:** `login()` legt die Rolle in der Session ab (`$_SESSION['role']`). Neue Methoden `isAdmin()` und `requireAdmin()` (analog zu `require()`, antwortet bei fehlender Admin-Rolle mit HTTP 403).
|
||||
- **`src/Controllers/LoginController.php`:** Der bei der Ersteinrichtung angelegte erste Benutzer erhält automatisch die Rolle `admin`.
|
||||
- **`public/index.php`:** Zentraler Guard vor dem Routing – jede Anfrage unter `/useradmin` durchläuft zusätzlich `Auth::requireAdmin()`.
|
||||
- **`src/Views/layout/nav.php`:** Der Menüpunkt „Benutzer“ wird nur noch für Admins gerendert (rein kosmetisch, ersetzt nicht die serverseitige Prüfung).
|
||||
- **`scripts/e2e-user.php`:** Der E2E-Testnutzer wird als Admin angelegt bzw. beim erneuten Ausführen auf die Rolle `admin` zurückgesetzt, damit die E2E-Suite weiterhin auf `/useradmin` zugreifen kann.
|
||||
- **`tests/83_UserRoleTest.php`** (neu): Rollen-Default `user`, explizite Vergabe von `admin`, `setRole()`, `Auth::isAdmin()` anhand der Session.
|
||||
- **`tests/e2e/specs/useradmin-authorization.spec.js`** (neu): Ein frisch angelegter Benutzer ohne Admin-Rolle sieht den Nav-Link nicht und bekommt beim direkten Aufruf von `/useradmin` HTTP 403.
|
||||
|
||||
## 2026-08-03 – Fix: AfA-Buchungen umgehen das Booking-Modell (#155)
|
||||
|
||||
- `AssetController::bookAfa()` und `reverseAfaBooking()` setzten ihre `INSERT INTO bookings`-Statements bislang direkt ab statt über `Booking::create()`/`reverse()`. Vier Folgen: Die Sperre eines abgeschlossenen Geschäftsjahres griff nicht, ein zweiter, unabhängiger Nummernkreis (`AFA-NNNNN`) lief neben `BU-YYYY-NNNNNN` auf derselben Spalte, die Nummernvergabe stand außerhalb der Transaktion, und das Storno einer AfA-Buchung wurde immer dem *aktuellen* Kalenderjahr statt dem Jahr der Originalbuchung zugeordnet.
|
||||
|
|
|
|||
|
|
@ -145,4 +145,5 @@ Zuordnungsregeln: [berechnungslogik.md](berechnungslogik.md)
|
|||
|
||||
- Benutzer anlegen, bearbeiten, deaktivieren und löschen
|
||||
- Session-basierte Authentifizierung mit 24 h Timeout
|
||||
- Erste Benutzeranlage über Ersteinrichtungs-Dialog beim ersten Aufruf
|
||||
- Erste Benutzeranlage über Ersteinrichtungs-Dialog beim ersten Aufruf, dieser Benutzer erhält automatisch die Rolle `admin`
|
||||
- Rollen `admin`/`user`: `/useradmin` (Benutzer anlegen, Passwörter setzen, löschen, deaktivieren) ist ausschließlich Admins vorbehalten (`Auth::requireAdmin()`); der Menüpunkt ist für andere Benutzer ausgeblendet
|
||||
|
|
|
|||
13
migrations/093_users_role.sql
Normal file
13
migrations/093_users_role.sql
Normal file
|
|
@ -0,0 +1,13 @@
|
|||
-- Migration 093: Rollen-Feld für users (Fix #147 - fehlende Autorisierung
|
||||
-- in der Benutzerverwaltung, jeder eingeloggte Benutzer konnte bislang
|
||||
-- beliebige Konten anlegen/löschen/übernehmen).
|
||||
ALTER TABLE users
|
||||
ADD COLUMN IF NOT EXISTS role ENUM('admin','user') NOT NULL DEFAULT 'user' AFTER password_hash;
|
||||
|
||||
-- Bestandsinstallationen: den ältesten Benutzer automatisch zum Admin
|
||||
-- machen, damit die Benutzerverwaltung nach dem Update nicht für alle
|
||||
-- Bestandsnutzer gesperrt ist.
|
||||
UPDATE users u
|
||||
JOIN (SELECT MIN(id) AS min_id FROM users) m ON u.id = m.min_id
|
||||
SET u.role = 'admin'
|
||||
WHERE u.role != 'admin';
|
||||
|
|
@ -109,6 +109,9 @@ $requestPath = parse_url($_SERVER['REQUEST_URI'] ?? '/', PHP_URL_PATH);
|
|||
if ($requestPath !== '/login') {
|
||||
Auth::require();
|
||||
}
|
||||
if (str_starts_with($requestPath ?? '', '/useradmin')) {
|
||||
Auth::requireAdmin();
|
||||
}
|
||||
|
||||
$router = new Router();
|
||||
|
||||
|
|
|
|||
|
|
@ -59,10 +59,13 @@ if ($existing) {
|
|||
$id = (int)$existing['id'];
|
||||
User::update($id, $username, $password);
|
||||
User::setActive($id, true);
|
||||
User::setRole($id, 'admin');
|
||||
User::resetFailedLogins($id);
|
||||
echo "Testnutzer \"{$username}\" aktualisiert (Passwort neu gesetzt, aktiv, entsperrt).\n";
|
||||
echo "Testnutzer \"{$username}\" aktualisiert (Passwort neu gesetzt, aktiv, entsperrt, Rolle admin).\n";
|
||||
} else {
|
||||
User::create($username, $password);
|
||||
// Rolle admin, damit die E2E-Suite auch die Benutzerverwaltung
|
||||
// (/useradmin) erreichen kann (Fix #147).
|
||||
User::create($username, $password, 'admin');
|
||||
$created = User::findByUsername($username);
|
||||
if (!$created) {
|
||||
fwrite(STDERR, "Fehler: Testnutzer \"{$username}\" konnte nicht angelegt werden.\n");
|
||||
|
|
|
|||
23
src/Auth.php
23
src/Auth.php
|
|
@ -65,12 +65,33 @@ class Auth
|
|||
}
|
||||
}
|
||||
|
||||
public static function login(int $userId): void
|
||||
public static function login(int $userId, string $role = 'user'): void
|
||||
{
|
||||
self::startSession();
|
||||
session_regenerate_id(true);
|
||||
$_SESSION['user_id'] = $userId;
|
||||
$_SESSION['login_time'] = time();
|
||||
$_SESSION['role'] = $role;
|
||||
}
|
||||
|
||||
/**
|
||||
* Wie require(), verlangt zusätzlich die Rolle 'admin'. Schützt die
|
||||
* Benutzerverwaltung vor Privilege Escalation durch normale Benutzer
|
||||
* (Issue #147) – die Rolle steckt in der Session und wird erst beim
|
||||
* nächsten Login neu geladen.
|
||||
*/
|
||||
public static function requireAdmin(): void
|
||||
{
|
||||
self::require();
|
||||
if (!self::isAdmin()) {
|
||||
http_response_code(403);
|
||||
exit('Kein Zugriff.');
|
||||
}
|
||||
}
|
||||
|
||||
public static function isAdmin(): bool
|
||||
{
|
||||
return ($_SESSION['role'] ?? '') === 'admin';
|
||||
}
|
||||
|
||||
public static function logout(): void
|
||||
|
|
|
|||
|
|
@ -51,9 +51,9 @@ class LoginController
|
|||
return;
|
||||
}
|
||||
|
||||
User::create($username, $password);
|
||||
User::create($username, $password, 'admin');
|
||||
$user = User::findByUsername($username);
|
||||
Auth::login((int)$user['id']);
|
||||
Auth::login((int)$user['id'], (string)$user['role']);
|
||||
View::redirect('/');
|
||||
return;
|
||||
}
|
||||
|
|
@ -102,7 +102,7 @@ class LoginController
|
|||
}
|
||||
|
||||
User::resetFailedLogins((int)$user['id']);
|
||||
Auth::login((int)$user['id']);
|
||||
Auth::login((int)$user['id'], (string)$user['role']);
|
||||
return null;
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -14,14 +14,14 @@ class User
|
|||
public static function all(): array
|
||||
{
|
||||
return Database::fetchAll(
|
||||
'SELECT id, username, active, created_at FROM users ORDER BY username'
|
||||
'SELECT id, username, role, active, created_at FROM users ORDER BY username'
|
||||
);
|
||||
}
|
||||
|
||||
public static function find(int $id): array|false
|
||||
{
|
||||
return Database::fetchOne(
|
||||
'SELECT id, username, active, created_at FROM users WHERE id = ?',
|
||||
'SELECT id, username, role, active, created_at FROM users WHERE id = ?',
|
||||
[$id]
|
||||
);
|
||||
}
|
||||
|
|
@ -29,17 +29,17 @@ class User
|
|||
public static function findByUsername(string $username): array|false
|
||||
{
|
||||
return Database::fetchOne(
|
||||
'SELECT id, username, password_hash, active, failed_attempts, locked_until FROM users WHERE username = ?',
|
||||
'SELECT id, username, password_hash, role, active, failed_attempts, locked_until FROM users WHERE username = ?',
|
||||
[$username]
|
||||
);
|
||||
}
|
||||
|
||||
public static function create(string $username, string $password): void
|
||||
public static function create(string $username, string $password, string $role = 'user'): void
|
||||
{
|
||||
$hash = password_hash($password, PASSWORD_BCRYPT);
|
||||
Database::execute(
|
||||
'INSERT INTO users (username, password_hash) VALUES (?, ?)',
|
||||
[$username, $hash]
|
||||
'INSERT INTO users (username, password_hash, role) VALUES (?, ?, ?)',
|
||||
[$username, $hash, $role]
|
||||
);
|
||||
}
|
||||
|
||||
|
|
@ -72,6 +72,14 @@ class User
|
|||
);
|
||||
}
|
||||
|
||||
public static function setRole(int $id, string $role): void
|
||||
{
|
||||
Database::execute(
|
||||
'UPDATE users SET role = ? WHERE id = ?',
|
||||
[$role, $id]
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Zählt einen Fehlversuch. Ab LOCK_THRESHOLD Fehlversuchen wird das Konto
|
||||
* für ein mit jedem weiteren Fehlversuch steigendes Zeitfenster gesperrt.
|
||||
|
|
|
|||
|
|
@ -80,10 +80,12 @@ function navActive(string $prefix, string $current): string {
|
|||
<div class="sidebar-section">
|
||||
<div class="sidebar-label">Verwaltung</div>
|
||||
<nav>
|
||||
<?php if (\Steuerkram\Auth::isAdmin()): ?>
|
||||
<a href="/useradmin" class="<?= navActive('/useradmin', $path) ?>">
|
||||
<svg viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2"><path d="M17 21v-2a4 4 0 0 0-4-4H5a4 4 0 0 0-4 4v2"/><circle cx="9" cy="7" r="4"/><line x1="19" y1="8" x2="19" y2="14"/><line x1="22" y1="11" x2="16" y2="11"/></svg>
|
||||
Benutzer
|
||||
</a>
|
||||
<?php endif; ?>
|
||||
<a href="/ereignisse" class="<?= navActive('/ereignisse', $path) ?>">
|
||||
<svg viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2"><path d="M12 22V12M12 12L8 16M12 12l4 4"/><circle cx="12" cy="5" r="3"/><path d="M5.5 8.5A7 7 0 0 0 5 12"/><path d="M18.5 8.5A7 7 0 0 1 19 12"/></svg>
|
||||
Ereignisse
|
||||
|
|
|
|||
50
tests/83_UserRoleTest.php
Normal file
50
tests/83_UserRoleTest.php
Normal file
|
|
@ -0,0 +1,50 @@
|
|||
<?php
|
||||
|
||||
declare(strict_types=1);
|
||||
|
||||
use Steuerkram\Auth;
|
||||
use Steuerkram\Models\User;
|
||||
|
||||
class UserRoleTest extends TestCase
|
||||
{
|
||||
protected string $testName = 'Test 83: Rollen-Feld und Auth::isAdmin() (Fix #147)';
|
||||
|
||||
public function run(): void
|
||||
{
|
||||
User::create('roletest-user', 'password123');
|
||||
$normalUser = User::findByUsername('roletest-user');
|
||||
$this->assertEquals('user', $normalUser['role'], 'Ohne Angabe wird ein Benutzer mit Rolle "user" angelegt');
|
||||
|
||||
User::create('roletest-admin', 'password123', 'admin');
|
||||
$admin = User::findByUsername('roletest-admin');
|
||||
$this->assertEquals('admin', $admin['role'], 'Explizit angelegter Benutzer erhält die Rolle "admin"');
|
||||
|
||||
User::setRole((int)$normalUser['id'], 'admin');
|
||||
$promoted = User::find((int)$normalUser['id']);
|
||||
$this->assertEquals('admin', $promoted['role'], 'setRole() befördert einen Benutzer nachträglich zum Admin');
|
||||
|
||||
$listed = User::all();
|
||||
$found = null;
|
||||
foreach ($listed as $u) {
|
||||
if ($u['id'] === $admin['id']) { $found = $u; break; }
|
||||
}
|
||||
$this->assertNotEmpty($found, 'User::all() liefert den angelegten Admin-Benutzer');
|
||||
$this->assertEquals('admin', $found['role'] ?? null, 'User::all() liefert das Rollen-Feld mit');
|
||||
|
||||
// Auth::isAdmin() liest ausschließlich aus der Session (kein DB-Zugriff),
|
||||
// damit sich die Prüfung ohne echten Request isoliert testen lässt.
|
||||
$backup = $_SESSION ?? null;
|
||||
try {
|
||||
$_SESSION = [];
|
||||
$this->assert(!Auth::isAdmin(), 'Ohne Session-Rolle gilt der Benutzer nicht als Admin');
|
||||
|
||||
$_SESSION['role'] = 'user';
|
||||
$this->assert(!Auth::isAdmin(), 'Rolle "user" in der Session ist kein Admin');
|
||||
|
||||
$_SESSION['role'] = 'admin';
|
||||
$this->assert(Auth::isAdmin(), 'Rolle "admin" in der Session wird als Admin erkannt');
|
||||
} finally {
|
||||
$_SESSION = $backup ?? [];
|
||||
}
|
||||
}
|
||||
}
|
||||
73
tests/84_BookingStoreReflectedXssTest.php
Normal file
73
tests/84_BookingStoreReflectedXssTest.php
Normal file
|
|
@ -0,0 +1,73 @@
|
|||
<?php
|
||||
|
||||
declare(strict_types=1);
|
||||
|
||||
/**
|
||||
* Test 84: Reflektiertes XSS über kategorie_kontonummer/zahlungs_kontonummer
|
||||
* bei der Buchungserfassung (BookingController::store()) ist geschlossen.
|
||||
*/
|
||||
class BookingStoreReflectedXssTest extends TestCase
|
||||
{
|
||||
protected string $testName = 'Test 84: Kein reflektiertes XSS über Kontonummer-Freitextfeld bei Buchungserfassung';
|
||||
|
||||
public function run(): void
|
||||
{
|
||||
$datum = date('Y-m-d');
|
||||
$belegNr = 'XSS-KTO-' . uniqid();
|
||||
|
||||
$belegId = \Steuerkram\Models\Beleg::create([
|
||||
'beleg_nr' => $belegNr,
|
||||
'filename' => 'xss-test.pdf',
|
||||
'storage_path' => '',
|
||||
'mime_type' => 'application/pdf',
|
||||
'file_hash' => hash('sha256', $belegNr),
|
||||
'beleg_datum' => $datum,
|
||||
'belegtyp' => 'Eingangsrechnung',
|
||||
]);
|
||||
$this->assertGreaterThan(0, $belegId, 'Beleg angelegt');
|
||||
|
||||
$payload = '<script>alert(document.cookie)</script>';
|
||||
|
||||
// Nachbau der Fehlerauflösung aus BookingController::store() (Zeile 100-105):
|
||||
// eine nicht auflösbare Freitext-Kontonummer landet unescaped in $errors.
|
||||
$errors = [
|
||||
'kategorie_konto_id' => "Kontonummer '$payload' nicht gefunden.",
|
||||
];
|
||||
|
||||
// BookingController::renderBelegDetail() nur bis zum Content-Fragment
|
||||
// nachgebaut (belege/detail.php), ohne layout/base.php – ein zweiter
|
||||
// voller Seiten-Render im selben Testlauf würde an der Funktions-
|
||||
// deklaration in layout/nav.php scheitern (siehe Test 65).
|
||||
$beleg = \Steuerkram\Models\Beleg::find($belegId);
|
||||
$accounts = \Steuerkram\Models\Account::all(['aktiv' => 1]);
|
||||
|
||||
$controller = new \Steuerkram\Controllers\BookingController();
|
||||
$renderView = new \ReflectionMethod($controller, 'renderView');
|
||||
$renderView->setAccessible(true);
|
||||
$html = $renderView->invoke($controller, 'belege/detail', [
|
||||
'beleg' => $beleg,
|
||||
'bookings' => \Steuerkram\Models\Booking::forBeleg($belegId),
|
||||
'integrity' => \Steuerkram\Models\Beleg::verifyIntegrity($belegId),
|
||||
'accounts' => $accounts,
|
||||
'assets' => \Steuerkram\Models\Asset::all(),
|
||||
'formErrors' => $errors,
|
||||
'formData' => [],
|
||||
'bankTx' => null,
|
||||
'bankTxBrutto' => 0.0,
|
||||
'linkedBankTx' => null,
|
||||
]);
|
||||
|
||||
$this->assert(
|
||||
!str_contains($html, $payload),
|
||||
'Roher <script>-Payload aus kategorie_kontonummer erscheint nicht unescaped in der Ausgabe'
|
||||
);
|
||||
$this->assert(
|
||||
!str_contains($html, '<script>alert(document.cookie)'),
|
||||
'Kein ausführbares <script>-Tag in der Ausgabe (auch nicht durch Groß-/Kleinschreibung o.ä. verschleiert)'
|
||||
);
|
||||
$this->assert(
|
||||
str_contains($html, htmlspecialchars($payload, ENT_QUOTES, 'UTF-8')),
|
||||
'Escapte Version der Kontonummer erscheint in der Fehlermeldung (Kontonummer wurde nicht gefunden)'
|
||||
);
|
||||
}
|
||||
}
|
||||
54
tests/e2e/specs/useradmin-authorization.spec.js
Normal file
54
tests/e2e/specs/useradmin-authorization.spec.js
Normal file
|
|
@ -0,0 +1,54 @@
|
|||
// @ts-check
|
||||
// Fix #147: Vor der Änderung konnte jeder eingeloggte Benutzer über
|
||||
// /useradmin beliebige Konten anlegen, Passwörter setzen oder löschen
|
||||
// (Privilege Escalation). Dieser Test prüft die Durchsetzung auf HTTP-Ebene,
|
||||
// die die PHP-Suite nicht erreicht (Router-Guard, Redirect, Nav-Rendering).
|
||||
|
||||
const { test, expect } = require('@playwright/test')
|
||||
const { uniqueTag, gotoOk, acceptDialogs } = require('../helpers/app')
|
||||
|
||||
test.describe('Autorisierung: Benutzerverwaltung nur für Admins (Fix #147)', () => {
|
||||
test('Neu angelegter Benutzer ohne Admin-Rolle wird von /useradmin ausgesperrt', async ({ page, browser }) => {
|
||||
const username = uniqueTag('noadmin').toLowerCase().replace(/[^a-z0-9-]/g, '').slice(0, 64)
|
||||
const password = 'TempPass123!'
|
||||
|
||||
await gotoOk(page, '/useradmin')
|
||||
await page.fill('#field-username', username)
|
||||
await page.fill('#field-password', password)
|
||||
await page.click('#submit-btn')
|
||||
await page.waitForURL('/useradmin')
|
||||
await expect(page.locator('body')).toContainText('Benutzer angelegt.')
|
||||
|
||||
const row = page.locator('tr', { hasText: username })
|
||||
await expect(row).toBeVisible()
|
||||
|
||||
try {
|
||||
// Eigener, unangemeldeter Kontext: Der neu angelegte Benutzer meldet
|
||||
// sich selbst an und ist per Default kein Admin.
|
||||
const context = await browser.newContext({ storageState: { cookies: [], origins: [] } })
|
||||
const userPage = await context.newPage()
|
||||
await userPage.goto('/login')
|
||||
await userPage.fill('#username', username)
|
||||
await userPage.fill('#password', password)
|
||||
await userPage.click('form[action="/login"] button[type="submit"]')
|
||||
await userPage.waitForURL(url => !url.pathname.startsWith('/login'))
|
||||
|
||||
// Kosmetischer Schutz: der Nav-Link ist für Nicht-Admins ausgeblendet.
|
||||
await expect(userPage.locator('a[href="/useradmin"]')).toHaveCount(0)
|
||||
|
||||
// Serverseitige Durchsetzung: direkter Aufruf bleibt trotzdem gesperrt.
|
||||
const response = await userPage.goto('/useradmin')
|
||||
expect(response?.status()).toBe(403)
|
||||
await expect(userPage.locator('body')).toContainText('Kein Zugriff')
|
||||
|
||||
await context.close()
|
||||
} finally {
|
||||
acceptDialogs(page)
|
||||
const delForm = row.locator('form[action$="/delete"]')
|
||||
if (await delForm.count() > 0) {
|
||||
await delForm.locator('button[type="submit"]').click()
|
||||
await page.waitForLoadState('networkidle')
|
||||
}
|
||||
}
|
||||
})
|
||||
})
|
||||
Loading…
Add table
Reference in a new issue