fa/breadcrumb-the-shire
1
0
Fork 1
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

fix(helpdesk): repair security-level persistence, enrichment, and list UX

Browse Source 
Saving security levels appeared broken in both the domain detail view and
the domains grid. Root causes:

- Repository never unwrapped MintyPHP DB rows (nested by table name), so
  reads always returned defaults even though writes succeeded.
- Batch queries bound tenant_id last while SQL expected it first, so
  list/detail enrichment for the grid matched nothing.
- Detail page AJAX submit looked up CSRF via a non-existent data attribute
  and dropped the note field entirely.
- Endpoint used a hand-rolled CSRF check instead of Session::checkCsrfToken().

Fixes:
- DomainSecurityLevelRepository: use RepositoryArrayHelper::unwrap(List),
  swap param order in listLevelsByDomainNos / listDetailsByDomainNos,
  centralize table name in a const.
- security-level-data endpoint: canonical Session::checkCsrfToken(),
  unified JSON/PRG error path with proper HTTP status codes and flash.
- Detail page: drop broken AJAX hijack, rely on native form POST + PRG
  (GR-CORE-012).
- Grid list: refetch via gridRef.grid.forceRender() after dialog save
  instead of DOM-only mutation, so Grid.js internal state stays in sync.
- Add dedicated Note column next to the badge with ellipsis + title
  tooltip.
- Replace console.* in the dialog with showAsyncFlash for user-visible
  errors; add i18n keys (de/en) for all error messages.
- Drop unused postAction import and TENANT_ID_OTHER test constant.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Felix Schneider
2026-04-21 14:04:21 +02:00
parent 6ac685084d
commit fb86c02427
9 changed files with 151 additions and 161 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
modules/helpdesk/i18n/default_de.json
View File

@@ -545,5 +545,11 @@
"Low": "Niedrig",
"Normal": "Normal",
"High": "Hoch",
"Critical": "Kritisch"
"Critical": "Kritisch",
"Save failed": "Speichern fehlgeschlagen",
"Invalid CSRF token": "Ungültiges CSRF-Token",
"No tenant context": "Kein Mandantenkontext",
"Missing domain number": "Domain-Nummer fehlt",
"Invalid security level": "Ungültige Sicherheitsstufe",
"Failed to save security level": "Sicherheitsstufe konnte nicht gespeichert werden"
}

8
modules/helpdesk/i18n/default_en.json
View File

@@ -545,5 +545,11 @@
"Low": "Low",
"Normal": "Normal",
"High": "High",
"Critical": "Critical"
"Critical": "Critical",
"Save failed": "Save failed",
"Invalid CSRF token": "Invalid CSRF token",
"No tenant context": "No tenant context",
"Missing domain number": "Missing domain number",
"Invalid security level": "Invalid security level",
"Failed to save security level": "Failed to save security level"
}

60
modules/helpdesk/lib/Module/Helpdesk/Repository/DomainSecurityLevelRepository.php
View File

@@ -3,9 +3,11 @@
namespace MintyPHP\Module\Helpdesk\Repository;
use MintyPHP\DB;
use MintyPHP\Repository\Support\RepositoryArrayHelper;
class DomainSecurityLevelRepository
{
private const TABLE = 'helpdesk_domain_security_levels';
private const ALLOWED_LEVELS = ['niedrig', 'normal', 'hoch', 'kritisch'];
public function findByTenantAndDomainNo(int $tenantId, string $domainNo): ?array
@@ -15,18 +17,16 @@ class DomainSecurityLevelRepository
}
$row = DB::selectOne(
'SELECT * FROM helpdesk_domain_security_levels WHERE tenant_id = ? AND domain_no = ? LIMIT 1',
'SELECT id, tenant_id, domain_no, security_level, note, created_by, updated_by, created_at, updated_at FROM ' . self::TABLE . ' WHERE tenant_id = ? AND domain_no = ? LIMIT 1',
(string) $tenantId,
$domainNo
);
if (!is_array($row) || $row === []) {
return null;
}
return $row;
return RepositoryArrayHelper::unwrap(is_array($row) ? $row : null, self::TABLE);
}
/**
* @api Called from DomainSecurityLevelService
* @param list<string> $domainNos
* @return array<string, string> Map of domain_no => security_level
*/
@@ -37,22 +37,19 @@ class DomainSecurityLevelRepository
}
$placeholders = implode(',', array_fill(0, count($domainNos), '?'));
$params = array_map('strval', $domainNos);
$params[] = (string) $tenantId;
$params = [(string) $tenantId, ...array_map('strval', $domainNos)];
$rows = DB::select(
'SELECT domain_no, security_level FROM helpdesk_domain_security_levels WHERE tenant_id = ? AND domain_no IN (' . $placeholders . ')',
'SELECT domain_no, security_level FROM ' . self::TABLE . ' WHERE tenant_id = ? AND domain_no IN (' . $placeholders . ')',
...$params
);
$result = [];
if (is_array($rows)) {
foreach ($rows as $row) {
$domainNo = (string) ($row['domain_no'] ?? '');
$level = (string) ($row['security_level'] ?? 'normal');
if ($domainNo !== '') {
$result[$domainNo] = in_array($level, self::ALLOWED_LEVELS, true) ? $level : 'normal';
}
foreach (RepositoryArrayHelper::unwrapList($rows, self::TABLE) as $row) {
$domainNo = (string) ($row['domain_no'] ?? '');
$level = (string) ($row['security_level'] ?? 'normal');
if ($domainNo !== '') {
$result[$domainNo] = in_array($level, self::ALLOWED_LEVELS, true) ? $level : 'normal';
}
}
@@ -60,6 +57,7 @@ class DomainSecurityLevelRepository
}
/**
* @api Called from DomainSecurityLevelService
* @param list<string> $domainNos
* @return array<string, array{level: string, note: string}> Map of domain_no => {level, note}
*/
@@ -70,26 +68,23 @@ class DomainSecurityLevelRepository
}
$placeholders = implode(',', array_fill(0, count($domainNos), '?'));
$params = array_map('strval', $domainNos);
$params[] = (string) $tenantId;
$params = [(string) $tenantId, ...array_map('strval', $domainNos)];
$rows = DB::select(
'SELECT domain_no, security_level, note FROM helpdesk_domain_security_levels WHERE tenant_id = ? AND domain_no IN (' . $placeholders . ')',
'SELECT domain_no, security_level, note FROM ' . self::TABLE . ' WHERE tenant_id = ? AND domain_no IN (' . $placeholders . ')',
...$params
);
$result = [];
if (is_array($rows)) {
foreach ($rows as $row) {
$domainNo = (string) ($row['domain_no'] ?? '');
$level = (string) ($row['security_level'] ?? 'normal');
$note = (string) ($row['note'] ?? '');
if ($domainNo !== '') {
$result[$domainNo] = [
'level' => in_array($level, self::ALLOWED_LEVELS, true) ? $level : 'normal',
'note' => $note,
];
}
foreach (RepositoryArrayHelper::unwrapList($rows, self::TABLE) as $row) {
$domainNo = (string) ($row['domain_no'] ?? '');
$level = (string) ($row['security_level'] ?? 'normal');
$note = (string) ($row['note'] ?? '');
if ($domainNo !== '') {
$result[$domainNo] = [
'level' => in_array($level, self::ALLOWED_LEVELS, true) ? $level : 'normal',
'note' => $note,
];
}
}
@@ -99,6 +94,7 @@ class DomainSecurityLevelRepository
/**
* Insert or update security level for a domain.
*
* @api Called from DomainSecurityLevelService
* @return bool True on success
*/
public function upsert(int $tenantId, string $domainNo, string $level, ?int $userId, ?string $note = null): bool
@@ -111,7 +107,7 @@ class DomainSecurityLevelRepository
if ($existing !== null) {
$result = DB::update(
'UPDATE helpdesk_domain_security_levels SET security_level = ?, note = ?, updated_by = ? WHERE tenant_id = ? AND domain_no = ?',
'UPDATE ' . self::TABLE . ' SET security_level = ?, note = ?, updated_by = ? WHERE tenant_id = ? AND domain_no = ?',
$level,
$note,
$userId !== null ? (string) $userId : null,
@@ -123,7 +119,7 @@ class DomainSecurityLevelRepository
}
$result = DB::insert(
'INSERT INTO helpdesk_domain_security_levels (tenant_id, domain_no, security_level, note, created_by, updated_by) VALUES (?, ?, ?, ?, ?, ?)',
'INSERT INTO ' . self::TABLE . ' (tenant_id, domain_no, security_level, note, created_by, updated_by) VALUES (?, ?, ?, ?, ?, ?)',
(string) $tenantId,
$domainNo,
$level,
@@ -145,7 +141,7 @@ class DomainSecurityLevelRepository
}
$result = DB::update(
'DELETE FROM helpdesk_domain_security_levels WHERE tenant_id = ? AND domain_no = ?',
'DELETE FROM ' . self::TABLE . ' WHERE tenant_id = ? AND domain_no = ?',
(string) $tenantId,
$domainNo
);

2
modules/helpdesk/pages/helpdesk/domains/index(default).phtml
View File

@@ -84,6 +84,8 @@ require templatePath('partials/app-list-filters.phtml');
'administration' => t('Administration'),
'contractType' => t('Contract type'),
'securityLevel' => t('Security level'),
'note' => t('Note'),
'saveError' => t('Save failed'),
],
]); ?></script>
<script type="module" src="<?php e(assetVersion('modules/helpdesk/js/pages/helpdesk-domains-index.js')); ?>"></script>

129
modules/helpdesk/pages/helpdesk/domains/security-level-data().php
View File

@@ -15,85 +15,76 @@ $request = requestInput();
$session = app(SessionStoreInterface::class)->all();
$tenantId = (int) ($session['current_tenant']['id'] ?? 0);
$userId = (int) ($session['user']['id'] ?? 0);
$service = app(DomainSecurityLevelService::class);
$isPost = $request->isMethod('POST');
$wantsJson = $request->wantsJson();
$domainNo = trim((string) ($isPost
? $request->body('domain_no', '')
: $request->query('domain_no', '')));
$redirectUrl = $domainNo !== ''
? 'helpdesk/domain/' . rawurlencode($domainNo)
: 'helpdesk/domains';
$fail = static function (string $error, int $status = 400) use ($wantsJson, $redirectUrl): void {
if ($wantsJson) {
http_response_code($status);
Router::json(['ok' => false, 'error' => $error]);
return;
}
Flash::error(t($error), 'helpdesk-domains', 'security_level_error');
Router::redirect($redirectUrl);
};
if ($tenantId <= 0) {
Router::json(['ok' => false, 'error' => 'No tenant context']);
$fail('No tenant context', 400);
return;
}
$domainNo = trim((string) ($request->body('domain_no', '') ?: $request->query('domain_no', '')));
if ($domainNo === '') {
Router::json(['ok' => false, 'error' => 'Missing domain number']);
$fail('Missing domain number', 400);
return;
}
$service = app(DomainSecurityLevelService::class);
if ($request->method() === 'POST') {
// CSRF check: Compare token from request body with session token
$csrfKey = Session::$csrfSessionKey;
$csrfSession = (string) ($session[$csrfKey] ?? '');
$csrfRequest = (string) $request->body($csrfKey, '');
if ($csrfRequest === '' || $csrfRequest !== $csrfSession) {
// For AJAX requests, return JSON error
if ($request->wantsJson()) {
Router::json(['ok' => false, 'error' => 'Invalid CSRF token']);
return;
}
// For full-page POST, redirect with flash error
Flash::error(t('Invalid CSRF token'), 'helpdesk-domains', 'csrf');
Router::redirect('helpdesk/domain/' . rawurlencode($domainNo));
return;
}
$level = trim((string) $request->body('security_level', ''));
if ($level === '') {
Router::json(['ok' => false, 'error' => 'Missing security level']);
return;
}
$allowed = DomainSecurityLevelService::getAllowedLevels();
if (!in_array($level, $allowed, true)) {
Router::json(['ok' => false, 'error' => 'Invalid security level']);
return;
}
$note = trim((string) $request->body('note', ''));
$success = $service->setLevel($tenantId, $domainNo, $level, $userId, $note !== '' ? $note : null);
if (!$success) {
Router::json(['ok' => false, 'error' => 'Failed to save security level']);
return;
}
// For AJAX requests, return JSON
if ($request->wantsJson()) {
Router::json([
'ok' => true,
'level' => $level,
'badge_variant' => DomainSecurityLevelService::getBadgeVariant($level),
'note' => $note,
]);
return;
}
// For full-page POST, redirect with flash success
Flash::success(t('Security level updated'), 'helpdesk-domains', 'security_level_saved');
Router::redirect('helpdesk/domain/' . rawurlencode($domainNo));
if (!$isPost) {
$details = $service->getDetails($tenantId, $domainNo);
Router::json([
'ok' => true,
'level' => $details['level'],
'badge_variant' => DomainSecurityLevelService::getBadgeVariant($details['level']),
'note' => $details['note'],
]);
return;
}
$details = $service->getDetails($tenantId, $domainNo);
if (!Session::checkCsrfToken()) {
$fail('Invalid CSRF token', 403);
return;
}
Router::json([
'ok' => true,
'level' => $details['level'],
'badge_variant' => DomainSecurityLevelService::getBadgeVariant($details['level']),
'note' => $details['note'],
]);
$level = trim((string) $request->body('security_level', ''));
if (!in_array($level, DomainSecurityLevelService::getAllowedLevels(), true)) {
$fail('Invalid security level', 422);
return;
}
$note = trim((string) $request->body('note', ''));
if (!$service->setLevel($tenantId, $domainNo, $level, $userId > 0 ? $userId : null, $note !== '' ? $note : null)) {
$fail('Failed to save security level', 500);
return;
}
if ($wantsJson) {
Router::json([
'ok' => true,
'level' => $level,
'badge_variant' => DomainSecurityLevelService::getBadgeVariant($level),
'note' => $note,
]);
return;
}
Flash::success(t('Security level updated'), 'helpdesk-domains', 'security_level_saved');
Router::redirect($redirectUrl);

1
modules/helpdesk/tests/Module/Helpdesk/Service/DomainSecurityLevelServiceTest.php
View File

@@ -9,7 +9,6 @@ use PHPUnit\Framework\TestCase;
class DomainSecurityLevelServiceTest extends TestCase
{
private const TENANT_ID = 1;
private const TENANT_ID_OTHER = 2;
public function testGetLevelReturnsDefaultForUnknownDomain(): void
{

10
modules/helpdesk/web/css/helpdesk.css
View File

@@ -3123,4 +3123,14 @@
outline: 2px solid var(--app-primary, #0d6efd);
outline-offset: 1px;
}
td.gridjs-td .app-cell-note {
display: inline-block;
max-width: 24ch;
overflow: hidden;
text-overflow: ellipsis;
white-space: nowrap;
vertical-align: middle;
color: var(--app-muted, #6c757d);
}
}

34
modules/helpdesk/web/js/helpdesk-domain-detail.js
View File

@@ -8,7 +8,7 @@
* into innerHTML. This matches the pattern used in helpdesk-detail.js
* and other helpdesk JS modules in this codebase.
*/
import { getJson, postForm } from '/js/core/app-http.js';
import { getJson } from '/js/core/app-http.js';
import { resolveHost } from '/js/core/app-dom.js';
import { renderEmptyState } from './helpdesk-empty-state.js';
@@ -54,8 +54,6 @@ function init(container) {
if (!domainNo || !detailUrl) return;
bindSecurityLevelForm();
const i18nEl = document.getElementById('helpdesk-domain-i18n');
const i18n = i18nEl ? JSON.parse(i18nEl.textContent) : {};
const t = (key) => i18n[key] || key;
@@ -179,36 +177,6 @@ function init(container) {
}
}
function bindSecurityLevelForm() {
const form = document.querySelector('[data-security-level-form]');
if (!form) return;
form.addEventListener('submit', async (e) => {
e.preventDefault();
const select = form.querySelector('[data-security-level-select]');
const domainNo = form.querySelector('input[name="domain_no"]')?.value || '';
const level = select?.value || '';
if (!domainNo || !level) return;
const csrfKey = document.querySelector('[data-csrf-key]')?.dataset.csrfKey || '_csrf';
const csrfToken = document.querySelector('[data-csrf-key]')?.dataset.csrfToken || '';
const body = new URLSearchParams({
domain_no: domainNo,
security_level: level,
[csrfKey]: csrfToken,
});
try {
const resp = await postForm('/helpdesk/domains/security-level-data', body);
if (resp?.ok) {
renderSecurityLevel(resp.level, resp.badge_variant);
}
} catch {
// silent fail — user can retry
}
});
}
function renderHandoversList(handovers) {
const el = document.getElementById('domain-handovers-list');
const countEl = document.getElementById('domain-handover-count');

60
modules/helpdesk/web/js/pages/helpdesk-domains-index.js
View File

@@ -1,6 +1,7 @@
import { createListPageModule } from '/js/core/app-list-page-module.js';
import { escapeHtml, withCurrentListReturn, postAction } from '/js/pages/app-list-utils.js';
import { escapeHtml, withCurrentListReturn } from '/js/pages/app-list-utils.js';
import { postForm } from '/js/core/app-http.js';
import { showAsyncFlash } from '/js/components/app-async-flash.js';
const LEVEL_LABELS = {
niedrig: 'Low',
@@ -26,7 +27,7 @@ function resolveDialog() {
};
}
function openSecurityDialog(badge, securityLevelUrl, labels) {
function openSecurityDialog(badge, securityLevelUrl, labels, onSaved) {
const els = resolveDialog();
if (!els) {
return;
@@ -78,10 +79,10 @@ function openSecurityDialog(badge, securityLevelUrl, labels) {
const csrfKey = els.csrfInput?.name || 'csrf_token';
const csrfToken = els.csrfInput?.value || '';
const errorLabel = labels?.saveError || 'Save failed';
if (!csrfToken) {
console.error('[security-dialog] No CSRF token available');
els.saveButton.disabled = false;
els.saveButton.removeAttribute('aria-busy');
showAsyncFlash('error', errorLabel);
return;
}
@@ -98,24 +99,16 @@ function openSecurityDialog(badge, securityLevelUrl, labels) {
try {
const resp = await postForm(securityLevelUrl, body);
if (resp?.ok) {
const levelLabel = labels?.[resp.level] ?? LEVEL_LABELS[resp.level] ?? resp.level;
const textNode = Array.from(badge.childNodes).find(
(n) => n.nodeType === Node.TEXT_NODE && n.textContent.trim() !== ''
);
if (textNode) {
textNode.textContent = levelLabel;
}
badge.dataset.variant = resp.badge_variant ?? 'neutral';
badge.dataset.level = resp.level ?? newLevel;
badge.dataset.note = resp.note ?? note;
closeDialog();
} else {
console.warn('[security-dialog] Save returned ok=false:', resp);
els.saveButton.disabled = false;
els.saveButton.removeAttribute('aria-busy');
if (typeof onSaved === 'function') {
onSaved();
}
return;
}
showAsyncFlash('error', resp?.error || errorLabel);
} catch (error) {
console.error('[security-dialog] Save failed:', error);
showAsyncFlash('error', error?.message || errorLabel);
} finally {
els.saveButton.disabled = false;
els.saveButton.removeAttribute('aria-busy');
}
@@ -178,9 +171,13 @@ createListPageModule({
const labels = config.labels || {};
const domainBaseUrl = config.domainBaseUrl || '';
const securityLevelUrl = (config.securityLevelDataUrl || '').trim();
const domainUrlIndex = 7;
const domainUrlIndex = 8;
const gridContainer = document.querySelector('#helpdesk-domains-grid');
const gridRef = { grid: null };
const refreshGrid = () => {
gridRef.grid?.forceRender();
};
gridContainer.addEventListener('click', (event) => {
const badge = event.target.closest('td .badge[data-domain-no][role="button"]');
@@ -189,7 +186,7 @@ createListPageModule({
}
event.preventDefault();
event.stopPropagation();
openSecurityDialog(badge, securityLevelUrl, labels);
openSecurityDialog(badge, securityLevelUrl, labels, refreshGrid);
});
gridContainer.addEventListener('keydown', (event) => {
@@ -202,7 +199,7 @@ createListPageModule({
}
event.preventDefault();
event.stopPropagation();
openSecurityDialog(badge, securityLevelUrl, labels);
openSecurityDialog(badge, securityLevelUrl, labels, refreshGrid);
});
const { gridConfig } = initListPage({
@@ -264,10 +261,22 @@ createListPageModule({
return gridjs.html(`<span class="badge" data-variant="${escapeHtml(variant)}" data-domain-no="${escapeHtml(domainNo)}" data-level="${escapeHtml(level)}" data-domain-name="${escapeHtml(domainName)}" role="button" tabindex="0"${noteAttr}>${escapeHtml(levelLabel)}</span>`);
},
},
{
name: labels.note || 'Note',
sort: true,
formatter: (cell) => {
const note = String(cell || '').trim();
if (note === '') {
return gridjs.html('<span class="text-muted">—</span>');
}
const escaped = escapeHtml(note);
return gridjs.html(`<span class="app-cell-note" title="${escaped}">${escaped}</span>`);
},
},
{ name: 'debitor_url', hidden: true },
{ name: 'domain_detail_url', hidden: true },
],
sortColumns: ['No', 'Customer_Name', 'URL', 'contract_type', 'State', 'Administration', 'security_level', null, null],
sortColumns: ['No', 'Customer_Name', 'URL', 'contract_type', 'State', 'Administration', 'security_level', 'security_level_note', null, null],
paginationLimit: 10,
language: config.gridLang || {},
mapData: (data) => (data.data || []).map((row) => {
@@ -282,6 +291,7 @@ createListPageModule({
{ state: row.State || '', variant: row.state_variant || 'neutral' },
row.Administration || '',
{ level: row.security_level || 'normal', variant: row.security_level_variant || 'neutral', domainNo: row.No || '', note: row.security_level_note || '', domainName: row.Customer_Name || row.No || '' },
row.security_level_note || '',
row.debitor_url || '',
domainDetailUrl,
];
@@ -302,6 +312,8 @@ createListPageModule({
},
}) || {};
gridRef.grid = gridConfig?.grid || null;
return gridConfig;
},
});