From 1ce0e8256bda4528baa6b5f8a81308a030323825 Mon Sep 17 00:00:00 2001 From: Tim Abbott Date: Thu, 28 Feb 2019 15:12:40 -0800 Subject: [PATCH] zoom: Avoid sending Zoom API secret to other admin clients. Fixing this involves fixing the backend to handle unchanged field submissions of the Zoom credentials without trying to re-validate the credentials (for performance) as well as to fetch the already-sent secret. --- .../organization-settings-admin.handlebars | 2 +- zerver/lib/actions.py | 4 +++ zerver/lib/events.py | 4 +++ zerver/tests/test_events.py | 6 +++- zerver/tests/test_realm.py | 28 ++++++++++++++++++- zerver/views/realm.py | 10 ++++++- 6 files changed, 50 insertions(+), 4 deletions(-) diff --git a/static/templates/settings/organization-settings-admin.handlebars b/static/templates/settings/organization-settings-admin.handlebars index 3616976fc3..85c17d30cb 100644 --- a/static/templates/settings/organization-settings-admin.handlebars +++ b/static/templates/settings/organization-settings-admin.handlebars @@ -189,7 +189,7 @@ class="admin-realm-zoom-field"/>
- + None: setattr(realm, name, value) realm.save(update_fields=[name]) + + if name == 'zoom_api_secret': + # Send '' as the value through the API for the API secret + value = '' event = dict( type='realm', op='update', diff --git a/zerver/lib/events.py b/zerver/lib/events.py index 602784cfe1..f9482d793c 100644 --- a/zerver/lib/events.py +++ b/zerver/lib/events.py @@ -168,6 +168,10 @@ def fetch_initial_state_data(user_profile: UserProfile, for property_name in Realm.property_types: state['realm_' + property_name] = getattr(realm, property_name) + # Don't send the zoom API secret to clients. + if state.get('realm_zoom_api_secret'): + state['realm_zoom_api_secret'] = '' + # Most state is handled via the property_types framework; # these manual entries are for those realm settings that don't # fit into that framework. diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index 053cf61241..88ac4d878c 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -1567,8 +1567,12 @@ class EventsRegisterTest(ZulipTestCase): raise AssertionError('No test created for %s' % (name)) do_set_realm_property(self.user_profile.realm, name, vals[0]) for val in vals[1:]: + state_change_expected = True + if name == "zoom_api_secret": + state_change_expected = False events = self.do_test( - lambda: do_set_realm_property(self.user_profile.realm, name, val)) + lambda: do_set_realm_property(self.user_profile.realm, name, val), + state_change_expected=state_change_expected) error = schema_checker('events[0]', events[0]) self.assert_on_error(error) diff --git a/zerver/tests/test_realm.py b/zerver/tests/test_realm.py index de03ef9059..158035cede 100644 --- a/zerver/tests/test_realm.py +++ b/zerver/tests/test_realm.py @@ -441,7 +441,8 @@ class RealmTest(ZulipTestCase): result = self.client_patch('/json/realm', req) self.assert_json_error(result, "Invalid credentials for the Zoom API.") - with mock.patch("zerver.views.realm.request_zoom_video_call_url", return_value={'join_url': 'example.com'}): + with mock.patch("zerver.views.realm.request_zoom_video_call_url", + return_value={'join_url': 'example.com'}) as mock_validation: req = { "video_chat_provider": ujson.dumps("Zoom"), "zoom_user_id": ujson.dumps("example@example.com"), @@ -450,6 +451,31 @@ class RealmTest(ZulipTestCase): } result = self.client_patch('/json/realm', req) self.assert_json_success(result) + mock_validation.assert_called_once() + + with mock.patch("zerver.views.realm.request_zoom_video_call_url", + return_value={'join_url': 'example.com'}) as mock_validation: + req = { + "video_chat_provider": ujson.dumps("Zoom"), + "zoom_user_id": ujson.dumps("example@example.com"), + "zoom_api_key": ujson.dumps("abc"), + "zoom_api_secret": ujson.dumps("abc"), + } + result = self.client_patch('/json/realm', req) + self.assert_json_success(result) + mock_validation.assert_not_called() + + with mock.patch("zerver.views.realm.request_zoom_video_call_url", + return_value={'join_url': 'example.com'}) as mock_validation: + req = { + "video_chat_provider": ujson.dumps("Zoom"), + "zoom_user_id": ujson.dumps("example@example.com"), + "zoom_api_key": ujson.dumps("abc"), + "zoom_api_secret": ujson.dumps(""), + } + result = self.client_patch('/json/realm', req) + self.assert_json_success(result) + mock_validation.assert_not_called() def test_initial_plan_type(self) -> None: with self.settings(BILLING_ENABLED=True): diff --git a/zerver/views/realm.py b/zerver/views/realm.py index 0711ba8fce..3eb38417b8 100644 --- a/zerver/views/realm.py +++ b/zerver/views/realm.py @@ -86,18 +86,26 @@ def update_realm( except ValidationError as e: return json_error(_('Invalid domain: {}').format(e.messages[0])) if video_chat_provider == "Zoom": + if not zoom_api_secret: + # Use the saved Zoom API secret if a new value isn't being sent + zoom_api_secret = user_profile.realm.zoom_api_secret if not zoom_user_id: return json_error(_('User ID cannot be empty')) if not zoom_api_key: return json_error(_('API key cannot be empty')) if not zoom_api_secret: return json_error(_('API secret cannot be empty')) + # If any of the Zoom settings have changed, validate the Zoom credentials. + # # Technically, we could call some other API endpoint that # doesn't create a video call link, but this is a nicer # end-to-end test, since it verifies that the Zoom API user's # scopes includes the ability to create video calls, which is # the only capabiility we use. - if not request_zoom_video_call_url(zoom_user_id, zoom_api_key, zoom_api_secret): + if ((zoom_user_id != realm.zoom_user_id or + zoom_api_key != realm.zoom_api_key or + zoom_api_secret != realm.zoom_api_secret) and + not request_zoom_video_call_url(zoom_user_id, zoom_api_key, zoom_api_secret)): return json_error(_('Invalid credentials for the %(third_party_service)s API.') % dict( third_party_service="Zoom"))