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

Author: asydorchuk

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)

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':

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 '<a href="/tablemodelview/edit/3">birth_names</a>' not in data
+
+        resp = self.client.get('/dashboardmodelview/list/')
+        data = resp.data.decode('utf-8')
+        assert '<a href="/caravel/dashboard/births/">' 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 '<a href="/tablemodelview/edit/3">birth_names</a>' in data
+
+        resp = self.client.get('/dashboardmodelview/list/')
+        data = resp.data.decode('utf-8')
+        assert '<a href="/caravel/dashboard/births/">' 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 '<a href="/tablemodelview/edit/2">wb_health_population</a>' not in data
+
+        resp = self.client.get('/dashboardmodelview/list/')
+        data = resp.data.decode('utf-8')
+        assert '<a href="/caravel/dashboard/world_health/">' not in data
+
+        resp = self.client.get('/caravel/explore/table/2/', follow_redirects=True)
+        data = resp.data.decode('utf-8')
+        assert "You don&#39;t seem to have access to this datasource" in data
+
 
 SEGMENT_METADATA = [{
   "id": "some_id",