From dd2fd8edda17bb28f33433227ed5f80431cbbb5d Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Mon, 1 Aug 2022 18:21:20 -0400 Subject: [PATCH] rest: Implement get_target_view_function_or_response. As noted in the docstring, this is a temporary helper function that separates routing for paths that support multiple HTTP methods from `rest_dispatch` itself. We will need to replace this helper with class-based views in the future. The helper will also be handy to reduce duplication when splitting up `rest_dispatch` by authentication methods. Signed-off-by: Zixuan James Li --- zerver/lib/rest.py | 212 +++++++++++++++++++++++++-------------------- 1 file changed, 119 insertions(+), 93 deletions(-) diff --git a/zerver/lib/rest.py b/zerver/lib/rest.py index c861ea2f0c..09671a8c2f 100644 --- a/zerver/lib/rest.py +++ b/zerver/lib/rest.py @@ -41,30 +41,24 @@ def default_never_cache_responses(view_func: ViewFuncT) -> ViewFuncT: return cast(ViewFuncT, _wrapped_view_func) # https://github.com/python/mypy/issues/1927 -@default_never_cache_responses -@csrf_exempt -def rest_dispatch(request: HttpRequest, **kwargs: Any) -> HttpResponse: - """Dispatch to a REST API endpoint. +def get_target_view_function_or_response( + request: HttpRequest, rest_dispatch_kwargs: Dict[str, Any] +) -> Union[Tuple[Callable[..., HttpResponse], Set[str]], HttpResponse]: + """Helper for REST API request dispatch. The rest_dispatch_kwargs + parameter is expected to be a dictionary mapping HTTP methods to + a mix of view functions and (view_function, {view_flags}) tuples. - Unauthenticated endpoints should not use this, as authentication is verified - in the following ways: - * for paths beginning with /api, HTTP basic auth - * for paths beginning with /json (used by the web client), the session token + * Returns an error HttpResponse for unsupported HTTP methods. - This calls the function named in kwargs[request.method], if that request - method is supported, and after wrapping that function to: + * Otherwise, returns a tuple containing the view function + corresponding to the request's HTTP method, as well as the + appropriate set of view flags. - * protect against CSRF (if the user is already authenticated through - a Django session) - * authenticate via an API key (otherwise) - * coerce PUT/PATCH/DELETE into having POST-like semantics for - retrieving variables + HACK: Mutates the passed rest_dispatch_kwargs, removing the HTTP + method details but leaving any other parameters for the caller to + pass directly to the view function. We should see if we can remove + this feature; it's not clear it's actually used. - Any keyword args that are *not* HTTP methods are passed through to the - target function. - - Never make a urls.py pattern put user input into a variable called GET, POST, - etc, as that is where we route HTTP verbs to target functions. """ supported_methods: Dict[str, Any] = {} request_notes = RequestNotes.get_notes(request) @@ -73,11 +67,12 @@ def rest_dispatch(request: HttpRequest, **kwargs: Any) -> HttpResponse: # view function logic and just return the response. return request_notes.saved_response - # duplicate kwargs so we can mutate the original as we go - for arg in list(kwargs): + # The list() duplicates rest_dispatch_kwargs, since this loop + # mutates the original. + for arg in list(rest_dispatch_kwargs): if arg in METHODS: - supported_methods[arg] = kwargs[arg] - del kwargs[arg] + supported_methods[arg] = rest_dispatch_kwargs[arg] + del rest_dispatch_kwargs[arg] if "GET" in supported_methods: supported_methods.setdefault("HEAD", supported_methods["GET"]) @@ -95,79 +90,110 @@ def rest_dispatch(request: HttpRequest, **kwargs: Any) -> HttpResponse: if method_to_use in supported_methods: entry = supported_methods[method_to_use] if isinstance(entry, tuple): - target_function, view_flags = entry - else: - target_function = supported_methods[method_to_use] - view_flags = set() - - # Set request_notes.query for update_activity_user(), which is called - # by some of the later wrappers. - request_notes.query = target_function.__name__ - - # We want to support authentication by both cookies (web client) - # and API keys (API clients). In the former case, we want to - # do a check to ensure that CSRF etc is honored, but in the latter - # we can skip all of that. - # - # Security implications of this portion of the code are minimal, - # as we should worst-case fail closed if we miscategorize a request. - - # for some special views (e.g. serving a file that has been - # uploaded), we support using the same URL for web and API clients. - if "override_api_url_scheme" in view_flags and "Authorization" in request.headers: - # This request uses standard API based authentication. - # For override_api_url_scheme views, we skip our normal - # rate limiting, because there are good reasons clients - # might need to (e.g.) request a large number of uploaded - # files or avatars in quick succession. - target_function = authenticated_rest_api_view(skip_rate_limiting=True)(target_function) - elif "override_api_url_scheme" in view_flags and request.GET.get("api_key") is not None: - # This request uses legacy API authentication. We - # unfortunately need that in the React Native mobile apps, - # because there's no way to set the Authorization header in - # React Native. See last block for rate limiting notes. - target_function = authenticated_uploads_api_view(skip_rate_limiting=True)( - target_function - ) - # /json views (web client) validate with a session token (cookie) - elif not request.path.startswith("/api") and request.user.is_authenticated: - # Authenticated via sessions framework, only CSRF check needed - auth_kwargs = {} - if "override_api_url_scheme" in view_flags: - auth_kwargs["skip_rate_limiting"] = True - target_function = csrf_protect(authenticated_json_view(target_function, **auth_kwargs)) - - # most clients (mobile, bots, etc) use HTTP basic auth and REST calls, where instead of - # username:password, we use email:apiKey - elif "Authorization" in request.headers: - # Wrap function with decorator to authenticate the user before - # proceeding - target_function = authenticated_rest_api_view( - allow_webhook_access="allow_incoming_webhooks" in view_flags, - )(target_function) - elif ( - request.path.startswith(("/json", "/avatar", "/user_uploads", "/thumbnail")) - and "allow_anonymous_user_web" in view_flags - ): - # For endpoints that support anonymous web access, we do that. - # TODO: Allow /api calls when this is stable enough. - target_function = csrf_protect(public_json_view(target_function)) - else: - # Otherwise, throw an authentication error; our middleware - # will generate the appropriate HTTP response. - raise MissingAuthenticationError() - - if request.method in ["DELETE", "PATCH", "PUT"]: - # process_as_post needs to be the outer decorator, because - # otherwise we might access and thus cache a value for - # request.POST. - target_function = process_as_post(target_function) - - return target_function(request, **kwargs) + return entry + return supported_methods[method_to_use], set() return json_method_not_allowed(list(supported_methods.keys())) +@default_never_cache_responses +@csrf_exempt +def rest_dispatch(request: HttpRequest, **kwargs: Any) -> HttpResponse: + """Dispatch to a REST API endpoint. + + Authentication is verified in the following ways: + * for paths beginning with /api, HTTP basic auth + * for paths beginning with /json (used by the web client), the session token + + Unauthenticated requests may use this endpoint only with the + allow_anonymous_user_web view flag. + + This calls the function named in kwargs[request.method], if that request + method is supported, and after wrapping that function to: + + * protect against CSRF (if the user is already authenticated through + a Django session) + * authenticate via an API key (otherwise) + * coerce PUT/PATCH/DELETE into having POST-like semantics for + retrieving variables + + Any keyword args that are *not* HTTP methods are passed through to the + target function. + + Never make a urls.py pattern put user input into a variable called GET, POST, + etc, as that is where we route HTTP verbs to target functions. + + """ + result = get_target_view_function_or_response(request, kwargs) + if isinstance(result, HttpResponse): + return result + target_function, view_flags = result + request_notes = RequestNotes.get_notes(request) + + # Set request_notes.query for update_activity_user(), which is called + # by some of the later wrappers. + request_notes.query = target_function.__name__ + + # We want to support authentication by both cookies (web client) + # and API keys (API clients). In the former case, we want to + # do a check to ensure that CSRF etc is honored, but in the latter + # we can skip all of that. + # + # Security implications of this portion of the code are minimal, + # as we should worst-case fail closed if we miscategorize a request. + + # for some special views (e.g. serving a file that has been + # uploaded), we support using the same URL for web and API clients. + if "override_api_url_scheme" in view_flags and "Authorization" in request.headers: + # This request uses standard API based authentication. + # For override_api_url_scheme views, we skip our normal + # rate limiting, because there are good reasons clients + # might need to (e.g.) request a large number of uploaded + # files or avatars in quick succession. + target_function = authenticated_rest_api_view(skip_rate_limiting=True)(target_function) + elif "override_api_url_scheme" in view_flags and request.GET.get("api_key") is not None: + # This request uses legacy API authentication. We + # unfortunately need that in the React Native mobile apps, + # because there's no way to set the Authorization header in + # React Native. See last block for rate limiting notes. + target_function = authenticated_uploads_api_view(skip_rate_limiting=True)(target_function) + # /json views (web client) validate with a session token (cookie) + elif not request.path.startswith("/api") and request.user.is_authenticated: + # Authenticated via sessions framework, only CSRF check needed + auth_kwargs = {} + if "override_api_url_scheme" in view_flags: + auth_kwargs["skip_rate_limiting"] = True + target_function = csrf_protect(authenticated_json_view(target_function, **auth_kwargs)) + + # most clients (mobile, bots, etc) use HTTP basic auth and REST calls, where instead of + # username:password, we use email:apiKey + elif "Authorization" in request.headers: + # Wrap function with decorator to authenticate the user before + # proceeding + target_function = authenticated_rest_api_view( + allow_webhook_access="allow_incoming_webhooks" in view_flags, + )(target_function) + elif ( + request.path.startswith(("/json", "/avatar", "/user_uploads", "/thumbnail")) + and "allow_anonymous_user_web" in view_flags + ): + # For endpoints that support anonymous web access, we do that. + # TODO: Allow /api calls when this is stable enough. + target_function = csrf_protect(public_json_view(target_function)) + else: + # Otherwise, throw an authentication error; our middleware + # will generate the appropriate HTTP response. + raise MissingAuthenticationError() + + if request.method in ["DELETE", "PATCH", "PUT"]: + # process_as_post needs to be the outer decorator, because + # otherwise we might access and thus cache a value for + # request.POST. + target_function = process_as_post(target_function) + + return target_function(request, **kwargs) + + def rest_path( route: str, kwargs: Mapping[str, object] = {},