Pass the HttpRequest explicitly through the two webhooks that log to
the webhook loggers.
get_current_request is now unused, so remove it (in the same commit
for test coverage reasons).
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Because the third party might not be expecting a 400 from our
webhooks, we now instead use 200 status code for unknown events,
while sending back the error to Sentry. Because it is no longer an error
response, the response type should now be "success".
Fixes#24721.
Creates `MutableJsonResponse` as a subclass of Django's `HttpResponse`
that we can modify for ignored parameters in the response content.
Updates responses to include `ignored_parameters_unsupported` in
the response data through `has_request_variables`. Creates unit
test for this implementation in `test_decorators.py`.
The `method` parameter processed in `rest_dispatch` is not in the
`REQ` framework, so for any tests that pass that parameter, assert
for the ignored parameter with a comment.
Updates OpenAPI documentation for `ignored_parameters_unsupported`
being returned in the JSON success response for all endpoints.
Adds detailed documentation in the error handling article, and
links to that page in relevant locations throughout the API docs.
For the majority of endpoints, the documentation does not include
the array in any examples of return values, and instead links to
the error handling page. The exceptions are the three endpoints
that had previously supported this return value. The changes note
and example for these endpoints is also used in the error
handling page.
The password parameter being passed in the `_do_test` helper
function for `TestAuthenticatedJsonPostViewDecorator` tests was
being ignored, as the user needs to be logged in. Removes the
parameter from the helper function and updates the success test
to use `assert_json_success` instead of just checking the status
code.
Also adds a test case for when a user is not logged in to confirm
that it returns an UnauthorizedError.
The Client.name field is only 30 characters long, but there is no
limit to the length of parsed User-Agent value which we may attempt to
store in it. This can cause requests with long user-agents to 500
when the creation of the Client row fails.
Truncate the name at 30 characters for the cache key, and passing
`name` to `get_or_create`.
This commits update the code to use user-level email_address_visibility
setting instead of realm-level to set or update the value of UserProfile.email
field and to send the emails to clients.
Major changes are -
- UserProfile.email field is set while creating the user according to
RealmUserDefault.email_address_visbility.
- UserProfile.email field is updated according to change in the setting.
- 'email_address_visibility' is added to person objects in user add event
and in avatar change event.
- client_gravatar can be different for different users when computing
avatar_url for messages and user objects since email available to clients
is dependent on user-level setting.
- For bots, email_address_visibility is set to EVERYONE while creating
them irrespective of realm-default value.
- Test changes are basically setting user-level setting instead of realm
setting and modifying the checks accordingly.
cachify has been removed in 9d448e73d2.
We don't need to keep its tests.
TODO: functools.lru_cache can be replaced by functools.cache when we
drop Python 3.8.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
Although our POST /messages handler accepts the ‘to’ parameter with or
without JSON encoding, there are two problems with passing it as an
unencoded string.
Firstly, you’d fail to send a message to a stream named ‘true’ or
‘false’ or ‘null’ or ‘2022’, as the JSON interpretation is prioritized
over the plain string interpretation.
Secondly, and more importantly for our tests, it violates our OpenAPI
schema, which requires the parameter to be JSON-encoded. This is
because OpenAPI has no concept of a parameter that’s “optionally
JSON-encoded”, nor should it: such a parameter cannot be unambiguously
decoded for the reason above.
Our version of openapi-core doesn’t currently detect this schema
violation, but after the next upgrade it will.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Because rate_limit_request_by_ip is the only caller of it, it is safe
for us to inline RateLimitedIpAddr and remove this helper. This ensures
that we have consistent internals for rate limiting functions, which all
have a should_rate_limit check.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
This change incorporate should_rate_limit into rate_limit_user and
rate_limit_request_by_ip. Note a slight behavior change to other callers
to rate_limit_request_by_ip is made as we now check if the client is
eligible to be exempted from rate limiting now, which was previously
only done as a part of zerver.lib.rate_limiter.rate_limit.
Now we mock zerver.lib.rate_limiter.RateLimitedUser instead of
zerver.decorator.rate_limit_user in
zerver.tests.test_decorators.RateLimitTestCase, because rate_limit_user
will always be called but rate limit only happens the should_rate_limit
check passes;
we can continue to mock zerver.lib.rate_limiter.rate_limit_ip, because the
decorated view functions call rate_limit_request_by_ip that calls
rate_limit_ip when the should_rate_limit check passes.
We need to mock zerver.decorator.rate_limit_user for SkipRateLimitingTest
now because rate_limit has been removed. We don't need to mock
RateLimitedUser in this case because we are only verifying that
the skip_rate_limiting flag works.
To ensure coverage in add_logging_data, a new test case is added to use
a web_public_view (which decorates the view function with
add_logging_data) with a new flag to check_rate_limit_public_or_user_views.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
This allows us to avoid importing from zilencer conditionally in
zerver.lib.rate_limiter, as we make rate limiting self-contained now.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
- RateLimitTestCase.get_ratelimited_view is replaced by a view
function directly decorated by public_json_view.
- the META dict is initialized with "PATH_INFO": "test" because now the
tests cover the process_client codepath;
- HostRequestMock is initialized with host="zulip.testserver" to pass
the validate_account_and_subdomain check;
- check_rate_limit_public_or_user_views replaces both
test_rate_limiting_happens_in_normal_case and
test_rate_limiting_happens_by_ip_if_unauthed.
Overall, we deduplicate the test cases in this change, and make sure
that they also cover the view function decorators for authentication.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
The test setup for some of the test cases are largely similar, so it
would be cleaner to be able to reuse them.
Note that we use "check" in the name of this helper because later we
will extend it to take a flag to set whether rate limiting is expected.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
This refactors the test case alongside, since normal views accessed by
remote server do not get rate limited by remote server anymore.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
This allows us to separate the zilencer paths from other JSON paths,
with explicit type annotation expecting `RemoteZulipServer` as the
second parameter of the handler using
authenticated_remote_server_view.
The test case is also updated to remove a test for a situation that no
longer occurs anymore, since we don't perform subdomain checks on
remote servers.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
This refactoring is necessary to separate the expected type annotation
for view functions with different authentication methods. Currently the
signature aren't actually check against view functions because
`rest_path` does not support type checking parameter types, but it will
become useful once we do.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
This refactors rate limit related functions from `zerver.decorator` to
zerver.lib.rate_limiter.
We conditionally import `RemoteZulipServer`, `RequestNotes`, and
`RateLimitedRemoteZulipServer` to avoid circular dependency.
Most instances of importing these functions from `zerver.decorator` got
updated, with a few exceptions in `zerver.tests.test_decorators`, where
we do want to mock the rate limiting functions imported in
`zerver.decorator`. The same goes with the mocking example in the
"testing-with-django" documentation.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
This refactors `rate_limit` so that we no longer use it as a decorator.
This is a workaround to https://github.com/python/mypy/issues/12909 as
`rate_limit` previous expects different parameters than its callers.
Our approach to test logging handlers also needs to be updated because
the view function is not decorated by `rate_limit`.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
`context` as `AccessDeniedError` is incompatible with
`RequestVariableMissingError`. Mypy does not allow such redefinition.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
This eliminates the possibility of having `request.user` as
`RemoteZulipServer` by refactoring it as an attribute of `RequestNotes`.
So we can effectively narrow the type of `request.user` by testing
`user.is_authenticated` in most cases (except that of `SCIMClient`) in
code paths that require access to `.format_requestor_for_logs` where we
previously expect either `UserProfile` or `RemoteZulipServer` backed by
the implied polymorphism.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
`HostRequestMock` has `user` default to `None`, which later gets
initialized as `AnonymousUser`. The separate initialization here is
unnecessary.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
It's hard to come up with a realistic story where this would matter:
SHARED_SECRET is generated automatically during server setup at the
same time as SECRET_KEY, which is a required setting, but it seems
preferable to be explicit that this is a required parameter for the
internal_notify authentication model.
Instead of using request.POST to get any potential `secret`
parameter used in `authenticate_notify` for `internal_notify_view`
decorator, moves it to the REQ framework parameters as `req_secret`.
Updates existing tests to explicitly test for a request without
`secret` parameter, which defaults to `None`; this is also tested
in `test_event_system.py`.
A request that has went through the auth middleware shouldn't have
`.user` being `None`. We should use `AnonymousUser` by default to
represent unauthenticated users.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
The name does not really comply with the actual behavior of
the decorator since it returns True for an unauthenticated user.
This makes it clear that the 2fa check only applies to users that
are already logged in.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
This replaces user.is_verified with is_2fa_verified.
The helper does extra checks such that the user being checked for 2fa
authentication status is valid.
`request.user.is_verified` is functionally the same as `is_verified`
from `django_otp.middleware`, except that the former is monkey-patched
onto the user object by the 2FA middleware. We use the latter wrapped
in `is_2fa_verified` instead to avoid accessing the patched attribute.
See also: 6b24d56e59/docs/source/overview.rst (authentication-and-verification)
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
This simulates the situation in which the user is not
authenticated (as an AnonymousUser) and have 2FA enabled.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
Since `HttpResponse` is an inaccurate representation of the
monkey-patched response object returned by the Django test client, we
replace it with `_MonkeyPatchedWSGIResponse` as `TestHttpResponse`.
This replaces `HttpResponse` in zerver/tests, analytics/tests, coporate/tests,
zerver/lib/test_classes.py, and zerver/lib/test_helpers.py with
`TestHttpResponse`. Several files in zerver/tests are excluded
from this substitution.
This commit is auto-generated by a script, with manual adjustments on certain
files squashed into it.
This is a part of the django-stubs refactorings.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
Django caches some information on HttpRequest objects, including the
headers dict, under the assumption that requests won’t be reused.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
To provide a smoother experience of accessing a web public stream,
we don't ask user to login unless user directly requests a
`/login` URL.
Fixes#21690.
Mateusz Mandera
Anders Kaseorg
Zixuan James Li
Alex Vandiver
Lauryn Menard
Sahil Batra
Tim Abbott
Aman Agrawal