Fix initialization of Database sqlalchemy_uri and password (#1137)

* move initialization of Database sqlalchemy_uri and password from DatabaseView.pre_add to utils.get_or_create_main_db.
Unit tests for mysql and postgres include username and password in the SQLALCHEMY_DATABASE_URI.

* modified test_testconn to work with sqlalchemy uri with a username and password.

.travis.yml

@@ -27,7 +27,10 @@ before_install:
   - npm install -g npm@'>=3.9.5'
 before_script:
   - mysql -e 'drop database if exists caravel; create database caravel DEFAULT CHARACTER SET utf8 COLLATE utf8_unicode_ci' -u root
+  - mysql -u root -e "CREATE USER 'mysqluser'@'localhost' IDENTIFIED BY 'mysqluserpassword';"
+  - mysql -u root -e "GRANT ALL ON caravel.* TO 'mysqluser'@'localhost';"
   - psql -c 'create database caravel;' -U postgres
+  - psql -c "CREATE USER postgresuser WITH PASSWORD 'pguserpassword';" -U postgres
   - export PATH=${PATH}:/tmp/hive/bin
 install:
   - pip install --upgrade pip

caravel/models.py

@@ -433,6 +433,12 @@ class Database(Model, AuditMixinNullable):
         url = make_url(self.sqlalchemy_uri_decrypted)
         return url.get_backend_name()
 
+    def set_sqlalchemy_uri(self, uri):
+        conn = sqla.engine.url.make_url(uri)
+        self.password = conn.password
+        conn.password = "X" * 10 if conn.password else None
+        self.sqlalchemy_uri = str(conn)  # hides the password
+
     def get_sqla_engine(self, schema=None):
         extra = self.get_extra()
         url = make_url(self.sqlalchemy_uri_decrypted)

caravel/utils.py

@@ -100,7 +100,7 @@ def get_or_create_main_db(caravel):
     if not dbobj:
         dbobj = DB(database_name="main")
     logging.info(config.get("SQLALCHEMY_DATABASE_URI"))
-    dbobj.sqlalchemy_uri = config.get("SQLALCHEMY_DATABASE_URI")
+    dbobj.set_sqlalchemy_uri(config.get("SQLALCHEMY_DATABASE_URI"))
     dbobj.expose_in_sqllab = True
     db.session.add(dbobj)
     db.session.commit()

caravel/views.py

@@ -497,10 +497,6 @@ class DatabaseView(CaravelModelView, DeleteMixin):  # noqa
     }
 
     def pre_add(self, db):
-        conn = sqla.engine.url.make_url(db.sqlalchemy_uri)
-        db.password = conn.password
-        conn.password = "X" * 10 if conn.password else None
-        db.sqlalchemy_uri = str(conn)  # hides the password
         utils.merge_perm(sm, 'database_access', db.perm)
 
     def pre_update(self, db):
@@ -1252,15 +1248,17 @@ class Caravel(BaseCaravelView):
         try:
             uri = request.json.get('uri')
             db_name = request.json.get('name')
-            if db_name and ':XXXXXXXXXX@' in uri:
+            if db_name:
                 database = (
-                    db.session()
+                    db.session
                     .query(models.Database)
-                    .filter_by(database_name=db_name).first()
+                    .filter_by(database_name=db_name)
+                    .first()
                 )
-                if database is not None:
+                if uri == database.safe_sqlalchemy_uri():
+                    # the password-masked uri was passed
+                    # use the URI associated with this database
                     uri = database.sqlalchemy_uri_decrypted
-
             connect_args = (
                 request.json
                 .get('extras', {})

tests/core_tests.py

@@ -115,17 +115,30 @@ class CoreTests(CaravelTestCase):
         assert self.client.get('/ping').data.decode('utf-8') == "OK"
 
     def test_testconn(self):
-        data = json.dumps({'uri': 'sqlite:////tmp/caravel_unittests.db'})
-        response = self.client.post('/caravel/testconn', data=data, content_type='application/json')
-        assert response.status_code == 200
+        database = (
+            db.session
+            .query(models.Database)
+            .filter_by(database_name='main')
+            .first()
+        )
 
+        # validate that the endpoint works with the password-masked sqlalchemy uri
         data = json.dumps({
-            'uri': 'postgresql+psycopg2://foo:XXXXXXXXXX@127.0.0.1/bar',
+            'uri': database.safe_sqlalchemy_uri(),
             'name': 'main'
         })
         response = self.client.post('/caravel/testconn', data=data, content_type='application/json')
         assert response.status_code == 200
 
+        # validate that the endpoint works with the decrypted sqlalchemy uri
+        data = json.dumps({
+            'uri': database.sqlalchemy_uri_decrypted,
+            'name': 'main'
+        })
+        response = self.client.post('/caravel/testconn', data=data, content_type='application/json')
+        assert response.status_code == 200
+
+
     def test_warm_up_cache(self):
         slice = db.session.query(models.Slice).first()
         resp = self.client.get(

tox.ini

@@ -37,17 +37,17 @@ commands =
 [testenv:py27-mysql]
 basepython = python2.7
 setenv =
-    CARAVEL__SQLALCHEMY_DATABASE_URI = mysql://root@localhost/caravel
+    CARAVEL__SQLALCHEMY_DATABASE_URI = mysql://mysqluser:mysqluserpassword@localhost/caravel
 
 [testenv:py34-mysql]
 basepython = python3.4
 setenv =
-    CARAVEL__SQLALCHEMY_DATABASE_URI = mysql://root@localhost/caravel
+    CARAVEL__SQLALCHEMY_DATABASE_URI = mysql://mysqluser:mysqluserpassword@localhost/caravel
 
 [testenv:py35-mysql]
 basepython = python3.5
 setenv =
-    CARAVEL__SQLALCHEMY_DATABASE_URI = mysql://root@localhost/caravel
+    CARAVEL__SQLALCHEMY_DATABASE_URI = mysql://mysqluser:mysqluserpassword@localhost/caravel
 
 [testenv:py27-sqlite]
 basepython = python2.7
@@ -62,12 +62,12 @@ setenv =
 [testenv:py27-postgres]
 basepython = python2.7
 setenv =
-    CARAVEL__SQLALCHEMY_DATABASE_URI = postgresql+psycopg2://postgres@localhost/caravel
+    CARAVEL__SQLALCHEMY_DATABASE_URI = postgresql+psycopg2://postgresuser:pguserpassword@localhost/caravel
 
 [testenv:py34-postgres]
 basepython = python3.4
 setenv =
-    CARAVEL__SQLALCHEMY_DATABASE_URI = postgresql+psycopg2://postgres@localhost/caravel
+    CARAVEL__SQLALCHEMY_DATABASE_URI = postgresql+psycopg2://postgresuser:pguserpassword@localhost/caravel
 
 [testenv:javascript]
 commands = {toxinidir}/caravel/assets/js_build.sh