john-bodley opened a new pull request, #20499: URL: https://github.com/apache/superset/pull/20499
<!--- Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/ Example: fix(dashboard): load charts correctly --> ### SUMMARY Yak shaving at its finest. Originally I drafted this PR to fix an issue with inconsistencies between ownership checks as described in [this](https://apache-superset.slack.com/archives/G013HAE6Y0K/p1655921947616779) Slack thread. Things quickly spiraled out of control as I realized that the DAO `check_access` method checks whether the actor is an owner alongside a check whether the current user is an admin—this clearly is wrong, i.e., both checks should be for the same user. Digging further I realize that the actor is _always_ `g.user` and thus continuing my crusade to simplify the whole user space I opted to fully deprecate the actor concept—which touched a huge swatch of files. The main reason being is that Superset is really only configured to work with the logged in user and overrides should be done—if needed—via the `override_user` context manager. The TL;DR of this PR: 1. Removes the `actor` construct across all the DAO models. 2. Refactors `check_ownership` which either raised or returned a `bool` into the `raise_for_owernship` and `is_owner` methods—the later being a wrapper of the former. This helps to improve code readability, i.e., it is apparent that `raise_for_owernship` raises. These methods have been moved to the security manager to allow for customization. 3. Removes duplicate methods which person the same check, i.e., whether a user is an admin. ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF <!--- Skip this if not applicable --> ### TESTING INSTRUCTIONS <!--- Required! What steps can be taken to manually verify the changes? --> ### ADDITIONAL INFORMATION <!--- Check any relevant boxes with "x" --> <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue --> - [ ] Has associated issue: - [ ] Required feature flags: - [ ] Changes UI - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351)) - [ ] Migration is atomic, supports rollback & is backwards-compatible - [ ] Confirm DB migration upgrade and downgrade tested - [ ] Runtime estimates and downtime expectations provided - [ ] Introduces new feature or API - [ ] Removes existing feature or API -- 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]
