From af47fa705e6d2bbb028097beb0a861bf61d0f4a1 Mon Sep 17 00:00:00 2001 From: Tim Abbott Date: Mon, 5 Jul 2021 11:28:24 -0700 Subject: [PATCH] exceptions: Use HTTP 401 code for authentication errors. --- templates/zerver/api/changelog.md | 12 +++++++++ templates/zerver/api/rest-error-handling.md | 6 +++-- version.py | 2 +- zerver/lib/exceptions.py | 3 +-- zerver/openapi/zulip.yaml | 10 ++++++-- zerver/tests/test_auth_backends.py | 28 ++++++++++----------- zerver/tests/test_decorators.py | 16 ++++++------ 7 files changed, 48 insertions(+), 29 deletions(-) diff --git a/templates/zerver/api/changelog.md b/templates/zerver/api/changelog.md index f04cefe14e..69ece02dfb 100644 --- a/templates/zerver/api/changelog.md +++ b/templates/zerver/api/changelog.md @@ -11,6 +11,18 @@ below features are supported. ## Changes in Zulip 5.0 +**Feature level 76** + +* [`POST /fetch_api_key`](/api/fetch_api_key), [`POST + /dev_fetch_api_key`](/api/dev_fetch_api_key): The HTTP status for + authentication errors is now 401. This was previously 403. +* All API endpoints now use the HTTP 401 error status for API requests + involving a deactivated user or realm. This was previously 403. +* Mobile push notifications now include the `mentioned_user_group_id` + and `mentioned_user_group_name` fields when a user group containing + the user is mentioned. Previously, these were indistinguishable + from personal mentions (as both types have `trigger="mention"`). + **Feature level 75** * [`POST /register`](/api/register-queue), `PATCH /realm`: Replaced `allow_community_topic_editing` diff --git a/templates/zerver/api/rest-error-handling.md b/templates/zerver/api/rest-error-handling.md index 22f272c1c2..622d62a70c 100644 --- a/templates/zerver/api/rest-error-handling.md +++ b/templates/zerver/api/rest-error-handling.md @@ -19,6 +19,10 @@ errors common to many endpoints: {generate_code_example|/rest-error-handling:post|fixture(400)} +{generate_code_example|/rest-error-handling:post|fixture(401)} + +{generate_code_example|/rest-error-handling:post|fixture(403)} + {generate_code_example|/rest-error-handling:post|fixture(429)} The `retry-after` paremeter in the response indicates how many seconds @@ -49,5 +53,3 @@ limit. **Changes**: The `code` field in the response is new in Zulip 4.0 (feature level 36). - -{generate_code_example|/rest-error-handling:post|fixture(403)} diff --git a/version.py b/version.py index d9a423fbd1..377c2e4809 100644 --- a/version.py +++ b/version.py @@ -33,7 +33,7 @@ DESKTOP_WARNING_VERSION = "5.4.3" # Changes should be accompanied by documentation explaining what the # new level means in templates/zerver/api/changelog.md, as well as # "**Changes**" entries in the endpoint's documentation in `zulip.yaml`. -API_FEATURE_LEVEL = 75 +API_FEATURE_LEVEL = 76 # Bump the minor PROVISION_VERSION to indicate that folks should provision # only when going from an old version of the code to a newer version. Bump diff --git a/zerver/lib/exceptions.py b/zerver/lib/exceptions.py index 6d1e78f81d..3515f445ca 100644 --- a/zerver/lib/exceptions.py +++ b/zerver/lib/exceptions.py @@ -280,8 +280,7 @@ class StreamAdministratorRequired(JsonableError): class AuthenticationFailedError(JsonableError): # Generic class for authentication failures code: ErrorCode = ErrorCode.AUTHENTICATION_FAILED - # Bug: Shouldn't this be 401? - http_status_code = 403 + http_status_code = 401 def __init__(self) -> None: pass diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index c7b1cc24f5..13c134c357 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -11151,9 +11151,9 @@ paths: - $ref: "#/components/schemas/InvalidApiKeyError" - $ref: "#/components/schemas/MissingArgumentError" - $ref: "#/components/schemas/UserNotAuthorizedError" - "403": + "401": description: | - Forbidden. + Unauthorized. content: application/json: schema: @@ -12855,6 +12855,9 @@ components: description: | ## User account deactivated + **Changes**: These errors used the HTTP 403 status code + before Zulip 5.0 (feature level 76). + A typical failed json response for when user's account is deactivated RateLimitedError: allOf: @@ -12882,6 +12885,9 @@ components: description: | ## Realm deactivated + **Changes**: These errors used the HTTP 403 status code + before Zulip 5.0 (feature level 76). + A typical failed json response for when user's organization is deactivated ################### diff --git a/zerver/tests/test_auth_backends.py b/zerver/tests/test_auth_backends.py index df89a79b5f..84c3184f7c 100644 --- a/zerver/tests/test_auth_backends.py +++ b/zerver/tests/test_auth_backends.py @@ -4057,7 +4057,7 @@ class FetchAPIKeyTest(ZulipTestCase): result = self.client_post( "/api/v1/fetch_api_key", dict(username=self.email, password="wrong") ) - self.assert_json_error(result, "Your username or password is incorrect", 403) + self.assert_json_error(result, "Your username or password is incorrect", 401) def test_invalid_subdomain(self) -> None: with mock.patch("zerver.views.auth.get_realm_from_request", return_value=None): @@ -4074,7 +4074,7 @@ class FetchAPIKeyTest(ZulipTestCase): dict(username=self.email, password=initial_password(self.email)), ) self.assert_json_error_contains( - result, "Password authentication is disabled in this organization", 403 + result, "Password authentication is disabled in this organization", 401 ) @override_settings(AUTHENTICATION_BACKENDS=("zproject.backends.ZulipLDAPAuthBackend",)) @@ -4101,14 +4101,14 @@ class FetchAPIKeyTest(ZulipTestCase): "/api/v1/fetch_api_key", dict(username="hamlet", password=self.ldap_password("hamlet")), ) - self.assert_json_error(result, "Your username or password is incorrect", 403) + self.assert_json_error(result, "Your username or password is incorrect", 401) self.change_ldap_user_attr("hamlet", "department", "testWrongRealm") result = self.client_post( "/api/v1/fetch_api_key", dict(username="hamlet", password=self.ldap_password("hamlet")), ) - self.assert_json_error(result, "Your username or password is incorrect", 403) + self.assert_json_error(result, "Your username or password is incorrect", 401) self.change_ldap_user_attr("hamlet", "department", "zulip") result = self.client_post( @@ -4134,7 +4134,7 @@ class FetchAPIKeyTest(ZulipTestCase): "/api/v1/fetch_api_key", dict(username="hamlet", password=self.ldap_password("hamlet")), ) - self.assert_json_error(result, "Your username or password is incorrect", 403) + self.assert_json_error(result, "Your username or password is incorrect", 401) self.change_ldap_user_attr("hamlet", "test2", "testing") # Check with only one set @@ -4142,7 +4142,7 @@ class FetchAPIKeyTest(ZulipTestCase): "/api/v1/fetch_api_key", dict(username="hamlet", password=self.ldap_password("hamlet")), ) - self.assert_json_error(result, "Your username or password is incorrect", 403) + self.assert_json_error(result, "Your username or password is incorrect", 401) self.change_ldap_user_attr("hamlet", "test1", "test") # Setting org_membership to not cause django_ldap_auth to warn, when synchronising @@ -4177,7 +4177,7 @@ class FetchAPIKeyTest(ZulipTestCase): "/api/v1/fetch_api_key", dict(username="hamlet", password=self.ldap_password("hamlet")), ) - self.assert_json_error(result, "Your username or password is incorrect", 403) + self.assert_json_error(result, "Your username or password is incorrect", 401) # Override access with `org_membership` self.change_ldap_user_attr("hamlet", "department", "zulip") @@ -4196,7 +4196,7 @@ class FetchAPIKeyTest(ZulipTestCase): "/api/v1/fetch_api_key", dict(username="hamlet", password=self.ldap_password("hamlet")), ) - self.assert_json_error(result, "Your username or password is incorrect", 403) + self.assert_json_error(result, "Your username or password is incorrect", 401) def test_inactive_user(self) -> None: do_deactivate_user(self.user_profile, acting_user=None) @@ -4204,7 +4204,7 @@ class FetchAPIKeyTest(ZulipTestCase): "/api/v1/fetch_api_key", dict(username=self.email, password=initial_password(self.email)), ) - self.assert_json_error_contains(result, "Account is deactivated", 403) + self.assert_json_error_contains(result, "Account is deactivated", 401) def test_deactivated_realm(self) -> None: do_deactivate_realm(self.user_profile.realm, acting_user=None) @@ -4212,7 +4212,7 @@ class FetchAPIKeyTest(ZulipTestCase): "/api/v1/fetch_api_key", dict(username=self.email, password=initial_password(self.email)), ) - self.assert_json_error_contains(result, "This organization has been deactivated", 403) + self.assert_json_error_contains(result, "This organization has been deactivated", 401) def test_old_weak_password_after_hasher_change(self) -> None: user_profile = self.example_user("hamlet") @@ -4234,7 +4234,7 @@ class FetchAPIKeyTest(ZulipTestCase): dict(username=self.email, password=password), ) self.assert_json_error( - result, "Your password has been disabled and needs to be reset", 403 + result, "Your password has been disabled and needs to be reset", 401 ) @@ -4260,17 +4260,17 @@ class DevFetchAPIKeyTest(ZulipTestCase): def test_unregistered_user(self) -> None: email = "foo@zulip.com" result = self.client_post("/api/v1/dev_fetch_api_key", dict(username=email)) - self.assert_json_error_contains(result, "Your username or password is incorrect", 403) + self.assert_json_error_contains(result, "Your username or password is incorrect", 401) def test_inactive_user(self) -> None: do_deactivate_user(self.user_profile, acting_user=None) result = self.client_post("/api/v1/dev_fetch_api_key", dict(username=self.email)) - self.assert_json_error_contains(result, "Account is deactivated", 403) + self.assert_json_error_contains(result, "Account is deactivated", 401) def test_deactivated_realm(self) -> None: do_deactivate_realm(self.user_profile.realm, acting_user=None) result = self.client_post("/api/v1/dev_fetch_api_key", dict(username=self.email)) - self.assert_json_error_contains(result, "This organization has been deactivated", 403) + self.assert_json_error_contains(result, "This organization has been deactivated", 401) def test_dev_auth_disabled(self) -> None: with mock.patch("zerver.views.development.dev_login.dev_auth_enabled", return_value=False): diff --git a/zerver/tests/test_decorators.py b/zerver/tests/test_decorators.py index 94a2482b7f..9c703d01d7 100644 --- a/zerver/tests/test_decorators.py +++ b/zerver/tests/test_decorators.py @@ -1096,7 +1096,7 @@ class DeactivatedRealmTest(ZulipTestCase): }, ) self.assert_json_error_contains( - result, "This organization has been deactivated", status_code=403 + result, "This organization has been deactivated", status_code=401 ) result = self.api_post( @@ -1128,7 +1128,7 @@ class DeactivatedRealmTest(ZulipTestCase): realm.save() result = self.client_post("/json/fetch_api_key", {"password": test_password}) self.assert_json_error_contains( - result, "This organization has been deactivated", status_code=403 + result, "This organization has been deactivated", status_code=401 ) def test_webhook_deactivated_realm(self) -> None: @@ -1143,7 +1143,7 @@ class DeactivatedRealmTest(ZulipTestCase): data = self.webhook_fixture_data("jira", "created_v2") result = self.client_post(url, data, content_type="application/json") self.assert_json_error_contains( - result, "This organization has been deactivated", status_code=403 + result, "This organization has been deactivated", status_code=401 ) @@ -1246,7 +1246,7 @@ class InactiveUserTest(ZulipTestCase): "to": self.example_email("othello"), }, ) - self.assert_json_error_contains(result, "Account is deactivated", status_code=403) + self.assert_json_error_contains(result, "Account is deactivated", status_code=401) result = self.api_post( self.example_user("hamlet"), @@ -1275,7 +1275,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=403) + self.assert_json_error_contains(result, "Account is deactivated", status_code=401) def test_login_deactivated_user(self) -> None: """ @@ -1341,7 +1341,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=403) + self.assert_json_error_contains(result, "Account is deactivated", status_code=401) class TestIncomingWebhookBot(ZulipTestCase): @@ -1689,7 +1689,7 @@ class TestAuthenticatedJsonPostViewDecorator(ZulipTestCase): # 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", status_code=403 + self._do_test(user_profile), "Account is deactivated", status_code=401 ) do_reactivate_user(user_profile, acting_user=None) @@ -1702,7 +1702,7 @@ class TestAuthenticatedJsonPostViewDecorator(ZulipTestCase): self.assert_json_error_contains( self._do_test(user_profile), "This organization has been deactivated", - status_code=403, + status_code=401, ) do_reactivate_realm(user_profile.realm)