fix: Refactor ownership checks

[john-bodley]

SUMMARY

Yak shaving at its finest. Originally I drafted this PR to fix an issue with inconsistencies between ownership checks as described in this Slack thread. BTW this PR does not address said issue—I believe we only need a migration for legacy charts/datasets.

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 (removing passing g.user all around the place) 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, i.e., whether a user is an admin or owner.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • 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

[codecov]

Codecov Report

Merging #20499 (844a8b0) into master (f910958) will decrease coverage by 0.23%.
The diff coverage is 88.58%.

❗ Current head 844a8b0 differs from pull request most recent head 823b9d3. Consider uploading reports for the commit 823b9d3 to get more accurate results

@@            Coverage Diff             @@
##           master   #20499      +/-   ##
==========================================
- Coverage   66.82%   66.59%   -0.24%     
==========================================
  Files        1752     1751       -1     
  Lines       65620    65504     -116     
  Branches     6938     6940       +2     
==========================================
- Hits        43853    43620     -233     
- Misses      20007    20122     +115     
- Partials     1760     1762       +2     

Flag	Coverage Δ
hive	?
mysql	82.33% <93.37%> (-0.06%)	⬇️
postgres	82.40% <93.37%> (-0.06%)	⬇️
presto	?
python	82.48% <93.37%> (-0.41%)	⬇️
sqlite	82.19% <93.37%> (-0.06%)	⬇️
unit	?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...frontend/src/components/ImportModal/ErrorAlert.tsx	33.33% <ø> (ø)
superset-frontend/src/views/CRUD/hooks.ts	46.56% <ø> (ø)
superset/dashboards/permalink/commands/create.py	88.88% <ø> (-0.77%)	⬇️
superset/datasets/dao.py	94.11% <ø> (+0.54%)	⬆️
superset/explore/form_data/commands/get.py	90.90% <ø> (-0.27%)	⬇️
superset/explore/form_data/commands/parameters.py	100.00% <ø> (ø)
superset/temporary_cache/commands/parameters.py	100.00% <ø> (ø)
superset/views/base.py	75.36% <ø> (-1.36%)	⬇️
superset/views/dashboard/views.py	66.27% <0.00%> (+0.76%)	⬆️
superset/views/utils.py	80.84% <ø> (-0.22%)	⬇️
... and 103 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f910958...823b9d3. Read the comment docs.

[eschutho]

I was comparing this method to the old one which pulled out the original object from the db rather than use the one that was passed in. Could the resource here come from user input rather than the db? I'm thinking of the scenario where a non-owner might try to grant themselves ownership by adding themselves to the list of owners on a resource. Since it's not making a db call to the original resource could we be allowing them to define their own set of owners? One example perhaps is the pre_update hook for the SliceModelView.

[john-bodley]

@eschutho apologies for the delay in responding as I was away on PTO. I think the point you raise is a good call and I'll revert the logic.

As a side note I think a lot of the pre_update logic is possibly obsolete with the DAO model—which merely complicates the code base—and we likely could deprecate some of that. I'm planning on authoring another PR to address said issue.

[john-bodley]

@eschutho I've addressed your question and thus this should be ready for re-review. @dpgaspar would you mind also reviewing said change given that it updates the DAO logic that you and @villebro authored.

[ktmud]

lgtm

[dpgaspar]

This does shave the Yak a bit more ;)

On the other hand we will loose some future flexibility if we would ever wanted to use DAO or commands with different users, celery context for example?

[dpgaspar]

nit: db.session.query(resource.__class__).filter_by(id=resource.id).one_or_none() or db.session.query(resource.__class__).get(resource.id)

[john-bodley]

@dpgaspar I agree with the one_or_none. This was a copy-and-paste from check_ownership but I'll make the update.

[john-bodley]

@dpgaspar

On the other hand we will loose some future flexibility if we would ever wanted to use DAO or commands with different users, celery context for example?

I don't disagree in terms of lose of flexibility, however the g.user is so engrained in the entire application workflow that we would need a very major refactor in order to support this. Per the PR description the actor is somewhat misleading given that some checks are in relation to the specified actor whereas others (like is_user_admin et al.) are based on g.user.