Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(security): Clean up session/commit logic #29381

Merged
merged 2 commits into from
Jun 26, 2024
Merged

chore(security): Clean up session/commit logic #29381

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
769f5f0
chore(security): Clean up session/commit logic
john-bodley Jun 26, 2024
dde1548
ruff
john-bodley Jun 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 7 additions & 18 deletions superset/security/manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1017,9 +1017,9 @@ def clean_perms(self) -> None:
== None, # noqa: E711
)
)
self.get_session.commit()
if deleted_count := pvms.delete():
logger.info("Deleted %i faulty permissions", deleted_count)
self.get_session.commit()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really sure what's going on here. The commit was after a SELECT as opposed to the DELETE.


def sync_role_definitions(self) -> None:
"""
Expand Down Expand Up @@ -1047,9 +1047,6 @@ def sync_role_definitions(self) -> None:
)

self.create_missing_perms()

# commit role and view menu updates
self.get_session.commit()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The create_missing_perms logic already commits.

self.clean_perms()

def _get_all_pvms(self) -> list[PermissionView]:
Expand Down Expand Up @@ -2446,31 +2443,26 @@ def get_rls_filters(self, table: "BaseDatasource") -> list[SqlaQuery]:

user_roles = [role.id for role in self.get_user_roles(g.user)]
regular_filter_roles = (
self.get_session()
.query(RLSFilterRoles.c.rls_filter_id)
self.get_session.query(RLSFilterRoles.c.rls_filter_id)
.join(RowLevelSecurityFilter)
.filter(
RowLevelSecurityFilter.filter_type == RowLevelSecurityFilterType.REGULAR
)
.filter(RLSFilterRoles.c.role_id.in_(user_roles))
)
base_filter_roles = (
self.get_session()
.query(RLSFilterRoles.c.rls_filter_id)
self.get_session.query(RLSFilterRoles.c.rls_filter_id)
.join(RowLevelSecurityFilter)
.filter(
RowLevelSecurityFilter.filter_type == RowLevelSecurityFilterType.BASE
)
.filter(RLSFilterRoles.c.role_id.in_(user_roles))
)
filter_tables = (
self.get_session()
.query(RLSFilterTables.c.rls_filter_id)
.filter(RLSFilterTables.c.table_id == table.id)
filter_tables = self.get_session.query(RLSFilterTables.c.rls_filter_id).filter(
RLSFilterTables.c.table_id == table.id
)
query = (
self.get_session()
.query(
self.get_session.query(
RowLevelSecurityFilter.id,
RowLevelSecurityFilter.group_key,
RowLevelSecurityFilter.clause,
Expand Down Expand Up @@ -2673,12 +2665,9 @@ def raise_for_ownership(self, resource: Model) -> None:
:raises SupersetSecurityException: If the current user is not an owner
"""

# pylint: disable=import-outside-toplevel
from superset import db

if self.is_admin():
return
orig_resource = db.session.query(resource.__class__).get(resource.id)
orig_resource = self.get_session.query(resource.__class__).get(resource.id)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use the Flask-AppBuilder session everywhere for consistency. Note that db.session and self.get_session are the same Flask-SQLAlchemy session so the logic remains unchanged.

owners = orig_resource.owners if hasattr(orig_resource, "owners") else []

if g.user.is_anonymous or g.user not in owners:
Expand Down
Loading