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

Enhancement: Session Middleware should create session id right away #3116

Closed
aranvir opened this issue Feb 15, 2024 · 6 comments · Fixed by #3127
Closed

Enhancement: Session Middleware should create session id right away #3116

aranvir opened this issue Feb 15, 2024 · 6 comments · Fixed by #3127
Labels
Enhancement This is a new feature or request

Comments

@aranvir
Copy link
Contributor

aranvir commented Feb 15, 2024

Summary

Follow-up on: https://github.com/orgs/litestar-org/discussions/3112#discussioncomment-8477410

Currently, the SessionMiddleware of the SessionAuth backend creates the session id in the response wrapper and not when the session is first created, e.g., on user login. This is counter-intuitive since it gives the impression the session is created within the route handler when it's actually not. Furthermore, this makes it difficult or even impossible to add any custom handling of the session id during the login procedure since the session id does not exist yet.

Current state:

@post("/login")
async def login(
        data: UserLogin,
        request: Request,
) -> User:
    user = handle_auth(data)
    request.set_session(
        {"user-id": user.id, "foo": "bar"}
    )
    logger.info(f"Session content: {request.session}")  # This will have content
    logger.info(f"Cookies: {request.cookies}")  # This will be empty. The send wrapper will add a session id here once created
    return user

Basic Example

My first best guess:

On registering the SessionAuth on the app, add a dependency so that it can be accessed as kwarg anywhere (not sure if the reference should be to that objectm, the session backend, or the middleware). On this reference, the user can call a method to pass the session data. This method will:

  • create a session id
  • store the session data in the store with that key
  • add the session data to the connection scope
  • potentially also already add it to the cookie? (not sure if this can be done here or should still be done in the send wrapper)
  • return the session id

So for the user, it would look something like this:

@post("/login")
async def login(
        data: UserLogin,
        request: Request,
        session_backend: SessionBackend
) -> User:
    user = handle_auth(data)
    session_backend.create_session(request=request, session_data={"user-id": user.id, "foo": bar})
    return user

Drawbacks and Impact

No response

Unresolved questions

Only just started digging through the code to understand the complete flow so might have missed something. Yet, the user perspective should be clear.


Note

While we are open for sponsoring on GitHub Sponsors and
OpenCollective, we also utilize Polar.sh to engage in pledge-based sponsorship.

Check out all issues funded or available for funding on our Polar.sh dashboard

  • If you would like to see an issue prioritized, make a pledge towards it!
  • We receive the pledge once the issue is completed & verified
  • This, along with engagement in the community, helps us know which features are a priority to our users.
Fund with Polar
@aranvir aranvir added the Enhancement This is a new feature or request label Feb 15, 2024
@aranvir
Copy link
Contributor Author

aranvir commented Feb 17, 2024

Thought about this some more. In my example, I only consider the use case where a session requires authentication. But there is also the case where you'd want "anonymous" sessions, e.g., you put stuff in a shopping cart and want to check out without a user account. In that case you'd want any route to be able to start a session instead of creating a session via a "logino" route.

I don't know if it makes sense to try and put both use cases in one session solution... probably cleaner to have separate implementations for separate needs.

@provinzkraut
Copy link
Member

I don't know if it makes sense to try and put both use cases in one session solution... probably cleaner to have separate implementations for separate needs.

It does. Sessions aren't tied to authentication. How your app handles users is entirely up to you; If you add an anonymous user, their session would be indistinguishable from an authenticated user. The only difference is what kind of data you store; If you have anonymous user sessions, you shouldn't treat "has a session" as "is authenticated", but store that information inside the session.

@guacs
Copy link
Member

guacs commented Feb 17, 2024

Instead of having to inject the session backend as a dependency, how about we just return the current session ID whenever the user calls request.set_session?

I was thinking that we could create it or get the existing one in the SessionMiddleware before we invoke the route handler. So something like this:

    async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None:
        """ASGI-callable.

        Args:
            scope: The ASGI connection scope.
            receive: The ASGI receive function.
            send: The ASGI send function.

        Returns:
            None
        """

        connection = ASGIConnection[Any, Any, Any, Any](scope, receive=receive, send=send)
        scope["session"] = await self.backend.load_from_connection(connection)

        # get existing session ID or create new one here
        scope["_session_id"] = self.backend.get_session_id(connection)

        await self.app(scope, receive, self.create_send_wrapper(connection))

@aranvir
Copy link
Contributor Author

aranvir commented Feb 21, 2024

Based on @guacs suggestion I implemented and tested the following overrides. Used the session example code from the docs with this CustomServerSideSessionConfig and it works fine.

  • The session id is available before the route handler scope is reached
  • It will be used as session id by the store_in _message function
  • It's available to the user via request.scope.get('_session_id'). Might not be the most explicit way but simpler than an extra dependency I guess? And for convenience, this can always be wrapped in a function and made available as dependency injection if that's preferred.
from typing import Any

from litestar.connection import ASGIConnection
from litestar.types import Receive, Scope, Send
from litestar.middleware.base import DefineMiddleware
from litestar.middleware.session.base import SessionMiddleware
from litestar.middleware.session.server_side import ServerSideSessionConfig, ServerSideSessionBackend
from litestar.datastructures import Cookie, MutableScopeHeaders
from litestar.types import Empty, Message, ScopeSession
from litestar.utils.dataclass import extract_dataclass_items


class CustomSessionMiddleware(SessionMiddleware):
    """Custom override to generate a session id before the route handler."""
    async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None:
        """ASGI-callable.

        Args:
            scope: The ASGI connection scope.
            receive: The ASGI receive function.
            send: The ASGI send function.

        Returns:
            None
        """
        connection = ASGIConnection[Any, Any, Any, Any](scope, receive=receive, send=send)
        scope["session"] = await self.backend.load_from_connection(connection)

        # get existing session ID or create new one here
        scope["_session_id"] = self.backend.get_session_id(connection)

        await self.app(scope, receive, self.create_send_wrapper(connection))


class CustomServerSideSessionBackend(ServerSideSessionBackend):
    """Custom override to use the session id generated by the Middleware before the route handler instead of
    generating it after the route handler.
    """
    def get_session_id(self, connection: ASGIConnection):
        session_id = connection.cookies.get(self.config.key)
        if not session_id or session_id == "null":
            session_id = self.generate_session_id()
        return session_id

    async def store_in_message(self, scope_session: ScopeSession, message: Message, connection: ASGIConnection) -> None:
        scope = connection.scope
        store = self.config.get_store_from_app(scope["app"])
        headers = MutableScopeHeaders.from_message(message)
        session_id = connection.cookies.get(self.config.key)
        if not session_id or session_id == "null":
            session_id = scope["_session_id"]  # replaced id generation with loading from scope

        cookie_params = dict(extract_dataclass_items(self.config, exclude_none=True, include=Cookie.__dict__.keys()))

        if scope_session is Empty:
            await self.delete(session_id, store=store)
            headers.add(
                "Set-Cookie",
                Cookie(value="null", key=self.config.key, expires=0, **cookie_params).to_header(header=""),
            )
        else:
            serialised_data = self.serialize_data(scope_session, scope)
            await self.set(session_id=session_id, data=serialised_data, store=store)
            headers.add(
                "Set-Cookie", Cookie(value=session_id, key=self.config.key, **cookie_params).to_header(header="")
            )


class CustomServerSideSessionConfig(ServerSideSessionConfig):
    """Load custom overrides for session id generation before route handler."""
    _backend_class = CustomServerSideSessionBackend

    @property
    def middleware(self) -> DefineMiddleware:
        return DefineMiddleware(CustomSessionMiddleware, backend=self._backend_class(config=self))

@guacs
Copy link
Member

guacs commented Feb 22, 2024

@aranvir would you like to make a PR based on this?

Also, it may be better if we just return the session ID whenever the user calls request.set_session. Currently that returns None, but we could change that to just return the value of _session_id from the scope. Also, while I marked it as private, I think it may be better to just make the session ID part of the public API i.e. add it to the TypedDict that defines Scope.

Copy link

A fix for this issue has been released in v2.7.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement This is a new feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants