remote_billing: Tweak /self-hosted-billing/ endpoints access model.

It's best for these to just be consistent. Therefore:
1. The .../not-configured/ error page endpoint should be restricted to
   .has_billing_access users only.
2. For consistency, self_hosting_auth_view_common is tweaked to also do
   the .has_billing_access check as the first thing, to avoid revealing
   configuration information via its redirect/error-handling behavior.

The revealed configuration information seems super harmless, but it's
simpler to not have to worry about it and just be consistent.
This commit is contained in:
Mateusz Mandera
2024-03-03 23:20:49 +01:00
committed by Tim Abbott
parent 898e47fbdd
commit e952c3b627
2 changed files with 30 additions and 7 deletions

View File

@@ -191,6 +191,17 @@ class RemoteRealmBillingTestCase(BouncerTestCase):
class SelfHostedBillingEndpointBasicTest(RemoteRealmBillingTestCase):
@responses.activate
def test_self_hosted_billing_endpoints(self) -> None:
# An ordinary user doesn't have access to these endpoints.
self.login("hamlet")
for url in [
"/self-hosted-billing/",
"/json/self-hosted-billing",
"/self-hosted-billing/not-configured/",
]:
result = self.client_get(url)
self.assert_json_error(result, "Must be an organization owner")
# Login as an organization owner to gain access.
self.login("desdemona")
self.add_mock_response()
@@ -270,6 +281,8 @@ class SelfHostedBillingEndpointBasicTest(RemoteRealmBillingTestCase):
@override_settings(PUSH_NOTIFICATION_BOUNCER_URL="https://push.zulip.org.example.com")
class RemoteBillingAuthenticationTest(RemoteRealmBillingTestCase):
def test_self_hosted_config_error_page(self) -> None:
self.login("desdemona")
with self.settings(
CORPORATE_ENABLED=False, PUSH_NOTIFICATION_BOUNCER_URL=None
), self.assertLogs("django.request"):

View File

@@ -124,6 +124,13 @@ def send_test_push_notification_api(
def self_hosting_auth_view_common(
request: HttpRequest, user_profile: UserProfile, next_page: Optional[str] = None
) -> str:
if not user_profile.has_billing_access:
# We may want to replace this with an html error page at some point,
# but this endpoint shouldn't be accessible via the UI to an unauthorized
# user_profile - and they need to directly enter the URL in their browser. So a json
# error may be sufficient.
raise OrganizationOwnerRequiredError
if not uses_notification_bouncer():
if settings.CORPORATE_ENABLED:
# This endpoint makes no sense on zulipchat.com, so just 404.
@@ -131,13 +138,6 @@ def self_hosting_auth_view_common(
else:
return reverse(self_hosting_auth_not_configured)
if not user_profile.has_billing_access: # nocoverage
# We may want to replace this with an html error page at some point,
# but this endpoint shouldn't be accessible via the UI to an unauthorized
# user_profile - and they need to directly enter the URL in their browser. So a json
# error may be sufficient.
raise OrganizationOwnerRequiredError
realm_info = get_realms_info_for_push_bouncer(user_profile.realm_id)[0]
user_info = UserDataForRemoteBilling(
@@ -218,7 +218,17 @@ def self_hosting_auth_json_endpoint(
return json_success(request, data={"billing_access_url": redirect_url})
@zulip_login_required
def self_hosting_auth_not_configured(request: HttpRequest) -> HttpResponse:
# Use the same access model as the main endpoints for consistency
# and to not have to worry about this endpoint leaking some kind of
# sensitive configuration information in the future.
user = request.user
assert user.is_authenticated
assert isinstance(user, UserProfile)
if not user.has_billing_access:
raise OrganizationOwnerRequiredError
if settings.CORPORATE_ENABLED or uses_notification_bouncer():
# This error page should only be available if the config error
# is actually real.