Fix round: point at the real tab, hide the button nobody can press, prove the quiet page load
Fixes four Important findings from the review of Task 12.
Corrected nine check `tab` values at the source (BillingChecks was already
correct; OnboardingChecks/ProvisioningChecks/DeliveryChecks all carried the
pre-redesign 'integrations' value, which is not a member of
Integrations::TABS): four onboarding checks now point at 'platform' or 'env'
depending on where their field actually is, five provisioning/delivery
checks point at 'services'. checkUrl()'s match-block resolver — praised as
correct for the six checks that point at a genuinely different admin page —
is unchanged; its `default` arm is now a pure safety net, not a route any
check actually relies on.
Wrapped the "Prüfen" button in @can('secrets.manage'): mount() admits
hosts.manage OR secrets.manage, but runCheck() requires secrets.manage alone,
so an Admin-role operator could see a button that 403s on press.
Fixed a docblock on HEARTBEAT_KEYS that asserted a staleness threshold was
kept in sync with OperationChecks::STALE_AFTER_MINUTES — that constant is a
key/settings-name map with no threshold of its own, and no such
synchronisation exists.
Three new tests, each confirmed red against a deliberately reintroduced
version of the bug it covers before being confirmed green: every check's tab
value against where its field actually lives, the run-check button hidden
from an operator who cannot press it, and Http::assertNothingSent() after a
plain page load.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
feature/betriebsmodus
nexxo
parent
e83b17ec37
commit
49d528dbea
|
|
@ -54,12 +54,19 @@ class Readiness extends Component
|
|||
];
|
||||
|
||||
/**
|
||||
* A heartbeat older than this is not shown as fresh even where the check
|
||||
* itself already says so — a display concern only. Kept in sync with
|
||||
* App\Support\Readiness\OperationChecks::STALE_AFTER_MINUTES on purpose
|
||||
* (see the docblock there for why five minutes), not re-derived from it:
|
||||
* that constant is private, and duplicating the reasoning in a comment
|
||||
* here is cheaper than making it public for one reader.
|
||||
* Check::key → the App\Support\Settings row it is written under.
|
||||
*
|
||||
* Display only, and carries no threshold of its own: whether a heartbeat
|
||||
* counts as fresh is decided exactly once, by
|
||||
* App\Support\Readiness\OperationChecks::isFresh(), and shows up here
|
||||
* only as the check's own `satisfied`/severity — this map exists solely
|
||||
* so heartbeatDisplay() knows which stored value to format for which
|
||||
* check. (A previous version of this comment claimed a staleness
|
||||
* threshold was kept in sync with OperationChecks::STALE_AFTER_MINUTES —
|
||||
* there is no such value here to keep in sync. CLAUDE.md's R19 names this
|
||||
* exact shape of mistake — a call that reads as an assurance and is not
|
||||
* one — as worse than no comment at all, because it stops the next
|
||||
* reader from checking for themselves.)
|
||||
*/
|
||||
private const HEARTBEAT_KEYS = [
|
||||
'operation.scheduler' => 'heartbeat.scheduler',
|
||||
|
|
@ -100,23 +107,21 @@ class Readiness extends Component
|
|||
/**
|
||||
* Where an operator goes to actually fix ONE entry.
|
||||
*
|
||||
* Not always Admin\Integrations, and not always $check->tab handed
|
||||
* straight through as its ?tab=: several of the five check groups
|
||||
* predate the Integrations::TABS redesign and still carry a tab value
|
||||
* from before it — 'hosts', 'plans', 'datacenters', 'mail', 'templates'
|
||||
* and 'company' each name a DIFFERENT admin page entirely, and the
|
||||
* leftover value 'integrations' is the exact bug Task 10's fix round
|
||||
* already found and corrected for OperationChecks (it does not exist in
|
||||
* Integrations::TABS, so a link built from it lands nowhere in
|
||||
* particular — Integrations::mount() silently falls back to its own
|
||||
* first tab). Rather than repeat that history by forwarding $check->tab
|
||||
* unchecked, a value only ever becomes a ?tab= when it actually IS one
|
||||
* of Integrations::TABS; every other known value gets its own real
|
||||
* route, and anything unrecognised — including the eight checks in
|
||||
* Onboarding/Provisioning/Delivery still carrying the pre-redesign
|
||||
* 'integrations' value (Task 8's own deferred note; out of scope for
|
||||
* this task to rename) — falls back to the Integrations page's own
|
||||
* default tab rather than a dead link.
|
||||
* Not always Admin\Integrations: six of the nine `tab` values used across
|
||||
* the five check groups name a DIFFERENT admin page entirely — 'hosts',
|
||||
* 'plans', 'datacenters', 'mail', 'templates' and 'company' are pages of
|
||||
* their own, not Integrations tabs, and forwarding them as `?tab=` would
|
||||
* either 404 or (since Integrations::mount() falls back silently to its
|
||||
* own first tab for an unrecognised value) land quietly on the wrong
|
||||
* section. Every OTHER `tab` value that exists in the five check group
|
||||
* files today (checked at the source, Fix-Runde) really IS one of
|
||||
* Integrations::TABS — nine of them were not, until that same Fix-Runde
|
||||
* corrected them there rather than papering over it here (see
|
||||
* BillingChecks/OnboardingChecks/ProvisioningChecks/DeliveryChecks). The
|
||||
* `default` arm below is therefore a pure safety net for a value nobody
|
||||
* currently produces, not a route any check relies on — kept so a future
|
||||
* typo degrades to the Integrations landing tab instead of an
|
||||
* UnhandledMatchError taking the whole page down.
|
||||
*/
|
||||
public function checkUrl(Check $check): string
|
||||
{
|
||||
|
|
|
|||
|
|
@ -61,7 +61,10 @@ final class DeliveryChecks
|
|||
severity: Check::SEVERITY_WARNING,
|
||||
label: __('readiness.delivery.inbound_password'),
|
||||
breaks: __('readiness.delivery.inbound_password_breaks'),
|
||||
tab: 'integrations',
|
||||
// The inbound-mail card (host/port/user/folder + this
|
||||
// password) is on the 'services' tab — 'integrations' is not
|
||||
// a member of Integrations::TABS at all (Fix-Runde, Befund 1).
|
||||
tab: 'services',
|
||||
satisfied: filled(app(SecretVault::class)->get('inbound_mail.password')),
|
||||
),
|
||||
];
|
||||
|
|
|
|||
|
|
@ -33,7 +33,13 @@ final class OnboardingChecks
|
|||
severity: Check::SEVERITY_BLOCKING,
|
||||
label: __('readiness.onboarding.ssh_key'),
|
||||
breaks: __('readiness.onboarding.ssh_key_breaks'),
|
||||
tab: 'integrations',
|
||||
// The SSH identity card lives on the 'platform' tab
|
||||
// (integrations.blade.php: "own machines"), beside the
|
||||
// public-key setting it pairs with — not 'integrations',
|
||||
// which is not a member of Integrations::TABS at all and
|
||||
// sent an operator clicking this exact blocking check to the
|
||||
// wrong tab (Fix-Runde, Befund 1).
|
||||
tab: 'platform',
|
||||
// EstablishSshTrust only deploys the PUBLIC half. Every step
|
||||
// after it (HostStep's own shell helper), and later every
|
||||
// Traefik write on a host already onboarded (SshTraefikWriter),
|
||||
|
|
@ -46,7 +52,10 @@ final class OnboardingChecks
|
|||
severity: Check::SEVERITY_BLOCKING,
|
||||
label: __('readiness.onboarding.secrets_key'),
|
||||
breaks: __('readiness.onboarding.secrets_key_breaks'),
|
||||
tab: 'integrations',
|
||||
// SECRETS_KEY has no field anywhere on Integrations' 'services'
|
||||
// or 'platform' tabs — the only place an operator can actually
|
||||
// set it is the raw .env editor on the 'env' tab.
|
||||
tab: 'env',
|
||||
// isUsable() rather than a bare filled() on the config value —
|
||||
// SecretCipher's own docblock warns that a second, shorter
|
||||
// rule that only checks emptiness lets a malformed-but-non-
|
||||
|
|
@ -60,7 +69,10 @@ final class OnboardingChecks
|
|||
severity: Check::SEVERITY_BLOCKING,
|
||||
label: __('readiness.onboarding.vpn_config_key'),
|
||||
breaks: __('readiness.onboarding.vpn_config_key_breaks'),
|
||||
tab: 'integrations',
|
||||
// Same reasoning as onboarding.secrets_key just above:
|
||||
// VPN_CONFIG_KEY has no console field either, only the raw
|
||||
// .env editor.
|
||||
tab: 'env',
|
||||
// Same reuse-not-reimplement reasoning as isUsable() above:
|
||||
// ConfigVault::available() is the one place that already knows
|
||||
// what counts as a usable key.
|
||||
|
|
@ -72,7 +84,12 @@ final class OnboardingChecks
|
|||
severity: Check::SEVERITY_BLOCKING,
|
||||
label: __('readiness.onboarding.wg_hub'),
|
||||
breaks: __('readiness.onboarding.wg_hub_breaks'),
|
||||
tab: 'integrations',
|
||||
// Two of the three values this check reads (wgHubPubkey,
|
||||
// wgEndpoint) are 'platform'-tab fields; only the third
|
||||
// (subnet) is .env-only and cannot be changed from the
|
||||
// console at all (see the docblock on satisfied: below). The
|
||||
// tab an operator can actually act on is 'platform'.
|
||||
tab: 'platform',
|
||||
// Two of three is no half a tunnel, it is none: ConfigureWireguard
|
||||
// writes the peer config from all three at once. Subnet is read
|
||||
// straight from config(), never through Settings/ProvisioningSettings
|
||||
|
|
|
|||
|
|
@ -26,7 +26,10 @@ final class ProvisioningChecks
|
|||
severity: Check::SEVERITY_BLOCKING,
|
||||
label: __('readiness.provisioning.dns_token'),
|
||||
breaks: __('readiness.provisioning.dns_token_breaks'),
|
||||
tab: 'integrations',
|
||||
// The DNS card (token + zone + Traefik path) is on the
|
||||
// 'services' tab — 'integrations' is not a member of
|
||||
// Integrations::TABS at all (Fix-Runde, Befund 1).
|
||||
tab: 'services',
|
||||
satisfied: filled(app(SecretVault::class)->get('dns.token')),
|
||||
),
|
||||
new Check(
|
||||
|
|
@ -35,7 +38,8 @@ final class ProvisioningChecks
|
|||
severity: Check::SEVERITY_BLOCKING,
|
||||
label: __('readiness.provisioning.dns_zone'),
|
||||
breaks: __('readiness.provisioning.dns_zone_breaks'),
|
||||
tab: 'integrations',
|
||||
// Same DNS card as provisioning.dns_token just above.
|
||||
tab: 'services',
|
||||
satisfied: filled(ProvisioningSettings::dnsZone()),
|
||||
),
|
||||
new Check(
|
||||
|
|
@ -44,7 +48,8 @@ final class ProvisioningChecks
|
|||
severity: Check::SEVERITY_WARNING,
|
||||
label: __('readiness.provisioning.traefik_path'),
|
||||
breaks: __('readiness.provisioning.traefik_path_breaks'),
|
||||
tab: 'integrations',
|
||||
// Same DNS card — traefikDynamicPath is a field on it.
|
||||
tab: 'services',
|
||||
satisfied: filled(ProvisioningSettings::traefikDynamicPath()),
|
||||
),
|
||||
new Check(
|
||||
|
|
@ -93,7 +98,8 @@ final class ProvisioningChecks
|
|||
: Check::SEVERITY_WARNING,
|
||||
label: __('readiness.provisioning.monitoring_token'),
|
||||
breaks: __('readiness.provisioning.monitoring_token_breaks'),
|
||||
tab: 'integrations',
|
||||
// The monitoring card is on the 'services' tab too.
|
||||
tab: 'services',
|
||||
satisfied: filled(app(SecretVault::class)->get('monitoring.token')),
|
||||
),
|
||||
];
|
||||
|
|
|
|||
|
|
@ -64,14 +64,20 @@
|
|||
</div>
|
||||
|
||||
<div class="flex shrink-0 items-center gap-3">
|
||||
{{-- mount() only requires hosts.manage OR secrets.manage, so an
|
||||
Admin (hosts.manage alone) can reach this page — but runCheck()
|
||||
itself demands secrets.manage and 403s otherwise. A button that
|
||||
is visible and throws when pressed is worse than no button. --}}
|
||||
@if (in_array($check->key, $runnable, true))
|
||||
<x-ui.button variant="secondary" size="sm"
|
||||
wire:click="runCheck('{{ $check->key }}')"
|
||||
wire:loading.attr="disabled"
|
||||
wire:target="runCheck('{{ $check->key }}')">
|
||||
<x-ui.icon name="refresh" class="size-4" />
|
||||
{{ __('readiness.run_check') }}
|
||||
</x-ui.button>
|
||||
@can('secrets.manage')
|
||||
<x-ui.button variant="secondary" size="sm"
|
||||
wire:click="runCheck('{{ $check->key }}')"
|
||||
wire:loading.attr="disabled"
|
||||
wire:target="runCheck('{{ $check->key }}')">
|
||||
<x-ui.icon name="refresh" class="size-4" />
|
||||
{{ __('readiness.run_check') }}
|
||||
</x-ui.button>
|
||||
@endcan
|
||||
@endif
|
||||
|
||||
<a href="{{ $this->checkUrl($check) }}" wire:navigate
|
||||
|
|
|
|||
|
|
@ -87,3 +87,81 @@ it('covers every entry in the secret registry', function () {
|
|||
);
|
||||
}
|
||||
});
|
||||
|
||||
/**
|
||||
* Fix-Runde, Befund 1: der Auflöser (checkUrl()) ist nur so gut wie seine
|
||||
* Quelle. Neun Prüfschlüssel trugen `tab: 'integrations'` — kein Mitglied von
|
||||
* Integrations::TABS — und landeten deshalb alle auf dem Standard-Reiter
|
||||
* ('services'), unabhängig davon, wo ihr Feld tatsächlich liegt. Dieser Test
|
||||
* hält JEDEN Prüfschlüssel gegen die eine Stelle, an der sein Feld wirklich
|
||||
* steht — nicht nur die neun, die diesmal falsch waren — damit der nächste
|
||||
* falsche Wert genauso durchrutscht wie diese.
|
||||
*/
|
||||
it('sends every check to the tab or page where its field actually lives', function () {
|
||||
$expectedTab = [
|
||||
'billing.stripe_secret' => 'services',
|
||||
'billing.webhook_secret' => 'env',
|
||||
'billing.company_details' => 'company',
|
||||
'billing.invoice_series' => 'company',
|
||||
'billing.catalogue_synced' => 'services',
|
||||
'onboarding.ssh_private_key' => 'platform',
|
||||
'onboarding.secrets_key' => 'env',
|
||||
'onboarding.vpn_config_key' => 'env',
|
||||
'onboarding.wg_hub' => 'platform',
|
||||
'onboarding.datacenter' => 'datacenters',
|
||||
'provisioning.dns_token' => 'services',
|
||||
'provisioning.dns_zone' => 'services',
|
||||
'provisioning.traefik_path' => 'services',
|
||||
'provisioning.usable_host' => 'hosts',
|
||||
'provisioning.vm_template' => 'plans',
|
||||
'provisioning.monitoring_token' => 'services',
|
||||
'delivery.mailer_not_log' => 'mail',
|
||||
'delivery.mailbox' => 'mail',
|
||||
'delivery.mail_templates' => 'templates',
|
||||
'delivery.inbound_mail_password' => 'services',
|
||||
'operation.scheduler' => 'env',
|
||||
'operation.queue_provisioning' => 'env',
|
||||
];
|
||||
|
||||
foreach (Readiness::all() as $check) {
|
||||
// NICHT expect($expectedTab)->toHaveKey($check->key, $message) — genau
|
||||
// dieselbe Falle wie bei toContain() weiter oben: toHaveKey()s zweites
|
||||
// Argument ist der erwartete WERT des Schlüssels, keine Fehlermeldung.
|
||||
expect(array_key_exists($check->key, $expectedTab))->toBeTrue(
|
||||
"Neuer Prüfschlüssel [{$check->key}] hat keinen erwarteten tab-Wert in diesem Test — bitte nachtragen."
|
||||
);
|
||||
|
||||
expect($check->tab)->toBe(
|
||||
$expectedTab[$check->key],
|
||||
"{$check->key} zeigt auf '{$check->tab}', sein Feld liegt aber unter '{$expectedTab[$check->key]}'."
|
||||
);
|
||||
}
|
||||
});
|
||||
|
||||
/**
|
||||
* Fix-Runde, Befund 2: mount() lässt hosts.manage ODER secrets.manage durch,
|
||||
* runCheck() verlangt aber secrets.manage allein. Ein Admin (nur
|
||||
* hosts.manage) erreicht die Seite und sah bislang auch den Knopf — der beim
|
||||
* Drücken 403 geworfen hätte. Ein sichtbarer Knopf, der beim Drücken
|
||||
* scheitert, ist schlimmer als gar keiner.
|
||||
*/
|
||||
it('hides the check button from an operator who could not press it anyway', function () {
|
||||
Livewire::actingAs(Operator::factory()->role('Admin')->create(), 'operator')
|
||||
->test(ReadinessPage::class)
|
||||
->assertDontSee(__('readiness.run_check'));
|
||||
});
|
||||
|
||||
/**
|
||||
* Fix-Runde, Befund 3: die eine Eigenschaft, wegen der diese ganze
|
||||
* Konstruktion existiert — die vier RUNNABLE-Prüfungen laufen NUR auf
|
||||
* Knopfdruck, weil DnsTokenCheck einen echten TXT-Eintrag in die
|
||||
* Produktivzone schreibt — war bislang ungeprüft.
|
||||
*/
|
||||
it('sends nothing over the network on a plain page load', function () {
|
||||
Http::fake();
|
||||
|
||||
Livewire::actingAs(Operator::factory()->role('Owner')->create(), 'operator')
|
||||
->test(ReadinessPage::class);
|
||||
|
||||
Http::assertNothingSent();
|
||||
});
|
||||
|
|
|
|||
Loading…
Reference in New Issue