From ee7636b87105d9ae66a48fc0adf3c8e90cab1f25 Mon Sep 17 00:00:00 2001 From: Omer Lachish Date: Wed, 20 Mar 2019 12:36:28 +0200 Subject: [PATCH 1/7] check that e-mail server is configured before marking the email address as not verified and sending out a verification e-mail --- redash/handlers/users.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/redash/handlers/users.py b/redash/handlers/users.py index cabbe39dc5..ece3358373 100644 --- a/redash/handlers/users.py +++ b/redash/handlers/users.py @@ -37,6 +37,10 @@ ) +def email_server_is_configured(): + return settings.MAIL_DEFAULT_SENDER is not None + + def invite_user(org, inviter, user, send_email=True): email_configured = settings.MAIL_DEFAULT_SENDER is not None d = user.to_dict() @@ -229,15 +233,15 @@ 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 + if email_address_changed and email_server_is_configured(): user.is_email_verified = False try: self.update_model(user, params) models.db.session.commit() - if email_changed: + if email_address_changed and email_server_is_configured(): send_verify_email(user, self.current_org) # The user has updated their email or password. This should invalidate all _other_ sessions, From 89af564fc25d72cdcd449fccb6c33198aaf19135 Mon Sep 17 00:00:00 2001 From: Omer Lachish Date: Wed, 20 Mar 2019 12:37:30 +0200 Subject: [PATCH 2/7] use helper method in `invite_user` --- redash/handlers/users.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/redash/handlers/users.py b/redash/handlers/users.py index ece3358373..cac7840424 100644 --- a/redash/handlers/users.py +++ b/redash/handlers/users.py @@ -42,11 +42,10 @@ def email_server_is_configured(): 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 email_server_is_configured() and send_email: send_invite_email(inviter, user, invite_url, org) else: d['invite_link'] = invite_url From 76575946fec80e23465703aa3a89ac667fd56261 Mon Sep 17 00:00:00 2001 From: Omer Lachish Date: Wed, 20 Mar 2019 13:13:27 +0200 Subject: [PATCH 3/7] move email_server_configured helper to settings --- redash/handlers/authentication.py | 2 +- redash/handlers/users.py | 10 +++------- redash/settings/__init__.py | 5 +++++ 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/redash/handlers/authentication.py b/redash/handlers/authentication.py index 610575c1ab..61b647eb5c 100644 --- a/redash/handlers/authentication.py +++ b/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': settings.email_server_is_configured(), 'dashboardRefreshIntervals': settings.DASHBOARD_REFRESH_INTERVALS, 'queryRefreshIntervals': settings.QUERY_REFRESH_INTERVALS, 'googleLoginEnabled': settings.GOOGLE_OAUTH_ENABLED, diff --git a/redash/handlers/users.py b/redash/handlers/users.py index cac7840424..1117d68b4c 100644 --- a/redash/handlers/users.py +++ b/redash/handlers/users.py @@ -37,15 +37,11 @@ ) -def email_server_is_configured(): - return settings.MAIL_DEFAULT_SENDER is not None - - def invite_user(org, inviter, user, send_email=True): d = user.to_dict() invite_url = invite_link_for_user(user) - if email_server_is_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 @@ -233,14 +229,14 @@ def post(self, user_id): abort(400, message='Bad email address.') email_address_changed = 'email' in params and params['email'] != user.email - if email_address_changed and email_server_is_configured(): + if email_address_changed and settings.email_server_is_configured(): user.is_email_verified = False try: self.update_model(user, params) models.db.session.commit() - if email_address_changed and email_server_is_configured(): + if email_address_changed and settings.email_server_is_configured(): send_verify_email(user, self.current_org) # The user has updated their email or password. This should invalidate all _other_ sessions, diff --git a/redash/settings/__init__.py b/redash/settings/__init__.py index efb2a1cd25..460f8d04dd 100644 --- a/redash/settings/__init__.py +++ b/redash/settings/__init__.py @@ -141,6 +141,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}") From fc4b791b6c64d15c0ae2a24c1a77870d559c5869 Mon Sep 17 00:00:00 2001 From: Omer Lachish Date: Wed, 20 Mar 2019 13:13:39 +0200 Subject: [PATCH 4/7] add test to verify that email addresses arent marked as unverified if there's no e-mail server to verify them --- tests/handlers/test_users.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tests/handlers/test_users.py b/tests/handlers/test_users.py index 1541e12e2b..615cf2d951 100644 --- a/tests/handlers/test_users.py +++ b/tests/handlers/test_users.py @@ -230,12 +230,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() From 1b7d0b6b58631105690d33d017c3a0c2ece392b5 Mon Sep 17 00:00:00 2001 From: Omer Lachish Date: Wed, 20 Mar 2019 13:17:08 +0200 Subject: [PATCH 5/7] simplify a couple of tests with patch --- tests/handlers/test_users.py | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/tests/handlers/test_users.py b/tests/handlers/test_users.py index 615cf2d951..7f4d55e0d8 100644 --- a/tests/handlers/test_users.py +++ b/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() From 2674f16161d5042220cf8317b21ce78f63c606f2 Mon Sep 17 00:00:00 2001 From: Omer Lachish Date: Wed, 20 Mar 2019 13:20:15 +0200 Subject: [PATCH 6/7] combine conditions into single variable --- redash/handlers/users.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/redash/handlers/users.py b/redash/handlers/users.py index 1117d68b4c..7a57e0a99f 100644 --- a/redash/handlers/users.py +++ b/redash/handlers/users.py @@ -229,14 +229,15 @@ def post(self, user_id): abort(400, message='Bad email address.') email_address_changed = 'email' in params and params['email'] != user.email - if email_address_changed and settings.email_server_is_configured(): + 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_address_changed and settings.email_server_is_configured(): + 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, From 8d1a7fd95529c8613837c43faf162dc6abae7868 Mon Sep 17 00:00:00 2001 From: Omer Lachish Date: Wed, 20 Mar 2019 14:39:30 +0200 Subject: [PATCH 7/7] Booleans, gotta love 'em --- redash/handlers/authentication.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/redash/handlers/authentication.py b/redash/handlers/authentication.py index 61b647eb5c..2776816033 100644 --- a/redash/handlers/authentication.py +++ b/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.email_server_is_configured(), + 'mailSettingsMissing': not settings.email_server_is_configured(), 'dashboardRefreshIntervals': settings.DASHBOARD_REFRESH_INTERVALS, 'queryRefreshIntervals': settings.QUERY_REFRESH_INTERVALS, 'googleLoginEnabled': settings.GOOGLE_OAUTH_ENABLED,