Enhancement: Allow disabling openapi documentation sites and schema download endpoints via config

Goldziher commented 2 years ago

Currently the OpenAPIController always exposes redoc, swagger-ui and stoplight elements static sites, and a yaml + json schema download endpoints. This though might not be desirable.

Tasks:

add a new method on the controller called render_404_not_found or something such like, which renders a simple 404 page.
add a configuration option to the OpenAPIConfig class that determines which endpoints are enabled. For example, a list or set of literal identifiers - which by default includes all values.
add logic to return a 404 if a particular endpoint is not enabled in the config.

Note:

Access the app level config is possble using the request instance. See how this is currently implemented in the OpenAPIController.root method as an example.

ingjavierpinilla commented 2 years ago

Hello there, writing a method that returns an html string would make the tests green even if the endpoint is not authorized because you would still have an HTTP 200 code.

Any idea how this could be solved? I thought about using the class ExceptionResponseContent but as far as I understood, it doesn't allow you to render html.

Goldziher commented 2 years ago

You can return the following:

from starlite import Get, MediaType, Response
from starlette.status import HTTP_404_NOT_FOUND

@get(...)
def handler() -> Response:
   return Response(content=<html string>, status_code=HTTP_404_NOT_FOUND, media_type=MediaType.HTML)

ingjavierpinilla commented 2 years ago

Thanks for your answer.

Not quite, the to_response function treats the response as flat data and tries to construct a new response using a response object as the content parameter.

Or am I missing something? Is there any way to avoid this behavior and return the response directly?

Maybe it is an issue we want to address before this (418) one?

Goldziher commented 2 years ago

are you sure you are on an updated version of starlite? Look here tests/handlers/http/test_to_response.py - this is working as expected.

RSPersonal commented 2 years ago

Hi can i pick this up?

ingjavierpinilla commented 2 years ago

Hi can i pick this up?

Sure @RSPersonal

RSPersonal commented 2 years ago

So bear with me, its my first issue on a open source project. I also seem to run into the same issue as @ingjavierpinilla.

I wrote this method in the controller.

I then tried this in the swagger part.

Even tho i pass the 404 in the status code, the sever still treats it as an 200 request.

Also the page does not render the html.

peterschutt commented 2 years ago

Hey @RSPersonal - thanks for looking at this.

How about raising NotFoundException and register a handler for that on the Controller that returns the html response?

RSPersonal commented 2 years ago

alright, will look at it after work and update you

Goldziher commented 2 years ago

alright, will look at it after work and update you

It would be beneficial to have a PR with your existing code. I'd like to understand what is going on there, because it should be working.

peterschutt commented 2 years ago

Looks like the route is returning None - no return

Goldziher commented 2 years ago

Looks like the route is returning None - no return

Well then

ingjavierpinilla commented 2 years ago

Hey @RSPersonal - thanks for looking at this.

How about raising NotFoundException and register a handler for that on the Controller that returns the html response?

Hi, this works but wouldn't PermissionDeniedException be a better response?

peterschutt commented 2 years ago

I think 404 is the correct response - it should be as if the route doesn't exist.

Instead of the exception handler approach, it might be better to register a before_request hook on the controller that would intercept the request before it reaches the handler, inspect the request path to ascertain if the endpoint should be served or not and if not, return the 404 response object. I think that would better allow you to centralize the logic.

Goldziher commented 2 years ago

I think 404 is the correct response - it should be as if the route doesn't exist.

Instead of the exception handler approach, it might be better to register a before_request hook on the controller that would intercept the request before it reaches the handler, inspect the request path to ascertain if the endpoint should be served or not and if not, return the 404 response object. I think that would better allow you to centralize the logic.

Sorry for butting in We want to make the 404 template customizable, that's why a render method is the best and simplest approach. If this is not working, I think we have a bug.

Obviously the implementation needs to be fixed first to check.

ingjavierpinilla commented 2 years ago

I think 404 is the correct response - it should be as if the route doesn't exist.

Instead of the exception handler approach, it might be better to register a before_request hook on the controller that would intercept the request before it reaches the handler, inspect the request path to ascertain if the endpoint should be served or not and if not, return the 404 response object. I think that would better allow you to centralize the logic.

I thought about that too, but I didn't see an easy and clean way to check the path.

You propose using a before_request hook -> raise a NotFoundException -> use an exception_handler to catch the exception?

peterschutt commented 2 years ago

I think 404 is the correct response - it should be as if the route doesn't exist. Instead of the exception handler approach, it might be better to register a before_request hook on the controller that would intercept the request before it reaches the handler, inspect the request path to ascertain if the endpoint should be served or not and if not, return the 404 response object. I think that would better allow you to centralize the logic.

But we want to make the 404 template customizable

Sorry, missed that spec - disregard the above as I agree in that case the method approach is the way to go.

Perhaps we should be returning response objects from the routes instead of str then:

    @get(path="/swagger", include_in_schema=False)
    def swagger_ui(self, request: Request) -> Response:
        ...

Goldziher commented 2 years ago

I think 404 is the correct response - it should be as if the route doesn't exist. Instead of the exception handler approach, it might be better to register a before_request hook on the controller that would intercept the request before it reaches the handler, inspect the request path to ascertain if the endpoint should be served or not and if not, return the 404 response object. I think that would better allow you to centralize the logic.

But we want to make the 404 template customizable

Sorry, missed that spec - disregard the above as I agree in that case the method approach is the way to go.

Perhaps we should be returning response objects from the routes instead of str then:
    @get(path="/swagger", include_in_schema=False)
    def swagger_ui(self, request: Request) -> Response:
        ...

What would be the advantage?

peterschutt commented 2 years ago

I think 404 is the correct response - it should be as if the route doesn't exist. Instead of the exception handler approach, it might be better to register a before_request hook on the controller that would intercept the request before it reaches the handler, inspect the request path to ascertain if the endpoint should be served or not and if not, return the 404 response object. I think that would better allow you to centralize the logic.

I thought about that too, but I didn't see an easy and clean way to check the path.

You propose using a before_request hook -> raise a NotFoundException -> use an exception_handler to catch the exception?

You wouldn't need the exception handler when using the before_request approach, you'd just return the 404 response object directly from the hook. But see @Goldziher's comment above - that approach wouldn't allow the 404 template to be easily customized.

peterschutt commented 2 years ago

I think 404 is the correct response - it should be as if the route doesn't exist. Instead of the exception handler approach, it might be better to register a before_request hook on the controller that would intercept the request before it reaches the handler, inspect the request path to ascertain if the endpoint should be served or not and if not, return the 404 response object. I think that would better allow you to centralize the logic.

But we want to make the 404 template customizable

Sorry, missed that spec - disregard the above as I agree in that case the method approach is the way to go. Perhaps we should be returning response objects from the routes instead of str then:
    @get(path="/swagger", include_in_schema=False)
    def swagger_ui(self, request: Request) -> Response:
        ...
What would be the advantage?

Just consistency - otherwise the route handlers would have to return a string for the 200 response, or a Response object in the case of the 404 to override the status code.. not that big a deal.

Goldziher commented 2 years ago

I think 404 is the correct response - it should be as if the route doesn't exist. Instead of the exception handler approach, it might be better to register a before_request hook on the controller that would intercept the request before it reaches the handler, inspect the request path to ascertain if the endpoint should be served or not and if not, return the 404 response object. I think that would better allow you to centralize the logic.

But we want to make the 404 template customizable

Sorry, missed that spec - disregard the above as I agree in that case the method approach is the way to go. Perhaps we should be returning response objects from the routes instead of str then:
    @get(path="/swagger", include_in_schema=False)
    def swagger_ui(self, request: Request) -> Response:
        ...
What would be the advantage?
Just consistency - otherwise the route handlers would have to return a string for the 200 response, or a Response object in the case of the 404 to override the status code.. not that big a deal.

Fair point . Can you show @RSPersonal how this would look?

peterschutt commented 2 years ago

but I didn't see an easy and clean way to check the path.

Don't forget that the Request object extends starlette.Request - so you have request.url.path for example. We need to work out how best to get those starlette attributes to show in our ref. docs.

peterschutt commented 2 years ago

I think 404 is the correct response - it should be as if the route doesn't exist. Instead of the exception handler approach, it might be better to register a before_request hook on the controller that would intercept the request before it reaches the handler, inspect the request path to ascertain if the endpoint should be served or not and if not, return the 404 response object. I think that would better allow you to centralize the logic.

But we want to make the 404 template customizable

Sorry, missed that spec - disregard the above as I agree in that case the method approach is the way to go. Perhaps we should be returning response objects from the routes instead of str then:
    @get(path="/swagger", include_in_schema=False)
    def swagger_ui(self, request: Request) -> Response:
        ...
What would be the advantage?
Just consistency - otherwise the route handlers would have to return a string for the 200 response, or a Response object in the case of the 404 to override the status code.. not that big a deal.
Fair point . Can you show @RSPersonal how this would look?

@RSPersonal @ingjavierpinilla it is a suggestion to change the handler return types to return Response objects. Check out that part of the docs, see if you agree or not, and lets start talking with a PR.

ingjavierpinilla commented 2 years ago

Sounds good to me, should we implement this change in the return types and the matter of this issue in the same PR or should we keep these two implementations in separate PRs?

RSPersonal commented 2 years ago

@ingjavierpinilla which part would you like to pickup ?

peterschutt commented 2 years ago

Sounds good to me, should we implement this change in the return types and the matter of this issue in the same PR or should we keep these two implementations in separate PRs?

Fine to roll them together :+1:

peterschutt commented 2 years ago

closed in #430

litestar-org / litestar

Enhancement: Allow disabling openapi documentation sites and schema download endpoints via config #418