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

Added allow_path_regex to the SessionMiddleware #2316

Closed
wants to merge 4 commits into from
Closed

Added allow_path_regex to the SessionMiddleware #2316

wants to merge 4 commits into from

Conversation

hasansezertasan
Copy link

Summary

SessionMiddleware updates (re assignes) session data even if it's not updated. This causes problems when we use a sub application and update that session data in it, in my case it's a Flask Application (which uses another way to sign session data) mounted to a Starlette Application.

With allow_path_regex I can restrict SessionMiddleware from doing what its doing for a given regex like: '/api/.*' or '/admin/.*' and partially solves #2018

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

@hasansezertasan
Copy link
Author

In addition, I was thinking to open a PR that modifies the session data in a way that Flask does to sign it, I have referenced the thing here at #1922 but I believe it'll break some things for many users.

@hasansezertasan hasansezertasan marked this pull request as draft October 31, 2023 02:41
@hasansezertasan hasansezertasan marked this pull request as ready for review October 31, 2023 02:54
@hasansezertasan hasansezertasan changed the title Session middleware Added allow_path_regex to the SessionMiddleware Nov 12, 2023
@abersheeran
Copy link
Member

We already have the ability to use middleware on a specific route. It seems unnecessary to add this parameter in Session Middleware. https://www.starlette.io/middleware/#applying-middleware-to-groups-of-routes

@Kludex Kludex closed this Dec 22, 2023
@alex-oleshkevich
Copy link
Member

@abersheeran yes, I agree and this is a way I do it.

@hasansezertasan
Copy link
Author

hasansezertasan commented Dec 23, 2023

Thank you @alex-oleshkevich and @abersheeran for your honest reviews. 🙏

This one is mostly inspired by CORSMiddleware which has a parameter called allow_origin_regex. I'm using Starlette with FastAPI and doing .include_router() to add routers and .mount() to add sub applications, which .mount skippes middlewares parameter.

class CORSMiddleware:
def __init__(
self,
app: ASGIApp,
allow_origins: typing.Sequence[str] = (),
allow_methods: typing.Sequence[str] = ("GET",),
allow_headers: typing.Sequence[str] = (),
allow_credentials: bool = False,
allow_origin_regex: typing.Optional[str] = None,
expose_headers: typing.Sequence[str] = (),
max_age: int = 600,
) -> None:

We already have the ability to use middleware on a specific route. It seems unnecessary to add this parameter in Session Middleware. https://www.starlette.io/middleware/#applying-middleware-to-groups-of-routes

I don't think it's the same. I don't want to use middleware on a specific route, I want it not to work on a specific route, I want to use middleware on application level but skip some routes.

On one hand you can add a middleware on application level and add one line to that middleware to skip some of the routes or path regex.

On the other hand you can add same middleware for 25 routes (which probably 50 to 75 lines of code) and skip the 5 routes you want your middleware to skip. (or maybe just one or two, you just want to skip two routes but you need to write 50 to 75 lines more now...) 😞

I'm not insisting on PR to be merged but I believe if CORSMiddleware has this feature, SessionMiddleware also deserves it, since it keeps re-signing the session data even if its not updated. I'mean what's the point of "re-signing" a data if there is no change? 😕

@alex-oleshkevich
Copy link
Member

@hasansezertasan here is a code snippet that solves your need

routes = [
   Route('/'), # no mw
   Mount('/api', routes[ # mounted route set w\o mw
        Route(), Route()
    ]),  
   Mount('/admin', # enable session only to routes starting with '/admin' 
      routes[ # mounted route set w\o mw
        Route(), Route()
     ],
    middleware=[Middleware(SessionMiddleware)]
  ),  
]

@hasansezertasan
Copy link
Author

hasansezertasan commented Dec 23, 2023

@hasansezertasan here is a code snippet that solves your need

routes = [
   Route('/'), # no mw
   Mount('/api', routes[ # mounted route set w\o mw
        Route(), Route()
    ]),  
   Mount('/admin', # enable session only to routes starting with '/admin' 
      routes[ # mounted route set w\o mw
        Route(), Route()
     ],
    middleware=[Middleware(SessionMiddleware)]
  ),  
]

Thank you @alex-oleshkevich for your answer but that's not the case...

I don't want to use middleware on a specific route, I want it not to work on a specific route, I want to use middleware on application level but skip some routes.

As I said...

Here is an example of what I need:

from a2wsgi import WSGIMiddleware
from bottle import Bottle
from flask import Flask
from starlette.applications import Starlette
from starlette.middleware import Middleware
from starlette.middleware.sessions import SessionMiddleware
from starlette.responses import PlainTextResponse
from starlette.routing import Mount, Route

flask_app = Flask(__name__)
botte_app = Bottle()


@flask_app.route("/")
def flask_hello_world():
    return "I'm a Flask app!"


@botte_app.route("/")
def bottle_hello_world():
    return "I'm a Bottle app!"


def starlette_hello_world(request):
    return PlainTextResponse("I'm a Starlette app!")


def starlette_dummy_route(request):
    return PlainTextResponse("Dummy Route!")


sapp = Starlette(
    routes=[
        Route(
            "/",
            starlette_hello_world,
        ),
        Route(
            "/route-a",
            starlette_dummy_route,
        ),
        Route(
            "/route-b",
            starlette_dummy_route,
        ),
        Route(
            "/route-c",
            starlette_dummy_route,
        ),
        Route(
            "/route-n",
            starlette_dummy_route,
        ),
        Mount(
            path="/flask-app",
            app=WSGIMiddleware(flask_app),
        ),
        Mount(
            path="/bottle-app",
            app=WSGIMiddleware(botte_app),
        ),
    ],
    middleware=[
        # App level middleware to apply to all routes
        Middleware(
            SessionMiddleware,
            secret_key="secret-key",
            https_only=False,
            # Allow any route but /flask-app and /bottle-app and /route-c
            allow_path_regex="^(?!/flask-app|/bottle-app|/route-c).*$",
        ),
    ],
)

Why? Because the way of session signing on Flask, Starlette and Bottle are different and there is a collision.

@alex-oleshkevich
Copy link
Member

alex-oleshkevich commented Dec 23, 2023

why does this don't work?

middleware = [
        Middleware(
            SessionMiddleware,
            secret_key="secret-key",
            https_only=False,
        ),
]
routes = [
		Mount('', 
   			routes=[
	        	Route(
	            	"/route-c",
		            starlette_dummy_route,
		        ),
			],
		    middleware=middleware,
	),
       Mount(
            path="/flask-app",
            app=WSGIMiddleware(flask_app),

        ),
        Mount(
            path="/bottle-app",
            app=WSGIMiddleware(botte_app),

        ),
]

@alex-oleshkevich
Copy link
Member

This is rather a question of route organization, not about Starlette.
As the last resort, you can extend SessionMiddleware and apply filtering in it.

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.

4 participants