mikebridge commented on PR #39977: URL: https://github.com/apache/superset/pull/39977#issuecomment-4442718263
**Follow-up: moved the per-request bypass into `raise_for_ownership` itself.** In `1fb3aa4851`, I refactored the soft-delete↔ownership-check coupling so the per-request bypass lives inside `security_manager.raise_for_ownership` instead of being wrapped at the caller site. **Before** (what landed in `44e1d1710b` / `c499e318fe`): - `BaseRestoreCommand.validate` wrapped its `raise_for_ownership(model)` call with a per-request `g.skip_visibility_filter = True` (try/finally, restore previous value). - The bypass was visible at every restore-command site that needed it. - Dedicated unit tests pinned the bypass contract. **Now**: - `raise_for_ownership` scopes the bypass around its own internal re-query and restores the previous value before doing the actual ownership check (which still reads `g.user`). - `BaseRestoreCommand.validate` is back to a plain `raise_for_ownership(model)` call. - The bypass becomes a tightly-scoped internal mechanism of `raise_for_ownership` rather than a caller responsibility. Diff: **+28, −160** in this branch. The 5-line addition to `security/manager.py` is a `try/finally` around the existing re-query plus a local `SKIP_VISIBILITY_FILTER` import. Why this is better than the previous approach: 1. **Single point of soft-delete↔ownership coupling.** Every caller of `raise_for_ownership` — restore, delete, update, refresh, FAB `pre_delete`, etc. — now gets the correct behavior automatically. If a future caller ever needs to ownership-check a soft-deleted resource, it just works. 2. **Preserves the defense-in-depth re-query** (form-data tampering defense from the original 2016 PR #507) — we don't lose that for non-soft-delete callers. 3. **Eliminates the dedicated bypass-contract tests** (`tests/unit_tests/commands/test_restore.py`, ~120 lines) — there's no caller-side mechanism to test anymore. 4. **Supersedes sc-106314** (context-manager refactor to consolidate boilerplate) — there's only one in-tree caller of the per-request flag now and it's internal. 5. **Reframes sc-106319** (remove the re-query entirely) — that ticket was driven primarily by the soft-delete coupling; with the coupling resolved here, it drops from "correctness fix" to "optional perf optimization." Bundling caveat: this *is* a touch of `security/manager.py` inside a soft-delete PR. The change is small (12 lines net) and tightly justified — the soft-delete listener and `raise_for_ownership` are now consistent. Happy to discuss if you'd prefer this lands as its own stacked PR before sc-106188. -- 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]
