fix: dataset update permission out of sync #25043

zephyring · 2023-08-21T16:17:23Z

SUMMARY

The perm and schema_perm attributes for dataset model is out of sync due to unreliably using sqlalchemy get_history method where the history state depends on whether the session has flushed or not.
Previously we have logic in before_update and after_update and permission sync logic in after_update will not be deterministic due to session flush.

This PR moves permission sync logic into before_update and directly using connection.execute to check current db state instead of relying on session state to make sure we can detect actual db/table/schema changes to trigger permission update.

It also clean up shadow_dataset_write logic out as we no longer support it but the database attribute is still not lazily loaded when dataset.database attribute is accessed. Thus I leave the load_database method there still.

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

zephyring · 2023-08-21T16:20:03Z

superset/views/base.py

@@ -284,34 +282,6 @@ def wraps(self: "BaseSupersetView", *args: Any, **kwargs: Any) -> FlaskResponse:
    return functools.update_wrapper(wraps, f)


-def validate_sqlatable(table: models.SqlaTable) -> None:


this method is not used anywhere, thus removing

zephyring · 2023-08-21T16:26:15Z

superset/connectors/sqla/models.py

-        else:
-            return None
-
-        if not DatasetDAO.validate_uniqueness(


this check has already been made in dataset update command and proper domain exception thrown there, thus removing this extra check here.
We also have unique constraints at db layer for (db, schema, table) tuple

craig-rueda

No tests?

jfrag1

LGTM! I still think it would be best to eventually move this logic into the DAO layer, but this is a great way to get things working properly consistently with sqlalchemy events.

To @craig-rueda's point, there are several existing tests prefixed test_after_update_dataset that test this functionality. I'm not sure any new tests are needed since this PR is pretty much just a refactor/improvement of this existing functionality, but it would probably be good to rename them to test_before_update_dataset to be consistent with naming

zephyring · 2023-08-21T22:53:55Z

LGTM! I still think it would be best to eventually move this logic into the DAO layer, but this is a great way to get things working properly consistently with sqlalchemy events.

To @craig-rueda's point, there are several existing tests prefixed test_after_update_dataset that test this functionality. I'm not sure any new tests are needed since this PR is pretty much just a refactor/improvement of this existing functionality, but it would probably be good to rename them to test_before_update_dataset to be consistent with naming

agreed. Consolidating to domain's command layer is likely the better way of handling side effect like permission update, as there's existing patterns here.
But I would defer the problem to a larger refactoring until we all agree upon ideal state/target state of how permissions system should work in Superset.

This PR is purely continuing with existing patterns but more on a fix side of thing.

john-bodley · 2023-08-22T05:11:58Z

superset/connectors/sqla/models.py

        if self.database_id and (
            not self.database or self.database.id != self.database_id
        ):
+            session = inspect(self).session


Thanks for ensuring we're using the same session.

zephyring · 2023-08-22T05:50:52Z

superset/daos/base.py

+        copy.id = model.id
+        # create a copy of the model here
+        # so it doesn't trigger before_update/after_update sqla listener each time
+        # we set a property


this is an important change to the base update method.
By creating a out of session copy of the original model instance, we can set attributes on the copy without worrying the before/after update sqla listener will be triggered for each property.

The listener will only be triggered when session is committed

there's one test test_run_sync_query_dont_exist that I cannot get passed by with this baseDao.update method change(using a copy model instance out of session and merge)
We probably need more thoughts on how to reduce number of events triggered on each property update, thus reverting here

eschutho

Thanks @zephyring!

dpgaspar

nice! just one comment, would be nice to have a test also to assert what changed on this PR

dpgaspar · 2023-08-24T15:50:35Z

superset/security/manager.py


        # When database name changes
-        if history_database.has_changes() and history_database.deleted:
+        if current_db_id != target.database_id:


the dataset permission is a string that includes the database_id, table_name and schema if those change we have to update the permission string also

forget it, those cases are covered on the next if statements

…s have changes

… sure sqla before/after update listener is only called once

…c method since we already have create. If we want upsert, we can create it separately.

This reverts commit 8c8098ee4bbe5b3d45d7fcf2eb5597cf0b74bbe0.

…ve atomic method since we already have create. If we want upsert, we can create it separately." This reverts commit f4c45680b2e0073afd339295e80388ab801bef1e.

This reverts commit 5380ef80f1790711abd389e9c9e82f937d191b88.

… to make sure sqla before/after update listener is only called once" This reverts commit c69412684368e6aadfc456a5c7100df33a03e3d2.

pull-request-size bot added the size/L label Aug 21, 2023

zephyring commented Aug 21, 2023

View reviewed changes

john-bodley requested review from eschutho, john-bodley and villebro August 21, 2023 16:52

craig-rueda reviewed Aug 21, 2023

View reviewed changes

jfrag1 approved these changes Aug 21, 2023

View reviewed changes

john-bodley reviewed Aug 22, 2023

View reviewed changes

zephyring commented Aug 22, 2023

View reviewed changes

zephyring force-pushed the zef/ch73388/fixing_dataset_update_perm branch from cc1ec80 to f4c4568 Compare August 22, 2023 18:05

eschutho approved these changes Aug 22, 2023

View reviewed changes

zephyring force-pushed the zef/ch73388/fixing_dataset_update_perm branch from ed4d9f8 to 6dbee2f Compare August 22, 2023 23:48

dpgaspar reviewed Aug 24, 2023

View reviewed changes

zef added 15 commits August 24, 2023 12:00

fix: refactor to not use sqlalchemy get_history to determine if field…

525a7d5

…s have changes

minor

2591ce1

formatting and logging

9073cf5

reverting back to using history

49ad621

first pass fixing perm update and removed shadow_write code

96d0861

lint

85a22b6

lint

01ada73

lint

12e7743

adding back database attribute load for tables

7984bb7

when updating, using session merge from a copy model instance to make…

ed94a03

… sure sqla before/after update listener is only called once

cautious check on session.merge copy model

6de1ef4

making item required args for Dao.update method. Better to have atomi…

e364f5f

…c method since we already have create. If we want upsert, we can create it separately.

lint

3680835

Revert "lint"

48d085c

This reverts commit 8c8098ee4bbe5b3d45d7fcf2eb5597cf0b74bbe0.

Revert "making item required args for Dao.update method. Better to ha…

4943320

…ve atomic method since we already have create. If we want upsert, we can create it separately." This reverts commit f4c45680b2e0073afd339295e80388ab801bef1e.

zef added 2 commits August 24, 2023 12:00

Revert "cautious check on session.merge copy model"

7827767

This reverts commit 5380ef80f1790711abd389e9c9e82f937d191b88.

Revert "when updating, using session merge from a copy model instance…

225be2b

… to make sure sqla before/after update listener is only called once" This reverts commit c69412684368e6aadfc456a5c7100df33a03e3d2.

zephyring force-pushed the zef/ch73388/fixing_dataset_update_perm branch from 6dbee2f to 225be2b Compare August 24, 2023 19:02

dpgaspar approved these changes Aug 25, 2023

View reviewed changes

eschutho merged commit 5168475 into apache:master Aug 25, 2023
29 checks passed

eschutho deleted the zef/ch73388/fixing_dataset_update_perm branch August 25, 2023 18:34

michael-s-molina added the v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch label Aug 29, 2023

michael-s-molina pushed a commit that referenced this pull request Aug 30, 2023

fix: dataset update permission out of sync (#25043)

7d5cd72

cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024

fix: dataset update permission out of sync (apache#25043)

461127d

mistercrunch added 🍒 3.0.0 🍒 3.0.1 🍒 3.0.2 🍒 3.0.3 🍒 3.0.4 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.1.0 labels Mar 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: dataset update permission out of sync #25043

fix: dataset update permission out of sync #25043

zephyring commented Aug 21, 2023

zephyring Aug 21, 2023

zephyring Aug 21, 2023

craig-rueda left a comment

jfrag1 left a comment

zephyring commented Aug 21, 2023

john-bodley Aug 22, 2023

zephyring Aug 22, 2023

zephyring Aug 22, 2023

eschutho left a comment

dpgaspar left a comment

dpgaspar Aug 24, 2023

dpgaspar Aug 25, 2023

		@@ -284,34 +282,6 @@ def wraps(self: "BaseSupersetView", args: Any, *kwargs: Any) -> FlaskResponse:
		return functools.update_wrapper(wraps, f)


		def validate_sqlatable(table: models.SqlaTable) -> None:

fix: dataset update permission out of sync #25043

fix: dataset update permission out of sync #25043

Conversation

zephyring commented Aug 21, 2023

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

zephyring Aug 21, 2023

Choose a reason for hiding this comment

zephyring Aug 21, 2023

Choose a reason for hiding this comment

craig-rueda left a comment

Choose a reason for hiding this comment

jfrag1 left a comment

Choose a reason for hiding this comment

zephyring commented Aug 21, 2023

john-bodley Aug 22, 2023

Choose a reason for hiding this comment

zephyring Aug 22, 2023

Choose a reason for hiding this comment

zephyring Aug 22, 2023

Choose a reason for hiding this comment

eschutho left a comment

Choose a reason for hiding this comment

dpgaspar left a comment

Choose a reason for hiding this comment

dpgaspar Aug 24, 2023

Choose a reason for hiding this comment

dpgaspar Aug 25, 2023

Choose a reason for hiding this comment