Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Concatenate parameters on Request/WebSocket discovery in requires() #1337

Closed
wants to merge 1 commit into from

Conversation

Kludex
Copy link
Sponsor Member

@Kludex Kludex commented Nov 15, 2021

This is an study about the differences between Starlette and FastAPI using the AuthenticationMiddleware. The problem is on how FastAPI manipulates the function signature, as we can see below:

Starlette Application
import typing

from starlette.applications import Starlette
from starlette.authentication import (AuthCredentials, AuthenticationBackend,
                                      BaseUser, SimpleUser, requires)
from starlette.middleware import Middleware
from starlette.middleware.authentication import AuthenticationMiddleware
from starlette.requests import HTTPConnection, Request
from starlette.responses import JSONResponse
from starlette.routing import Route


class AuthBackend(AuthenticationBackend):
    async def authenticate(
        self, conn: HTTPConnection
    ) -> typing.Optional[typing.Tuple["AuthCredentials", "BaseUser"]]:
        return AuthCredentials(["TheScope"]), SimpleUser("TheUser)")


class Main:
    @requires("TheScope")
    def some_route(self, request: Request):
        return JSONResponse({"hello": "world"})


main = Main()
routes = [Route("/", endpoint=main.some_route, methods=["GET"])]
middleware = [Middleware(AuthenticationMiddleware, backend=AuthBackend())]
app = Starlette(routes=routes, middleware=middleware)

Args: (<main_starlette.Main object at 0x7f0f9e6da400>, <starlette.requests.Request object at 0x7f0f9e6d4370>)
Kwargs: {}

FastAPI Application Working
import typing

from fastapi import FastAPI
from starlette.authentication import (
    AuthCredentials,
    AuthenticationBackend,
    BaseUser,
    SimpleUser,
    requires,
)
from starlette.middleware.authentication import AuthenticationMiddleware
from starlette.requests import HTTPConnection, Request


class AuthBackend(AuthenticationBackend):
    async def authenticate(
        self, conn: HTTPConnection
    ) -> typing.Optional[typing.Tuple["AuthCredentials", "BaseUser"]]:
        return AuthCredentials(["TheScope"]), SimpleUser("TheUser)")


app = FastAPI()
app.add_middleware(AuthenticationMiddleware, backend=AuthBackend())


@app.get("/")
@requires("TheScope")
def some_route(request: Request):
    return {"hello": "world"}

Args: ()
Kwargs: {'request': <starlette.requests.Request object at 0x7fbc12d376d0>}

FastAPI Application not working
# main_fastapi.py

import typing

from fastapi import APIRouter, FastAPI
from starlette.authentication import (
    AuthCredentials,
    AuthenticationBackend,
    BaseUser,
    SimpleUser,
    requires,
)
from starlette.middleware.authentication import AuthenticationMiddleware
from starlette.requests import HTTPConnection, Request


class AuthBackend(AuthenticationBackend):
    async def authenticate(
        self, conn: HTTPConnection
    ) -> typing.Optional[typing.Tuple["AuthCredentials", "BaseUser"]]:
        return AuthCredentials(["TheScope"]), SimpleUser("TheUser)")


app = FastAPI()
app.add_middleware(AuthenticationMiddleware, backend=AuthBackend())


class Main:
    @requires("TheScope")
    def some_route(self, request: Request):
        return {"hello": "world"}


main = Main()
router = APIRouter()
router.add_api_route("/", main.some_route, methods=["GET"])

app.include_router(router)

Args: (<main_fastapi.Main object at 0x7f03b57ef580>,)
Kwargs: {'request': <starlette.requests.Request object at 0x7f03b57fdac0>}

To be fair, this will solve the issue on FastAPI, but it doesn't seem right for Starlette to consider this case.

For the above reason, I'm not very keen on having this merged. I'll let this PR live to see if anyone else has a different opinion.

@LarsStegman
Copy link
Contributor

LarsStegman commented Nov 15, 2021

Ah I do like that solution better, but I don't think the order of kwargs is guaranteed. That shouldn't really matter when request is in kwargs, though.

def print_args(*args, **kwargs):
    print(list(kwargs.values()))
    
print_args(a=2,b=3)
print_args(b=3,a=2)

results in

[2,3]
[3,2]

@LarsStegman
Copy link
Contributor

I have been thinking about it a little more.

I think I like my solution more because it runs in O(1) instead of O(n) (constructing and concatenating the tuple) and my solution is more correct because the order of kwarg is not used.

My solution checks whether the args tuple contains request by checking whether the index of the request parameter falls within the tuple length.
There are two possibilities:

  1. request is in args: all parameters before request must also be in args, since they cannot be a kwarg as kwargs are not allowed to be followed by args and request is an arg. The index of request will be lower than the length of the args tuple.
  2. request is in kwargs: the argument in the args tuple is irrelevant, as the request is taken from kwargs.

In both cases the correct argument is used to find request.

@Kludex
Copy link
Sponsor Member Author

Kludex commented Nov 15, 2021

You're right.

@Kludex Kludex closed this Nov 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Authentication middleware IndexError on instance method
2 participants