Skip to content

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

Closed
Kludex wants to merge 1 commit intomasterfrom
authentication-params
Closed

Concatenate parameters on Request/WebSocket discovery in requires()#1337
Kludex wants to merge 1 commit intomasterfrom
authentication-params

Conversation

@Kludex
Copy link
Copy Markdown
Owner

@Kludex Kludex commented Nov 15, 2021 •
edited
Loading

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.

@Kludex Kludex mentioned this pull request Nov 15, 2021
Merged
@LarsStegman
Copy link
Copy Markdown
Contributor

LarsStegman commented Nov 15, 2021 •
edited
Loading

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
Copy Markdown
Contributor

LarsStegman commented Nov 15, 2021

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
Copy Markdown
Owner 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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Authentication middleware IndexError on instance method

2 participants

@Kludex @LarsStegman