From 2eeb82edba2b0dd20bb04d54b34d064584f8a300 Mon Sep 17 00:00:00 2001 From: m-e-l-u-h-a-n Date: Wed, 31 Mar 2021 15:30:56 +0530 Subject: [PATCH] api: Add USER_DEACTIVATED error code. In validate_account_and_subdomain we check if user's account is not deactivated. In case of failure of this check we raise our standard JsonableError. While this works well in most cases but it creates difficulties in handling of deactivated accounts for non-browser clients. So we register a new USER_DEACTIVATED error code so that clients can distinguish if error is because of deactivated account. Following these changes `validate_account_and_subdomain` raises UserDeactivatedError if user's account is deactivated. This error is also documented in `/api/rest-error-handling`. Testing: I have mostly relied on automated backend tests to test this. Partially addresses issue #17763. --- templates/zerver/api/rest-error-handling.md | 6 ++++++ tools/test-api | 20 ++++++++++++++++++-- zerver/decorator.py | 3 ++- zerver/lib/exceptions.py | 13 +++++++++++++ zerver/openapi/python_examples.py | 9 +++++++++ zerver/openapi/zulip.yaml | 17 +++++++++++++++++ zerver/tests/test_decorators.py | 10 ++++++---- 7 files changed, 71 insertions(+), 7 deletions(-) diff --git a/templates/zerver/api/rest-error-handling.md b/templates/zerver/api/rest-error-handling.md index ed99e682c7..b2e93b30b4 100644 --- a/templates/zerver/api/rest-error-handling.md +++ b/templates/zerver/api/rest-error-handling.md @@ -36,3 +36,9 @@ A typical failed JSON response for when the user is not authorized for a query: {generate_code_example|/rest-error-handling:post|fixture(400_2)} + +## User account deactivated + +A typical failed json response for when user's account is deactivated: + +{generate_code_example|/rest-error-handling:post|fixture(403_0)} diff --git a/tools/test-api b/tools/test-api index 249650ed4a..2b831e1eef 100755 --- a/tools/test-api +++ b/tools/test-api @@ -31,12 +31,16 @@ with test_server_running( ): # Zerver imports should happen after `django.setup()` is run # by the test_server_running decorator. - from zerver.lib.actions import do_create_user + from zerver.lib.actions import change_user_is_active, do_create_user, do_reactivate_user from zerver.lib.test_helpers import reset_emails_in_zulip_realm from zerver.lib.users import get_api_key from zerver.models import get_realm, get_user from zerver.openapi.javascript_examples import test_js_bindings - from zerver.openapi.python_examples import test_invalid_api_key, test_the_api + from zerver.openapi.python_examples import ( + test_invalid_api_key, + test_the_api, + test_user_account_deactivated, + ) from zerver.openapi.test_curl_examples import test_generated_curl_examples_for_success print("Running API tests...") @@ -106,5 +110,17 @@ with test_server_running( ) test_invalid_api_key(client) + # Test account deactivated error + # we deactivate user manually because do_deactivate_user removes user session + change_user_is_active(guest_user, False) + client = Client( + email=email, + api_key=api_key, + site=site, + ) + test_user_account_deactivated(client) + # reactivate user to avoid any side-effects in other tests. + do_reactivate_user(guest_user, acting_user=None) + print("API tests passed!") diff --git a/zerver/decorator.py b/zerver/decorator.py index 7ddcd60da8..f208e48a40 100644 --- a/zerver/decorator.py +++ b/zerver/decorator.py @@ -33,6 +33,7 @@ from zerver.lib.exceptions import ( OrganizationMemberRequired, OrganizationOwnerRequired, UnsupportedWebhookEventType, + UserDeactivatedError, ) from zerver.lib.queue import queue_json_publish from zerver.lib.rate_limiter import RateLimitedUser @@ -270,7 +271,7 @@ def validate_account_and_subdomain(request: HttpRequest, user_profile: UserProfi if user_profile.realm.deactivated: raise JsonableError(_("This organization has been deactivated")) if not user_profile.is_active: - raise JsonableError(_("Account is deactivated")) + raise UserDeactivatedError() # Either the subdomain matches, or we're accessing Tornado from # and to localhost (aka spoofing a request as the user). diff --git a/zerver/lib/exceptions.py b/zerver/lib/exceptions.py index 6239af6cd8..01a225dcea 100644 --- a/zerver/lib/exceptions.py +++ b/zerver/lib/exceptions.py @@ -50,6 +50,7 @@ class ErrorCode(AbstractEnum): UNAUTHENTICATED_USER = () NONEXISTENT_SUBDOMAIN = () RATE_LIMIT_HIT = () + USER_DEACTIVATED = () class JsonableError(Exception): @@ -271,6 +272,18 @@ class StreamAdministratorRequired(JsonableError): return _("Must be an organization or stream administrator") +class UserDeactivatedError(JsonableError): + code: ErrorCode = ErrorCode.USER_DEACTIVATED + http_status_code = 403 + + def __init__(self) -> None: + pass + + @staticmethod + def msg_format() -> str: + return _("Account is deactivated") + + class MarkdownRenderingException(Exception): pass diff --git a/zerver/openapi/python_examples.py b/zerver/openapi/python_examples.py index 85ded3aaae..9f2734518e 100644 --- a/zerver/openapi/python_examples.py +++ b/zerver/openapi/python_examples.py @@ -1224,6 +1224,15 @@ def test_missing_request_argument(client: Client) -> None: validate_against_openapi_schema(result, "/rest-error-handling", "post", "400_1") +def test_user_account_deactivated(client: Client) -> None: + request = { + "content": "**foo**", + } + result = client.render_message(request) + + validate_against_openapi_schema(result, "/rest-error-handling", "post", "403_0") + + def test_invalid_stream_error(client: Client) -> None: result = client.get_stream_id("nonexistent") diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 7a0f28d0dc..0ecad620e5 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -9266,6 +9266,14 @@ paths: - $ref: "#/components/schemas/InvalidApiKeyError" - $ref: "#/components/schemas/MissingArgumentError" - $ref: "#/components/schemas/UserNotAuthorizedError" + "403": + description: | + Forbidden + content: + application/json: + schema: + oneOf: + - $ref: "#/components/schemas/UserDeactivatedError" /zulip-outgoing-webhook: post: operationId: zulip_outgoing_webhooks @@ -10855,6 +10863,15 @@ components: "msg": "User not authorized for this query", "result": "error", } + UserDeactivatedError: + allOf: + - $ref: "#/components/schemas/CodedError" + - example: + { + "code": "USER_DEACTIVATED", + "msg": "Account is deactivated", + "result": "error", + } ################### # Shared responses diff --git a/zerver/tests/test_decorators.py b/zerver/tests/test_decorators.py index ff26cfce22..4106a153d8 100644 --- a/zerver/tests/test_decorators.py +++ b/zerver/tests/test_decorators.py @@ -1209,7 +1209,7 @@ class InactiveUserTest(ZulipTestCase): "to": self.example_email("othello"), }, ) - self.assert_json_error_contains(result, "Account is deactivated", status_code=400) + self.assert_json_error_contains(result, "Account is deactivated", status_code=403) result = self.api_post( self.example_user("hamlet"), @@ -1238,7 +1238,7 @@ class InactiveUserTest(ZulipTestCase): change_user_is_active(user_profile, False) result = self.client_post("/json/fetch_api_key", {"password": test_password}) - self.assert_json_error_contains(result, "Account is deactivated", status_code=400) + self.assert_json_error_contains(result, "Account is deactivated", status_code=403) def test_login_deactivated_user(self) -> None: """ @@ -1304,7 +1304,7 @@ class InactiveUserTest(ZulipTestCase): url = f"/api/v1/external/jira?api_key={api_key}&stream=jira_custom" data = self.webhook_fixture_data("jira", "created_v2") result = self.client_post(url, data, content_type="application/json") - self.assert_json_error_contains(result, "Account is deactivated", status_code=400) + self.assert_json_error_contains(result, "Account is deactivated", status_code=403) class TestIncomingWebhookBot(ZulipTestCase): @@ -1647,7 +1647,9 @@ class TestAuthenticatedJsonPostViewDecorator(ZulipTestCase): self.login_user(user_profile) # we deactivate user manually because do_deactivate_user removes user session change_user_is_active(user_profile, False) - self.assert_json_error_contains(self._do_test(user_profile), "Account is deactivated") + self.assert_json_error_contains( + self._do_test(user_profile), "Account is deactivated", status_code=403 + ) do_reactivate_user(user_profile, acting_user=None) def test_authenticated_json_post_view_if_user_realm_is_deactivated(self) -> None: