You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by ri...@apache.org on 2026/09/14 22:11:53 UTC
(superset) branch master updated: fix(archive): hide recovery actions from read-only users (#44179)
This is an automated email from the ASF dual-hosted git repository.
richardfogaca pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 9526ce8082d fix(archive): hide recovery actions from read-only users (#44179)
9526ce8082d is described below
commit 9526ce8082d78befdd1155e4313d1a3b55ace2d1
Author: Mike Bridge <mi...@preset.io>
AuthorDate: Mon Sep 14 16:11:45 2026 -0600
fix(archive): hide recovery actions from read-only users (#44179)
Co-authored-by: Claude Fable 5 <no...@anthropic.com>
---
docs/docs/using-superset/recently-archived.mdx | 10 +-
.../src/pages/ArchivedList/ArchivedList.test.tsx | 146 ++++++++++++++++++++-
superset-frontend/src/pages/ArchivedList/index.tsx | 22 +++-
3 files changed, 166 insertions(+), 12 deletions(-)
diff --git a/docs/docs/using-superset/recently-archived.mdx b/docs/docs/using-superset/recently-archived.mdx
index f000bbc9311..60735bc9460 100644
--- a/docs/docs/using-superset/recently-archived.mdx
+++ b/docs/docs/using-superset/recently-archived.mdx
@@ -54,6 +54,12 @@ returns to its normal list and disappears from the archive. Recovering is
limited to the object's editors and admins; you can only recover objects you
are able to see in this view.
+Seeing an archived object and acting on it are separate permissions: objects
+you can view but not edit still appear in the list. The **Recover** and
+**Delete permanently** buttons appear only if you have edit permission for
+that object type; recovering a specific object can still be refused if you
+are not one of its editors.
+
## Deleting an object permanently
Use the **Delete permanently** action on a row to remove an object for good.
@@ -62,7 +68,9 @@ You will be asked to confirm.
This cannot be undone. Unlike archiving, it does not move the object anywhere
— the object and its version history are erased, and no retention window
applies. The same audience that can recover an object can delete it
-permanently: its editors and admins.
+permanently: its editors and admins. As with recovery, the action appears only
+if you have edit permission for that object type; a specific object can still
+be refused.
Some objects cannot be deleted permanently while something still depends on
them. A chart used by an alert or report, for example, is refused until that
diff --git a/superset-frontend/src/pages/ArchivedList/ArchivedList.test.tsx b/superset-frontend/src/pages/ArchivedList/ArchivedList.test.tsx
index d0017b5adb9..f666414f5d1 100644
--- a/superset-frontend/src/pages/ArchivedList/ArchivedList.test.tsx
+++ b/superset-frontend/src/pages/ArchivedList/ArchivedList.test.tsx
@@ -157,21 +157,32 @@ const mockRoutes = (
restoreStatus = 200,
purgeResponse: Parameters<typeof fetchMock.post>[1] = {},
impactResponse: Parameters<typeof fetchMock.get>[1] = buildPositiveImpact(),
+ infoResponses: Partial<
+ Record<
+ 'chart' | 'dashboard' | 'dataset',
+ Parameters<typeof fetchMock.get>[1]
+ >
+ > = {},
) => {
- fetchMock.get(infoEndpoint, { permissions: ['can_read', 'can_write'] });
+ fetchMock.get(
+ infoEndpoint,
+ infoResponses.chart ?? { permissions: ['can_read', 'can_write'] },
+ );
fetchMock.get(listEndpoint, { result: mockCharts, count: mockCharts.length });
fetchMock.post(restoreEndpoint, restoreStatus === 200 ? {} : restoreStatus);
fetchMock.post(purgeEndpoint, purgeResponse);
- fetchMock.get(dashboardInfoEndpoint, {
- permissions: ['can_read', 'can_write'],
- });
+ fetchMock.get(
+ dashboardInfoEndpoint,
+ infoResponses.dashboard ?? { permissions: ['can_read', 'can_write'] },
+ );
fetchMock.get(dashboardListEndpoint, {
result: mockDashboards,
count: mockDashboards.length,
});
- fetchMock.get(datasetInfoEndpoint, {
- permissions: ['can_read', 'can_write'],
- });
+ fetchMock.get(
+ datasetInfoEndpoint,
+ infoResponses.dataset ?? { permissions: ['can_read', 'can_write'] },
+ );
fetchMock.get(datasetListEndpoint, {
result: mockDatasets,
count: mockDatasets.length,
@@ -227,6 +238,127 @@ test('issues the deleted-only baseline filter on the list request', async () =>
});
});
+test.each([
+ { resource: 'chart', label: 'Chart', name: 'Deleted Chart One' },
+ { resource: 'dashboard', label: 'Dashboard', name: 'Deleted Dashboard One' },
+ { resource: 'dataset', label: 'Dataset', name: 'deleted_table_one' },
+])(
+ 'read-only $label rows do not offer recovery or purge',
+ async ({ resource, label, name }) => {
+ mockRoutes(200, {}, buildPositiveImpact(), {
+ [resource]: { permissions: ['can_read', 'can_export'] },
+ });
+ renderArchivedList(storeWithReadAccess(label));
+
+ const rowName = await screen.findByText(name);
+ expect(
+ screen.queryByRole('columnheader', { name: 'Actions' }),
+ ).not.toBeInTheDocument();
+ expect(screen.queryAllByTestId('archived-row-restore')).toHaveLength(0);
+ expect(screen.queryAllByTestId('archived-row-purge')).toHaveLength(0);
+ // findBy waits out antd's tooltip mouseEnterDelay, so the read-only copy
+ // appearing is the timing anchor — only then is asserting the absence of
+ // the editor copy meaningful (a query fired before the delay elapses
+ // passes vacuously for either string).
+ await userEvent.hover(rowName);
+ expect(
+ await screen.findByText(
+ 'Archived items must be recovered before they can be opened.',
+ ),
+ ).toBeInTheDocument();
+ expect(
+ screen.queryByText('Recover this item to open it'),
+ ).not.toBeInTheDocument();
+ expect(
+ fetchMock.callHistory.calls(/\/(restore|purge|purge-impact)$/),
+ ).toHaveLength(0);
+ },
+);
+
+test.each([
+ { label: 'Chart', name: 'Deleted Chart One' },
+ { label: 'Dashboard', name: 'Deleted Dashboard One' },
+ { label: 'Dataset', name: 'deleted_table_one' },
+])(
+ '$label write permission retains archived row actions',
+ async ({ label, name }) => {
+ mockRoutes();
+ renderArchivedList(storeWithReadAccess(label));
+
+ expect(
+ (await screen.findAllByTestId('archived-row-restore'))[0],
+ ).toBeEnabled();
+ expect(screen.getAllByTestId('archived-row-purge')[0]).toBeEnabled();
+
+ // Positive tooltip control: the editor DOES get the recover prompt —
+ // proving the hover-and-wait mechanism can surface a tooltip at all,
+ // which is what makes the read-only test's absence assertion credible.
+ await userEvent.hover(screen.getByText(name));
+ expect(
+ await screen.findByText('Recover this item to open it'),
+ ).toBeInTheDocument();
+ },
+);
+
+test('delete permission alone does not authorize archived dashboard actions', async () => {
+ mockRoutes(200, {}, buildPositiveImpact(), {
+ dashboard: { permissions: ['can_read', 'can_delete'] },
+ });
+ renderArchivedList(storeWithReadAccess('Dashboard'));
+
+ await screen.findByText('Deleted Dashboard One');
+ expect(screen.queryAllByTestId('archived-row-restore')).toHaveLength(0);
+ expect(screen.queryAllByTestId('archived-row-purge')).toHaveLength(0);
+});
+
+test('switching to a read-only type does not retain write actions', async () => {
+ mockRoutes(200, {}, buildPositiveImpact(), {
+ dashboard: { permissions: ['can_read'] },
+ });
+ renderArchivedList();
+ await screen.findAllByTestId('archived-row-restore');
+
+ await selectOption('Dashboard', 'Type');
+ await screen.findByText('Deleted Dashboard One');
+ expect(screen.queryAllByTestId('archived-row-restore')).toHaveLength(0);
+ expect(screen.queryAllByTestId('archived-row-purge')).toHaveLength(0);
+
+ await selectOption('Chart', 'Type');
+ expect(
+ (await screen.findAllByTestId('archived-row-restore'))[0],
+ ).toBeEnabled();
+});
+
+test('archived actions remain hidden until write permissions load', async () => {
+ let resolveInfo!: (response: { permissions: string[] }) => void;
+ const infoResponse = new Promise<{ permissions: string[] }>(resolve => {
+ resolveInfo = resolve;
+ });
+ mockRoutes(200, {}, buildPositiveImpact(), {
+ dashboard: () => infoResponse,
+ });
+ renderArchivedList(storeWithReadAccess('Dashboard'));
+
+ await screen.findByText('Deleted Dashboard One');
+ expect(screen.queryAllByTestId('archived-row-restore')).toHaveLength(0);
+ expect(screen.queryAllByTestId('archived-row-purge')).toHaveLength(0);
+
+ resolveInfo({ permissions: ['can_read', 'can_write'] });
+ expect(
+ (await screen.findAllByTestId('archived-row-restore'))[0],
+ ).toBeEnabled();
+});
+
+test('archived actions remain hidden when permissions cannot be loaded', async () => {
+ mockRoutes(200, {}, buildPositiveImpact(), { dashboard: 500 });
+ renderArchivedList(storeWithReadAccess('Dashboard'));
+
+ await screen.findByText('Deleted Dashboard One');
+ await waitFor(() => expect(mockAddDangerToast).toHaveBeenCalled());
+ expect(screen.queryAllByTestId('archived-row-restore')).toHaveLength(0);
+ expect(screen.queryAllByTestId('archived-row-purge')).toHaveLength(0);
+});
+
test('restore posts to the per-type endpoint and refetches on success', async () => {
mockRoutes();
renderArchivedList();
diff --git a/superset-frontend/src/pages/ArchivedList/index.tsx b/superset-frontend/src/pages/ArchivedList/index.tsx
index 4b2e109d1fd..d4a7e6a065c 100644
--- a/superset-frontend/src/pages/ArchivedList/index.tsx
+++ b/superset-frontend/src/pages/ArchivedList/index.tsx
@@ -179,6 +179,7 @@ function ArchivedListBody({
state: { loading, resourceCount, resourceCollection },
fetchData,
refreshData,
+ hasPerm,
} = useListViewResource<ArchivedItem>(
config.resource,
TYPE_LABELS[type](),
@@ -187,11 +188,13 @@ function ArchivedListBody({
[],
baseFilters,
);
+ // Restore and purge both require the selected resource's write permission.
+ const canWrite = hasPerm('can_write');
// Restore is immediate (no confirm dialog). On success, refetch the full page
// so the server-side count/pagination stays consistent and the row drops out;
- // on any error surface a danger toast and leave the row in place. The list
- // read is already owner-scoped, so every visible row is restorable.
+ // on any error surface a danger toast and leave the row in place. List
+ // visibility does not imply write permission; the API also checks ownership.
// A second activation while a request is in flight races the first: by the
// time the retry lands the row is already restored (or purged), so the
// server answers 404 and the user is shown a failure after a success. The
@@ -431,9 +434,18 @@ function ArchivedListBody({
// error — the reader is shown what looks like an empty new chart
// rather than told anything. Neither is a preview, and the silent
// one is the worse of the two, so no row links out until the object
- // is recovered.
+ // is recovered. Both audiences get told why: editors are prompted
+ // to recover; readers, who cannot recover, learn the precondition.
return (
- <Tooltip title={t('Recover this item to open it')}>
+ <Tooltip
+ title={
+ canWrite
+ ? t('Recover this item to open it')
+ : t(
+ 'Archived items must be recovered before they can be opened.',
+ )
+ }
+ >
<span>{name}</span>
</Tooltip>
);
@@ -490,12 +502,14 @@ function ArchivedListBody({
),
Header: t('Actions'),
id: 'actions',
+ hidden: !canWrite,
disableSortBy: true,
size: 'sm',
},
],
[
config.nameField,
+ canWrite,
type,
handleRestore,
handlePurge,