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.
`cachify` is essentially caching the return value of a function using only
the non-keyword-only arguments as the key.
The use case of the function in the backend can be sufficiently covered by
`functools.lru_cache` as an unbound cache. There is no signficant difference
apart from `cachify` overlooking keyword-only arguments, and
`functools.lru_cache` being conveniently typed.
Signed-off-by: Zixuan James Li <359101898@qq.com>
If an API request specified a `client` parameter, we were
already prioritizing that value over parsing the UserAgent.
In order to have these parameters logged in the `RequestNotes`
as processed parameters instead of ignored parameters, we add
the `has_request_variables` decorator to `parse_client` and
then process the potential `client` parameter through the REQ
framework.
Co-authored by: Tim Abbott <tabbott@zulip.com>
Removes `client` parameter from backend tests using the
`POST /messages` endpoint when the test can use the default
`User-Agent` as the client, which is set to `ZulipMobile` for API
requests and a browser user agent string for web app requests.
Adds request as a parameter to json_success as a refactor towards
making `ignored_parameters_unsupported` functionality available
for all API endpoints.
Also, removes any data parameters that are an empty dict or
a dict with the generic success response values.
Given that these values are uuids, it's better to use UUIDField which is
meant for exactly that, rather than an arbitrary CharField.
This requires modifying some tests to use valid uuids.
Requests to the root subdomain weren't getting request_notes.realm set
even if a realm exists on the root subdomain - which is actually a
common scenario, because simply having one organization, on the root
subdomain, is the simplest and common way for self-hosted deployments.
Anders Kaseorg
Mateusz Mandera
Zixuan James Li
Tim Abbott
Lauryn Menard
Sahil Batra
Aman Agrawal
Zixuan James Li
Lauryn Menard
jai2201