-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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.
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
Dashboard level access control #5099
Changes from 9 commits
1caa0d4
ae284d8
ec3e4e4
d700e01
f66a5ea
0874737
db3c854
90e8d01
2194a3d
b7da648
0147c3c
f375a8b
94a1ad4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
bleach==2.1.2 | ||
boto3==1.4.7 | ||
celery==4.1.0 | ||
celery==4.1.1 | ||
colorama==0.3.9 | ||
cryptography==1.9 | ||
flask==0.12.2 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
"""empty message | ||
|
||
Revision ID: 668b05dbfd7b | ||
Revises: ('e502db2af7be', '82c2867e532b') | ||
Create Date: 2018-05-29 16:05:20.110737 | ||
|
||
""" | ||
|
||
# revision identifiers, used by Alembic. | ||
revision = '668b05dbfd7b' | ||
down_revision = ('e502db2af7be', '82c2867e532b') | ||
|
||
from alembic import op | ||
import sqlalchemy as sa | ||
|
||
|
||
def upgrade(): | ||
pass | ||
|
||
|
||
def downgrade(): | ||
pass |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
"""dashboard role many to many | ||
|
||
Revision ID: b21a4c518cab | ||
Revises: e866bd2d4976 | ||
Create Date: 2018-03-08 16:30:36.924096 | ||
|
||
""" | ||
|
||
# revision identifiers, used by Alembic. | ||
revision = '76a4d742cf04' | ||
down_revision = '5ccf602336a0' | ||
|
||
from alembic import op | ||
import sqlalchemy as sa | ||
|
||
|
||
def upgrade(): | ||
op.create_table('dashboard_role', | ||
sa.Column('id', sa.Integer(), nullable=False), | ||
sa.Column('role_id', sa.Integer(), nullable=True), | ||
sa.Column('dashboard_id', sa.Integer(), nullable=True), | ||
sa.ForeignKeyConstraint(['dashboard_id'], [u'dashboards.id'], ), | ||
sa.ForeignKeyConstraint(['role_id'], [u'ab_role.id'], ), | ||
sa.PrimaryKeyConstraint('id'), | ||
) | ||
|
||
|
||
def downgrade(): | ||
op.drop_table('dashboard_role') |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
"""empty message | ||
|
||
Revision ID: 82c2867e532b | ||
Revises: 76a4d742cf04 | ||
Create Date: 2018-05-29 15:21:07.127779 | ||
|
||
""" | ||
|
||
# revision identifiers, used by Alembic. | ||
revision = '82c2867e532b' | ||
down_revision = '76a4d742cf04' | ||
|
||
from alembic import op | ||
import sqlalchemy as sa | ||
|
||
|
||
def upgrade(): | ||
pass | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this migration is no-op, let's remove it before merge. |
||
|
||
|
||
def downgrade(): | ||
pass |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
# -*- coding: utf-8 -*- | ||
# pylint: disable=C,R,W | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please do not disable pylint on new files. |
||
from __future__ import absolute_import | ||
from __future__ import division | ||
from __future__ import print_function | ||
from __future__ import unicode_literals | ||
|
||
import logging | ||
import re | ||
|
||
from flask_appbuilder.security.sqla.manager import SecurityManager | ||
from flask_appbuilder.security.views import RoleModelView | ||
from flask_babel import lazy_gettext as _ | ||
|
||
|
||
class SupersetRoleModelView(RoleModelView): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's update the file name to remove the abbreviation ( |
||
|
||
def add_dashboard_role(self, role, dash): | ||
from superset import db | ||
|
||
if dash not in role.dashboards: | ||
try: | ||
role.dashboards.append(dash) | ||
db.session.merge(role) | ||
db.session.commit() | ||
except Exception as e: | ||
logging.error(e) | ||
|
||
def remove_dashboard_role(self, role, dash): | ||
from superset import db | ||
|
||
try: | ||
role.dashboards.remove(dash) | ||
db.session.merge(role) | ||
db.session.commit() | ||
except Exception as e: | ||
logging.error(e) | ||
|
||
def post_update(self, role): | ||
from superset.models.core import Dashboard | ||
from superset import db | ||
|
||
role_dash_perms = [] | ||
for perm_view in role.permissions: | ||
if perm_view.permission.name == 'dashboard_access': | ||
m = re.search(r'dash_id\:(\d+)\)$', perm_view.view_menu.name) | ||
if m: | ||
dash_id = m.groups()[0] | ||
role_dash_perms.append(int(dash_id)) | ||
try: | ||
dash = db.session.query(Dashboard).filter_by(id=dash_id).first() | ||
except Exception as e: | ||
logging.error(e) | ||
|
||
self.add_dashboard_role(role, dash) | ||
else: | ||
logging.error( | ||
_("'dashboard_access name '{}' not as expected" | ||
.format(perm_view.view_menu.name))) | ||
role_dashboards = role.dashboards | ||
for dash in role_dashboards: | ||
if dash.id not in role_dash_perms: | ||
self.remove_dashboard_role(role, dash) | ||
|
||
|
||
class SupersetSecurityManager(SecurityManager): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, this code was left over from the merge (I had defined my own SupersetSecurityManager before it was added to security.py). I've removed it. This new rolemodelview is already defined in the class in security.py. |
||
rolemodelview = SupersetRoleModelView |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey so the model so far has been to not create these many-to-many tables, and instead creating perms for each object (table, database, schema). This has pros/cons, one of the pros being to have the security model limited to FAB's RBAC tables. Meaning roles are only made out of users and perms. Then perms act as imperfect FKs to the object table. Cons is the fact that the join is imperfect as we want the perm object to be human readable, but contains the id.
I think I like the approach here, made possible by the fact that we now have
SupersetSecurityManager
and can specifyrolemodelview
. I can see how we'd move to do pattern forDatabases
,DruidDatasource
,SqlaTable
at some point. I'm not sure what others may think about this approach.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a fan of proper foreign keys in join tables over FKs in permisssion names - it makes the data model more explicit and we can more easily enforce data consistency.