diff --git a/superset-frontend/src/components/Datasource/DatasourceModal/DatasourceModal.test.tsx b/superset-frontend/src/components/Datasource/DatasourceModal/DatasourceModal.test.tsx index 942cddb6648..fa63e8b40a5 100644 --- a/superset-frontend/src/components/Datasource/DatasourceModal/DatasourceModal.test.tsx +++ b/superset-frontend/src/components/Datasource/DatasourceModal/DatasourceModal.test.tsx @@ -67,14 +67,23 @@ async function renderAndWait(props = mockedProps) { container = renderedContainer; } -beforeEach(() => { +// A modal that wasn't handed an `etag` reads the dataset itself and can't save +// until that lands, so tests must wait before acting on the Save button. +async function waitForSaveEnabled() { + await waitFor(() => + expect(screen.getByTestId('datasource-modal-save')).toBeEnabled(), + ); +} + +beforeEach(async () => { fetchMock.clearHistory().removeRoutes(); cleanup(); - renderAndWait(); fetchMock.post(SAVE_ENDPOINT, SAVE_PAYLOAD); fetchMock.put(SAVE_DATASOURCE_ENDPOINT, {}); fetchMock.get(GET_DATASOURCE_ENDPOINT, { result: {} }); fetchMock.get(GET_DATABASE_ENDPOINT, { result: [] }); + renderAndWait(); + await waitForSaveEnabled(); }); // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks @@ -118,6 +127,7 @@ describe('DatasourceModal', () => { onDatasourceSave: onDatasourceSave as unknown as typeof mockedProps.onDatasourceSave, }); + await waitForSaveEnabled(); const saveButton = screen.getByTestId('datasource-modal-save'); fireEvent.click(saveButton); const okButton = await screen.findByRole('button', { name: 'Confirm' }); @@ -151,6 +161,96 @@ describe('DatasourceModal', () => { putSpy.mockRestore(); }); + test('sends the supplied etag as If-Match so a stale save is refused', async () => { + cleanup(); + renderAndWait({ ...mockedProps, etag: '"v1"' } as typeof mockedProps); + + fireEvent.click(screen.getByTestId('datasource-modal-save')); + fireEvent.click(await screen.findByRole('button', { name: 'Confirm' })); + + await waitFor(() => { + const putCall = fetchMock.callHistory + .calls() + .find(call => call.options?.method === 'put'); + expect( + new Headers(putCall?.options?.headers as HeadersInit).get('If-Match'), + ).toEqual('"v1"'); + }); + }); + + test('reads the etag from the dataset when the caller supplies none', async () => { + cleanup(); + fetchMock.clearHistory().removeRoutes(); + fetchMock.put(SAVE_DATASOURCE_ENDPOINT, {}); + fetchMock.get(GET_DATASOURCE_ENDPOINT, { + body: { result: {} }, + headers: { ETag: '"v2"' }, + }); + fetchMock.get(GET_DATABASE_ENDPOINT, { result: [] }); + + renderAndWait(); + + // The form is seeded from the same read as the validator, so saving is + // unavailable until it lands. + expect(screen.getByTestId('datasource-modal-save')).toBeDisabled(); + await screen.findByTestId('datasource-editor'); + + fireEvent.click(screen.getByTestId('datasource-modal-save')); + fireEvent.click(await screen.findByRole('button', { name: 'Confirm' })); + + await waitFor(() => { + const putCall = fetchMock.callHistory + .calls() + .find(call => call.options?.method === 'put'); + expect( + new Headers(putCall?.options?.headers as HeadersInit).get('If-Match'), + ).toEqual('"v2"'); + }); + }); + + test('never saves unguarded while the validator read is in flight', async () => { + cleanup(); + fetchMock.clearHistory().removeRoutes(); + fetchMock.put(SAVE_DATASOURCE_ENDPOINT, {}); + // A read that never resolves: the save path must stay closed rather than + // fall through to an unconditional PUT. + fetchMock.get(GET_DATASOURCE_ENDPOINT, new Promise(() => {})); + fetchMock.get(GET_DATABASE_ENDPOINT, { result: [] }); + + renderAndWait(); + + const saveButton = await screen.findByTestId('datasource-modal-save'); + expect(saveButton).toBeDisabled(); + fireEvent.click(saveButton); + + expect( + fetchMock.callHistory + .calls() + .find(call => call.options?.method === 'put'), + ).toBeUndefined(); + }); + + test('shows a conflict dialog instead of a generic error on 412', async () => { + const putSpy = jest + .spyOn(SupersetClient, 'put') + .mockRejectedValue(new Response('', { status: 412 })); + + try { + fireEvent.click(screen.getByTestId('datasource-modal-save')); + fireEvent.click(await screen.findByRole('button', { name: 'Confirm' })); + + const conflictElements = await screen.findAllByText( + 'Dataset changed since you opened it', + ); + expect(conflictElements.length).toBeGreaterThan(0); + expect( + screen.queryByText('Error saving dataset'), + ).not.toBeInTheDocument(); + } finally { + putSpy.mockRestore(); + } + }); + test('shows sync columns checkbox when SQL changes', async () => { cleanup(); const datasourceWithSQL = { @@ -163,15 +263,24 @@ describe('DatasourceModal', () => { }; const { rerender } = render( - , + , { store, useRouter: true }, ); // Update with modified SQL rerender( - , + , ); + await waitForSaveEnabled(); const saveButton = screen.getByTestId('datasource-modal-save'); fireEvent.click(saveButton); @@ -208,15 +317,24 @@ describe('DatasourceModal', () => { fetchMock.get(GET_DATABASE_ENDPOINT, { result: [] }); const { rerender } = render( - , + , { store, useRouter: true }, ); // Update with modified SQL to trigger checkbox rerender( - , + , ); + await waitForSaveEnabled(); const saveButton = screen.getByTestId('datasource-modal-save'); fireEvent.click(saveButton); @@ -269,15 +387,24 @@ describe('DatasourceModal', () => { fetchMock.get(GET_DATABASE_ENDPOINT, { result: [] }); const { rerender } = render( - , + , { store, useRouter: true }, ); // Update with modified SQL to trigger checkbox rerender( - , + , ); + await waitForSaveEnabled(); const saveButton = screen.getByTestId('datasource-modal-save'); fireEvent.click(saveButton); diff --git a/superset-frontend/src/components/Datasource/DatasourceModal/DatasourceModal.useModal.test.tsx b/superset-frontend/src/components/Datasource/DatasourceModal/DatasourceModal.useModal.test.tsx index 2610190b47b..f3397cd793b 100644 --- a/superset-frontend/src/components/Datasource/DatasourceModal/DatasourceModal.useModal.test.tsx +++ b/superset-frontend/src/components/Datasource/DatasourceModal/DatasourceModal.useModal.test.tsx @@ -21,6 +21,7 @@ import { screen, fireEvent, act, + waitFor, defaultStore as store, } from 'spec/helpers/testing-library'; import fetchMock from 'fetch-mock'; @@ -72,6 +73,9 @@ test('DatasourceModal - should handle sync columns state without imperative moda render(, { store }); const saveButton = screen.getByTestId('datasource-modal-save'); + // The modal fetches the current dataset version on open; save stays disabled + // until that settles + await waitFor(() => expect(saveButton).toBeEnabled()); // This should not throw any DOM errors await act(async () => { diff --git a/superset-frontend/src/components/Datasource/DatasourceModal/index.tsx b/superset-frontend/src/components/Datasource/DatasourceModal/index.tsx index 69c7ff293da..ae296f9c9fa 100644 --- a/superset-frontend/src/components/Datasource/DatasourceModal/index.tsx +++ b/superset-frontend/src/components/Datasource/DatasourceModal/index.tsx @@ -33,12 +33,14 @@ import { Icons, Button, Checkbox, + Loading, Modal, AsyncEsmComponent, } from '@superset-ui/core/components'; import withToasts from 'src/components/MessageToasts/withToasts'; import { ErrorMessageWithStackTrace } from 'src/components'; import type { DatasetObject } from 'src/features/datasets/types'; +import { withCertificationFields } from '../utils'; import { mapSubjectValuesToIds } from 'src/features/subjects/SubjectPicker'; import type { DatasourceModalProps } from '../types'; @@ -91,12 +93,18 @@ export function buildExtraJsonObject( const DatasourceModal: FunctionComponent = ({ addSuccessToast, datasource, + etag, onDatasourceSave, onHide, show, }) => { const theme = useTheme(); const [currentDatasource, setCurrentDatasource] = useState(datasource); + // SQL of the server snapshot the form started from. The caller's, unless + // this modal read the dataset itself — then "did the SQL change?" has to be + // asked against the snapshot the payload is actually built from. + const [seededSql, setSeededSql] = useState(); + const [versionEtag, setVersionEtag] = useState(etag); const [syncColumns, setSyncColumns] = useState(false); const currencies = useSelector< { @@ -111,6 +119,52 @@ const DatasourceModal: FunctionComponent = ({ const [isEditing, setIsEditing] = useState(false); const [modal, contextHolder] = Modal.useModal(); const [confirmModalOpen, setConfirmModalOpen] = useState(false); + const [isLoadingDatasource, setIsLoadingDatasource] = useState(false); + + // Callers that read the dataset themselves (the dataset list) hand down the + // ETag of that read. The rest — Explore, where `datasource` comes from the + // page's bootstrap state — read it here, and must seed the form from the + // *same* response: a payload built from an older snapshot than the ETag + // guarding it would still be accepted, and would still clobber. + useEffect(() => { + setVersionEtag(etag); + if (etag || !show || !datasource.id) { + return undefined; + } + let cancelled = false; + setIsLoadingDatasource(true); + SupersetClient.get({ + endpoint: `/api/v1/dataset/${datasource.id}`, + }) + .then(({ json, response }) => { + if (cancelled) { + return; + } + const seeded = { + ...datasource, + ...json.result, + columns: withCertificationFields(json.result.columns), + }; + setSeededSql(seeded.sql); + setCurrentDatasource(seeded); + setVersionEtag(response.headers.get('ETag') ?? undefined); + }) + .catch(() => { + // The read failed outright, so there is no fresher snapshot to edit + // and no validator to send. Fall back to the caller's snapshot and an + // unconditional save, which is what this modal did before the guard. + }) + .finally(() => { + if (!cancelled) { + setIsLoadingDatasource(false); + } + }); + return () => { + cancelled = true; + }; + }, [datasource.id, etag, show]); + const baselineSql = seededSql ?? datasource.sql; + const buildPayload = (datasource: Record) => { const payload: Record = { table_name: datasource.table_name, @@ -197,11 +251,13 @@ const DatasourceModal: FunctionComponent = ({ await SupersetClient.put({ endpoint: `/api/v1/dataset/${currentDatasource.id}?override_columns=${syncColumns}`, jsonPayload: buildPayload(currentDatasource), + ...(versionEtag ? { headers: { 'If-Match': versionEtag } } : {}), }); - const { json } = await SupersetClient.get({ + const { json, response } = await SupersetClient.get({ endpoint: `/api/v1/dataset/${currentDatasource?.id}`, }); + setVersionEtag(response.headers.get('ETag') ?? undefined); addSuccessToast(t('The dataset has been saved')); // eslint-disable-next-line no-param-reassign @@ -213,6 +269,19 @@ const DatasourceModal: FunctionComponent = ({ onHide(); } catch (response) { setIsSaving(false); + if ((response as Response)?.status === 412) { + modal.error({ + title: t('Dataset changed since you opened it'), + okButtonProps: { danger: true, className: 'btn-danger' }, + content: t( + 'Someone else, or another one of your browser tabs, saved this ' + + 'dataset after you opened it. Saving now would undo those ' + + 'changes, so it was cancelled. Copy your edits, close this ' + + 'dialog, and reopen the dataset to reapply them.', + ), + }); + return; + } const error = await getClientErrorObject(response); let errorResponse: SupersetError | undefined; let errorText: string | undefined; @@ -264,7 +333,7 @@ const DatasourceModal: FunctionComponent = ({ here may affect other charts in undesirable ways.`)} /> - {datasource.sql !== currentDatasource.sql && ( + {baselineSql !== currentDatasource.sql && (
({ marginBottom: theme.marginMD, @@ -298,14 +367,14 @@ const DatasourceModal: FunctionComponent = ({ {t('Are you sure you want to save and apply changes?')}
), - [currentDatasource.sql, datasource.sql, syncColumns], + [currentDatasource.sql, baselineSql, syncColumns], ); useEffect(() => { - if (datasource.sql !== currentDatasource.sql) { + if (baselineSql !== currentDatasource.sql) { setSyncColumns(true); } - }, [datasource.sql, currentDatasource.sql]); + }, [baselineSql, currentDatasource.sql]); const onClickSave = () => { setConfirmModalOpen(true); @@ -356,6 +425,7 @@ const DatasourceModal: FunctionComponent = ({ onClick={onClickSave} disabled={ isSaving || + isLoadingDatasource || errors.length > 0 || currentDatasource.is_managed_externally } @@ -381,14 +451,18 @@ const DatasourceModal: FunctionComponent = ({ }} draggable > - + {isLoadingDatasource ? ( + + ) : ( + + )} {contextHolder} void; addDangerToast: (msg: string) => void; datasource: DatasetObject; + /** + * ETag of the dataset read the form was seeded from. Replayed as `If-Match` + * on save so a stale form can't clobber a newer write. Fetched by the modal + * when the caller doesn't already have one. + */ + etag?: string; onChange: () => {}; onDatasourceSave: (datasource: object, errors?: Array) => {}; onHide: () => {}; diff --git a/superset-frontend/src/components/Datasource/utils/index.ts b/superset-frontend/src/components/Datasource/utils/index.ts index 221be9d8abf..2740ed92111 100644 --- a/superset-frontend/src/components/Datasource/utils/index.ts +++ b/superset-frontend/src/components/Datasource/utils/index.ts @@ -27,6 +27,7 @@ import { nanoid } from 'nanoid'; import { SupersetClient } from '@superset-ui/core'; import { tn } from '@apache-superset/core/translation'; import rison from 'rison'; +import type { ColumnObject } from 'src/features/datasets/types'; // Type definitions @@ -248,3 +249,29 @@ export async function fetchSyncedColumns( const { json } = await SupersetClient.get({ endpoint, signal }); return json as ColumnMetadata[]; } + +/** + * Lift each column's certification out of its `extra` JSON into the flat + * fields the datasource editor binds to. + */ +export function withCertificationFields(columns: ColumnObject[] = []) { + return columns.map(column => { + // Malformed `extra` must not take out the whole column list, the way an + // uncaught parse would — same fallback as `hydrateMetricExtra`. + let parsedExtra; + try { + parsedExtra = JSON.parse(column.extra || '{}') || {}; + } catch { + parsedExtra = {}; + } + const { + certification: { details = '', certified_by: certifiedBy = '' } = {}, + } = parsedExtra; + return { + ...column, + certification_details: details || '', + certified_by: certifiedBy || '', + is_certified: details || certifiedBy, + }; + }); +} diff --git a/superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx b/superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx index 07d6365f6c6..5158a342dfd 100644 --- a/superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx +++ b/superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx @@ -297,6 +297,12 @@ test('Click on Edit dataset', async () => { const props = createProps(); fetchMock.removeRoute(getDbWithQuery); fetchMock.get(getDbWithQuery, { result: [] }, { name: getDbWithQuery }); + fetchMock.removeRoute(getDatasetWithAllMockRouteName); + fetchMock.get( + getDatasetWithAll, + { result: {} }, + { name: getDatasetWithAllMockRouteName }, + ); render(, { useRedux: true, useRouter: true, @@ -307,7 +313,9 @@ test('Click on Edit dataset', async () => { await userEvent.click(screen.getByText('Edit dataset')); }); - expect(screen.getByTestId('mock-datasource-editor')).toBeInTheDocument(); + expect( + await screen.findByTestId('mock-datasource-editor'), + ).toBeInTheDocument(); }); test('Edit dataset should be disabled when user is not admin', async () => { diff --git a/superset-frontend/src/pages/DatasetList/index.tsx b/superset-frontend/src/pages/DatasetList/index.tsx index 9d387ca0ea0..386654f8a53 100644 --- a/superset-frontend/src/pages/DatasetList/index.tsx +++ b/superset-frontend/src/pages/DatasetList/index.tsx @@ -43,7 +43,6 @@ import { } from 'src/views/CRUD/utils'; import { SUBJECT_OPTION_FILTER_PROPS } from 'src/features/subjects/SubjectSelectLabel'; import { SubjectPile } from 'src/features/subjects/SubjectPile'; -import { ColumnObject } from 'src/features/datasets/types'; import { useListViewResource } from 'src/views/CRUD/hooks'; import { ActionButton, @@ -62,6 +61,7 @@ import { } from '@superset-ui/core/components'; import { DatasourceModal, + withCertificationFields, GenericLink, ImportModal as ImportModelsModal, ModifiedInfo, @@ -496,6 +496,8 @@ const DatasetList: FunctionComponent = ({ const [datasetCurrentlyEditing, setDatasetCurrentlyEditing] = useState(null); + const [datasetCurrentlyEditingEtag, setDatasetCurrentlyEditingEtag] = + useState(); const [datasetCurrentlyDuplicating, setDatasetCurrentlyDuplicating] = useState(null); @@ -565,24 +567,11 @@ const DatasetList: FunctionComponent = ({ SupersetClient.get({ endpoint: `/api/v1/dataset/${id}`, }) - .then(({ json = {} }) => { - const addCertificationFields = json.result.columns.map( - (column: ColumnObject) => { - const { - certification: { - details = '', - certified_by: certifiedBy = '', - } = {}, - } = JSON.parse(column.extra || '{}') || {}; - return { - ...column, - certification_details: details || '', - certified_by: certifiedBy || '', - is_certified: details || certifiedBy, - }; - }, + .then(({ json = {}, response }) => { + setDatasetCurrentlyEditingEtag( + response.headers.get('ETag') ?? undefined, ); - json.result.columns = [...addCertificationFields]; + json.result.columns = withCertificationFields(json.result.columns); setDatasetCurrentlyEditing(json.result); }) .catch(() => { @@ -1524,6 +1513,7 @@ const DatasetList: FunctionComponent = ({ {datasetCurrentlyEditing && ( - + Optional optimistic-concurrency guard. Pass the ``ETag`` returned + by a prior read of this dataset; the update is rejected with 412 + if the dataset has changed since. requestBody: description: Dataset schema required: true @@ -618,6 +633,17 @@ class DatasetRestApi(SoftDeleteApiMixin, BaseSupersetModelRestApi): $ref: '#/components/responses/403' 404: $ref: '#/components/responses/404' + 412: + description: >- + The dataset changed since the version identified by the + request's ``If-Match`` header; the update was not applied. + content: + application/json: + schema: + type: object + properties: + message: + type: string 422: $ref: '#/components/responses/422' 500: @@ -634,10 +660,32 @@ class DatasetRestApi(SoftDeleteApiMixin, BaseSupersetModelRestApi): except ValidationError as error: return self.response_400(message=error.messages) + # Serialise conditional saves on this dataset: the guard below reads + # the live version, the command writes, and the two must not interleave + # with another request's. Only a conditional save pays for the lock; an + # unconditional PUT behaves exactly as it did before the guard existed. + if is_conditional_write(): + lock_entity_for_update(SqlaTable, pk) + # Live version identifiers before the update (empty + query-free when # ``ENABLE_VERSIONING_CAPTURE`` is off). old_info = current_entity_version_info(SqlaTable, pk) + try: + raise_for_stale_write(concurrency_token_from(old_info)) + except StaleEntityError: + return set_version_etag( + self.response( + 412, + message=_( + "The dataset was changed by another user or browser tab " + "after you opened it. Reopen it to pick up the latest " + "version, then reapply your changes." + ), + ), + concurrency_token_from(old_info), + ) + try: # Two commands, two commits, two Continuum transactions for an # ``override_columns`` save — deliberately NOT merged into one @@ -661,13 +709,13 @@ class DatasetRestApi(SoftDeleteApiMixin, BaseSupersetModelRestApi): new_info = current_entity_version_info( SqlaTable, changed_model.id, changed_model.uuid ) - etag_version_uuid = new_info.version_uuid + etag_version_uuid = concurrency_token_from(new_info) if override_columns: RefreshDatasetCommand(pk).run() # The ETag must reflect the entity's *current live* version, # which after the refresh is the refresh's transaction — # re-read it rather than reusing the pre-refresh uuid. - etag_version_uuid = current_entity_etag_uuid( + etag_version_uuid = entity_concurrency_token( SqlaTable, changed_model.id, changed_model.uuid ) response = self.response( @@ -1700,7 +1748,7 @@ class DatasetRestApi(SoftDeleteApiMixin, BaseSupersetModelRestApi): return set_version_etag( self.response(200, **response), - current_entity_etag_uuid(SqlaTable, table.id, table.uuid), + entity_concurrency_token(SqlaTable, table.id, table.uuid), ) @expose("//drill_info/", methods=("GET",)) diff --git a/superset/versioning/api_helpers.py b/superset/versioning/api_helpers.py index 9b717c7353e..6d01948dfee 100644 --- a/superset/versioning/api_helpers.py +++ b/superset/versioning/api_helpers.py @@ -72,6 +72,11 @@ class EntityVersionInfo: version: int | None = None transaction_id: int | None = None version_uuid: str | None = None + #: Resolved uuid of the entity itself, carried so callers that need a + #: concurrency token for an entity with no version rows yet don't have to + #: re-run the ``SELECT uuid`` this helper already issued. Not part of the + #: API response. + entity_uuid: UUID | None = None def _capture_enabled() -> bool: @@ -123,6 +128,7 @@ def current_entity_version_info( version=version, transaction_id=transaction_id, version_uuid=str(version_uuid) if version_uuid else None, + entity_uuid=entity_uuid, ) @@ -144,6 +150,77 @@ def current_entity_etag_uuid( return str(version_uuid) if version_uuid else None +# Sentinel Continuum transaction id for an entity that has no version rows +# yet. Continuum sequences start at 1, so it can never collide with a real +# one, and the derived uuid stops matching the moment the first version row +# lands — which is exactly the transition a concurrency guard must catch. +_UNVERSIONED_TRANSACTION_ID = 0 + + +def unversioned_entity_token(entity_uuid: UUID) -> str: + """Concurrency token for an entity Continuum hasn't versioned yet.""" + return str(VersionDAO.derive_version_uuid(entity_uuid, _UNVERSIONED_TRANSACTION_ID)) + + +def entity_concurrency_token( + model_cls: type[Model], + entity_id: int | None, + entity_uuid: UUID | None, +) -> str | None: + """Resolve the optimistic-concurrency validator for *entity*. + + Differs from :func:`current_entity_etag_uuid` in what it does for an + entity with no version rows: baseline rows are written lazily, on the + first update after the versioning migration, so a never-since-saved + entity has none. Reporting ``None`` there would leave the *first* + concurrent save on every such entity unguarded — the exact case a + two-tab race hits on a pristine entity. Those entities get a + deterministic unversioned token instead. + + ``None`` still means "no validator exists": capture is off, or the + entity is missing. + """ + if entity_id is None or entity_uuid is None or not _capture_enabled(): + return None + return current_entity_etag_uuid( + model_cls, entity_id, entity_uuid + ) or unversioned_entity_token(entity_uuid) + + +def lock_entity_for_update(model_cls: type[Model], entity_id: int | None) -> None: + """Row-lock *entity* so a conditional write's check and its update are atomic. + + ``If-Match`` is verified against a read taken before the update command + runs. Without a lock two overlapping requests can both read the same live + version, both pass the check, and then commit one after the other, + reintroducing the lost update the check exists to prevent. The lock is + held until the command commits, because both run in the same scoped + session. + + Renders no ``FOR UPDATE`` on SQLite, which serialises writers anyway. + """ + try: + # The PUT route declares ``/`` (a string segment), so a non-numeric + # id must not raise a SQL cast error ahead of the command's 404. + entity_id = int(entity_id) # type: ignore[arg-type] + except (TypeError, ValueError): + return + db.session.execute( + sa.select(model_cls.id).where(model_cls.id == entity_id).with_for_update() + ) + + +def concurrency_token_from(info: EntityVersionInfo) -> str | None: + """Concurrency token for an already-resolved :class:`EntityVersionInfo`. + + Lets a write endpoint reuse the pre-update version lookup it already + made rather than issuing a second one. + """ + if info.entity_uuid is None: + return None + return info.version_uuid or unversioned_entity_token(info.entity_uuid) + + # Maps the versioned model class name to the keyword argument # ``security_manager.raise_for_access`` expects for the per-resource # gate. Slice → ``chart=``, Dashboard → ``dashboard=``, SqlaTable → diff --git a/superset/versioning/etag.py b/superset/versioning/etag.py index 39afb56fb8a..38422cb03f9 100644 --- a/superset/versioning/etag.py +++ b/superset/versioning/etag.py @@ -22,6 +22,7 @@ from typing import TYPE_CHECKING from uuid import UUID import sqlalchemy as sa +from flask import request from flask_appbuilder import Model from superset.extensions import db @@ -76,3 +77,46 @@ def set_version_etag_by_uuid( response, VersionDAO.current_live_version_uuid(model_cls, entity_id, entity_uuid), ) + + +class StaleEntityError(Exception): + """The request's ``If-Match`` doesn't match the entity's live version.""" + + +def _entity_tag(tag: str) -> str: + """Strip the content-coding suffix ``Flask-Compress`` appends to ETags. + + A compressed response legitimately carries a different validator than the + identity one — Flask-Compress rewrites ``""`` to ``":zstd"`` + (see ``flask_compress``) — so a client replaying the ETag it read never + matches the raw version uuid. Version uuids contain no ``:``, so cutting + at the first one recovers the entity identity from either form. + """ + return tag.split(":", 1)[0] + + +def is_conditional_write() -> bool: + """Whether the request carries an ``If-Match`` precondition.""" + return bool(request.if_match) + + +def raise_for_stale_write(current_version_uuid: str | None) -> None: + """Enforce ``If-Match`` on a write request, if the client sent one. + + Clients that read an entity's ``ETag`` may replay it as ``If-Match`` on a + subsequent write to get optimistic concurrency: the write is rejected when + the entity moved on in the meantime, instead of silently clobbering + whatever landed in between. + + The condition is skipped — rather than failing closed — when the caller + has no validator to offer (``ENABLE_VERSIONING_CAPTURE`` off). Failing + closed there would block every conditional write on deployments running + without version capture, and those are no worse off than before they sent + the header. + """ + if_match = request.if_match + if not if_match or if_match.star_tag or current_version_uuid is None: + return + live = _entity_tag(str(current_version_uuid)) + if not any(_entity_tag(tag) == live for tag in if_match.as_set(True)): + raise StaleEntityError() diff --git a/tests/unit_tests/datasets/api_tests.py b/tests/unit_tests/datasets/api_tests.py index 047090def31..2d676c45870 100644 --- a/tests/unit_tests/datasets/api_tests.py +++ b/tests/unit_tests/datasets/api_tests.py @@ -217,6 +217,91 @@ def test_handle_filters_args_returns_request_scoped_filters( assert fresh_filters.get_joined_filters.call_count == 2 +def _create_dataset(name: str) -> Any: + from superset.connectors.sqla.models import SqlaTable + from superset.models.core import Database + + SqlaTable.metadata.create_all(db.session.get_bind()) + dataset = SqlaTable( + table_name=name, + database=Database(database_name=f"{name}_db", sqlalchemy_uri="sqlite://"), + ) + db.session.add(dataset) + db.session.flush() + return dataset + + +def test_put_dataset_rejects_stale_if_match( + session: Session, + client: Any, + full_api_access: None, +) -> None: + """ + A PUT carrying an ``If-Match`` from an older version is refused with 412. + """ + from superset.versioning.api_helpers import EntityVersionInfo + + dataset = _create_dataset("test_put_stale_if_match") + + with patch( + "superset.datasets.api.current_entity_version_info", + return_value=EntityVersionInfo( + version=1, + transaction_id=2, + version_uuid="new", + entity_uuid=dataset.uuid, + ), + ): + response = client.put( + f"/api/v1/dataset/{dataset.id}", + json={"description": "from a stale tab"}, + headers={"If-Match": '"old"'}, + ) + + assert response.status_code == 412 + assert response.headers["ETag"] == '"new"' + db.session.expire(dataset) + assert dataset.description is None + + +def test_put_dataset_guards_a_dataset_with_no_version_rows( + session: Session, + client: Any, + full_api_access: None, +) -> None: + """Baseline rows are written lazily on the first update, so a dataset that + has never been saved has no version rows — it must still be guarded, or + the first concurrent save on every pristine dataset goes unprotected. + """ + from superset.versioning.api_helpers import ( + EntityVersionInfo, + unversioned_entity_token, + ) + + dataset = _create_dataset("test_put_unversioned_guard") + entity_uuid = dataset.uuid + + with patch( + "superset.datasets.api.current_entity_version_info", + # A dataset that has since been versioned by another tab's save. + return_value=EntityVersionInfo( + version=0, + transaction_id=1, + version_uuid="written-by-the-other-tab", + entity_uuid=entity_uuid, + ), + ): + response = client.put( + f"/api/v1/dataset/{dataset.id}", + json={"description": "from the tab that opened first"}, + headers={"If-Match": f'"{unversioned_entity_token(entity_uuid)}"'}, + ) + + assert response.status_code == 412 + db.session.expire(dataset) + assert dataset.description is None + + def test_get_dataset_exposes_certification_metadata( session: Session, client: Any, diff --git a/tests/unit_tests/versioning/test_etag.py b/tests/unit_tests/versioning/test_etag.py new file mode 100644 index 00000000000..4968f7ffe51 --- /dev/null +++ b/tests/unit_tests/versioning/test_etag.py @@ -0,0 +1,109 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from uuid import UUID + +import pytest +from flask import Flask + +from superset.versioning.etag import raise_for_stale_write, StaleEntityError + +LIVE = "9f1f4c1e-0000-4000-8000-000000000001" +ENTITY = UUID("9f1f4c1e-0000-4000-8000-0000000000aa") + + +def _put(app: Flask, if_match: str | None): + headers = {"If-Match": if_match} if if_match is not None else {} + return app.test_request_context("/api/v1/dataset/1", method="PUT", headers=headers) + + +def test_no_if_match_header_passes(app: Flask) -> None: + with _put(app, None): + raise_for_stale_write(LIVE) + + +def test_matching_if_match_passes(app: Flask) -> None: + with _put(app, f'"{LIVE}"'): + raise_for_stale_write(LIVE) + + +def test_star_if_match_passes(app: Flask) -> None: + with _put(app, "*"): + raise_for_stale_write(LIVE) + + +def test_compressed_if_match_passes(app: Flask) -> None: + """Flask-Compress rewrites the ETag of a compressed response to + ``":"``; a client replaying that must still match.""" + with _put(app, f'"{LIVE}:zstd"'): + raise_for_stale_write(LIVE) + + +def test_compressed_stale_if_match_still_raises(app: Flask) -> None: + with _put(app, '"9f1f4c1e-0000-4000-8000-000000000002:gzip"'): + with pytest.raises(StaleEntityError): + raise_for_stale_write(LIVE) + + +def test_stale_if_match_raises(app: Flask) -> None: + with _put(app, '"9f1f4c1e-0000-4000-8000-000000000002"'): + with pytest.raises(StaleEntityError): + raise_for_stale_write(LIVE) + + +def test_if_match_list_containing_live_passes(app: Flask) -> None: + with _put(app, f'"9f1f4c1e-0000-4000-8000-000000000002", "{LIVE}"'): + raise_for_stale_write(LIVE) + + +def test_no_validator_available_passes(app: Flask) -> None: + """Version capture off (or no version rows yet) degrades to an + unconditional write rather than blocking every save.""" + with _put(app, f'"{LIVE}"'): + raise_for_stale_write(None) + + +def test_unversioned_token_is_stable_and_entity_specific() -> None: + """A not-yet-versioned entity still gets a validator, derived from its own + uuid so two such entities never share one.""" + from superset.versioning.api_helpers import unversioned_entity_token + + other = UUID("9f1f4c1e-0000-4000-8000-0000000000ff") + assert unversioned_entity_token(ENTITY) == unversioned_entity_token(ENTITY) + assert unversioned_entity_token(ENTITY) != unversioned_entity_token(other) + + +def test_unversioned_token_differs_from_first_real_version(app: Flask) -> None: + """The first version row must invalidate the unversioned token, or the + first concurrent save on a pristine entity would go unguarded.""" + from superset.daos.version import derive_version_uuid + from superset.versioning.api_helpers import unversioned_entity_token + + stale = unversioned_entity_token(ENTITY) + first_real = str(derive_version_uuid(ENTITY, 1)) + assert stale != first_real + with _put(app, f'"{stale}"'): + with pytest.raises(StaleEntityError): + raise_for_stale_write(first_real) + + +def test_unversioned_token_matches_while_still_unversioned(app: Flask) -> None: + from superset.versioning.api_helpers import unversioned_entity_token + + token = unversioned_entity_token(ENTITY) + with _put(app, f'"{token}"'): + raise_for_stale_write(token)