shelkesays opened a new issue, #41623:
URL: https://github.com/apache/superset/issues/41623
## Bug description
The three many-to-many owner association tables (`dashboard_user`,
`slice_user`, `sqlatable_user`) are declared without a
`UniqueConstraint(<resource_id>, user_id)`. Because of this, the same user can
end up inserted multiple times as an owner of the same resource. When the DAO
later tries to remove that owner (e.g. via `PUT /api/v1/dashboard/{id}` with an
`owners` list that excludes that user), SQLAlchemy's relationship diff assumes
a 1:1 mapping and fails silently. The caller sees HTTP 422 with the generic
message `"Dashboard could not be updated."` (and the equivalents for
slices/datasets).
Worth calling out: the sibling `dashboard_slices` table, declared right next
to `dashboard_user` in `superset/models/dashboard.py`, does have
`UniqueConstraint("dashboard_id", "slice_id")`. So the pattern was clearly
known, it just wasn't applied to the ownership tables.
## Affected code
### `superset/models/dashboard.py`, no unique constraint
```python
dashboard_user = Table(
"dashboard_user",
metadata,
Column("id", Integer, primary_key=True),
Column("user_id", Integer, ForeignKey("ab_user.id", ondelete="CASCADE")),
Column("dashboard_id", Integer, ForeignKey("dashboards.id",
ondelete="CASCADE")),
# missing: UniqueConstraint("dashboard_id", "user_id"),
)
```
Compare to `dashboard_slices` a few lines above in the same file, which
correctly has:
```python
UniqueConstraint("dashboard_id", "slice_id"),
```
### `superset/models/slice.py`, no unique constraint
```python
slice_user = Table(
"slice_user",
metadata,
Column("id", Integer, primary_key=True),
Column("user_id", Integer, ForeignKey("ab_user.id", ondelete="CASCADE")),
Column("slice_id", Integer, ForeignKey("slices.id", ondelete="CASCADE")),
# missing: UniqueConstraint("slice_id", "user_id"),
)
```
### `superset/connectors/sqla/models.py`, no unique constraint
```python
sqlatable_user = DBTable(
"sqlatable_user",
metadata,
Column("id", Integer, primary_key=True),
Column("user_id", Integer, ForeignKey("ab_user.id", ondelete="CASCADE")),
Column("table_id", Integer, ForeignKey("tables.id", ondelete="CASCADE")),
# missing: UniqueConstraint("table_id", "user_id"),
)
```
## How I hit this
Reproduced on our 6.1.0 production instance (metadata DB: Postgres 14). We
were offboarding a user and the API refused to remove them as owner from a
specific dashboard. Every other dashboard worked. On inspection:
```sql
SELECT id, user_id, dashboard_id
FROM public.dashboard_user
WHERE dashboard_id = 121 AND user_id = 18;
id | user_id | dashboard_id
-----+---------+--------------
258 | 18 | 121
260 | 18 | 121
```
Two rows for the same `(dashboard_id, user_id)` pair. Once I deleted one of
them, the API PUT worked immediately.
## How duplicates can enter the table (via normal code paths, no direct SQL
required)
Before anyone asks "why would you ever insert directly via SQL", here are
the code paths I found where the API/UI itself can put a duplicate in:
### 1. The API PUT accepts duplicate ids in `owners` and does not dedupe
The Marshmallow schema for the dashboard `owners` field is:
```python
# superset/dashboards/schemas.py
owners = fields.List(fields.Nested(UserSchema(exclude=["username"])))
```
No `unique=True`, no length/uniqueness validator. So `PUT
/api/v1/dashboard/<id>` with a body like `{"owners": [17, 17, 18]}` passes
validation.
That list flows into `populate_owner_list` in `superset/commands/utils.py`:
```python
def populate_owner_list(owner_ids, default_to_user):
owner_ids = owner_ids or []
owners = []
...
for owner_id in owner_ids:
owner = security_manager.get_user_by_id(owner_id)
if not owner:
raise OwnersNotFoundValidationError()
owners.append(owner) # no dedup
return owners
```
The loop appends every id, unfiltered. Result: `dashboard.owners =
[User(17), User(17), User(18)]`. Without a unique constraint on
`dashboard_user`, both rows get inserted at flush. Same story for slices and
datasets (they use the same helper). Same for `populate_owners` on create.
### 2. The import path assigns owners directly from the payload
```python
# superset/connectors/sqla/models.py:701
self.owners = obj.get("owners", [])
```
If an imported YAML/ZIP has duplicate owner entries, they land in the
association table unfiltered.
### 3. `compute_owner_list` re-propagates any existing corruption
```python
# superset/commands/utils.py
owners_ids = (
[owner.id for owner in current_owners] if new_owners is None else
new_owners
)
```
If `current_owners` already contains duplicates from an earlier bad write,
every subsequent PUT (even one that does not touch `owners` in the payload)
rebuilds the list from those duplicated ids and reassigns them. The corruption
is sticky.
### 4. Concurrent PUTs
Two simultaneous PUTs both computing "current + new owner" and appending the
same user, with no DB constraint to reject the second insert. Not the most
common cause but possible.
## Reproduction
The cleanest repro that does not require any DB access at all:
```bash
# As any user who can PUT a dashboard, send a duplicate owner id
curl -X PUT "$SUPERSET_URL/api/v1/dashboard/<dashboard_id>" \
-H "Content-Type: application/json" \
-H "Cookie: $SUPERSET_COOKIE" \
-H "X-CSRFToken: $CSRF" \
-d '{"owners": [17, 17]}'
```
Verify in the metadata DB:
```sql
SELECT id, user_id, dashboard_id
FROM public.dashboard_user
WHERE dashboard_id = <dashboard_id> AND user_id = 17;
-- Two rows.
```
Now try to remove that user via the API:
```bash
curl -X PUT "$SUPERSET_URL/api/v1/dashboard/<dashboard_id>" \
-H "Content-Type: application/json" \
-H "Cookie: $SUPERSET_COOKIE" \
-H "X-CSRFToken: $CSRF" \
-d '{"owners": [<other_owner_ids_without_17>]}'
```
**Actual response:**
```
HTTP 422
{"message":"Dashboard could not be updated."}
```
**Expected response:**
```
HTTP 200 with the updated owners list.
```
(Same repro works for `/api/v1/chart/<id>` against `slice_user`, and for
`/api/v1/dataset/<id>` against `sqlatable_user`.)
## Root cause
The dashboard PUT command loads `dashboard.owners`, which SQLAlchemy dedupes
to a single `User` object. It then computes the diff against the submitted
owners list and tries to `session.remove(user)` from the relationship.
SQLAlchemy issues `DELETE FROM dashboard_user WHERE dashboard_id = ? AND
user_id = ?`, which affects both duplicate rows. The subsequent relationship
refresh finds a state that does not match its diff calculation and raises an
ORM error. The command layer catches it and re-raises as
`DashboardUpdateFailedError`, which surfaces as the generic 422.
If `UniqueConstraint("dashboard_id", "user_id")` had been on
`dashboard_user` (and the equivalents on `slice_user` and `sqlatable_user`),
the duplicate insert would never have succeeded in the first place, and any
future attempt would fail with a clear constraint violation instead of an
opaque downstream ORM error.
## Business impact
1. Any resource with a duplicate row in an ownership table becomes
unremovable from the affected user until someone runs a manual DB cleanup. This
blocks user offboarding, which is what caught us.
2. The error message ("Dashboard could not be updated.") gives no signal
that the underlying metadata table is corrupt. Only DB level inspection reveals
the duplicate. We spent time chasing wrong theories before running the SELECT.
3. Since the schema does not enforce uniqueness AND the command layer does
not dedupe, corruption can accumulate silently just from normal API traffic (a
scripted client that sends the same id twice, a UI double click, an import with
duplicated owner entries).
## Proposed fix (layered, all four layers needed)
A single-layer fix leaves gaps. The right shape is layered, and each layer
catches a failure mode the others do not:
### Layer 1: API schema, reject the malformed request cleanly
Add a Marshmallow validator on the `owners` field in dashboard, chart, and
dataset schemas that rejects duplicate ids with a clear 400. Something like:
```python
def _no_duplicate_owner_ids(value):
ids = [o["id"] for o in value if isinstance(o, dict) and "id" in o]
if len(ids) != len(set(ids)):
raise ValidationError("owners contains duplicate user ids")
owners = fields.List(
fields.Nested(UserSchema(exclude=["username"])),
validate=_no_duplicate_owner_ids,
)
```
So `{"owners": [17, 17]}` returns a 400 with a message the client can act
on, instead of silently going through.
### Layer 2: Command layer, dedupe so callers other than the REST API are
covered
The import path assigns `self.owners = obj.get("owners", [])` directly
(`superset/connectors/sqla/models.py:701`), so validator-only defence would
miss imports. Dedupe once at the top of `populate_owner_list` in
`superset/commands/utils.py`:
```python
def populate_owner_list(owner_ids, default_to_user):
owner_ids = list(dict.fromkeys(owner_ids or [])) # dedupe, preserve
order
...
```
This covers create, update, and any future caller of the helper.
### Layer 3: Alembic migration, dedupe existing corrupted rows
Any deployment that already has duplicates in `dashboard_user` /
`slice_user` / `sqlatable_user` (like ours) needs those cleaned before a unique
constraint can be added, otherwise `ALTER TABLE` blows up. Keep the row with
the lowest `id`:
```sql
DELETE FROM dashboard_user a USING dashboard_user b
WHERE a.dashboard_id = b.dashboard_id
AND a.user_id = b.user_id
AND a.id > b.id;
```
Repeat for `slice_user` (on `slice_id, user_id`) and `sqlatable_user` (on
`table_id, user_id`). Handle MySQL and SQLite dialect quirks in the same
migration.
### Layer 4: DB constraint, safety net so nothing slips past the first three
Add the composite unique constraint at the SQLAlchemy Table definition for
each table, so the schema itself refuses to store a duplicate:
```python
UniqueConstraint("dashboard_id", "user_id"), # in dashboard_user
UniqueConstraint("slice_id", "user_id"), # in slice_user
UniqueConstraint("table_id", "user_id"), # in sqlatable_user
```
This catches raw session inserts, future refactors, race conditions on
concurrent PUTs, and any code path that misses the command-layer helper.
### Why all four
| Layer | Catches | Skipping it would leave... |
|---|---|---|
| Marshmallow validator | Malformed API requests, gives a clean 400 | Ugly
500 / IntegrityError bubbling out of the DB layer when the constraint rejects
it |
| `populate_owner_list` dedupe | Import path, and any non-REST caller of the
helper | Import path corruption still possible |
| Alembic dedupe migration | Rows already in production DBs | The constraint
ALTER blows up on upgrade for any deployment with existing duplicates |
| DB unique constraint | Raw session inserts, race conditions, future
refactors that forget to dedupe | The bug keeps recurring the moment someone
bypasses the command layer |
Happy to open a PR with all four.
## Environment
- Apache Superset **6.1.0** (production incident)
- Also verified on `apache/superset` upstream **master** at commit
`7cc7e9f6e3` (top of tree at time of filing); all three tables still lack the
constraint.
- Metadata DB: **Postgres 14**
- Verified table DDLs directly from the metadata DB, no unique constraint
present.
## Additional context
`dashboard_slices` (dashboard to slice association, in the same
`superset/models/dashboard.py`) has `UniqueConstraint("dashboard_id",
"slice_id")`. The pattern was already there for at least some association
tables, the ownership tables were just missed.
**Prior art:** an in-flight branch
[`sc-107283-versioning-activity-view`](https://github.com/apache/superset/tree/sc-107283-versioning-activity-view)
already tackles Layers 3 and 4 as part of a broader versioning epic. The
relevant chain is 10 commits tagged `sc-105349`, headlined by
[`f537472291`](https://github.com/apache/superset/commit/f537472291568216498b40aa25ff760c513a7db0)
("refactor(db): composite PK on M2M association tables"), followed by 9
follow-ups covering MySQL/SQLite/Postgres migration quirks and review feedback.
Together they:
- Replace the synthetic `id` PK with a composite `PRIMARY KEY (fk1, fk2)` on
all eight pure-junction tables (`dashboard_user`, `slice_user`,
`sqlatable_user`, `dashboard_slices`, `dashboard_roles`,
`report_schedule_user`, `rls_filter_roles`, `rls_filter_tables`).
- Ship an Alembic migration
`2bee73611e32_composite_pk_association_tables.py` with per-dialect dedupe
(Postgres, MySQL ERROR 1093 subquery workaround, SQLite anonymous-constraint
reflection).
- Add 8 duplicate-rejection unit tests, 8 schema-shape assertions, and a
round-trip / idempotency test.
The chain is **not** merged to master yet (`git branch -r --contains
f537472291` shows only the sc-107283 branch). Files touched across the entire
10-commit chain are limited to `UPDATING.md`, the four ORM `Table()`
definitions, the Alembic migration, the three test files, and a
docker-compose/MySQL init tweak.
What the chain does **not** cover, and what still needs the Layer 1 and
Layer 2 fixes from this issue (verified by inspecting the file list of every
commit in the chain, zero touches to `commands/`, `schemas.py`, or any
`api.py`):
- **No Marshmallow validator** on the `owners` field in
dashboard/chart/dataset schemas. `{"owners": [17, 17]}` would still be accepted
at the API and would surface later as a bare Postgres IntegrityError to the
client, instead of a clean 400.
- **No dedupe in `populate_owner_list`** (`superset/commands/utils.py`). Any
non-REST caller of that helper still forwards duplicates unchanged.
- **No change to the import path**
(`superset/connectors/sqla/models.py:701`, `self.owners = obj.get("owners",
[])`). Import of a YAML/ZIP with duplicate owner entries would fail hard on the
DB constraint mid-transaction, instead of validating and rejecting cleanly.
If maintainers want to build on the sc-107283 chain rather than start from
scratch, Layers 1 and 2 could be added on top as a small follow-up PR. Happy to
open the follow-up either way.
## Checklist
- [x] I have searched Superset docs and Slack and didn't find a solution to
my problem.
- [x] I have searched the GitHub issue tracker and didn't find a similar bug
report.
- [x] I have checked Superset's logs for errors and if I found a relevant
Python stacktrace, I included it here as text in the "additional context"
section.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
