From 752fd2e2d158ebc50bc722493f92f47e9b40032a Mon Sep 17 00:00:00 2001 From: Vishnu KS Date: Fri, 9 Apr 2021 16:13:44 +0530 Subject: [PATCH] corporate: Fix string encoding in billing and sponsorship endpoints. --- corporate/tests/test_stripe.py | 35 +++++++++------- corporate/views.py | 25 +++++++----- frontend_tests/node_tests/billing.js | 3 +- frontend_tests/node_tests/billing_helpers.js | 18 ++++----- frontend_tests/node_tests/upgrade.js | 42 +++++++++----------- static/js/billing/billing.js | 4 +- static/js/billing/helpers.js | 16 ++------ static/js/billing/upgrade.js | 14 ++----- 8 files changed, 69 insertions(+), 88 deletions(-) diff --git a/corporate/tests/test_stripe.py b/corporate/tests/test_stripe.py index a4c2d6b3b1..a677c99461 100644 --- a/corporate/tests/test_stripe.py +++ b/corporate/tests/test_stripe.py @@ -422,8 +422,6 @@ class StripeTestCase(ZulipTestCase): for key in del_args: if key in params: del params[key] - for key, value in params.items(): - params[key] = orjson.dumps(value).decode() return self.client_post("/json/billing/upgrade", params, **host_args) # Upgrade without talking to Stripe @@ -1287,18 +1285,29 @@ class StripeTest(StripeTestCase): def test_check_upgrade_parameters(self) -> None: # Tests all the error paths except 'not enough licenses' def check_error( - error_description: str, upgrade_params: Mapping[str, Any], del_args: Sequence[str] = [] + error_message: str, + error_description: str, + upgrade_params: Mapping[str, Any], + del_args: Sequence[str] = [], ) -> None: response = self.upgrade(talk_to_stripe=False, del_args=del_args, **upgrade_params) - self.assert_json_error_contains(response, "Something went wrong. Please contact") - self.assertEqual(orjson.loads(response.content)["error_description"], error_description) + self.assert_json_error_contains(response, error_message) + if error_description: + self.assertEqual( + orjson.loads(response.content)["error_description"], error_description + ) hamlet = self.example_user("hamlet") self.login_user(hamlet) - check_error("unknown billing_modality", {"billing_modality": "invalid"}) - check_error("unknown schedule", {"schedule": "invalid"}) - check_error("unknown license_management", {"license_management": "invalid"}) - check_error("autopay with no card", {}, del_args=["stripe_token"]) + check_error("Invalid billing_modality", "", {"billing_modality": "invalid"}) + check_error("Invalid schedule", "", {"schedule": "invalid"}) + check_error("Invalid license_management", "", {"license_management": "invalid"}) + check_error( + "Something went wrong. Please contact", + "autopay with no card", + {}, + del_args=["stripe_token"], + ) def test_upgrade_license_counts(self) -> None: def check_min_licenses_error( @@ -1401,11 +1410,9 @@ class StripeTest(StripeTestCase): self.login_user(user) data = { - "organization-type": orjson.dumps("Open-source").decode(), - "website": orjson.dumps("https://infinispan.org/").decode(), - "description": orjson.dumps( - "Infinispan is a distributed in-memory key/value data store with optional schema." - ).decode(), + "organization-type": "Open-source", + "website": "https://infinispan.org/", + "description": "Infinispan is a distributed in-memory key/value data store with optional schema.", } response = self.client_post("/json/billing/sponsorship", data) self.assert_json_success(response) diff --git a/corporate/views.py b/corporate/views.py index cf7ecd8d7b..272bc2bbdf 100644 --- a/corporate/views.py +++ b/corporate/views.py @@ -46,7 +46,7 @@ from zerver.decorator import ( from zerver.lib.request import REQ, has_request_variables from zerver.lib.response import json_error, json_success from zerver.lib.send_email import FromAddress, send_email -from zerver.lib.validator import check_int, check_string +from zerver.lib.validator import check_int, check_string_in from zerver.models import UserProfile, get_realm billing_logger = logging.getLogger("corporate.stripe") @@ -127,14 +127,17 @@ def payment_method_string(stripe_customer: stripe.Customer) -> str: def upgrade( request: HttpRequest, user: UserProfile, - billing_modality: str = REQ(json_validator=check_string), - schedule: str = REQ(json_validator=check_string), - license_management: Optional[str] = REQ(json_validator=check_string, default=None), + billing_modality: str = REQ(str_validator=check_string_in(VALID_BILLING_MODALITY_VALUES)), + schedule: str = REQ(str_validator=check_string_in(VALID_BILLING_SCHEDULE_VALUES)), + signed_seat_count: str = REQ(), + salt: str = REQ(), + license_management: Optional[str] = REQ( + default=None, str_validator=check_string_in(VALID_LICENSE_MANAGEMENT_VALUES) + ), licenses: Optional[int] = REQ(json_validator=check_int, default=None), - stripe_token: Optional[str] = REQ(json_validator=check_string, default=None), - signed_seat_count: str = REQ(json_validator=check_string), - salt: str = REQ(json_validator=check_string), + stripe_token: Optional[str] = REQ(default=None), ) -> HttpResponse: + try: seat_count = unsign_seat_count(signed_seat_count, salt) if billing_modality == "charge_automatically" and license_management == "automatic": @@ -236,9 +239,9 @@ def initial_upgrade(request: HttpRequest) -> HttpResponse: def sponsorship( request: HttpRequest, user: UserProfile, - organization_type: str = REQ("organization-type", json_validator=check_string), - website: str = REQ("website", json_validator=check_string), - description: str = REQ("description", json_validator=check_string), + organization_type: str = REQ("organization-type"), + website: str = REQ(), + description: str = REQ(), ) -> HttpResponse: realm = user.realm @@ -388,7 +391,7 @@ def change_plan_status( def replace_payment_source( request: HttpRequest, user: UserProfile, - stripe_token: str = REQ("stripe_token", json_validator=check_string), + stripe_token: str = REQ(), ) -> HttpResponse: try: do_replace_payment_source(user, stripe_token, pay_invoices=True) diff --git a/frontend_tests/node_tests/billing.js b/frontend_tests/node_tests/billing.js index 01f1084db6..a1487ea05f 100644 --- a/frontend_tests/node_tests/billing.js +++ b/frontend_tests/node_tests/billing.js @@ -86,11 +86,10 @@ run_test("initialize", (override) => { }); create_ajax_request_called = false; - function plan_change_ajax(url, form_name, stripe_token, numeric_inputs) { + function plan_change_ajax(url, form_name, stripe_token) { assert.equal(url, "/json/billing/plan/change"); assert.equal(form_name, "planchange"); assert.equal(stripe_token, undefined); - assert.deepEqual(numeric_inputs, ["status"]); create_ajax_request_called = true; } diff --git a/frontend_tests/node_tests/billing_helpers.js b/frontend_tests/node_tests/billing_helpers.js index 951e388a27..f66e357776 100644 --- a/frontend_tests/node_tests/billing_helpers.js +++ b/frontend_tests/node_tests/billing_helpers.js @@ -127,13 +127,13 @@ run_test("create_ajax_request", (override) => { assert.equal(url, "/json/billing/upgrade"); assert.equal(Object.keys(data).length, 8); - assert.equal(data.stripe_token, '"stripe_token_id"'); - assert.equal(data.seat_count, '"{{ seat_count }}"'); - assert.equal(data.signed_seat_count, '"{{ signed_seat_count }}"'); - assert.equal(data.salt, '"{{ salt }}"'); - assert.equal(data.billing_modality, '"charge_automatically"'); - assert.equal(data.schedule, '"monthly"'); - assert.equal(data.license_management, '"automatic"'); + assert.equal(data.stripe_token, "stripe_token_id"); + assert.equal(data.seat_count, "{{ seat_count }}"); + assert.equal(data.signed_seat_count, "{{ signed_seat_count }}"); + assert.equal(data.salt, "{{ salt }}"); + assert.equal(data.billing_modality, "charge_automatically"); + assert.equal(data.schedule, "monthly"); + assert.equal(data.license_management, "automatic"); assert.equal(data.licenses, ""); history.pushState = (state_object, title, path) => { @@ -174,9 +174,7 @@ run_test("create_ajax_request", (override) => { assert.equal(state.free_trial_alert_message_show, 1); }); - helpers.create_ajax_request("/json/billing/upgrade", "autopay", {id: "stripe_token_id"}, [ - "licenses", - ]); + helpers.create_ajax_request("/json/billing/upgrade", "autopay", {id: "stripe_token_id"}); }); run_test("format_money", () => { diff --git a/frontend_tests/node_tests/upgrade.js b/frontend_tests/node_tests/upgrade.js index 1b1d427f73..9f4284776e 100644 --- a/frontend_tests/node_tests/upgrade.js +++ b/frontend_tests/node_tests/upgrade.js @@ -36,30 +36,24 @@ run_test("initialize", (override) => { }); let create_ajax_request_form_call_count = 0; - helpers.__Rewire__( - "create_ajax_request", - (url, form_name, stripe_token, numeric_inputs, redirect_to) => { - create_ajax_request_form_call_count += 1; - if (form_name === "autopay") { - assert.equal(url, "/json/billing/upgrade"); - assert.equal(stripe_token, "stripe_add_card_token"); - assert.deepEqual(numeric_inputs, ["licenses"]); - assert.equal(redirect_to, undefined); - } else if (form_name === "invoice") { - assert.equal(url, "/json/billing/upgrade"); - assert.equal(stripe_token, undefined); - assert.deepEqual(numeric_inputs, ["licenses"]); - assert.equal(redirect_to, undefined); - } else if (form_name === "sponsorship") { - assert.equal(url, "/json/billing/sponsorship"); - assert.equal(stripe_token, undefined); - assert.equal(numeric_inputs, undefined); - assert.equal(redirect_to, "/"); - } else { - throw new Error("Unhandled case"); - } - }, - ); + helpers.__Rewire__("create_ajax_request", (url, form_name, stripe_token, redirect_to) => { + create_ajax_request_form_call_count += 1; + if (form_name === "autopay") { + assert.equal(url, "/json/billing/upgrade"); + assert.equal(stripe_token, "stripe_add_card_token"); + assert.equal(redirect_to, undefined); + } else if (form_name === "invoice") { + assert.equal(url, "/json/billing/upgrade"); + assert.equal(stripe_token, undefined); + assert.equal(redirect_to, undefined); + } else if (form_name === "sponsorship") { + assert.equal(url, "/json/billing/sponsorship"); + assert.equal(stripe_token, undefined); + assert.equal(redirect_to, "/"); + } else { + throw new Error("Unhandled case"); + } + }); const open_func = (config_opts) => { assert.equal(config_opts.name, "Zulip"); diff --git a/static/js/billing/billing.js b/static/js/billing/billing.js index 692ea6871c..4d087aad3d 100644 --- a/static/js/billing/billing.js +++ b/static/js/billing/billing.js @@ -30,9 +30,7 @@ export function initialize() { }); $("#change-plan-status").on("click", (e) => { - helpers.create_ajax_request("/json/billing/plan/change", "planchange", undefined, [ - "status", - ]); + helpers.create_ajax_request("/json/billing/plan/change", "planchange"); e.preventDefault(); }); } diff --git a/static/js/billing/helpers.js b/static/js/billing/helpers.js index 2bf4a5249c..0c1b0c4876 100644 --- a/static/js/billing/helpers.js +++ b/static/js/billing/helpers.js @@ -3,13 +3,7 @@ import $ from "jquery"; import * as loading from "../loading"; import {page_params} from "../page_params"; -export function create_ajax_request( - url, - form_name, - stripe_token = null, - numeric_inputs = [], - redirect_to = "/billing", -) { +export function create_ajax_request(url, form_name, stripe_token = null, redirect_to = "/billing") { const form = $(`#${CSS.escape(form_name)}-form`); const form_loading_indicator = `#${CSS.escape(form_name)}_loading_indicator`; const form_input_section = `#${CSS.escape(form_name)}-input-section`; @@ -32,15 +26,11 @@ export function create_ajax_request( const data = {}; if (stripe_token) { - data.stripe_token = JSON.stringify(stripe_token.id); + data.stripe_token = stripe_token.id; } for (const item of form.serializeArray()) { - if (numeric_inputs.includes(item.name)) { - data[item.name] = item.value; - } else { - data[item.name] = JSON.stringify(item.value); - } + data[item.name] = item.value; } $.post({ diff --git a/static/js/billing/upgrade.js b/static/js/billing/upgrade.js index a5795ae06f..b37d1c4918 100644 --- a/static/js/billing/upgrade.js +++ b/static/js/billing/upgrade.js @@ -12,9 +12,7 @@ export const initialize = () => { image: "/static/images/logo/zulip-icon-128x128.png", locale: "auto", token(stripe_token) { - helpers.create_ajax_request("/json/billing/upgrade", "autopay", stripe_token, [ - "licenses", - ]); + helpers.create_ajax_request("/json/billing/upgrade", "autopay", stripe_token); }, }); @@ -43,7 +41,7 @@ export const initialize = () => { return; } e.preventDefault(); - helpers.create_ajax_request("/json/billing/upgrade", "invoice", undefined, ["licenses"]); + helpers.create_ajax_request("/json/billing/upgrade", "invoice"); }); $("#sponsorship-button").on("click", (e) => { @@ -51,13 +49,7 @@ export const initialize = () => { return; } e.preventDefault(); - helpers.create_ajax_request( - "/json/billing/sponsorship", - "sponsorship", - undefined, - undefined, - "/", - ); + helpers.create_ajax_request("/json/billing/sponsorship", "sponsorship", undefined, "/"); }); const prices = {};