Fix email shows as unverified when no email server is configured (#3613)

* check that e-mail server is configured before marking the email address
as not verified and sending out a verification e-mail

* use helper method in `invite_user`

* move email_server_configured helper to settings

* add test to verify that email addresses arent marked as unverified if
there's no e-mail server to verify them

* simplify a couple of tests with patch

* combine conditions into single variable

* Booleans, gotta love 'em

redash/handlers/authentication.py

@@ -224,7 +224,7 @@ def client_config():
         'showPermissionsControl': current_org.get_setting("feature_show_permissions_control"),
         'allowCustomJSVisualizations': settings.FEATURE_ALLOW_CUSTOM_JS_VISUALIZATIONS,
         'autoPublishNamedQueries': settings.FEATURE_AUTO_PUBLISH_NAMED_QUERIES,
-        'mailSettingsMissing': settings.MAIL_DEFAULT_SENDER is None,
+        'mailSettingsMissing': not settings.email_server_is_configured(),
         'dashboardRefreshIntervals': settings.DASHBOARD_REFRESH_INTERVALS,
         'queryRefreshIntervals': settings.QUERY_REFRESH_INTERVALS,
         'googleLoginEnabled': settings.GOOGLE_OAUTH_ENABLED,

redash/handlers/users.py

@@ -38,11 +38,10 @@
 
 
 def invite_user(org, inviter, user, send_email=True):
-    email_configured = settings.MAIL_DEFAULT_SENDER is not None
     d = user.to_dict()
 
     invite_url = invite_link_for_user(user)
-    if email_configured and send_email:
+    if settings.email_server_is_configured() and send_email:
         send_invite_email(inviter, user, invite_url, org)
     else:
         d['invite_link'] = invite_url
@@ -229,15 +228,16 @@ def post(self, user_id):
             if domain.lower() in blacklist or domain.lower() == 'qq.com':
                 abort(400, message='Bad email address.')
 
-        email_changed = 'email' in params and params['email'] != user.email
-        if email_changed:
+        email_address_changed = 'email' in params and params['email'] != user.email
+        needs_to_verify_email = email_address_changed and settings.email_server_is_configured()
+        if needs_to_verify_email:
             user.is_email_verified = False
 
         try:
             self.update_model(user, params)
             models.db.session.commit()
 
-            if email_changed:
+            if needs_to_verify_email:
                 send_verify_email(user, self.current_org)
 
             # The user has updated their email or password. This should invalidate all _other_ sessions,

redash/settings/__init__.py

@@ -215,6 +215,11 @@ def all_settings():
 MAIL_MAX_EMAILS = os.environ.get('REDASH_MAIL_MAX_EMAILS', None)
 MAIL_ASCII_ATTACHMENTS = parse_boolean(os.environ.get('REDASH_MAIL_ASCII_ATTACHMENTS', 'false'))
 
+
+def email_server_is_configured():
+    return MAIL_DEFAULT_SENDER is not None
+
+
 HOST = os.environ.get('REDASH_HOST', '')
 
 ALERTS_DEFAULT_MAIL_SUBJECT_TEMPLATE = os.environ.get('REDASH_ALERTS_DEFAULT_MAIL_SUBJECT_TEMPLATE', "({state}) {alert_name}")

tests/handlers/test_users.py

@@ -40,10 +40,8 @@ def test_creates_user(self):
         self.assertEqual(rv.json['name'], test_user['name'])
         self.assertEqual(rv.json['email'], test_user['email'])
 
-    def test_shows_invite_link_when_email_is_not_configured(self):
-        previous = settings.MAIL_DEFAULT_SENDER
-        settings.MAIL_DEFAULT_SENDER = None
-
+    @patch('redash.settings.email_server_is_configured', return_value=False)
+    def test_shows_invite_link_when_email_is_not_configured(self, _):
         admin = self.factory.create_admin()
 
         test_user = {'name': 'User', 'email': 'user@example.com'}
@@ -52,12 +50,8 @@ def test_shows_invite_link_when_email_is_not_configured(self):
         self.assertEqual(rv.status_code, 200)
         self.assertTrue('invite_link' in rv.json)
 
-        settings.MAIL_DEFAULT_SENDER = previous
-
-    def test_does_not_show_invite_link_when_email_is_configured(self):
-        previous = settings.MAIL_DEFAULT_SENDER
-        settings.MAIL_DEFAULT_SENDER = "john@doe.com"
-
+    @patch('redash.settings.email_server_is_configured', return_value=True)
+    def test_does_not_show_invite_link_when_email_is_configured(self, _):
         admin = self.factory.create_admin()
 
         test_user = {'name': 'User', 'email': 'user@example.com'}
@@ -66,8 +60,6 @@ def test_does_not_show_invite_link_when_email_is_configured(self):
         self.assertEqual(rv.status_code, 200)
         self.assertFalse('invite_link' in rv.json)
 
-        settings.MAIL_DEFAULT_SENDER = previous
-
     def test_creates_user_case_insensitive_email(self):
         admin = self.factory.create_admin()
 
@@ -230,12 +222,20 @@ def test_returns_200_for_non_admin_changing_his_own(self):
         rv = self.make_request('post', "/api/users/{}".format(self.factory.user.id), data={"name": "New Name"})
         self.assertEqual(rv.status_code, 200)
 
-    def test_marks_email_as_not_verified_when_changed(self):
+    @patch('redash.settings.email_server_is_configured', return_value=True)
+    def test_marks_email_as_not_verified_when_changed(self, _):
         user = self.factory.user
         user.is_email_verified = True
         rv = self.make_request('post', "/api/users/{}".format(user.id), data={"email": "donald@trump.biz"})
         self.assertFalse(user.is_email_verified)
 
+    @patch('redash.settings.email_server_is_configured', return_value=False)
+    def test_doesnt_mark_email_as_not_verified_when_changed_and_email_server_is_not_configured(self, _):
+        user = self.factory.user
+        user.is_email_verified = True
+        rv = self.make_request('post', "/api/users/{}".format(user.id), data={"email": "donald@trump.biz"})
+        self.assertTrue(user.is_email_verified)
+
     def test_returns_200_for_admin_changing_other_user(self):
         admin = self.factory.create_admin()