[security] allow for requesting access when denied on a dashboard view (#1192)

* Request access on dashboard view

* Fixing the unit tests

* Refactored much in the tests

caravel/migrations/versions/ab3d66c4246e_add_cache_timeout_to_druid_cluster.py

@@ -0,0 +1,23 @@
+"""add_cache_timeout_to_druid_cluster
+
+Revision ID: ab3d66c4246e
+Revises: eca4694defa7
+Create Date: 2016-09-30 18:01:30.579760
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = 'ab3d66c4246e'
+down_revision = 'eca4694defa7'
+
+from alembic import op
+import sqlalchemy as sa
+
+
+def upgrade():
+    op.add_column(
+        'clusters', sa.Column('cache_timeout', sa.Integer(), nullable=True))
+
+
+def downgrade():
+    op.drop_column('clusters', 'cache_timeout')

caravel/migrations/versions/ef8843b41dac_.py

@@ -0,0 +1,22 @@
+"""empty message
+
+Revision ID: ef8843b41dac
+Revises: ('3b626e2a6783', 'ab3d66c4246e')
+Create Date: 2016-10-02 10:35:38.825231
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = 'ef8843b41dac'
+down_revision = ('3b626e2a6783', 'ab3d66c4246e')
+
+from alembic import op
+import sqlalchemy as sa
+
+
+def upgrade():
+    pass
+
+
+def downgrade():
+    pass

caravel/models.py

@@ -337,6 +337,10 @@ class Dashboard(Model, AuditMixinNullable):
     def url(self):
         return "/caravel/dashboard/{}/".format(self.slug or self.id)
 
+    @property
+    def datasources(self):
+        return {slc.datasource for slc in self.slices}
+
     @property
     def metadata_dejson(self):
         if self.json_metadata:
@@ -1180,6 +1184,7 @@ class DruidCluster(Model, AuditMixinNullable):
     broker_port = Column(Integer)
     broker_endpoint = Column(String(255), default='druid/v2')
     metadata_last_refreshed = Column(DateTime)
+    cache_timeout = Column(Integer)
 
     def __repr__(self):
         return self.cluster_name
@@ -1245,6 +1250,10 @@ class DruidDatasource(Model, AuditMixinNullable, Queryable):
             [(m.metric_name, m.verbose_name) for m in self.metrics],
             key=lambda x: x[1])
 
+    @property
+    def database(self):
+        return self.cluster
+
     @property
     def num_cols(self):
         return [c.column_name for c in self.columns if c.isnum]

caravel/templates/caravel/request_access.html

@@ -1,24 +1,20 @@
 {% extends "caravel/basic.html" %}
 {% block title %}{{ _("No Access!") }}{% endblock %}
 {% block body %}
+  <div class="container">
     {% include "caravel/flash_wrapper.html" %}
-    <div class="container">
-        <h4>
-            {{ _("You do not have permissions to access the datasource %(name)s.",
-                 name=datasource_name)
-            }}
-        </h4>
-        <div id="buttons">
-            <button  onclick="window.location.href = '{{ request_access_url }}';"
-                     id="request"
-            >
-                {{ _("Request Permissions") }}
-            </button>
-            <button onclick="window.location.href = '{{ slicemodelview_link }}';"
-                    id="cancel"
-            >
-                {{ _("Cancel") }}
-            </button>
-        </div>
+    <h4>
+      {{ _("You do not have permissions to access the datasource(s): %(name)s.",
+      name=datasource_names)
+      }}
+    </h4>
+    <div>
+      <button  onclick="window.location += '&action=go';">
+        {{ _("Request Permissions") }}
+      </button>
+      <button onclick="window.location.href = '/slicemodelview/list/';">
+        {{ _("Cancel") }}
+      </button>
     </div>
-{% endblock %}
+  </div>
+{% endblock %}

caravel/views.py

@@ -57,12 +57,8 @@ class BaseCaravelView(BaseView):
                 self.can_access("database_access", database.perm))
 
     def datasource_access(self, datasource):
-        if hasattr(datasource, "cluster"):
-            return (self.database_access(datasource.cluster) or
-                    self.can_access("datasource_access", datasource.perm))
-        else:
-            return (self.database_access(datasource.database) or
-                    self.can_access("datasource_access", datasource.perm))
+        return (self.database_access(datasource.database) or
+                self.can_access("datasource_access", datasource.perm))
 
 
 class ListWidgetWithCheckboxes(ListWidget):
@@ -202,6 +198,7 @@ class FilterSlice(CaravelFilter):
 
 
 class FilterDashboard(CaravelFilter):
+    """List dashboards for which users have access to at least one slice"""
     def apply(self, query, func):  # noqa
         if any([r.name in ('Admin', 'Alpha') for r in get_user_roles()]):
             return query
@@ -662,7 +659,7 @@ class DruidClusterModelView(CaravelModelView, DeleteMixin):  # noqa
     add_columns = [
         'cluster_name',
         'coordinator_host', 'coordinator_port', 'coordinator_endpoint',
-        'broker_host', 'broker_port', 'broker_endpoint',
+        'broker_host', 'broker_port', 'broker_endpoint', 'cache_timeout',
     ]
     edit_columns = add_columns
     list_columns = ['cluster_name', 'metadata_last_refreshed']
@@ -998,53 +995,43 @@ class Caravel(BaseCaravelView):
     """The base views for Caravel!"""
     @log_this
     @has_access
-    @expose("/request_access_form/<datasource_type>/<datasource_id>/"
-            "<datasource_name>")
-    def request_access_form(
-            self, datasource_type, datasource_id, datasource_name):
-        request_access_url = (
-            '/caravel/request_access?datasource_type={}&datasource_id={}&'
-            'datasource_name=datasource_name'.format(
-                datasource_type, datasource_id, datasource_name)
-        )
-        return self.render_template(
-            'caravel/request_access.html',
-            request_access_url=request_access_url,
-            datasource_name=datasource_name,
-            slicemodelview_link='/slicemodelview/list/')
-
-    @log_this
-    @has_access
-    @expose("/request_access")
+    @expose("/request_access/")
     def request_access(self):
+        datasources = set()
+        dashboard_id = request.args.get('dashboard_id')
+        if dashboard_id:
+            dash = (
+                db.session.query(models.Dashboard)
+                .filter_by(id=int(dashboard_id))
+                .one()
+            )
+            datasources |= dash.datasources
         datasource_id = request.args.get('datasource_id')
         datasource_type = request.args.get('datasource_type')
-        datasource_name = request.args.get('datasource_name')
-        session = db.session
+        if datasource_id:
+            ds_class = SourceRegistry.sources.get(datasource_type)
+            datasource = (
+                db.session.query(ds_class)
+                .filter_by(id=int(datasource_id))
+                .one()
+            )
+            datasources.add(datasource)
+        if request.args.get('action') == 'go':
+            for datasource in datasources:
+                access_request = DAR(
+                    datasource_id=datasource.id,
+                    datasource_type=datasource.type)
+                db.session.add(access_request)
+                db.session.commit()
+            flash(__("Access was requested"), "info")
+            return redirect('/')
 
-        duplicates = (
-            session.query(DAR)
-            .filter(
-                DAR.datasource_id == datasource_id,
-                DAR.datasource_type == datasource_type,
-                DAR.created_by_fk == g.user.id)
-            .all()
+        return self.render_template(
+            'caravel/request_access.html',
+            datasources=datasources,
+            datasource_names=", ".join([o.name for o in datasources]),
         )
 
-        if duplicates:
-            flash(__(
-                "You have already requested access to the datasource %(name)s",
-                name=datasource_name), "warning")
-            return redirect('/slicemodelview/list/')
-
-        access_request = DAR(datasource_id=datasource_id,
-                             datasource_type=datasource_type)
-        db.session.add(access_request)
-        db.session.commit()
-        flash(__("Access to the datasource %(name)s was requested",
-                 name=datasource_name), "info")
-        return redirect('/slicemodelview/list/')
-
     @log_this
     @has_access
     @expose("/approve")
@@ -1132,8 +1119,11 @@ class Caravel(BaseCaravelView):
         if not self.datasource_access(datasource):
             flash(
                 __(get_datasource_access_error_msg(datasource.name)), "danger")
-            return redirect('caravel/request_access_form/{}/{}/{}'.format(
-                datasource_type, datasource_id, datasource.name))
+            return redirect(
+                'caravel/request_access/?'
+                'datasource_type={datasource_type}&'
+                'datasource_id={datasource_id}&'
+                ''.format(**locals()))
 
         request_args_multi_dict = request.args  # MultiDict
 
@@ -1524,7 +1514,17 @@ class Caravel(BaseCaravelView):
             qry = qry.filter_by(slug=dashboard_id)
 
         templates = session.query(models.CssTemplate).all()
-        dash = qry.first()
+        dash = qry.one()
+        datasources = {slc.datasource for slc in dash.slices}
+        for datasource in datasources:
+            if not self.datasource_access(datasource):
+                flash(
+                    __(get_datasource_access_error_msg(datasource.name)),
+                    "danger")
+                return redirect(
+                    'caravel/request_access/?'
+                    'dashboard_id={dash.id}&'
+                    ''.format(**locals()))
 
         # Hack to log the dashboard_id properly, even when getting a slug
         @log_this
@@ -1532,7 +1532,8 @@ class Caravel(BaseCaravelView):
             pass
         dashboard(dashboard_id=dash.id)
         dash_edit_perm = check_ownership(dash, raise_if_false=False)
-        dash_save_perm = dash_edit_perm and self.can_access('can_save_dash', 'Caravel')
+        dash_save_perm = \
+            dash_edit_perm and self.can_access('can_save_dash', 'Caravel')
         return self.render_template(
             "caravel/dashboard.html", dashboard=dash,
             user_id=g.user.get_id(),

run_specific_test.sh

@@ -0,0 +1,8 @@
+#!/usr/bin/env bash
+echo $DB
+rm -f .coverage
+export CARAVEL_CONFIG=tests.caravel_test_config
+set -e
+caravel/bin/caravel version -v
+export SOLO_TEST=1
+nosetests tests.core_tests:CoreTests.test_public_user_dashboard_access

tests/access_requests.py

@@ -0,0 +1,222 @@
+"""Unit tests for Caravel"""
+from __future__ import absolute_import
+from __future__ import division
+from __future__ import print_function
+from __future__ import unicode_literals
+
+import unittest
+
+from caravel import db, models, sm
+from caravel.source_registry import SourceRegistry
+
+from .base_tests import CaravelTestCase
+
+
+class RequestAccessTests(CaravelTestCase):
+
+    requires_examples = True
+
+    def test_approve(self):
+        session = db.session
+        TEST_ROLE_NAME = 'table_role'
+        sm.add_role(TEST_ROLE_NAME)
+        self.login('admin')
+
+        def create_access_request(ds_type, ds_name, role_name):
+            ds_class = SourceRegistry.sources[ds_type]
+            # TODO: generalize datasource names
+            if ds_type == 'table':
+                ds = session.query(ds_class).filter(
+                    ds_class.table_name == ds_name).first()
+            else:
+                ds = session.query(ds_class).filter(
+                    ds_class.datasource_name == ds_name).first()
+            ds_perm_view = sm.find_permission_view_menu(
+                'datasource_access', ds.perm)
+            sm.add_permission_role(sm.find_role(role_name), ds_perm_view)
+            access_request = models.DatasourceAccessRequest(
+                datasource_id=ds.id,
+                datasource_type=ds_type,
+                created_by_fk=sm.find_user(username='gamma').id,
+            )
+            session.add(access_request)
+            session.commit()
+            return access_request
+
+        EXTEND_ROLE_REQUEST = (
+            '/caravel/approve?datasource_type={}&datasource_id={}&'
+            'created_by={}&role_to_extend={}')
+        GRANT_ROLE_REQUEST = (
+            '/caravel/approve?datasource_type={}&datasource_id={}&'
+            'created_by={}&role_to_grant={}')
+
+        # Case 1. Grant new role to the user.
+
+        access_request1 = create_access_request(
+            'table', 'unicode_test', TEST_ROLE_NAME)
+        ds_1_id = access_request1.datasource_id
+        self.get_resp(GRANT_ROLE_REQUEST.format(
+            'table', ds_1_id, 'gamma', TEST_ROLE_NAME))
+
+        access_requests = self.get_access_requests('gamma', 'table', ds_1_id)
+        # request was removed
+        self.assertFalse(access_requests)
+        # user was granted table_role
+        user_roles = [r.name for r in sm.find_user('gamma').roles]
+        self.assertIn(TEST_ROLE_NAME, user_roles)
+
+        # Case 2. Extend the role to have access to the table
+
+        access_request2 = create_access_request('table', 'long_lat', TEST_ROLE_NAME)
+        ds_2_id = access_request2.datasource_id
+        long_lat_perm = access_request2.datasource.perm
+
+        self.client.get(EXTEND_ROLE_REQUEST.format(
+            'table', access_request2.datasource_id, 'gamma', TEST_ROLE_NAME))
+        access_requests = self.get_access_requests('gamma', 'table', ds_2_id)
+        # request was removed
+        self.assertFalse(access_requests)
+        # table_role was extended to grant access to the long_lat table/
+        perm_view = sm.find_permission_view_menu(
+            'datasource_access', long_lat_perm)
+        TEST_ROLE = sm.find_role(TEST_ROLE_NAME)
+        self.assertIn(perm_view, TEST_ROLE.permissions)
+
+        # Case 3. Grant new role to the user to access the druid datasource.
+
+        sm.add_role('druid_role')
+        access_request3 = create_access_request('druid', 'druid_ds_1', 'druid_role')
+        self.get_resp(GRANT_ROLE_REQUEST.format(
+            'druid', access_request3.datasource_id, 'gamma', 'druid_role'))
+
+        # user was granted table_role
+        user_roles = [r.name for r in sm.find_user('gamma').roles]
+        self.assertIn('druid_role', user_roles)
+
+        # Case 4. Extend the role to have access to the druid datasource
+
+        access_request4 = create_access_request('druid', 'druid_ds_2', 'druid_role')
+        druid_ds_2_perm = access_request4.datasource.perm
+
+        self.client.get(EXTEND_ROLE_REQUEST.format(
+            'druid', access_request4.datasource_id, 'gamma', 'druid_role'))
+        # druid_role was extended to grant access to the druid_access_ds_2
+        druid_role = sm.find_role('druid_role')
+        perm_view = sm.find_permission_view_menu(
+            'datasource_access', druid_ds_2_perm)
+        self.assertIn(perm_view, druid_role.permissions)
+
+        # cleanup
+        gamma_user = sm.find_user(username='gamma')
+        gamma_user.roles.remove(sm.find_role('druid_role'))
+        gamma_user.roles.remove(sm.find_role(TEST_ROLE_NAME))
+        session.delete(sm.find_role('druid_role'))
+        session.delete(sm.find_role(TEST_ROLE_NAME))
+        session.commit()
+
+    def test_request_access(self):
+        session = db.session
+        self.login(username='gamma')
+        gamma_user = sm.find_user(username='gamma')
+        sm.add_role('dummy_role')
+        gamma_user.roles.append(sm.find_role('dummy_role'))
+        session.commit()
+
+        ACCESS_REQUEST = (
+            '/caravel/request_access?'
+            'datasource_type={}&'
+            'datasource_id={}&'
+            'action={}&')
+        ROLE_EXTEND_LINK = (
+            '<a href="/caravel/approve?datasource_type={}&datasource_id={}&'
+            'created_by={}&role_to_extend={}">Extend {} Role</a>')
+        ROLE_GRANT_LINK = (
+            '<a href="/caravel/approve?datasource_type={}&datasource_id={}&'
+            'created_by={}&role_to_grant={}">Grant {} Role</a>')
+
+        # Request table access, there are no roles have this table.
+
+        table1 = session.query(models.SqlaTable).filter_by(
+            table_name='random_time_series').first()
+        table_1_id = table1.id
+
+        # request access to the table
+        resp = self.get_resp(
+            ACCESS_REQUEST.format('table', table_1_id, 'go'))
+        assert "Access was requested" in resp
+        access_request1 = self.get_access_requests('gamma', 'table', table_1_id)
+        assert access_request1 is not None
+
+        # Request access, roles exist that contains the table.
+        # add table to the existing roles
+        table3 = session.query(models.SqlaTable).filter_by(
+            table_name='energy_usage').first()
+        table_3_id = table3.id
+        table3_perm = table3.perm
+
+        sm.add_role('energy_usage_role')
+        alpha_role = sm.find_role('Alpha')
+        sm.add_permission_role(
+            alpha_role,
+            sm.find_permission_view_menu('datasource_access', table3_perm))
+        sm.add_permission_role(
+            sm.find_role("energy_usage_role"),
+            sm.find_permission_view_menu('datasource_access', table3_perm))
+        session.commit()
+
+        self.get_resp(
+            ACCESS_REQUEST.format('table', table_3_id, 'go'))
+        access_request3 = self.get_access_requests('gamma', 'table', table_3_id)
+        approve_link_3 = ROLE_GRANT_LINK.format(
+            'table', table_3_id, 'gamma', 'energy_usage_role',
+            'energy_usage_role')
+        self.assertEqual(access_request3.roles_with_datasource,
+                         '<ul><li>{}</li></ul>'.format(approve_link_3))
+
+        # Request druid access, there are no roles have this table.
+        druid_ds_4 = session.query(models.DruidDatasource).filter_by(
+            datasource_name='druid_ds_1').first()
+        druid_ds_4_id = druid_ds_4.id
+
+        # request access to the table
+        self.get_resp(ACCESS_REQUEST.format('druid', druid_ds_4_id, 'go'))
+        access_request4 = self.get_access_requests('gamma', 'druid', druid_ds_4_id)
+
+        self.assertEqual(
+            access_request4.roles_with_datasource,
+            '<ul></ul>'.format(access_request4.id))
+
+        # Case 5. Roles exist that contains the druid datasource.
+        # add druid ds to the existing roles
+        druid_ds_5 = session.query(models.DruidDatasource).filter_by(
+            datasource_name='druid_ds_2').first()
+        druid_ds_5_id = druid_ds_5.id
+        druid_ds_5_perm = druid_ds_5.perm
+
+        druid_ds_2_role = sm.add_role('druid_ds_2_role')
+        admin_role = sm.find_role('Admin')
+        sm.add_permission_role(
+            admin_role,
+            sm.find_permission_view_menu('datasource_access', druid_ds_5_perm))
+        sm.add_permission_role(
+            druid_ds_2_role,
+            sm.find_permission_view_menu('datasource_access', druid_ds_5_perm))
+        session.commit()
+
+        self.get_resp(ACCESS_REQUEST.format('druid', druid_ds_5_id, 'go'))
+        access_request5 = self.get_access_requests(
+            'gamma', 'druid', druid_ds_5_id)
+        approve_link_5 = ROLE_GRANT_LINK.format(
+            'druid', druid_ds_5_id, 'gamma', 'druid_ds_2_role',
+            'druid_ds_2_role')
+        self.assertEqual(access_request5.roles_with_datasource,
+                         '<ul><li>{}</li></ul>'.format(approve_link_5))
+
+        # cleanup
+        gamma_user = sm.find_user(username='gamma')
+        gamma_user.roles.remove(sm.find_role('dummy_role'))
+        session.commit()
+
+
+if __name__ == '__main__':
+    unittest.main()

tests/base_tests.py

@@ -4,6 +4,7 @@ from __future__ import division
 from __future__ import print_function
 from __future__ import unicode_literals
 
+import imp
 import os
 import unittest
 
@@ -15,11 +16,22 @@ from caravel import app, db, models, utils, appbuilder, sm
 os.environ['CARAVEL_CONFIG'] = 'tests.caravel_test_config'
 
 BASE_DIR = app.config.get("BASE_DIR")
+cli = imp.load_source('cli', BASE_DIR + "/bin/caravel")
 
 
 class CaravelTestCase(unittest.TestCase):
+    requires_examples = False
+    examples_loaded = False
 
     def __init__(self, *args, **kwargs):
+        if (
+                self.requires_examples and
+                not os.environ.get('SOLO_TEST') and
+                not os.environ.get('examples_loaded')
+            ):
+            cli.load_examples(load_test_data=True)
+            utils.init(caravel)
+            os.environ['examples_loaded'] = '1'
         super(CaravelTestCase, self).__init__(*args, **kwargs)
         self.client = app.test_client()
         self.maxDiff = None
@@ -92,13 +104,22 @@ class CaravelTestCase(unittest.TestCase):
         session.close()
         return query
 
+    def get_resp(self, url):
+        """Shortcut to get the parsed results while following redirects"""
+        resp = self.client.get(url, follow_redirects=True)
+        return resp.data.decode('utf-8')
+
     def get_access_requests(self, username, ds_type, ds_id):
-            return db.session.query(models.DatasourceAccessRequest).filter(
-                models.DatasourceAccessRequest.created_by_fk ==
-                sm.find_user(username=username).id,
-                models.DatasourceAccessRequest.datasource_type == ds_type,
-                models.DatasourceAccessRequest.datasource_id == ds_id
-            ).all()
+            DAR = models.DatasourceAccessRequest
+            return (
+                db.session.query(DAR)
+                .filter(
+                    DAR.created_by == sm.find_user(username=username),
+                    DAR.datasource_type == ds_type,
+                    DAR.datasource_id == ds_id,
+                )
+                .first()
+            )
 
     def logout(self):
         self.client.get('/logout/', follow_redirects=True)

tests/core_tests.py

@@ -6,7 +6,6 @@ from __future__ import unicode_literals
 
 import csv
 import doctest
-import imp
 import json
 import io
 import random
@@ -16,18 +15,15 @@ import unittest
 from flask import escape
 from flask_appbuilder.security.sqla import models as ab_models
 
-import caravel
-from caravel import app, db, models, utils, appbuilder, sm
-from caravel.source_registry import SourceRegistry
-from caravel.models import DruidDatasource
+from caravel import db, models, utils, appbuilder, sm
 
 from .base_tests import CaravelTestCase
 
-BASE_DIR = app.config.get("BASE_DIR")
-cli = imp.load_source('cli', BASE_DIR + "/bin/caravel")
 
 class CoreTests(CaravelTestCase):
 
+    requires_examples = True
+
     def __init__(self, *args, **kwargs):
         # Load examples first, so that we setup proper permission-view
         # relations for all example data sources.
@@ -35,8 +31,6 @@ class CoreTests(CaravelTestCase):
 
     @classmethod
     def setUpClass(cls):
-        cli.load_examples(load_test_data=True)
-        utils.init(caravel)
         cls.table_ids = {tbl.table_name: tbl.id for tbl in (
             db.session
             .query(models.SqlaTable)
@@ -96,14 +90,10 @@ class CoreTests(CaravelTestCase):
             "datasource_id=1&datasource_type=table&previous_viz_type=sankey")
 
         db.session.commit()
-        resp = self.client.get(
-            url.format(tbl_id, slice_id, copy_name, 'save'),
-            follow_redirects=True)
-        assert copy_name in resp.data.decode('utf-8')
-        resp = self.client.get(
-            url.format(tbl_id, slice_id, copy_name, 'overwrite'),
-            follow_redirects=True)
-        assert 'Energy' in resp.data.decode('utf-8')
+        resp = self.get_resp(url.format(tbl_id, slice_id, copy_name, 'save'))
+        assert copy_name in resp
+        assert 'Energy' in self.get_resp(
+            url.format(tbl_id, slice_id, copy_name, 'overwrite'))
 
     def test_slices(self):
         # Testing by hitting the two supported end points for all slices
@@ -138,8 +128,8 @@ class CoreTests(CaravelTestCase):
                 raise Exception("Failed a doctest")
 
     def test_misc(self):
-        assert self.client.get('/health').data.decode('utf-8') == "OK"
-        assert self.client.get('/ping').data.decode('utf-8') == "OK"
+        assert self.get_resp('/health') == "OK"
+        assert self.get_resp('/ping') == "OK"
 
     def test_testconn(self):
         database = (
@@ -168,16 +158,14 @@ class CoreTests(CaravelTestCase):
 
     def test_warm_up_cache(self):
         slice = db.session.query(models.Slice).first()
-        resp = self.client.get(
-            '/caravel/warm_up_cache?slice_id={}'.format(slice.id),
-            follow_redirects=True)
-        data = json.loads(resp.data.decode('utf-8'))
+        resp = self.get_resp(
+            '/caravel/warm_up_cache?slice_id={}'.format(slice.id))
+        data = json.loads(resp)
         assert data == [{'slice_id': slice.id, 'slice_name': slice.slice_name}]
 
-        resp = self.client.get(
-            '/caravel/warm_up_cache?table_name=energy_usage&db_name=main',
-            follow_redirects=True)
-        data = json.loads(resp.data.decode('utf-8'))
+        resp = self.get_resp(
+            '/caravel/warm_up_cache?table_name=energy_usage&db_name=main')
+        data = json.loads(resp)
         assert len(data) == 3
 
     def test_shortner(self):
@@ -238,339 +226,17 @@ class CoreTests(CaravelTestCase):
         assert new_slice in dash.slices
         assert len(set(dash.slices)) == len(dash.slices)
 
-    def test_approve(self):
-        session = db.session
-        sm.add_role('table_role')
-        self.login('admin')
-
-        def prepare_request(ds_type, ds_name, role):
-            ds_class = SourceRegistry.sources[ds_type]
-            # TODO: generalize datasource names
-            if ds_type == 'table':
-                ds = session.query(ds_class).filter(
-                    ds_class.table_name == ds_name).first()
-            else:
-                ds = session.query(ds_class).filter(
-                    ds_class.datasource_name == ds_name).first()
-            ds_perm_view = sm.find_permission_view_menu(
-                'datasource_access', ds.perm)
-            sm.add_permission_role(sm.find_role(role), ds_perm_view)
-            access_request = models.DatasourceAccessRequest(
-                datasource_id=ds.id,
-                datasource_type=ds_type,
-                created_by_fk=sm.find_user(username='gamma').id,
-            )
-            session.add(access_request)
-            session.commit()
-            return access_request
-
-        EXTEND_ROLE_REQUEST = (
-            '/caravel/approve?datasource_type={}&datasource_id={}&'
-            'created_by={}&role_to_extend={}')
-        GRANT_ROLE_REQUEST = (
-            '/caravel/approve?datasource_type={}&datasource_id={}&'
-            'created_by={}&role_to_grant={}')
-
-        # Case 1. Grant new role to the user.
-
-        access_request1 = prepare_request(
-            'table', 'unicode_test', 'table_role')
-        ds_1_id = access_request1.datasource_id
-        self.client.get(GRANT_ROLE_REQUEST.format(
-            'table', ds_1_id, 'gamma', 'table_role'))
-        access_requests = self.get_access_requests('gamma', 'table', ds_1_id)
-        # request was removed
-        self.assertFalse(access_requests)
-        # user was granted table_role
-        user_roles = [r.name for r in sm.find_user('gamma').roles]
-        self.assertIn('table_role', user_roles)
-
-        # Case 2. Extend the role to have access to the table
-
-        access_request2 = prepare_request('table', 'long_lat', 'table_role')
-        ds_2_id = access_request2.datasource_id
-        long_lat_perm = access_request2.datasource.perm
-
-        self.client.get(EXTEND_ROLE_REQUEST.format(
-            'table', access_request2.datasource_id, 'gamma', 'table_role'))
-        access_requests = self.get_access_requests('gamma', 'table', ds_2_id)
-        # request was removed
-        self.assertFalse(access_requests)
-        # table_role was extended to grant access to the long_lat table/
-        table_role = sm.find_role('table_role')
-        perm_view = sm.find_permission_view_menu(
-            'datasource_access', long_lat_perm)
-        self.assertIn(perm_view, table_role.permissions)
-
-        # Case 3. Grant new role to the user to access the druid datasource.
-
-        sm.add_role('druid_role')
-        access_request3 = prepare_request('druid', 'druid_ds_1', 'druid_role')
-        self.client.get(GRANT_ROLE_REQUEST.format(
-            'druid', access_request3.datasource_id, 'gamma', 'druid_role'))
-
-        # user was granted table_role
-        user_roles = [r.name for r in sm.find_user('gamma').roles]
-        self.assertIn('druid_role', user_roles)
-
-        # Case 4. Extend the role to have access to the druid datasource
-
-        access_request4 = prepare_request('druid', 'druid_ds_2', 'druid_role')
-        druid_ds_2_perm = access_request4.datasource.perm
-
-        self.client.get(EXTEND_ROLE_REQUEST.format(
-            'druid', access_request4.datasource_id, 'gamma', 'druid_role'))
-        # druid_role was extended to grant access to the druid_access_ds_2
-        druid_role = sm.find_role('druid_role')
-        perm_view = sm.find_permission_view_menu(
-            'datasource_access', druid_ds_2_perm)
-        self.assertIn(perm_view, druid_role.permissions)
-
-        # cleanup
-        gamma_user = sm.find_user(username='gamma')
-        gamma_user.roles.remove(sm.find_role('druid_role'))
-        gamma_user.roles.remove(sm.find_role('table_role'))
-        session.delete(sm.find_role('druid_role'))
-        session.delete(sm.find_role('table_role'))
-        session.commit()
-
-    def test_request_access(self):
-        session = db.session
-        self.login(username='gamma')
-        gamma_user = sm.find_user(username='gamma')
-        sm.add_role('dummy_role')
-        gamma_user.roles.append(sm.find_role('dummy_role'))
-        session.commit()
-
-        ACCESS_REQUEST = (
-            '/caravel/request_access?datasource_type={}&datasource_id={}')
-        ROLE_EXTEND_LINK = (
-            '<a href="/caravel/approve?datasource_type={}&datasource_id={}&'
-            'created_by={}&role_to_extend={}">Extend {} Role</a>')
-        ROLE_GRANT_LINK = (
-            '<a href="/caravel/approve?datasource_type={}&datasource_id={}&'
-            'created_by={}&role_to_grant={}">Grant {} Role</a>')
-
-        # Case 1. Request table access, there are no roles have this table.
-
-        table1 = session.query(models.SqlaTable).filter_by(
-            table_name='random_time_series').first()
-        table_1_id = table1.id
-
-        # request access to the table
-        self.client.get(ACCESS_REQUEST.format('table', table_1_id))
-
-        access_request1 = self.get_access_requests(
-            'gamma', 'table', table_1_id)[0]
-        approve_link_1 = ROLE_EXTEND_LINK.format(
-            'table', table_1_id, 'gamma', 'dummy_role', 'dummy_role')
-        self.assertEqual(
-            access_request1.user_roles,
-            '<ul><li>Gamma Role</li><li>{}</li></ul>'.format(approve_link_1))
-        self.assertEqual(access_request1.roles_with_datasource, '<ul></ul>')
-
-        # Case 2. Duplicate request.
-
-        self.client.get(ACCESS_REQUEST.format('table', table_1_id))
-        access_requests_2 = self.get_access_requests(
-            'gamma', 'table', table_1_id)
-        self.assertEqual(len(access_requests_2), 1)
-
-        # Case 3. Request access, roles exist that contains the table.
-
-        # add table to the existing roles
-        table3 = session.query(models.SqlaTable).filter_by(
-            table_name='energy_usage').first()
-        table_3_id = table3.id
-        table3_perm = table3.perm
-
-        sm.add_role('energy_usage_role')
-        alpha_role = sm.find_role('Alpha')
-        sm.add_permission_role(
-            alpha_role,
-            sm.find_permission_view_menu('datasource_access', table3_perm))
-        sm.add_permission_role(
-            sm.find_role("energy_usage_role"),
-            sm.find_permission_view_menu('datasource_access', table3_perm))
-        session.commit()
-
-        self.client.get(ACCESS_REQUEST.format('table', table_3_id))
-
-        access_request3 = self.get_access_requests(
-            'gamma', 'table', table_3_id)[0]
-        approve_link_3 = ROLE_GRANT_LINK.format(
-            'table', table_3_id, 'gamma', 'energy_usage_role',
-            'energy_usage_role')
-        self.assertEqual(access_request3.roles_with_datasource,
-                         '<ul><li>{}</li></ul>'.format(approve_link_3))
-
-        # Case 4. Request druid access, there are no roles have this table.
-        druid_ds_4 = session.query(models.DruidDatasource).filter_by(
-            datasource_name='druid_ds_1').first()
-        druid_ds_4_id = druid_ds_4.id
-
-        # request access to the table
-        self.client.get(ACCESS_REQUEST.format('druid', druid_ds_4_id))
-        access_request4 = self.get_access_requests(
-            'gamma', 'druid', druid_ds_4_id)[0]
-        approve_link_4 = ROLE_EXTEND_LINK.format(
-            'druid', druid_ds_4_id, 'gamma', 'dummy_role', 'dummy_role')
-        self.assertEqual(
-            access_request4.user_roles,
-            '<ul><li>Gamma Role</li><li>{}</li></ul>'.format(approve_link_4))
-
-        self.assertEqual(
-            access_request4.roles_with_datasource,
-            '<ul></ul>'.format(access_request4.id))
-
-        # Case 5. Roles exist that contains the druid datasource.
-        # add druid ds to the existing roles
-        druid_ds_5 = session.query(models.DruidDatasource).filter_by(
-            datasource_name='druid_ds_2').first()
-        druid_ds_5_id = druid_ds_5.id
-        druid_ds_5_perm = druid_ds_5.perm
-
-        druid_ds_2_role = sm.add_role('druid_ds_2_role')
-        admin_role = sm.find_role('Admin')
-        sm.add_permission_role(
-            admin_role,
-            sm.find_permission_view_menu('datasource_access', druid_ds_5_perm))
-        sm.add_permission_role(
-            druid_ds_2_role,
-            sm.find_permission_view_menu('datasource_access', druid_ds_5_perm))
-        session.commit()
-
-        self.client.get(ACCESS_REQUEST.format('druid', druid_ds_5_id))
-        access_request5 = self.get_access_requests(
-            'gamma', 'druid', druid_ds_5_id)[0]
-        approve_link_5 = ROLE_GRANT_LINK.format(
-            'druid', druid_ds_5_id, 'gamma', 'druid_ds_2_role',
-            'druid_ds_2_role')
-
-        self.assertEqual(access_request5.roles_with_datasource,
-                         '<ul><li>{}</li></ul>'.format(approve_link_5))
-
-        # cleanup
-        gamma_user = sm.find_user(username='gamma')
-        gamma_user.roles.remove(sm.find_role('dummy_role'))
-        session.commit()
-
-    def test_druid_sync_from_config(self):
-        self.login()
-        cluster = models.DruidCluster(cluster_name="new_druid")
-        db.session.add(cluster)
+        # cleaning up
+        dash = db.session.query(models.Dashboard).filter_by(
+            slug="births").first()
+        dash.slices = [
+            o for o in dash.slices if o.slice_name != "Mapbox Long/Lat"]
         db.session.commit()
 
-        cfg = {
-            "user": "admin",
-            "cluster": "new_druid",
-            "config": {
-                "name": "test_click",
-                "dimensions": ["affiliate_id", "campaign", "first_seen"],
-                "metrics_spec": [{"type": "count", "name": "count"},
-                                 {"type": "sum", "name": "sum"}],
-                "batch_ingestion": {
-                    "sql": "SELECT * FROM clicks WHERE d='{{ ds }}'",
-                    "ts_column": "d",
-                    "sources": [{
-                        "table": "clicks",
-                        "partition": "d='{{ ds }}'"
-                    }]
-                }
-            }
-        }
-        resp = self.client.post('/caravel/sync_druid/', data=json.dumps(cfg))
-
-        druid_ds = db.session.query(DruidDatasource).filter_by(
-            datasource_name="test_click").first()
-        assert set([c.column_name for c in druid_ds.columns]) == set(
-            ["affiliate_id", "campaign", "first_seen"])
-        assert set([m.metric_name for m in druid_ds.metrics]) == set(
-            ["count", "sum"])
-        assert resp.status_code == 201
-
-        # datasource exists, not changes required
-        resp = self.client.post('/caravel/sync_druid/', data=json.dumps(cfg))
-        druid_ds = db.session.query(DruidDatasource).filter_by(
-            datasource_name="test_click").first()
-        assert set([c.column_name for c in druid_ds.columns]) == set(
-            ["affiliate_id", "campaign", "first_seen"])
-        assert set([m.metric_name for m in druid_ds.metrics]) == set(
-            ["count", "sum"])
-        assert resp.status_code == 201
-
-        # datasource exists, add new metrics and dimentions
-        cfg = {
-            "user": "admin",
-            "cluster": "new_druid",
-            "config": {
-                "name": "test_click",
-                "dimensions": ["affiliate_id", "second_seen"],
-                "metrics_spec": [
-                    {"type": "bla", "name": "sum"},
-                    {"type": "unique", "name": "unique"}
-                ],
-            }
-        }
-        resp = self.client.post('/caravel/sync_druid/', data=json.dumps(cfg))
-        druid_ds = db.session.query(DruidDatasource).filter_by(
-            datasource_name="test_click").first()
-        # columns and metrics are not deleted if config is changed as
-        # user could define his own dimensions / metrics and want to keep them
-        assert set([c.column_name for c in druid_ds.columns]) == set(
-            ["affiliate_id", "campaign", "first_seen", "second_seen"])
-        assert set([m.metric_name for m in druid_ds.metrics]) == set(
-            ["count", "sum", "unique"])
-        # metric type will not be overridden, sum stays instead of bla
-        assert set([m.metric_type for m in druid_ds.metrics]) == set(
-            ["longSum", "sum", "unique"])
-        assert resp.status_code == 201
-
-    def test_filter_druid_datasource(self):
-        gamma_ds = DruidDatasource(
-            datasource_name="datasource_for_gamma",
-        )
-        db.session.add(gamma_ds)
-        no_gamma_ds = DruidDatasource(
-            datasource_name="datasource_not_for_gamma",
-        )
-        db.session.add(no_gamma_ds)
-        db.session.commit()
-        utils.merge_perm(sm, 'datasource_access', gamma_ds.perm)
-        utils.merge_perm(sm, 'datasource_access', no_gamma_ds.perm)
-        db.session.commit()
-
-        gamma_ds_permission_view = (
-            db.session.query(ab_models.PermissionView)
-            .join(ab_models.ViewMenu)
-            .filter(ab_models.ViewMenu.name == gamma_ds.perm)
-            .first()
-        )
-        sm.add_permission_role(sm.find_role('Gamma'), gamma_ds_permission_view)
-
-        self.login(username='gamma')
-        url = '/druiddatasourcemodelview/list/'
-        resp = self.client.get(url, follow_redirects=True)
-        assert 'datasource_for_gamma' in resp.data.decode('utf-8')
-        assert 'datasource_not_for_gamma' not in resp.data.decode('utf-8')
-
-    def test_add_filter(self, username='admin'):
-        # navigate to energy_usage slice with "Electricity,heat" in filter values
-        data = (
-            "/caravel/explore/table/1/?viz_type=table&groupby=source&metric=count&flt_col_1=source&flt_op_1=in&flt_eq_1=%27Electricity%2Cheat%27"
-            "&userid=1&datasource_name=energy_usage&datasource_id=1&datasource_type=tablerdo_save=saveas")
-        resp = self.client.get(
-            data,
-            follow_redirects=True)
-        assert ("source" in resp.data.decode('utf-8'))
-
     def test_gamma(self):
         self.login(username='gamma')
-        resp = self.client.get('/slicemodelview/list/')
-        assert "List Slice" in resp.data.decode('utf-8')
-
-        resp = self.client.get('/dashboardmodelview/list/')
-        assert "List Dashboard" in resp.data.decode('utf-8')
+        assert "List Slice" in self.get_resp('/slicemodelview/list/')
+        assert "List Dashboard" in self.get_resp('/dashboardmodelview/list/')
 
     def run_sql(self, sql, user_name, client_id):
         self.login(username=user_name)
@@ -633,8 +299,8 @@ class CoreTests(CaravelTestCase):
         self.run_sql(sql, 'admin', client_id)
 
         self.login('admin')
-        resp = self.client.get('/caravel/csv/{}'.format(client_id))
-        data = csv.reader(io.StringIO(resp.data.decode('utf-8')))
+        resp = self.get_resp('/caravel/csv/{}'.format(client_id))
+        data = csv.reader(io.StringIO(resp))
         expected_data = csv.reader(
             io.StringIO("first_name,last_name\nadmin, user\n"))
 
@@ -646,16 +312,16 @@ class CoreTests(CaravelTestCase):
         self.assertEquals(403, resp.status_code)
 
         self.login('admin')
-        resp = self.client.get('/caravel/queries/{}'.format(0))
-        data = json.loads(resp.data.decode('utf-8'))
+        resp = self.get_resp('/caravel/queries/{}'.format(0))
+        data = json.loads(resp)
         self.assertEquals(0, len(data))
         self.logout()
 
         self.run_sql("SELECT * FROM ab_user", 'admin', client_id='client_id_1')
         self.run_sql("SELECT * FROM ab_user1", 'admin', client_id='client_id_2')
         self.login('admin')
-        resp = self.client.get('/caravel/queries/{}'.format(0))
-        data = json.loads(resp.data.decode('utf-8'))
+        resp = self.get_resp('/caravel/queries/{}'.format(0))
+        data = json.loads(resp)
         self.assertEquals(2, len(data))
 
         query = db.session.query(models.Query).filter_by(
@@ -663,8 +329,8 @@ class CoreTests(CaravelTestCase):
         query.changed_on = utils.EPOCH
         db.session.commit()
 
-        resp = self.client.get('/caravel/queries/{}'.format(123456000))
-        data = json.loads(resp.data.decode('utf-8'))
+        resp = self.get_resp('/caravel/queries/{}'.format(123456000))
+        data = json.loads(resp)
         self.assertEquals(1, len(data))
 
         self.logout()
@@ -685,38 +351,29 @@ class CoreTests(CaravelTestCase):
         self.revoke_public_access('birth_names')
         self.logout()
 
-        resp = self.client.get('/slicemodelview/list/')
-        data = resp.data.decode('utf-8')
+        resp = self.get_resp('/slicemodelview/list/')
+        assert 'birth_names</a>' not in resp
 
-        assert 'birth_names</a>' not in data
-
-        resp = self.client.get('/dashboardmodelview/list/')
-        data = resp.data.decode('utf-8')
-        assert '/caravel/dashboard/births/' not in data
+        resp = self.get_resp('/dashboardmodelview/list/')
+        assert '/caravel/dashboard/births/' not in resp
 
         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
+        assert 'birth_names' in self.get_resp('/slicemodelview/list/')
 
-        resp = self.client.get('/dashboardmodelview/list/')
-        data = resp.data.decode('utf-8')
-        assert "/caravel/dashboard/births/" in data
+        resp = self.get_resp('/dashboardmodelview/list/')
+        assert "/caravel/dashboard/births/" in resp
 
-        resp = self.client.get('/caravel/dashboard/births/')
-        data = resp.data.decode('utf-8')
-        assert 'Births' in data
+        print(self.get_resp('/caravel/dashboard/births/'))
+        assert 'Births' in self.get_resp('/caravel/dashboard/births/')
 
         # 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</a>' not in data
+        resp = self.get_resp('/slicemodelview/list/')
+        assert 'wb_health_population</a>' not in resp
 
-        resp = self.client.get('/dashboardmodelview/list/')
-        data = resp.data.decode('utf-8')
-        assert "/caravel/dashboard/world_health/" not in data
+        resp = self.get_resp('/dashboardmodelview/list/')
+        assert "/caravel/dashboard/world_health/" not in resp
 
     def test_only_owners_can_save(self):
         dash = (

tests/druid_tests.py

@@ -5,14 +5,16 @@ from __future__ import print_function
 from __future__ import unicode_literals
 
 from datetime import datetime
+import json
 import unittest
 
 from mock import Mock, patch
 
-from caravel import db
-from caravel.models import DruidCluster
+from caravel import db, sm, utils
+from caravel.models import DruidCluster, DruidDatasource
 
 from .base_tests import CaravelTestCase
+from flask_appbuilder.security.sqla import models as ab_models
 
 
 SEGMENT_METADATA = [{
@@ -126,5 +128,112 @@ class DruidTests(CaravelTestCase):
             'force=true'.format(datasource_id, datasource_id))
         assert "Canada" in resp.data.decode('utf-8')
 
+    def test_druid_sync_from_config(self):
+        self.login()
+        cluster = DruidCluster(cluster_name="new_druid")
+        db.session.add(cluster)
+        db.session.commit()
+
+        cfg = {
+            "user": "admin",
+            "cluster": "new_druid",
+            "config": {
+                "name": "test_click",
+                "dimensions": ["affiliate_id", "campaign", "first_seen"],
+                "metrics_spec": [{"type": "count", "name": "count"},
+                                 {"type": "sum", "name": "sum"}],
+                "batch_ingestion": {
+                    "sql": "SELECT * FROM clicks WHERE d='{{ ds }}'",
+                    "ts_column": "d",
+                    "sources": [{
+                        "table": "clicks",
+                        "partition": "d='{{ ds }}'"
+                    }]
+                }
+            }
+        }
+        resp = self.client.post('/caravel/sync_druid/', data=json.dumps(cfg))
+
+        druid_ds = db.session.query(DruidDatasource).filter_by(
+            datasource_name="test_click").first()
+        assert set([c.column_name for c in druid_ds.columns]) == set(
+            ["affiliate_id", "campaign", "first_seen"])
+        assert set([m.metric_name for m in druid_ds.metrics]) == set(
+            ["count", "sum"])
+        assert resp.status_code == 201
+
+        # datasource exists, not changes required
+        resp = self.client.post('/caravel/sync_druid/', data=json.dumps(cfg))
+        druid_ds = db.session.query(DruidDatasource).filter_by(
+            datasource_name="test_click").first()
+        assert set([c.column_name for c in druid_ds.columns]) == set(
+            ["affiliate_id", "campaign", "first_seen"])
+        assert set([m.metric_name for m in druid_ds.metrics]) == set(
+            ["count", "sum"])
+        assert resp.status_code == 201
+
+        # datasource exists, add new metrics and dimentions
+        cfg = {
+            "user": "admin",
+            "cluster": "new_druid",
+            "config": {
+                "name": "test_click",
+                "dimensions": ["affiliate_id", "second_seen"],
+                "metrics_spec": [
+                    {"type": "bla", "name": "sum"},
+                    {"type": "unique", "name": "unique"}
+                ],
+            }
+        }
+        resp = self.client.post('/caravel/sync_druid/', data=json.dumps(cfg))
+        druid_ds = db.session.query(DruidDatasource).filter_by(
+            datasource_name="test_click").first()
+        # columns and metrics are not deleted if config is changed as
+        # user could define his own dimensions / metrics and want to keep them
+        assert set([c.column_name for c in druid_ds.columns]) == set(
+            ["affiliate_id", "campaign", "first_seen", "second_seen"])
+        assert set([m.metric_name for m in druid_ds.metrics]) == set(
+            ["count", "sum", "unique"])
+        # metric type will not be overridden, sum stays instead of bla
+        assert set([m.metric_type for m in druid_ds.metrics]) == set(
+            ["longSum", "sum", "unique"])
+        assert resp.status_code == 201
+
+    def test_filter_druid_datasource(self):
+        gamma_ds = DruidDatasource(
+            datasource_name="datasource_for_gamma",
+        )
+        db.session.add(gamma_ds)
+        no_gamma_ds = DruidDatasource(
+            datasource_name="datasource_not_for_gamma",
+        )
+        db.session.add(no_gamma_ds)
+        db.session.commit()
+        utils.merge_perm(sm, 'datasource_access', gamma_ds.perm)
+        utils.merge_perm(sm, 'datasource_access', no_gamma_ds.perm)
+        db.session.commit()
+
+        gamma_ds_permission_view = (
+            db.session.query(ab_models.PermissionView)
+            .join(ab_models.ViewMenu)
+            .filter(ab_models.ViewMenu.name == gamma_ds.perm)
+            .first()
+        )
+        sm.add_permission_role(sm.find_role('Gamma'), gamma_ds_permission_view)
+
+        self.login(username='gamma')
+        url = '/druiddatasourcemodelview/list/'
+        resp = self.get_resp(url)
+        assert 'datasource_for_gamma' in resp
+        assert 'datasource_not_for_gamma' not in resp
+
+    def test_add_filter(self, username='admin'):
+        # navigate to energy_usage slice with "Electricity,heat" in filter values
+        data = (
+            "/caravel/explore/table/1/?viz_type=table&groupby=source&metric=count&flt_col_1=source&flt_op_1=in&flt_eq_1=%27Electricity%2Cheat%27"
+            "&userid=1&datasource_name=energy_usage&datasource_id=1&datasource_type=tablerdo_save=saveas")
+        assert "source" in self.get_resp(data)
+
+
 if __name__ == '__main__':
     unittest.main()