From cb01a7f599ac7feb32eaebc0cbf31e752d920e29 Mon Sep 17 00:00:00 2001 From: Vishnu KS Date: Tue, 14 Jul 2020 12:40:39 +0000 Subject: [PATCH] billing: Restrict access to billing page to realm owners and billing admins. --- corporate/tests/test_stripe.py | 88 ++++++++++--------- templates/corporate/billing.html | 2 +- .../zerver/help/roles-and-permissions.md | 12 ++- zerver/decorator.py | 5 +- zerver/models.py | 2 +- zerver/tests/test_home.py | 16 +++- 6 files changed, 72 insertions(+), 53 deletions(-) diff --git a/corporate/tests/test_stripe.py b/corporate/tests/test_stripe.py index 9d00f36870..266e335b43 100644 --- a/corporate/tests/test_stripe.py +++ b/corporate/tests/test_stripe.py @@ -257,7 +257,7 @@ class StripeTestCase(ZulipTestCase): self.example_email('hamlet'), self.example_email('iago'), self.example_email('othello'), - self.example_email('prospero'), + self.example_email('desdemona'), self.example_email('polonius'), # guest self.example_email('default_bot'), # bot ] @@ -855,12 +855,8 @@ class StripeTest(StripeTestCase): @mock_stripe() def test_billing_page_permissions(self, *mocks: Mock) -> None: - hamlet = self.example_user('hamlet') - iago = self.example_user('iago') - cordelia = self.example_user('cordelia') - # Check that non-admins can access /upgrade via /billing, when there is no Customer object - self.login_user(hamlet) + self.login_user(self.example_user('hamlet')) response = self.client_get("/billing/") self.assertEqual(response.status_code, 302) self.assertEqual('/upgrade/', response.url) @@ -868,15 +864,20 @@ class StripeTest(StripeTestCase): self.upgrade() # Check that the non-admin hamlet can still access /billing response = self.client_get("/billing/") - self.assert_in_success_response(["for billing history or to make changes"], response) - # Check admins can access billing, even though they are not a billing admin - self.login_user(iago) + self.assert_in_success_response(["Your current plan is"], response) + + # Check realm owners can access billing, even though they are not a billing admin + desdemona = self.example_user('desdemona') + desdemona.role = UserProfile.ROLE_REALM_OWNER + desdemona.save(update_fields=["role"]) + self.login_user(self.example_user('desdemona')) response = self.client_get("/billing/") - self.assert_in_success_response(["for billing history or to make changes"], response) - # Check that a non-admin, non-billing admin user does not have access - self.login_user(cordelia) + self.assert_in_success_response(["Your current plan is"], response) + + # Check that member who is not a billing admin does not have access + self.login_user(self.example_user('cordelia')) response = self.client_get("/billing/") - self.assert_in_success_response(["You must be an organization administrator"], response) + self.assert_in_success_response(["You must be an organization owner or a billing administrator"], response) @mock_stripe(tested_timestamp_fields=["created"]) def test_upgrade_by_card_with_outdated_seat_count(self, *mocks: Mock) -> None: @@ -1109,7 +1110,7 @@ class StripeTest(StripeTestCase): self.login_user(self.example_user("othello")) response = self.client_get("/billing/") - self.assert_in_success_response(["You must be an organization administrator or a billing administrator to view this page."], response) + self.assert_in_success_response(["You must be an organization owner or a billing administrator to view this page."], response) def test_redirect_for_billing_home(self) -> None: user = self.example_user("iago") @@ -1824,21 +1825,45 @@ class RequiresBillingAccessTest(ZulipTestCase): hamlet.is_billing_admin = True hamlet.save(update_fields=["is_billing_admin"]) - def verify_non_admins_blocked_from_endpoint( - self, url: str, request_data: Dict[str, Any]={}) -> None: - cordelia = self.example_user('cordelia') - self.login_user(cordelia) - response = self.client_post(url, request_data) - self.assert_json_error_contains(response, "Must be a billing administrator or an organization") + desdemona = self.example_user('desdemona') + desdemona.role = UserProfile.ROLE_REALM_OWNER + desdemona.save(update_fields=["role"]) + + def test_who_can_access_json_endpoints(self) -> None: + # Billing admins have access + self.login_user(self.example_user('hamlet')) + with patch("corporate.views.do_replace_payment_source") as mocked1: + response = self.client_post("/json/billing/sources/change", + {'stripe_token': ujson.dumps('token')}) + self.assert_json_success(response) + mocked1.assert_called() + + # Realm owners have access, even if they are not billing admins + self.login_user(self.example_user('desdemona')) + with patch("corporate.views.do_replace_payment_source") as mocked2: + response = self.client_post("/json/billing/sources/change", + {'stripe_token': ujson.dumps('token')}) + self.assert_json_success(response) + mocked2.assert_called() + + def test_who_cant_access_json_endpoints(self) -> None: + def verify_user_cant_access_endpoint(user: UserProfile, url: str, request_data: Dict[str, Any]={}) -> None: + self.login_user(user) + response = self.client_post(url, request_data) + self.assert_json_error_contains(response, "Must be a billing administrator or an organization owner") - def test_non_admins_blocked_from_json_endpoints(self) -> None: params: List[Tuple[str, Dict[str, Any]]] = [ ("/json/billing/sources/change", {'stripe_token': ujson.dumps('token')}), ("/json/billing/plan/change", {'status': ujson.dumps(1)}), ] for (url, data) in params: - self.verify_non_admins_blocked_from_endpoint(url, data) + # Guests can't access + verify_user_cant_access_endpoint(self.example_user("polonius"), url, data) + # Members (not billing admin) can't access + verify_user_cant_access_endpoint(self.example_user("cordelia"), url, data) + # Realm admins (not billing admin) can't access + verify_user_cant_access_endpoint(self.example_user("iago"), url, data) # Make sure that we are testing all the JSON endpoints # Quite a hack, but probably fine for now @@ -1851,25 +1876,6 @@ class RequiresBillingAccessTest(ZulipTestCase): self.assertEqual(len(json_endpoints), len(params)) - def test_admins_and_billing_admins_can_access(self) -> None: - # Billing admins have access - hamlet = self.example_user('hamlet') - iago = self.example_user('iago') - self.login_user(hamlet) - with patch("corporate.views.do_replace_payment_source") as mocked1: - response = self.client_post("/json/billing/sources/change", - {'stripe_token': ujson.dumps('token')}) - self.assert_json_success(response) - mocked1.assert_called() - - # Realm admins have access, even if they are not billing admins - self.login_user(iago) - with patch("corporate.views.do_replace_payment_source") as mocked2: - response = self.client_post("/json/billing/sources/change", - {'stripe_token': ujson.dumps('token')}) - self.assert_json_success(response) - mocked2.assert_called() - class BillingHelpersTest(ZulipTestCase): def test_next_month(self) -> None: anchor = datetime(2019, 12, 31, 1, 2, 3, tzinfo=timezone.utc) diff --git a/templates/corporate/billing.html b/templates/corporate/billing.html index 834475e0e3..b2796bbfb1 100644 --- a/templates/corporate/billing.html +++ b/templates/corporate/billing.html @@ -171,7 +171,7 @@ {% else %}

- You must be an organization administrator or a billing administrator to view this page. + You must be an organization owner or a billing administrator to view this page.

{% endif %} diff --git a/templates/zerver/help/roles-and-permissions.md b/templates/zerver/help/roles-and-permissions.md index ea336874d4..f8e7e36acb 100644 --- a/templates/zerver/help/roles-and-permissions.md +++ b/templates/zerver/help/roles-and-permissions.md @@ -6,11 +6,17 @@ There are several possible roles in a Zulip organization. organization settings, and billing. * **Organization Administrator**: Can manage users, public streams, - organization settings, and billing. Cannot create or demote + and organization settings. Can upgrade the organization to a paid + plan and become a billing admin. Cannot create or demote organization owners. -* **Member**: Has access to all public streams. This is the default role for - most users. +* **Member**: Has access to all public streams. Can upgrade + the organization to a paid plan and become a billing administrator. + Member is the default role for most users. + +* **Billing Administrator**: Member or organization administrator who + upgrades the organization to a paid plan becomes a billing administrator. + Can manage billing in addition to the existing privileges. * **Guest**: Can only access streams they've been added to. Cannot create new streams or invite other users. diff --git a/zerver/decorator.py b/zerver/decorator.py index 9e49b39e6c..4ce888e143 100644 --- a/zerver/decorator.py +++ b/zerver/decorator.py @@ -133,12 +133,11 @@ def require_realm_admin(func: ViewFuncT) -> ViewFuncT: def require_billing_access(func: ViewFuncT) -> ViewFuncT: @wraps(func) def wrapper(request: HttpRequest, user_profile: UserProfile, *args: object, **kwargs: object) -> HttpResponse: - if not user_profile.is_realm_admin and not user_profile.is_billing_admin: - raise JsonableError(_("Must be a billing administrator or an organization administrator")) + if not user_profile.has_billing_access: + raise JsonableError(_("Must be a billing administrator or an organization owner")) return func(request, user_profile, *args, **kwargs) return cast(ViewFuncT, wrapper) # https://github.com/python/mypy/issues/1927 - def get_client_name(request: HttpRequest) -> str: # If the API request specified a client in the request content, # that has priority. Otherwise, extract the client from the diff --git a/zerver/models.py b/zerver/models.py index dbac96581c..abf3304a7a 100644 --- a/zerver/models.py +++ b/zerver/models.py @@ -1199,7 +1199,7 @@ class UserProfile(AbstractBaseUser, PermissionsMixin): @property def has_billing_access(self) -> bool: - return self.is_realm_admin or self.is_billing_admin + return self.is_realm_owner or self.is_billing_admin @property def is_realm_owner(self) -> bool: diff --git a/zerver/tests/test_home.py b/zerver/tests/test_home.py index 637f4324a9..5b75ba0c9b 100644 --- a/zerver/tests/test_home.py +++ b/zerver/tests/test_home.py @@ -317,7 +317,7 @@ class HomeTest(ZulipTestCase): result = self._get_home_page() self.assertEqual(result.status_code, 200) self.assert_length(cache_mock.call_args_list, 6) - self.assert_length(queries, 40) + self.assert_length(queries, 39) def test_num_queries_with_streams(self) -> None: main_user = self.example_user('hamlet') @@ -680,20 +680,28 @@ class HomeTest(ZulipTestCase): def test_show_billing(self) -> None: customer = Customer.objects.create(realm=get_realm("zulip"), stripe_customer_id="cus_id") + user = self.example_user('desdemona') - # realm admin, but no CustomerPlan -> no billing link - user = self.example_user('iago') + # realm owner, but no CustomerPlan -> no billing link + user.role = UserProfile.ROLE_REALM_OWNER + user.save(update_fields=["role"]) self.login_user(user) result_html = self._get_home_page().content.decode('utf-8') self.assertNotIn('Billing', result_html) - # realm admin, with inactive CustomerPlan -> show billing link + # realm owner, with inactive CustomerPlan -> show billing link CustomerPlan.objects.create(customer=customer, billing_cycle_anchor=timezone_now(), billing_schedule=CustomerPlan.ANNUAL, next_invoice_date=timezone_now(), tier=CustomerPlan.STANDARD, status=CustomerPlan.ENDED) result_html = self._get_home_page().content.decode('utf-8') self.assertIn('Billing', result_html) + # realm admin, with CustomerPlan -> no billing link + user.role = UserProfile.ROLE_REALM_ADMINISTRATOR + user.save(update_fields=["role"]) + result_html = self._get_home_page().content.decode('utf-8') + self.assertNotIn('Billing', result_html) + # billing admin, with CustomerPlan -> show billing link user.role = UserProfile.ROLE_MEMBER user.is_billing_admin = True