Fix #200: Admin-Prüfung für alle Settings-Verwaltungsendpoints ergänzt
This commit is contained in:
fds2610-bot
parent
aa5ab7c06d
commit
9c6bb597d2
3 changed files with 114 additions and 0 deletions
|
|
@ -2,6 +2,11 @@
|
|||
|
||||
---
|
||||
|
||||
## 2026-08-24 – Fehlende Admin-Prüfung bei mehreren SettingsController-Endpoints (#200)
|
||||
|
||||
- **`src/Controllers/SettingsController.php`:** `deleteLogo()`, `llmTest()`, `bankImport()`, `createBankImportFormat()`, `updateBankImportField()`, `deleteBankImportFormat()`, `storeSuggestion()` und `dismissSuggestion()` prüften bislang nur den generellen Login (`Auth::require()` im Router-Gate), nicht die Rolle `admin` – anders als `update()`/`mailTest()`, deren Code-Kommentar diesen Bereich bewusst als admin-pflichtig begründet. Ein eingeloggter Benutzer mit Rolle `user` konnte damit das Firmenlogo löschen, CSV-Bankimport-Formate anlegen/ändern/löschen (Integritätsrisiko für importierte Buchungen) und den LLM-Verbindungstest auslösen. Alle acht Methoden rufen jetzt `Auth::requireAdmin()` konsistent zu Beginn auf.
|
||||
- **`tests/e2e/specs/settings-admin-authorization.spec.js`** (neu): prüft auf HTTP-Ebene, dass ein Nicht-Admin-Account von den o.g. Endpoints mit 403 abgewiesen wird, während ein Admin weiterhin Zugriff hat.
|
||||
|
||||
## 2026-08-21 – scripts/ nach Ausführungsort aufgeräumt: Host / Container / intern (#196)
|
||||
|
||||
- **`scripts/`:** Bislang lagen 16 Skripte mit vier verschiedenen Aufrufkonventionen flach nebeneinander – von außen war nicht erkennbar, was direkt vom Host aufrufbar ist, was `docker compose exec` braucht und was nie manuell laufen darf. Jetzt gibt es genau ein Ordnungskriterium: `scripts/` (Host, ausnahmslos direkt aufrufbar), `scripts/container/` (`docker compose exec web ...`) und `scripts/intern/` (Entrypoint/Container-Start, nie manuell).
|
||||
|
|
|
|||
|
|
@ -192,6 +192,8 @@ class SettingsController extends BaseController
|
|||
|
||||
public function deleteLogo(array $params): void
|
||||
{
|
||||
Auth::requireAdmin();
|
||||
|
||||
Setting::deleteLogo();
|
||||
View::redirect('/settings', 'Logo gelöscht.');
|
||||
}
|
||||
|
|
@ -204,6 +206,8 @@ class SettingsController extends BaseController
|
|||
|
||||
public function llmTest(array $params): void
|
||||
{
|
||||
Auth::requireAdmin();
|
||||
|
||||
header('Content-Type: application/json');
|
||||
$result = (new \Steuerkram\Services\LlmService())->testConnection();
|
||||
echo json_encode($result);
|
||||
|
|
@ -211,6 +215,8 @@ class SettingsController extends BaseController
|
|||
|
||||
public function bankImport(array $params): void
|
||||
{
|
||||
Auth::requireAdmin();
|
||||
|
||||
$formats = BankSource::all('csv');
|
||||
$suggestion = $_SESSION['bank_import_suggestion'] ?? null;
|
||||
|
||||
|
|
@ -226,6 +232,8 @@ class SettingsController extends BaseController
|
|||
|
||||
public function deleteBankImportFormat(array $params): void
|
||||
{
|
||||
Auth::requireAdmin();
|
||||
|
||||
header('Content-Type: application/json');
|
||||
|
||||
$id = (int)($params['id'] ?? 0);
|
||||
|
|
@ -242,6 +250,8 @@ class SettingsController extends BaseController
|
|||
|
||||
public function storeSuggestion(array $params): void
|
||||
{
|
||||
Auth::requireAdmin();
|
||||
|
||||
header('Content-Type: application/json');
|
||||
$data = json_decode(file_get_contents('php://input'), true);
|
||||
if (is_array($data)) {
|
||||
|
|
@ -255,6 +265,8 @@ class SettingsController extends BaseController
|
|||
|
||||
public function dismissSuggestion(array $params): void
|
||||
{
|
||||
Auth::requireAdmin();
|
||||
|
||||
header('Content-Type: application/json');
|
||||
unset($_SESSION['bank_import_suggestion']);
|
||||
echo json_encode(['ok' => true]);
|
||||
|
|
@ -262,6 +274,8 @@ class SettingsController extends BaseController
|
|||
|
||||
public function createBankImportFormat(array $params): void
|
||||
{
|
||||
Auth::requireAdmin();
|
||||
|
||||
$dateFormats = ['d.m.Y', 'd.m.y', 'Y-m-d', 'd/m/Y', 'm/d/Y', 'Y.m.d', 'd-m-Y'];
|
||||
|
||||
$name = trim($_POST['name'] ?? '');
|
||||
|
|
@ -366,6 +380,8 @@ class SettingsController extends BaseController
|
|||
|
||||
public function updateBankImportField(array $params): void
|
||||
{
|
||||
Auth::requireAdmin();
|
||||
|
||||
header('Content-Type: application/json');
|
||||
|
||||
$id = (int)($params['id'] ?? 0);
|
||||
|
|
|
|||
93
tests/e2e/specs/settings-admin-authorization.spec.js
Normal file
93
tests/e2e/specs/settings-admin-authorization.spec.js
Normal file
|
|
@ -0,0 +1,93 @@
|
|||
// @ts-check
|
||||
// Fix #200: Mehrere SettingsController-Endpoints (Logo, LLM-Test,
|
||||
// CSV-Bankimport-Formate) prüften Auth::requireAdmin() nicht, obwohl der
|
||||
// Bereich laut Code-Kommentar bei update() bewusst admin-pflichtig sein
|
||||
// soll. Ein normaler "user"-Account konnte damit Firmenlogo löschen,
|
||||
// Bankimport-Formate anlegen/ändern/löschen und den LLM-Test auslösen.
|
||||
// Dieser Test prüft die Durchsetzung auf HTTP-Ebene, die die PHP-Suite
|
||||
// nicht erreicht (Router-Guard, Auth::requireAdmin()).
|
||||
|
||||
const { test, expect } = require('@playwright/test')
|
||||
const { uniqueTag } = require('../helpers/app')
|
||||
|
||||
test.describe('Autorisierung: Settings-Verwaltungsendpoints nur für Admins (Fix #200)', () => {
|
||||
test('Nicht-Admin wird von Logo-, Bankimport- und LLM-Test-Endpoints ausgesperrt', async ({ page, browser }) => {
|
||||
const username = uniqueTag('noadmin').toLowerCase().replace(/[^a-z0-9-]/g, '').slice(0, 64)
|
||||
const password = 'TempPass123!'
|
||||
|
||||
await page.goto('/useradmin')
|
||||
await page.fill('#field-username', username)
|
||||
await page.fill('#field-password', password)
|
||||
await page.click('#submit-btn')
|
||||
await page.waitForURL('/useradmin')
|
||||
|
||||
const row = page.locator('tr', { hasText: username })
|
||||
await expect(row).toBeVisible()
|
||||
|
||||
try {
|
||||
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'))
|
||||
|
||||
await userPage.goto('/settings')
|
||||
const csrfToken = await userPage.evaluate(() => document.querySelector('meta[name="csrf-token"]')?.content || '')
|
||||
expect(csrfToken).not.toBe('')
|
||||
|
||||
const headers = { 'X-CSRF-Token': csrfToken }
|
||||
|
||||
const bankImportGet = await userPage.request.get('/settings/bankimport')
|
||||
expect(bankImportGet.status()).toBe(403)
|
||||
|
||||
const deleteLogo = await userPage.request.post('/settings/logo/delete', { headers, form: {} })
|
||||
expect(deleteLogo.status()).toBe(403)
|
||||
|
||||
const llmTest = await userPage.request.post('/settings/llm-test', { headers, form: {} })
|
||||
expect(llmTest.status()).toBe(403)
|
||||
|
||||
const createFormat = await userPage.request.post('/settings/bankimport/create', {
|
||||
headers,
|
||||
form: { name: 'X', kuerzel: 'X' },
|
||||
})
|
||||
expect(createFormat.status()).toBe(403)
|
||||
|
||||
const suggest = await userPage.request.post('/settings/bankimport/suggest', {
|
||||
headers,
|
||||
data: {},
|
||||
})
|
||||
expect(suggest.status()).toBe(403)
|
||||
|
||||
const dismiss = await userPage.request.post('/settings/bankimport/dismiss-suggestion', { headers, form: {} })
|
||||
expect(dismiss.status()).toBe(403)
|
||||
|
||||
const updateField = await userPage.request.post('/settings/bankimport/1/field', {
|
||||
headers,
|
||||
form: { field: 'name', value: 'X' },
|
||||
})
|
||||
expect(updateField.status()).toBe(403)
|
||||
|
||||
const deleteFormat = await userPage.request.post('/settings/bankimport/1/delete', { headers, form: {} })
|
||||
expect(deleteFormat.status()).toBe(403)
|
||||
|
||||
for (const response of [bankImportGet, deleteLogo, llmTest, createFormat, suggest, dismiss, updateField, deleteFormat]) {
|
||||
await expect(response.text()).resolves.toContain('Kein Zugriff')
|
||||
}
|
||||
|
||||
await context.close()
|
||||
} finally {
|
||||
const delForm = row.locator('form[action$="/delete"]')
|
||||
if (await delForm.count() > 0) {
|
||||
await delForm.locator('button[type="submit"]').click()
|
||||
await page.waitForLoadState('networkidle')
|
||||
}
|
||||
}
|
||||
})
|
||||
|
||||
test('Admin darf /settings/bankimport weiterhin aufrufen (kein 403)', async ({ page }) => {
|
||||
const response = await page.goto('/settings/bankimport')
|
||||
expect(response.status()).not.toBe(403)
|
||||
})
|
||||
})
|
||||
Loading…
Add table
Reference in a new issue