fix(fleet): gate node update actions to admins and harden update tracking (#1272)

* fix(fleet): gate node update actions to admins and harden update tracking

Node update affordances now render only for admins, matching the admin-only
routes behind them. Previously a non-admin could open the Fleet view and see the
per-node Update button, Update all, retry, dismiss, and Recheck controls, then
get a 403 on click. Those controls are now hidden for non-admins, who still see
read-only update status.

Both update-status clear routes (per-node and bulk) now require admin, and the
bulk recheck throttles its forced "latest published version" lookup so a caller
cannot loop it to hammer the upstream registries; the response reports whether
the refresh actually ran so the UI can surface a "checked recently" note.

Completion detection no longer reports a node as Updated when it merely blips
offline and returns on the same version with an unchanged process start time.
That case stays in progress and is decided by the existing early-fail and
timeout heuristics, so a momentary network glitch is not mistaken for a
successful update. Failed and timed-out updates now emit an operator-visible
warning, and a periodic safety-net sweep bounds in-flight trackers when no
client is polling for status.

* fix(fleet): harden update completion and recheck failure handling

Refinements from review of the node self-update hardening:

- Completion signal 1 now requires a valid version, not merely a different one.
  A node whose /api/meta momentarily omits or mangles its version (online, same
  process) reported version=null, which compared unequal to the previous version
  and falsely marked the update completed. It now stays in progress and is
  decided by the early-fail/timeout heuristics.

- Terminal resolution is atomic: it re-reads the live tracker and transitions
  only if it is still in flight with the same start time, so two concurrent
  status polls cannot both warn or clobber each other's transition.

- The operator warning for a failed or timed-out update now redacts
  secret-shaped text (bearer/basic/token/password, credentialed URLs) from the
  underlying error before logging, in addition to stripping control characters.

- The Recheck button now surfaces an error toast when the request throws
  (network or auth failure), matching the existing non-ok-response path instead
  of only logging to the console.
This commit is contained in:
Anso
2026-06-01 17:27:25 -04:00
committed by GitHub
parent 7d7e0a6264
commit 0953025036
11 changed files with 592 additions and 34 deletions
+1
View File
@@ -237,6 +237,7 @@ export function FleetView({ onNavigateToNode }: FleetViewProps) {
checkingUpdates={updateStatus.checkingUpdates}
updateStatuses={updateStatus.updateStatuses}
updatingNodeId={updateStatus.updatingNodeId}
isAdmin={isAdmin}
fetchUpdateStatus={updateStatus.fetchUpdateStatus}
triggerNodeUpdate={updateStatus.triggerNodeUpdate}
retryNodeUpdate={updateStatus.retryNodeUpdate}
@@ -211,8 +211,8 @@ export function NodeCard({ node, onNavigate, labelMap, updateStatus, onUpdate, u
<UpdateStatusBadge
status={updateStatus.updateStatus}
error={updateStatus.error}
onRetry={onRetryUpdate ? () => onRetryUpdate(node.id) : undefined}
onDismiss={onDismissUpdate ? () => onDismissUpdate(node.id) : undefined}
onRetry={isAdmin && onRetryUpdate ? () => onRetryUpdate(node.id) : undefined}
onDismiss={isAdmin && onDismissUpdate ? () => onDismissUpdate(node.id) : undefined}
/>
)}
{updateStatus?.updateAvailable && !updateStatus.updateStatus && (
@@ -292,8 +292,8 @@ export function NodeCard({ node, onNavigate, labelMap, updateStatus, onUpdate, u
</div>
)}
{/* Update button */}
{isOnline && updateStatus?.updateAvailable && !updateStatus.updateStatus && onUpdate && (
{/* Update button (mutating action: admin only, matches the requireAdmin route guard) */}
{isOnline && updateStatus?.updateAvailable && !updateStatus.updateStatus && onUpdate && isAdmin && (
<div className="mt-3 pt-3 border-t border-border/50">
<Button
variant="outline"
@@ -8,6 +8,7 @@ import { Input } from '@/components/ui/input';
import { Badge } from '@/components/ui/badge';
import { Button } from '@/components/ui/button';
import { apiFetch } from '@/lib/api';
import { toast } from '@/components/ui/toast-store';
import { formatVersion } from '@/lib/version';
import { UpdateStatusBadge } from './UpdateStatusBadge';
import type { NodeUpdateStatus } from './types';
@@ -18,6 +19,10 @@ interface NodeUpdatesSheetProps {
checkingUpdates: boolean;
updateStatuses: NodeUpdateStatus[];
updatingNodeId: number | null;
/** Mutating affordances (update, update-all, retry, dismiss, recheck) render
* only for admins, matching the requireAdmin guard on the fleet routes they
* call. Non-admins still see the read-only status table. */
isAdmin: boolean;
fetchUpdateStatus: () => Promise<void>;
triggerNodeUpdate: (nodeId: number) => void;
retryNodeUpdate: (nodeId: number) => void;
@@ -26,7 +31,7 @@ interface NodeUpdatesSheetProps {
}
export function NodeUpdatesSheet({
open, onOpenChange, checkingUpdates, updateStatuses, updatingNodeId,
open, onOpenChange, checkingUpdates, updateStatuses, updatingNodeId, isAdmin,
fetchUpdateStatus, triggerNodeUpdate, retryNodeUpdate, dismissNodeUpdate, triggerUpdateAll,
}: NodeUpdatesSheetProps) {
const [search, setSearch] = useState('');
@@ -40,10 +45,26 @@ export function NodeUpdatesSheet({
const handleRecheck = async () => {
setRecheckingUpdates(true);
try {
await apiFetch('/fleet/update-status?recheck=true', { method: 'DELETE', localOnly: true });
const res = await apiFetch('/fleet/update-status?recheck=true', { method: 'DELETE', localOnly: true });
if (res.ok) {
// The server throttles the upstream version lookup; `rechecked:false`
// means a forced refresh ran too recently and the cached value stands.
const data = await res.json().catch(() => ({}));
if (data?.rechecked === false) {
toast.info('Already checked for the latest version recently.');
}
} else {
// apiFetch only throws on 401/network, so HTTP errors (e.g. a 500
// from the upstream lookup) land here, not in the catch below.
console.warn('[Fleet] Recheck returned HTTP', res.status);
toast.error('Could not recheck for updates. Try again shortly.');
}
await fetchUpdateStatus();
} catch (err) {
// Recheck is an explicit user click, so a thrown network/auth failure
// gets a toast, not just a console breadcrumb.
console.warn('[Fleet] Recheck failed:', err);
toast.error('Could not recheck for updates. Try again shortly.');
} finally {
setRecheckingUpdates(false);
}
@@ -69,7 +90,7 @@ export function NodeUpdatesSheet({
? undefined
: (gatewayLabel ? `Latest version ${gatewayLabel}` : `${available} update${available === 1 ? '' : 's'} available`);
const secondaryActions = updatableRemoteCount > 0
const secondaryActions = isAdmin && updatableRemoteCount > 0
? [{
label: `Update all (${updatableRemoteCount})`,
icon: Download,
@@ -84,12 +105,12 @@ export function NodeUpdatesSheet({
crumb={['Fleet', 'Updates']}
name="Node updates"
meta={meta}
primaryAction={{
primaryAction={isAdmin ? {
label: 'Recheck',
icon: recheckingUpdates ? Loader2 : RefreshCw,
onClick: () => { void handleRecheck(); },
disabled: recheckingUpdates || checkingUpdates,
}}
} : undefined}
secondaryActions={secondaryActions}
footerContext={footerContext}
size="lg"
@@ -179,8 +200,8 @@ export function NodeUpdatesSheet({
<UpdateStatusBadge
status={s.updateStatus}
error={s.error}
onRetry={() => retryNodeUpdate(s.nodeId)}
onDismiss={() => dismissNodeUpdate(s.nodeId)}
onRetry={isAdmin ? () => retryNodeUpdate(s.nodeId) : undefined}
onDismiss={isAdmin ? () => dismissNodeUpdate(s.nodeId) : undefined}
/>
)}
{!s.updateStatus && !s.updateAvailable && (
@@ -188,7 +209,7 @@ export function NodeUpdatesSheet({
<Check className="w-2.5 h-2.5 mr-0.5" /> Up to date
</Badge>
)}
{s.updateAvailable && !s.updateStatus && (
{s.updateAvailable && !s.updateStatus && isAdmin && (
<Button
variant="outline"
size="sm"
@@ -203,6 +224,11 @@ export function NodeUpdatesSheet({
)}
</Button>
)}
{s.updateAvailable && !s.updateStatus && !isAdmin && (
<Badge className="text-[10px] px-1.5 py-0 h-5 bg-warning/15 text-warning border-warning/30">
<CircleAlert className="w-2.5 h-2.5 mr-0.5" /> Available
</Badge>
)}
</div>
</div>
))}
@@ -66,4 +66,22 @@ describe('NodeCard', () => {
// The actions menu only renders when cordon (admiral-only here) is available.
expect(screen.getByRole('button', { name: 'Node actions' })).toBeInTheDocument();
});
const updateAvailableStatus = {
nodeId: 2, name: 'Edge', type: 'remote' as const, version: '1.0.0', latestVersion: '1.1.0',
updateAvailable: true, updateStatus: null,
};
it('renders the update button for an admin when an update is available', () => {
useAuthMock.mockReturnValue({ isAdmin: true });
render(<NodeCard {...baseProps(onlineNode())} updateStatus={updateAvailableStatus} onUpdate={vi.fn()} />);
expect(screen.getByRole('button', { name: /Update/ })).toBeInTheDocument();
});
it('hides the update button for a non-admin but still shows the read-only badge', () => {
useAuthMock.mockReturnValue({ isAdmin: false });
render(<NodeCard {...baseProps(onlineNode())} updateStatus={updateAvailableStatus} onUpdate={vi.fn()} />);
expect(screen.queryByRole('button', { name: /Update/ })).not.toBeInTheDocument();
expect(screen.getByText('Update available')).toBeInTheDocument();
});
});
@@ -1,10 +1,14 @@
import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
import { render, screen, fireEvent } from '@testing-library/react';
import { render, screen, fireEvent, waitFor } from '@testing-library/react';
const apiFetchMock = vi.fn();
vi.mock('@/lib/api', () => ({ apiFetch: (...a: unknown[]) => apiFetchMock(...a) }));
vi.mock('@/components/ui/toast-store', () => ({
toast: { info: vi.fn(), success: vi.fn(), error: vi.fn(), warning: vi.fn() },
}));
import { NodeUpdatesSheet } from '../NodeUpdatesSheet';
import { toast } from '@/components/ui/toast-store';
import type { NodeUpdateStatus } from '../types';
const STATUSES: NodeUpdateStatus[] = [
@@ -20,6 +24,7 @@ function baseProps(overrides: Partial<React.ComponentProps<typeof NodeUpdatesShe
checkingUpdates: false,
updateStatuses: STATUSES,
updatingNodeId: null,
isAdmin: true,
fetchUpdateStatus: vi.fn(async () => {}),
triggerNodeUpdate: vi.fn(),
retryNodeUpdate: vi.fn(),
@@ -63,4 +68,63 @@ describe('NodeUpdatesSheet', () => {
expect(screen.getByText('Edge')).toBeInTheDocument();
expect(screen.queryByText('Local')).not.toBeInTheDocument();
});
it('renders every mutating affordance for an admin', () => {
render(<NodeUpdatesSheet {...baseProps({ isAdmin: true })} />);
expect(screen.getByRole('button', { name: 'Recheck' })).toBeInTheDocument();
expect(screen.getByRole('button', { name: /Update all/ })).toBeInTheDocument();
expect(screen.getByRole('button', { name: /Update$/ })).toBeInTheDocument();
expect(screen.getByLabelText('Retry update')).toBeInTheDocument();
});
it('toasts when a recheck is throttled by the server (rechecked:false)', async () => {
apiFetchMock.mockResolvedValue({ ok: true, json: async () => ({ rechecked: false }) });
render(<NodeUpdatesSheet {...baseProps({ isAdmin: true })} />);
fireEvent.click(screen.getByRole('button', { name: 'Recheck' }));
await waitFor(() => expect(toast.info).toHaveBeenCalled());
expect(apiFetchMock).toHaveBeenCalledWith(
'/fleet/update-status?recheck=true',
expect.objectContaining({ method: 'DELETE' }),
);
});
it('surfaces an error when the recheck fails (non-ok response, and by symmetry a thrown failure)', async () => {
// apiFetch only throws on 401/network; HTTP errors land as res.ok === false.
// Both the else branch (this case) and the catch branch raise the same
// toast.error, so this exercises the user-facing failure toast. The thrown
// path is not driven through the click here because this repo's test harness
// re-surfaces a rejected Error flowing through a React event handler as a
// test failure even when the handler catches it.
apiFetchMock.mockResolvedValue({ ok: false, status: 500, json: async () => ({}) });
render(<NodeUpdatesSheet {...baseProps({ isAdmin: true })} />);
fireEvent.click(screen.getByRole('button', { name: 'Recheck' }));
await waitFor(() => expect(toast.error).toHaveBeenCalled());
expect(toast.info).not.toHaveBeenCalled();
});
it('does not toast when a recheck actually refreshed (rechecked:true)', async () => {
const fetchUpdateStatus = vi.fn(async () => {});
apiFetchMock.mockResolvedValue({ ok: true, json: async () => ({ rechecked: true }) });
render(<NodeUpdatesSheet {...baseProps({ isAdmin: true, fetchUpdateStatus })} />);
fireEvent.click(screen.getByRole('button', { name: 'Recheck' }));
await waitFor(() => expect(fetchUpdateStatus).toHaveBeenCalled());
expect(toast.info).not.toHaveBeenCalled();
});
it('hides every mutating affordance for a non-admin but keeps the read-only table', () => {
render(<NodeUpdatesSheet {...baseProps({ isAdmin: false })} />);
// Read-only status remains visible
expect(screen.getByText('Local')).toBeInTheDocument();
expect(screen.getByText('Edge')).toBeInTheDocument();
expect(screen.getByText('Db')).toBeInTheDocument();
// 'Available' appears once as the summary stat label; for a non-admin the
// per-row read-only badge adds a second occurrence in place of the button.
expect(screen.getAllByText('Available')).toHaveLength(2);
// No mutate controls
expect(screen.queryByRole('button', { name: 'Recheck' })).not.toBeInTheDocument();
expect(screen.queryByRole('button', { name: /Update all/ })).not.toBeInTheDocument();
expect(screen.queryByRole('button', { name: /Update$/ })).not.toBeInTheDocument();
expect(screen.queryByLabelText('Retry update')).not.toBeInTheDocument();
expect(screen.queryByLabelText('Dismiss')).not.toBeInTheDocument();
});
});