diff --git a/api/zulip/__init__.py b/api/zulip/__init__.py index 45a5756a2b..209be5df49 100644 --- a/api/zulip/__init__.py +++ b/api/zulip/__init__.py @@ -641,13 +641,30 @@ class Client(object): request=request, ) + def get_stream_id(self, stream): + # type: (str) -> Dict[str, Any] + ''' + Example usage: client.get_stream_id('devel') + ''' + stream_encoded = urllib.parse.quote(stream, safe='') + url = 'get_stream_id?stream=%s' % (stream_encoded,) + return self.call_endpoint( + url=url, + method='GET', + request=None, + ) + def get_subscribers(self, **request): # type: (**Any) -> Dict[str, Any] ''' Example usage: client.get_subscribers(stream='devel') ''' - stream = urllib.parse.quote(request['stream'], safe='') - url = 'streams/%s/members' % (stream,) + request_stream_id = self.get_stream_id(request['stream']) + try: + stream_id = request_stream_id['stream_id'] + except KeyError: + return request_stream_id + url = 'streams/%d/members' % (stream_id,) return self.call_endpoint( url=url, method='GET', diff --git a/static/js/admin.js b/static/js/admin.js index 274e661ecb..b0f2bfbd10 100644 --- a/static/js/admin.js +++ b/static/js/admin.js @@ -778,8 +778,10 @@ function _setup_page() { } $("#deactivation_stream_modal").modal("hide"); $(".active_stream_row button").prop("disabled", true).text("Working…"); + var stream_name = $(".active_stream_row").find('.stream_name').text(); + var stream_id = stream_data.get_sub(stream_name).stream_id; channel.del({ - url: '/json/streams/' + encodeURIComponent($(".active_stream_row").find('.stream_name').text()), + url: '/json/streams/' + stream_id, error: function (xhr) { if (xhr.status.toString().charAt(0) === "4") { $(".active_stream_row button").closest("td").html( diff --git a/static/js/subs.js b/static/js/subs.js index e7efe65cd4..61f69315f7 100644 --- a/static/js/subs.js +++ b/static/js/subs.js @@ -317,7 +317,7 @@ function show_subscription_settings(sub_row) { loading.make_indicator(indicator_elem); channel.get({ - url: "/json/streams/" + encodeURIComponent(sub.name) + "/members", + url: "/json/streams/" + stream_id + "/members", idempotent: true, success: function (data) { loading.destroy_indicator(indicator_elem); @@ -1204,15 +1204,13 @@ $(function () { e.preventDefault(); var sub_settings = $(e.target).closest('.subscription_settings'); var stream_id = $(e.target).closest(".subscription_settings").attr("data-stream-id"); - var sub = stream_data.get_sub_by_id(stream_id); var new_name_box = sub_settings.find('input[name="new-name"]'); var new_name = $.trim(new_name_box.val()); $("#subscriptions-status").hide(); channel.patch({ - // Stream names might contain unsafe characters so we must encode it first. - url: "/json/streams/" + encodeURIComponent(sub.name), + url: "/json/streams/" + stream_id, data: {new_name: JSON.stringify(new_name)}, success: function () { new_name_box.val(''); @@ -1230,13 +1228,13 @@ $(function () { e.preventDefault(); var sub_settings = $(e.target).closest('.subscription_settings'); var stream_name = get_stream_name(sub_settings); + var stream_id = stream_data.get_sub(stream_name).stream_id; var description = sub_settings.find('input[name="description"]').val(); $('#subscriptions-status').hide(); channel.patch({ - // Stream names might contain unsafe characters so we must encode it first. - url: '/json/streams/' + encodeURIComponent(stream_name), + url: '/json/streams/' + stream_id, data: { description: JSON.stringify(description), }, @@ -1290,7 +1288,7 @@ $(function () { var data = {stream_name: sub.name, is_private: is_private}; channel.patch({ - url: "/json/streams/" + encodeURIComponent(sub.name), + url: "/json/streams/" + stream_id, data: data, success: function () { sub = stream_data.get_sub_by_id(stream_id); diff --git a/zerver/tests/test_signup.py b/zerver/tests/test_signup.py index 1d19ef1bad..be43b24c69 100644 --- a/zerver/tests/test_signup.py +++ b/zerver/tests/test_signup.py @@ -75,12 +75,13 @@ class PublicURLTest(ZulipTestCase): # FIXME: We should also test the Tornado URLs -- this codepath # can't do so because this Django test mechanism doesn't go # through Tornado. + denmark_stream_id = Stream.objects.get(name='Denmark').id get_urls = {200: ["/accounts/home/", "/accounts/login/" "/en/accounts/home/", "/ru/accounts/home/", "/en/accounts/login/", "/ru/accounts/login/", "/help/"], 302: ["/", "/en/", "/ru/"], - 401: ["/json/streams/Denmark/members", + 401: ["/json/streams/%d/members" % (denmark_stream_id,), "/api/v1/users/me/subscriptions", "/api/v1/messages", "/json/messages", diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index 540eb2afa3..8c6380f621 100644 --- a/zerver/tests/test_subs.py +++ b/zerver/tests/test_subs.py @@ -120,7 +120,8 @@ class StreamAdminTest(ZulipTestCase): 'stream_name': ujson.dumps('private_stream'), 'is_private': ujson.dumps(False) } - result = self.client_patch("/json/streams/private_stream", params) + stream_id = Stream.objects.get(realm=user_profile.realm, name='private_stream').id + result = self.client_patch("/json/streams/%d" % (stream_id,), params) self.assert_json_error(result, 'You are not invited to this stream.') self.subscribe_to_stream(email, 'private_stream') @@ -130,7 +131,7 @@ class StreamAdminTest(ZulipTestCase): 'stream_name': ujson.dumps('private_stream'), 'is_private': ujson.dumps(False) } - result = self.client_patch("/json/streams/private_stream", params) + result = self.client_patch("/json/streams/%d" % (stream_id,), params) self.assert_json_success(result) realm = user_profile.realm @@ -150,7 +151,8 @@ class StreamAdminTest(ZulipTestCase): 'stream_name': ujson.dumps('public_stream'), 'is_private': ujson.dumps(True) } - result = self.client_patch("/json/streams/public_stream", params) + stream_id = Stream.objects.get(realm=user_profile.realm, name='public_stream').id + result = self.client_patch("/json/streams/%d" % (stream_id,), params) self.assert_json_success(result) stream = Stream.objects.get(name='public_stream', realm=realm) self.assertTrue(stream.invite_only) @@ -164,7 +166,7 @@ class StreamAdminTest(ZulipTestCase): self.subscribe_to_stream(user_profile.email, stream.name) do_change_is_admin(user_profile, True) - result = self.client_delete('/json/streams/new_stream') + result = self.client_delete('/json/streams/%d' % (stream.id,)) self.assert_json_success(result) subscription_exists = Subscription.objects.filter( user_profile=user_profile, @@ -182,8 +184,8 @@ class StreamAdminTest(ZulipTestCase): self.make_stream('new_stream') do_change_is_admin(user_profile, True) - result = self.client_delete('/json/streams/unknown_stream') - self.assert_json_error(result, 'No such stream name') + result = self.client_delete('/json/streams/999999999') + self.assert_json_error(result, u'Invalid stream id') def test_deactivate_stream_backend_requires_realm_admin(self): # type: () -> None @@ -191,7 +193,8 @@ class StreamAdminTest(ZulipTestCase): self.login(email) self.subscribe_to_stream(email, 'new_stream') - result = self.client_delete('/json/streams/new_stream') + stream_id = Stream.objects.get(name='new_stream').id + result = self.client_delete('/json/streams/%d' % (stream_id,)) self.assert_json_error(result, 'Must be a realm administrator') def test_private_stream_live_updates(self): @@ -208,7 +211,8 @@ class StreamAdminTest(ZulipTestCase): events = [] # type: List[Dict[str, Any]] with tornado_redirected_to_list(events): - result = self.client_patch('/json/streams/private_stream', + stream_id = Stream.objects.get(name='private_stream').id + result = self.client_patch('/json/streams/%d' % (stream_id,), {'description': ujson.dumps('Test description')}) self.assert_json_success(result) @@ -222,7 +226,8 @@ class StreamAdminTest(ZulipTestCase): events = [] with tornado_redirected_to_list(events): - result = self.client_patch('/json/streams/private_stream', + stream_id = Stream.objects.get(name='private_stream').id + result = self.client_patch('/json/streams/%d' % (stream_id,), {'new_name': ujson.dumps('whatever')}) self.assert_json_success(result) @@ -242,7 +247,8 @@ class StreamAdminTest(ZulipTestCase): events = [] # type: List[Dict[str, Any]] with tornado_redirected_to_list(events): - result = self.client_patch('/json/streams/stream_name1', + stream_id = Stream.objects.get(name='stream_name1').id + result = self.client_patch('/json/streams/%d' % (stream_id,), {'new_name': ujson.dumps('stream_name2')}) self.assert_json_success(result) @@ -270,7 +276,8 @@ class StreamAdminTest(ZulipTestCase): # Test case to handle unicode stream name change # *NOTE: Here Encoding is needed when Unicode string is passed as an argument* with tornado_redirected_to_list(events): - result = self.client_patch('/json/streams/stream_name2', + stream_id = stream_name2_exists.id + result = self.client_patch('/json/streams/%d' % (stream_id,), {'new_name': ujson.dumps(u'नया नाम'.encode('utf-8'))}) self.assert_json_success(result) # While querying, system can handle unicode strings. @@ -281,7 +288,8 @@ class StreamAdminTest(ZulipTestCase): # NOTE: Unicode string being part of URL is handled cleanly # by client_patch call, encoding of URL is not needed. with tornado_redirected_to_list(events): - result = self.client_patch('/json/streams/नया नाम', + stream_id = stream_name_uni_exists.id + result = self.client_patch('/json/streams/%d' % (stream_id,), {'new_name': ujson.dumps(u'नाम में क्या रक्खा हे'.encode('utf-8'))}) self.assert_json_success(result) # While querying, system can handle unicode strings. @@ -292,7 +300,8 @@ class StreamAdminTest(ZulipTestCase): # Test case to change name from one language to other. with tornado_redirected_to_list(events): - result = self.client_patch('/json/streams/नाम में क्या रक्खा हे', + stream_id = stream_name_new_uni_exists.id + result = self.client_patch('/json/streams/%d' % (stream_id,), {'new_name': ujson.dumps(u'français'.encode('utf-8'))}) self.assert_json_success(result) stream_name_fr_exists = get_stream(u'français', realm) @@ -300,7 +309,8 @@ class StreamAdminTest(ZulipTestCase): # Test case to change name to mixed language name. with tornado_redirected_to_list(events): - result = self.client_patch('/json/streams/français', + stream_id = stream_name_fr_exists.id + result = self.client_patch('/json/streams/%d' % (stream_id,), {'new_name': ujson.dumps(u'français name'.encode('utf-8'))}) self.assert_json_success(result) stream_name_mixed_exists = get_stream(u'français name', realm) @@ -312,7 +322,8 @@ class StreamAdminTest(ZulipTestCase): self.login(email) self.make_stream('stream_name1') - result = self.client_patch('/json/streams/stream_name1', + stream_id = Stream.objects.get(name='stream_name1').id + result = self.client_patch('/json/streams/%d' % (stream_id,), {'new_name': ujson.dumps('stream_name2')}) self.assert_json_error(result, 'Must be a realm administrator') @@ -327,7 +338,8 @@ class StreamAdminTest(ZulipTestCase): events = [] # type: List[Dict[str, Any]] with tornado_redirected_to_list(events): - result = self.client_patch('/json/streams/stream_name1', + stream_id = Stream.objects.get(realm=realm, name='stream_name1').id + result = self.client_patch('/json/streams/%d' % (stream_id,), {'description': ujson.dumps('Test description')}) self.assert_json_success(result) @@ -362,7 +374,8 @@ class StreamAdminTest(ZulipTestCase): self.subscribe_to_stream(email, 'stream_name1') do_change_is_admin(user_profile, False) - result = self.client_patch('/json/streams/stream_name1', + stream_id = Stream.objects.get(realm=user_profile.realm, name='stream_name1').id + result = self.client_patch('/json/streams/%d' % (stream_id,), {'description': ujson.dumps('Test description')}) self.assert_json_error(result, 'Must be a realm administrator') @@ -392,10 +405,11 @@ class StreamAdminTest(ZulipTestCase): """ active_name = stream.name realm = stream.realm + stream_id = stream.id events = [] # type: List[Dict[str, Any]] with tornado_redirected_to_list(events): - result = self.client_delete('/json/streams/' + active_name) + result = self.client_delete('/json/streams/' + str(stream_id)) self.assert_json_success(result) deletion_events = [e['event'] for e in events if e['event']['type'] == 'subscription'] @@ -467,7 +481,7 @@ class StreamAdminTest(ZulipTestCase): priv_stream = self.set_up_stream_for_deletion( "privstream", subscribed=False, invite_only=True) - result = self.client_delete('/json/streams/' + priv_stream.name) + result = self.client_delete('/json/streams/' + str(priv_stream.id)) self.assert_json_error( result, "Cannot administer invite-only streams this way") @@ -2036,7 +2050,8 @@ class InviteOnlyStreamTest(ZulipTestCase): self.assertEqual(json["already_subscribed"], {}) # Make sure both users are subscribed to this stream - result = self.client_get("/api/v1/streams/%s/members" % (stream_name,), + stream_id = Stream.objects.get(name=stream_name).id + result = self.client_get("/api/v1/streams/%d/members" % (stream_id,), **self.api_auth(email)) self.assert_json_success(result) json = ujson.loads(result.content) @@ -2068,16 +2083,17 @@ class GetSubscribersTest(ZulipTestCase): stream_name, realm)] self.assertEqual(sorted(result["subscribers"]), sorted(true_subscribers)) - def make_subscriber_request(self, stream_name, email=None): - # type: (Text, Optional[str]) -> HttpResponse + def make_subscriber_request(self, stream_id, email=None): + # type: (int, Optional[str]) -> HttpResponse if email is None: email = self.email - return self.client_get("/api/v1/streams/%s/members" % (stream_name,), + return self.client_get("/api/v1/streams/%d/members" % (stream_id,), **self.api_auth(email)) def make_successful_subscriber_request(self, stream_name): # type: (Text) -> None - result = self.make_subscriber_request(stream_name) + stream_id = Stream.objects.get(name=stream_name).id + result = self.make_subscriber_request(stream_id) self.assert_json_success(result) self.check_well_formed_result(ujson.loads(result.content), stream_name, self.user_profile.realm) @@ -2216,9 +2232,9 @@ class GetSubscribersTest(ZulipTestCase): """ json_get_subscribers also returns the list of subscribers for a stream. """ - stream_name = "unknown_stream" - result = self.client_get("/json/streams/%s/members" % (stream_name,)) - self.assert_json_error(result, "Stream does not exist: %s" % (stream_name,)) + stream_id = 99999999 + result = self.client_get("/json/streams/%d/members" % (stream_id,)) + self.assert_json_error(result, u'Invalid stream id') def test_json_get_subscribers(self): # type: () -> None @@ -2227,8 +2243,9 @@ class GetSubscribersTest(ZulipTestCase): also returns the list of subscribers for a stream. """ stream_name = gather_subscriptions(self.user_profile)[0][0]['name'] + stream_id = Stream.objects.get(realm=self.user_profile.realm, name=stream_name).id expected_subscribers = gather_subscriptions(self.user_profile)[0][0]['subscribers'] - result = self.client_get("/json/streams/%s/members" % (stream_name,)) + result = self.client_get("/json/streams/%d/members" % (stream_id,)) self.assert_json_success(result) result_dict = ujson.loads(result.content) self.assertIn('subscribers', result_dict) @@ -2251,6 +2268,7 @@ class GetSubscribersTest(ZulipTestCase): other_email = "othello@zulip.com" # Try to fetch the subscriber list as a non-member. - result = self.make_subscriber_request(stream_name, email=other_email) + stream_id = Stream.objects.get(name=stream_name).id + result = self.make_subscriber_request(stream_id, email=other_email) self.assert_json_error(result, "Unable to retrieve subscribers for invite-only stream") diff --git a/zerver/views/streams.py b/zerver/views/streams.py index 63a56db6d0..5b0c415966 100644 --- a/zerver/views/streams.py +++ b/zerver/views/streams.py @@ -20,7 +20,7 @@ from zerver.lib.actions import bulk_remove_subscriptions, \ from zerver.lib.response import json_success, json_error, json_response from zerver.lib.validator import check_string, check_list, check_dict, \ check_bool, check_variable_type -from zerver.models import UserProfile, Stream, Subscription, \ +from zerver.models import UserProfile, Stream, Realm, Subscription, \ Recipient, get_recipient, get_stream, bulk_get_streams, \ bulk_get_recipients, valid_stream_name, get_active_user_dicts_in_realm @@ -132,11 +132,9 @@ def principal_to_user_profile(agent, principal): return principal_user_profile @require_realm_admin -def deactivate_stream_backend(request, user_profile, stream_name): - # type: (HttpRequest, UserProfile, Text) -> HttpResponse - target = get_stream(stream_name, user_profile.realm) - if not target: - return json_error(_('No such stream name')) +def deactivate_stream_backend(request, user_profile, stream_id): + # type: (HttpRequest, UserProfile, int) -> HttpResponse + target = get_and_validate_stream_by_id(stream_id, user_profile.realm) if target.invite_only and not subscribed_to_stream(user_profile, target): return json_error(_('Cannot administer invite-only streams this way')) @@ -160,11 +158,14 @@ def remove_default_stream(request, user_profile, stream_name=REQ()): @require_realm_admin @has_request_variables -def update_stream_backend(request, user_profile, stream_name, +def update_stream_backend(request, user_profile, stream_id, description=REQ(validator=check_string, default=None), is_private=REQ(validator=check_bool, default=None), new_name=REQ(validator=check_string, default=None)): - # type: (HttpRequest, UserProfile, Text, Optional[Text], Optional[bool], Optional[Text]) -> HttpResponse + # type: (HttpRequest, UserProfile, int, Optional[Text], Optional[bool], Optional[Text]) -> HttpResponse + stream = get_and_validate_stream_by_id(stream_id, user_profile.realm) + stream_name = stream.name + if description is not None: do_change_stream_description(user_profile.realm, stream_name, description) if stream_name is not None and new_name is not None: @@ -406,12 +407,10 @@ def add_subscriptions_backend(request, user_profile, return json_success(result) @has_request_variables -def get_subscribers_backend(request, user_profile, stream_name=REQ('stream')): - # type: (HttpRequest, UserProfile, Text) -> HttpResponse - stream = get_stream(stream_name, user_profile.realm) - if stream is None: - raise JsonableError(_("Stream does not exist: %s") % (stream_name,)) - +def get_subscribers_backend(request, user_profile, + stream_id=REQ('stream', converter=to_non_negative_int)): + # type: (HttpRequest, UserProfile, int) -> HttpResponse + stream = get_and_validate_stream_by_id(stream_id, user_profile.realm) subscribers = get_subscriber_emails(stream, user_profile) return json_success({'subscribers': subscribers}) @@ -436,11 +435,7 @@ def get_streams_backend(request, user_profile, def get_topics_backend(request, user_profile, stream_id=REQ(converter=to_non_negative_int)): # type: (HttpRequest, UserProfile, int) -> HttpResponse - - try: - stream = Stream.objects.get(pk=stream_id) - except Stream.DoesNotExist: - return json_error(_("Invalid stream id")) + stream = get_and_validate_stream_by_id(stream_id, user_profile.realm) if stream.realm_id != user_profile.realm_id: return json_error(_("Invalid stream id")) @@ -468,13 +463,20 @@ def get_topics_backend(request, user_profile, def json_stream_exists(request, user_profile, stream=REQ(), autosubscribe=REQ(default=False)): # type: (HttpRequest, UserProfile, Text, bool) -> HttpResponse - return stream_exists_backend(request, user_profile, stream, autosubscribe) - -def stream_exists_backend(request, user_profile, stream_name, autosubscribe): - # type: (HttpRequest, UserProfile, Text, bool) -> HttpResponse - if not valid_stream_name(stream_name): + if not valid_stream_name(stream): return json_error(_("Invalid characters in stream name")) - stream = get_stream(stream_name, user_profile.realm) + try: + stream_id = Stream.objects.get(realm=user_profile.realm, name=stream).id + except Stream.DoesNotExist: + stream_id = None + return stream_exists_backend(request, user_profile, stream_id, autosubscribe) + +def stream_exists_backend(request, user_profile, stream_id, autosubscribe): + # type: (HttpRequest, UserProfile, int, bool) -> HttpResponse + try: + stream = get_and_validate_stream_by_id(stream_id, user_profile.realm) + except JsonableError: + stream = None result = {"exists": bool(stream)} if stream is not None: recipient = get_recipient(Recipient.STREAM, stream.id) @@ -487,6 +489,14 @@ def stream_exists_backend(request, user_profile, stream_name, autosubscribe): return json_success(result) # results are ignored for HEAD requests return json_response(data=result, status=404) +def get_and_validate_stream_by_id(stream_id, realm): + # type: (int, Realm) -> Stream + try: + stream = Stream.objects.get(pk=stream_id, realm_id=realm.id) + except Stream.DoesNotExist: + raise JsonableError(_("Invalid stream id")) + return stream + @has_request_variables def json_get_stream_id(request, user_profile, stream=REQ()): # type: (HttpRequest, UserProfile, Text) -> HttpResponse diff --git a/zproject/urls.py b/zproject/urls.py index bc001dbb2d..e4978bbe44 100644 --- a/zproject/urls.py +++ b/zproject/urls.py @@ -293,9 +293,9 @@ v1_api_and_json_patterns = [ {'GET': 'zerver.views.streams.json_get_stream_id'}), # GET returns "stream info" (undefined currently?), HEAD returns whether stream exists (200 or 404) - url(r'^streams/(?P.*)/members$', rest_dispatch, + url(r'^streams/(?P\d+)/members$', rest_dispatch, {'GET': 'zerver.views.streams.get_subscribers_backend'}), - url(r'^streams/(?P.*)$', rest_dispatch, + url(r'^streams/(?P\d+)$', rest_dispatch, {'HEAD': 'zerver.views.streams.stream_exists_backend', 'GET': 'zerver.views.streams.stream_exists_backend', 'PATCH': 'zerver.views.streams.update_stream_backend',