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

[security] Moving set/merge perm to security manager #5684

Merged
merged 1 commit into from
Aug 22, 2018
Merged

[security] Moving set/merge perm to security manager #5684

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 3 additions & 3 deletions superset/connectors/druid/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@
from superset.connectors.base.models import BaseColumn, BaseDatasource, BaseMetric
from superset.exceptions import MetricPermException, SupersetException
from superset.models.helpers import (
AuditMixinNullable, ImportMixin, QueryResult, set_perm,
AuditMixinNullable, ImportMixin, QueryResult,
)
from superset.utils import (
DimSelector, DTTM_ALIAS, flasher,
Expand Down Expand Up @@ -1601,5 +1601,5 @@ def external_metadata(self):
]


sa.event.listen(DruidDatasource, 'after_insert', set_perm)
sa.event.listen(DruidDatasource, 'after_update', set_perm)
sa.event.listen(DruidDatasource, 'after_insert', security_manager.set_perm)
sa.event.listen(DruidDatasource, 'after_update', security_manager.set_perm)
5 changes: 2 additions & 3 deletions superset/connectors/sqla/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
from superset.models.annotations import Annotation
from superset.models.core import Database
from superset.models.helpers import QueryResult
from superset.models.helpers import set_perm
from superset.utils import DTTM_ALIAS, QueryStatus

config = app.config
Expand Down Expand Up @@ -892,5 +891,5 @@ def default_query(qry):
return qry.filter_by(is_sqllab_view=False)


sa.event.listen(SqlaTable, 'after_insert', set_perm)
sa.event.listen(SqlaTable, 'after_update', set_perm)
sa.event.listen(SqlaTable, 'after_insert', security_manager.set_perm)
sa.event.listen(SqlaTable, 'after_update', security_manager.set_perm)
6 changes: 3 additions & 3 deletions superset/models/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
from superset import app, db, db_engine_specs, security_manager, utils
from superset.connectors.connector_registry import ConnectorRegistry
from superset.legacy import update_time_range
from superset.models.helpers import AuditMixinNullable, ImportMixin, set_perm
from superset.models.helpers import AuditMixinNullable, ImportMixin
from superset.models.user_attributes import UserAttribute
from superset.utils import MediumText
from superset.viz import viz_types
Expand Down Expand Up @@ -959,8 +959,8 @@ def get_dialect(self):
return sqla_url.get_dialect()()


sqla.event.listen(Database, 'after_insert', set_perm)
sqla.event.listen(Database, 'after_update', set_perm)
sqla.event.listen(Database, 'after_insert', security_manager.set_perm)
sqla.event.listen(Database, 'after_update', security_manager.set_perm)


class Log(Model):
Expand Down
51 changes: 0 additions & 51 deletions superset/models/helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
from sqlalchemy.orm.exc import MultipleResultsFound
import yaml

from superset import security_manager
from superset.utils import QueryStatus


Expand Down Expand Up @@ -312,53 +311,3 @@ def __init__( # noqa
self.duration = duration
self.status = status
self.error_message = error_message


def merge_perm(sm, permission_name, view_menu_name, connection):

permission = sm.find_permission(permission_name)
view_menu = sm.find_view_menu(view_menu_name)
pv = None

if not permission:
permission_table = sm.permission_model.__table__
connection.execute(
permission_table.insert()
.values(name=permission_name),
)
if not view_menu:
view_menu_table = sm.viewmenu_model.__table__
connection.execute(
view_menu_table.insert()
.values(name=view_menu_name),
)

permission = sm.find_permission(permission_name)
view_menu = sm.find_view_menu(view_menu_name)

if permission and view_menu:
pv = sm.get_session.query(sm.permissionview_model).filter_by(
permission=permission, view_menu=view_menu).first()
if not pv and permission and view_menu:
permission_view_table = sm.permissionview_model.__table__
connection.execute(
permission_view_table.insert()
.values(
permission_id=permission.id,
view_menu_id=view_menu.id,
),
)


def set_perm(mapper, connection, target): # noqa

if target.perm != target.get_perm():
link_table = target.__table__
connection.execute(
link_table.update()
.where(link_table.c.id == target.id)
.values(perm=target.get_perm()),
)

# add to view menu if not already exists
merge_perm(security_manager, 'datasource_access', target.get_perm(), connection)
44 changes: 44 additions & 0 deletions superset/security.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,3 +383,47 @@ def is_granter_pvm(self, pvm):
return pvm.permission.name in {
'can_override_role_permissions', 'can_approve',
}

def set_perm(self, mapper, connection, target): # noqa
if target.perm != target.get_perm():
link_table = target.__table__
connection.execute(
link_table.update()
.where(link_table.c.id == target.id)
.values(perm=target.get_perm()),
)

# add to view menu if not already exists
permission_name = 'datasource_access'
view_menu_name = target.get_perm()
permission = self.find_permission(permission_name)
view_menu = self.find_view_menu(view_menu_name)
pv = None

if not permission:
Copy link
Member

Choose a reason for hiding this comment

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

Hey I think we can get rid of most of this by calling self.add_permission_view_menu which takes care of all of this I think

Copy link
Member Author

Choose a reason for hiding this comment

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

@mistercrunch I tried that approach earlier (see description) which lead to the following exception being thrown: sqlalchemy.exc.ResourceClosedError: This transaction is closed

Copy link
Member

Choose a reason for hiding this comment

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

Yeah the context is funky here where we're inside some sort of SQLAlchemy trigger. Feel free to merge as is.

permission_table = self.permission_model.__table__ # noqa: E501 pylint: disable=no-member
connection.execute(
permission_table.insert()
.values(name=permission_name),
)
permission = self.find_permission(permission_name)
if not view_menu:
view_menu_table = self.viewmenu_model.__table__ # pylint: disable=no-member
connection.execute(
view_menu_table.insert()
.values(name=view_menu_name),
)
view_menu = self.find_view_menu(view_menu_name)

if permission and view_menu:
pv = self.get_session.query(self.permissionview_model).filter_by(
permission=permission, view_menu=view_menu).first()
if not pv and permission and view_menu:
permission_view_table = self.permissionview_model.__table__ # noqa: E501 pylint: disable=no-member
connection.execute(
permission_view_table.insert()
.values(
permission_id=permission.id,
view_menu_id=view_menu.id,
),
)