From 0bedaed3676c2185aff8f0de52f07fd40b45b985 Mon Sep 17 00:00:00 2001 From: Andrii Sydorchuk Date: Wed, 4 May 2016 07:31:37 +0200 Subject: [PATCH] Make sure anonymous user with proper permissions can access data (#415) * Make sure anonymous user with proper permissions can access data * Review fixes: naming changes * Review fixes: add more granular tests for public user dashboard access * Review fixes: test that public user has access only to permitted data sets --- caravel/models.py | 4 +-- caravel/views.py | 18 ++++++++---- tests/core_tests.py | 70 ++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 83 insertions(+), 9 deletions(-) diff --git a/caravel/models.py b/caravel/models.py index b3f8294ede66c..33152bbd1e54a 100644 --- a/caravel/models.py +++ b/caravel/models.py @@ -23,7 +23,7 @@ from flask.ext.appbuilder import Model from flask.ext.appbuilder.models.mixins import AuditMixin from flask.ext.appbuilder.models.decorators import renders -from flask.ext.babelpkg import lazy_gettext as _ +from flask.ext.babelpkg import gettext as _ from pydruid.client import PyDruid from pydruid.utils.filters import Dimension, Filter @@ -1240,7 +1240,7 @@ def log_this(cls, f): def wrapper(*args, **kwargs): user_id = None if g.user: - user_id = g.user.id + user_id = g.user.get_id() d = request.args.to_dict() d.update(kwargs) slice_id = d.get('slice_id', 0) diff --git a/caravel/views.py b/caravel/views.py index 48f203a770e75..aea86169c7b93 100644 --- a/caravel/views.py +++ b/caravel/views.py @@ -20,7 +20,7 @@ from flask.ext.appbuilder.actions import action from flask.ext.appbuilder.models.sqla.interface import SQLAInterface from flask.ext.appbuilder.security.decorators import has_access -from flask.ext.babelpkg import lazy_gettext as _ +from flask.ext.babelpkg import gettext as _ from flask_appbuilder.models.sqla.filters import BaseFilter from pydruid.client import doublesum @@ -35,10 +35,16 @@ log_this = models.Log.log_this +def get_user_roles(): + if g.user.is_anonymous(): + return [appbuilder.sm.find_role('Public')] + return g.user.roles + + class CaravelFilter(BaseFilter): def get_perms(self): perms = [] - for role in g.user.roles: + for role in get_user_roles(): for perm_view in role.permissions: if perm_view.permission.name == 'datasource_access': perms.append(perm_view.view_menu.name) @@ -47,7 +53,7 @@ def get_perms(self): class FilterSlice(CaravelFilter): def apply(self, query, func): # noqa - if any([r.name in ('Admin', 'Alpha') for r in g.user.roles]): + if any([r.name in ('Admin', 'Alpha') for r in get_user_roles()]): return query qry = query.filter(self.model.perm.in_(self.get_perms())) print(qry) @@ -56,7 +62,7 @@ def apply(self, query, func): # noqa class FilterDashboard(CaravelFilter): def apply(self, query, func): # noqa - if any([r.name in ('Admin', 'Alpha') for r in g.user.roles]): + if any([r.name in ('Admin', 'Alpha') for r in get_user_roles()]): return query Slice = models.Slice # noqa slice_ids_qry = ( @@ -721,12 +727,12 @@ def favstar(self, class_name, obj_id, action): FavStar = models.FavStar # noqa count = 0 favs = session.query(FavStar).filter_by( - class_name=class_name, obj_id=obj_id, user_id=g.user.id).all() + class_name=class_name, obj_id=obj_id, user_id=g.user.get_id()).all() if action == 'select': if not favs: session.add( FavStar( - class_name=class_name, obj_id=obj_id, user_id=g.user.id, + class_name=class_name, obj_id=obj_id, user_id=g.user.get_id(), dttm=datetime.now())) count = 1 elif action == 'unselect': diff --git a/tests/core_tests.py b/tests/core_tests.py index 4d8bd69fc93cb..bace46f4a3e83 100644 --- a/tests/core_tests.py +++ b/tests/core_tests.py @@ -12,6 +12,7 @@ from mock import Mock, patch from flask import escape +from flask_appbuilder.security.sqla import models as ab_models import caravel from caravel import app, db, models, utils, appbuilder @@ -63,17 +64,38 @@ def login_gamma(self): follow_redirects=True) assert 'Welcome' in resp.data.decode('utf-8') + def setup_public_access_for_dashboard(self, dashboard_name): + public_role = appbuilder.sm.find_role('Public') + perms = db.session.query(ab_models.PermissionView).all() + for perm in perms: + if perm.permission.name not in ( + 'can_list', + 'can_dashboard', + 'can_explore', + 'datasource_access'): + continue + if not perm.view_menu: + continue + if perm.view_menu.name not in ( + 'SliceModelView', + 'DashboardModelView', + 'Caravel') and dashboard_name not in perm.view_menu.name: + continue + appbuilder.sm.add_permission_role(public_role, perm) + class CoreTests(CaravelTestCase): def __init__(self, *args, **kwargs): + # Load examples first, so that we setup proper permission-view relations + # for all example data sources. + self.load_examples() super(CoreTests, self).__init__(*args, **kwargs) self.table_ids = {tbl.table_name: tbl.id for tbl in ( db.session .query(models.SqlaTable) .all() )} - self.load_examples() def setUp(self): pass @@ -162,6 +184,52 @@ def test_gamma(self): resp = self.client.get('/dashboardmodelview/list/') assert "List Dashboard" in resp.data.decode('utf-8') + def test_public_user_dashboard_access(self): + # Try access before adding appropriate permissions. + resp = self.client.get('/slicemodelview/list/') + data = resp.data.decode('utf-8') + assert 'birth_names' not in data + + resp = self.client.get('/dashboardmodelview/list/') + data = resp.data.decode('utf-8') + assert '' not in data + + resp = self.client.get('/caravel/dashboard/births/') + data = resp.data.decode('utf-8') + assert '[dashboard] Births' not in data + + self.setup_public_access_for_dashboard('birth_names') + + # Try access after adding appropriate permissions. + resp = self.client.get('/slicemodelview/list/') + data = resp.data.decode('utf-8') + assert 'birth_names' in data + + resp = self.client.get('/dashboardmodelview/list/') + data = resp.data.decode('utf-8') + assert '' in data + + resp = self.client.get('/caravel/dashboard/births/') + data = resp.data.decode('utf-8') + assert '[dashboard] Births' in data + + resp = self.client.get('/caravel/explore/table/3/') + data = resp.data.decode('utf-8') + assert '[explore] birth_names' in data + + # Confirm that public doesn't have access to other datasets. + resp = self.client.get('/slicemodelview/list/') + data = resp.data.decode('utf-8') + assert 'wb_health_population' not in data + + resp = self.client.get('/dashboardmodelview/list/') + data = resp.data.decode('utf-8') + assert '' not in data + + resp = self.client.get('/caravel/explore/table/2/', follow_redirects=True) + data = resp.data.decode('utf-8') + assert "You don't seem to have access to this datasource" in data + SEGMENT_METADATA = [{ "id": "some_id",