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: Server-side sessions #617

Closed
provinzkraut opened this issue Oct 18, 2022 · 28 comments · Fixed by #630
Closed

Enhancement: Server-side sessions #617

provinzkraut opened this issue Oct 18, 2022 · 28 comments · Fixed by #630
Assignees
Labels
Enhancement This is a new feature or request

Comments

@provinzkraut
Copy link
Member

provinzkraut commented Oct 18, 2022

What's the feature you'd like to ask for.
Currently only cookie-based (i.e. client-side) sessions are supported. A server-side session with a redis/memcached/file backend would be nice to have.

If that's something starlite wants to support, I'd be happy to work on it.

@provinzkraut provinzkraut added Enhancement This is a new feature or request Triage Required 🏥 This requires triage labels Oct 18, 2022
@Goldziher Goldziher removed the Triage Required 🏥 This requires triage label Oct 18, 2022
@Goldziher
Copy link
Contributor

Certainly. There was a suggestion by @infohash to put this as part of the starlite-sessions package, which I think is a good idea. I am though open to also doing it in the main package if there is a good reason to do this.

@provinzkraut
Copy link
Member Author

starlite-sessions describes itself as "Simple sessions authentication for Starlite". I wouldn't expect to find server-side session middleware there. Or rather, it wouldn't be the place I'd look for. Is there a good reason why you'd not want this in the core package?

@Goldziher
Copy link
Contributor

You got a point there. How would you suggest to implement this?

@infohash
Copy link
Contributor

It's nice to have server side sessions. We can keep it default to memory if no cache backend is provided just like caching. It must have similar session management as in client side sessions. We can consolidate these 2 to offer a single session interface for the user to access. The only difference being data for server side session should not go into cookies.

E.g. If the user is accessing request.session, it should contain data of both client side and server side sessions but only the data of client side should go into cookies. By using a flag, the user will decide to which session, new data will go to.

It's not a hard requirement though. It's just that, the user doesn't care where the data is coming from but it cares where the data will go to and this is where we need the destination flag.

@provinzkraut
Copy link
Member Author

@infohash I agree with having a consistent interface across session backends, I don't particularly agree on the configuration via flag thing.

You'd want to expose the backends directly as well, since the will have methods that should be accessible that the cookie based session does not have, for example to manually expire sessions.

I suggest something like this:

from abc import ABC, abstractmethod

class SessionBackend(ABC):
    @abstractmethod
    async def load(self, key: str) -> dict[str, Any] | None:
        pass

    @abstractmethod
    async def store(self, key: str, data: dict[str, Any]) -> None:
        pass

    @abstractmethod
    async def expire(self, key: str) -> None:
        pass

    @abstractmethod
    async def expire_all(self) -> None:
        pass


class ServerSideSessionMiddleware(MiddlewareProtocol):
    def __init__(self, app: ASGIApp, backend: SessionBackend, cookie_config: SessionCookieConfig) -> None:
        ...

    def define_middleware(self, backend: SessionBackend, cookie_config: SessionCookieConfig) -> DefineMiddleware:
        return DefineMiddleware(ServerSideSessionMiddleware, backend=backend, cookie_config=cookie_config)
from starlite import Starlite
from starlite.middleware.session import ServerSideSessionMiddleware, RedisBackend, SessionCookieConfig

session_backend = RedisBackend(redis=..., expires=3600)
session_cookie_config = SessionCookieConfig(secure=True, ...)
session_middleware = ServerSideSessionMiddleware.make_middleware(
    backend=session_backend, 
    cookie_config=session_cookie_config
)

app = Starlite(route_handlers=[...], middleware=[session_middleware])

In theory it would be possible to implement a backend agnostic SessionMiddleware, and make the cookie based session just another backend (and IMHO that would be the cleanest approach), but that's not possible without breaking backwards compatibility, since SessionMiddleware already is a specific implementation of the cookie-based approach.

Looking at the implementation, cookie-based and the yet to implement session middlewares have more differences than they have in common when it comes down to how the work internally. I'd say it's best for now to simply have them expose the same interface to the application, that is: store the data in as a dict scope["session"].

If we want to re-use the reusable logic in SessionMiddleware, we could do something like

from abc import ABC, abstractmethod
class BaseSessionMiddleware(MiddlewareProtocol, ABC):

    @abstractmethod
    def dump_data(self, data: Any, scope: Optional["Scope"] = None) -> List[bytes]:
        pass

    @abstractmethod
    def load_data(self, data: List[bytes]) -> Dict[str, Any]:
        pass

    def create_send_wrapper(
        self, scope: "Scope", send: "Send", cookie_keys: List[str]
    ) -> Callable[["Message"], Awaitable[None]]:
        # put the logic currently in Sessionmiddleware.create_send_wrapper here
        ...

    async def __call__(self, scope: "Scope", receive: "Receive", send: "Send") -> None:
      # put the logic currently in Sessionmiddleware.__call__ here
      ...


class SessionMiddleware(BaseSessionMiddleware):
    def dump_data(self, data: Any, scope: Optional["Scope"] = None) -> List[bytes]:
        # put the logic currently in Sessionmiddleware.load_data here

    def load_data(self, data: List[bytes]) -> Dict[str, Any]:
        # put the logic currently in Sessionmiddleware.load_data here


class ServerSideSessionMiddleware(BaseSessionMiddleware):
    def dump_data(self, data: Any, scope: Optional["Scope"] = None) -> List[bytes]:
        self.backend.dump(data)
        return <the session key> # so it will be put in the cookie again

    def load_data(self, data: List[bytes]) -> Dict[str, Any]:
        return self.backend.load(data)

But this would require some more abstractions and unnecessary workarounds to be compatible with the current interface of SessionMiddleware. IMHO that's not worth it to save maybe 30 LOC with low complexity.
If we have the chance to re-work the session interface where we can make breaking changes, the way to go would be to have a backend agnostic SessionMiddleware, with cookie-based just being another backend. This would be in line with how most frameworks I know of handle it as well.

@infohash
Copy link
Contributor

infohash commented Oct 19, 2022

I like the approach of treating cookie as just another backend. But how will the user dynamically choose which data goes to which backend e.g. if the user is using ServerSideSessionMiddleware and ClientSideSession.

Currently any new data that you set in session is destined to go to cookies.

userinfo = {...}
# Set data for client side session
request.session['user'] = userinfo

If the user wants to keep some data at server side:

username = 'some_user'
userkeys = [...]
request.session[username] = userkeys

What is the appropriate way to keep userkeys at server side session?

@provinzkraut
Copy link
Member Author

But how will the user dynamically choose which data goes to which backend

I have never seen a session uses liked this. Usually you choose one storage method for your session and that will be then used to store all the session data. Server-side sessions aren't a complementary session mechanism to client-side session. They're a replacement.

What would the use case be for something like this?

@infohash
Copy link
Contributor

infohash commented Oct 19, 2022

If the user wants to set persistent session data that will be used to authenticate his users, the user should have an option to set persistent session data at all times.

@provinzkraut
Copy link
Member Author

provinzkraut commented Oct 19, 2022

I feel like we're not talking about the same thing here.

Yes, the session should always be globally available if the middleware is used, and it should always be persistent. But why do we need client-side and server-side sessions at the same time for that?

A server-side session offer the exact same functionality to the user when it comes to accessing the session data.
From the consumer perspective, the request.session object should simply be "a dict I can put data in, that's associated with a specific client and persistent across requests". The storage mechanism shouldn't matter at all, nor needed to be exposed.

@infohash
Copy link
Contributor

I mean if we use server side session, we also need a way to set client side authentication cookies that are currently dumped from the session to the browser with every response and are loaded into the session on every request.

@provinzkraut
Copy link
Member Author

provinzkraut commented Oct 19, 2022

The point of a server-side session is that you do not have to store data on the client side.

It stores a random session ID in a browser cookie that can be used by the backend to retrieve the session information. On a request, it loads that information (e.g. from redis) into scope["session"], and before a response is sent, it will dump the data from scope["session"] to the storage backend again.

No data is stored on the client aside from the session ID.

From a consumer perspective, this works exactly the same as a cookie-based session. To quote myself here

From the consumer perspective, the request.session object should simply be "a dict I can put data in, that's associated with a specific client and persistent across requests"

@infohash
Copy link
Contributor

infohash commented Oct 19, 2022

I am seeing one problem if the user can't use client side session as complementary and it is with OIDC. It is recommended to store OIDC tokens in client side cookies. Keeping them in server-side session makes it hard to scale your application horizontally for SSO across web domains.

If you retrieve the session from the database every request, you will need to cluster and/or share your database with multiple web apps, each of which presents its own problems. If you keep them in memory, you tie them to the server instance, and interfere with load-balancing.

In case of server compromise, OIDC tokens of every single user including the inactive users can be compromised as well and because OIDC tokens can be used across web apps within a SSO organization, the compromised tokens now can be used across web apps and inactive users will probably never come to know.

Keeping OIDC tokens at caching backend also defeats one benefit of using OIDC/Oauth2 tokens which is that these tokens can be verified in memory without having to make a round trip to the backend service. Saving them in backend nullifies this benefit.

We should tie user authentication with the browser not with the server.

@provinzkraut
Copy link
Member Author

It is recommended to store OIDC tokens in client side cookies

Then the user should store them in a cookie, not the session.

If you retrieve the session from the database every request, you will need to cluster and/or share your database with multiple web

We cannot make every session implementation fit every use case. A user will have to decide which session they want to use. They all have their advantages and disadvantages. If a server-side session does not fit the use case, they need to pick something else.

What you're suggesting though doesn't sound like a session, it sounds more like a cache. If a user wants a cache, they can use a cache. The point of a session is that it's automatically injected into every incoming request based on the presence of a specific cookie / header.

Server-side sessions are widely used in the manner I have described I don't think we should reinvent the wheel here.

@infohash
Copy link
Contributor

infohash commented Oct 19, 2022

Then the user should store them in a cookie, not the session.

This is what the current design is. Session Middleware sets them as cookies with every response and loads them into session in every request. Then APIs can retrieve ID token from the session to learn who the user is and extensions can verify the ID Token, refresh the access token if it is needed, etc. These tokens are frequently rotated and this is why they are modified in the session and later dumped in cookies with the response. They cannot be cached.

@provinzkraut
Copy link
Member Author

@infohash I feel like we're stuck here and this is not really productive.

As I said before, we cannot create a session middleware that fits all use cases. If you have data that should only live on the client side and need to put it in a session, then use a client-side session. If you want data to be server-side, use a server-side session.

I am repeating myself here, but this is how most (I'm simply not saying all since I haven't seen all) frameworks implement sessions.

@jtraub
Copy link
Contributor

jtraub commented Oct 19, 2022

Then APIs can retrieve ID token from the session to learn who the user is and extensions can verify the ID Token, refresh the access token if it is needed, etc

What happens if this data will be stored in redis/memcached/memory/tmp files? Do you need to access session data on the client side as well?

In theory it would be possible to implement a backend agnostic SessionMiddleware, and make the cookie based session just another backend (and IMHO that would be the cleanest approach)

@provinzkraut it is a bit of a stretch but it would be great to add "frontends" as well. So a default frontend would read session token from cookies while another custom frontend would read it from a header or an url param. They should also take care of outputing session token in desirable format (e.g. setting cookie or attaching a custom header to response)

This is more like an edge case but it would be great to have flexibility if it is not too hard to implement swappable frontends.

@alex-oleshkevich
Copy link

I maintain the library that provides a feature you requested https://github.com/alex-oleshkevich/starsessions

It should be compatible with Starlite as it is just an ASGI middleware.

@infohash
Copy link
Contributor

What happens if this data will be stored in redis/memcached/memory/tmp files? Do you need to access session data on the client side as well?

@jtraub I have explained it here why tying SSO session with server side is not recommended. Short answer is it makes it harder for an SSO organization to scale its web apps horizontally.

@jtraub
Copy link
Contributor

jtraub commented Oct 20, 2022

@infohash sorry, I've missed that message.

Now storing OIDC stuff in cookies makes sense but I still don't get why it should be session cookies.

If we store OIDC tokens in a session cookie then other apps will have access to my app's session and since they rotate tokens they can also accidentally screw my app's session data. Wouldn't it make more sense to store OIDC tokens in a separate cookie not related to sessions?

@infohash
Copy link
Contributor

infohash commented Oct 20, 2022

@jtraub Within the same SSO organization, it is fine to let other web apps to modify OIDC session data. It will keep the OIDC session active by frequently refreshing the access token, ID token and refresh token. If there is at least one web app within the SSO keeping the OIDC session active, the user will remain logged in for other web apps even if he is not using them. This also means that if the user logs out from one web app, he will be logged out of SSO organization altogether. This is how Google web apps also operate which is a benefit of using OIDC.

Another benefit is, if the ID token is present in session, as long as it is not expired, one web app at any point can share it with other services to let the service know about the user's identity when processing some distributed task.

@jtraub
Copy link
Contributor

jtraub commented Oct 20, 2022

it is fine to let other web apps to modify OIDC session data

I have no doubts that OIDC middleware won't mess with my app sessions but I am not so sure about user code.

They've might be simply unaware of the fact that entire session (not only OIDC tokens) is shared across all apps. I was too very surprised to learn that with OIDC Middlware I can not longer do request.session({}) for example to clear app session.

@provinzkraut
Copy link
Member Author

@jtraub

@provinzkraut it is a bit of a stretch but it would be great to add "frontends" as well. So a default frontend would read session token from cookies while another custom frontend would read it from a header or an url param

That's actually a really good idea! Although I wouldn't necessarily add "frontends" for that. Since the options where to get the user-identity from are fairly limited, they can all easily be supported in the base server-side session implementation. This could then be configured via a simple field in the configurations with a string lateral.

@alex-oleshkevich

I maintain the library that provides a feature you requested https://github.com/alex-oleshkevich/starsessions

It should be compatible with Starlite as it is just an ASGI middleware.

Couldn't you have mentioned a few hours earlier yesterday? Now I already went ahead and did things 🙃

On a serious note though, what you have there looks great! I'm not sure if starlite simply wants to refer users to that, or implement its own thing. I guess there are pros and cons for using the already existing solution:

Pros

  • The code is already written
  • Less code in starlite > less code to maintain

Cons

  • No integration with starlite specific things (like using custom serializers from the handlers / scope, or the "middleware from config" pattern)
  • Dependency on starlette since your package wants to maintain compatible with starlette, and starlite might not (see Drop Starlette #612)
  • NIH

I guess a deciding factor also would be if starlite wants to have session backends like this built-in, which AFAICT majorly depends on the future scope. If it wants to strive to me more "battery included", built-in support for server-side session would be somewhat of a must have IMHO.

@Goldziher What do you think?

@Goldziher
Copy link
Contributor

I guess a deciding factor also would be if starlite wants to have session backends like this built-in, which AFAICT majorly depends on the future scope. If it wants to strive to me more "battery included", built-in support for server-side session would be somewhat of a must have IMHO.

@Goldziher What do you think?

I say go ahead and implement it 😉

@Goldziher
Copy link
Contributor

@provinzkraut - please send me a message via discord. I want a chat when you have time.

@infohash
Copy link
Contributor

@jtraub Have you tried request.session.clear()?

The OIDC tokens are actually kept under the user defined key when initializing OIDC. It is recommended for the user to give the name of the OIDC provider prefixed by an underscore to the key to avoid key collision and accidental modification. If the user code actually modifies anything under that key, that will be on the user. The web apps must be OIDC aware if they are part of an SSO.

@jtraub
Copy link
Contributor

jtraub commented Oct 20, 2022

If the user code actually modifies anything under that key, that will be on the user

This is the problem. Once I install OIDC middleware I effectively no longer can use builtin sessions.

@infohash
Copy link
Contributor

@jtraub Can you raise an issue https://github.com/starlite-api/starlite-oidc/issues ? I haven't encountered this problem so far.

@provinzkraut
Copy link
Member Author

provinzkraut commented Oct 22, 2022

Progress

  • Backend implementations (basic things done, details need some work)
    • Memory
    • File
    • Redis
    • Memcached
    • SQLAlchemy
  • Individual backend test
  • Existing integration test pass for new backends
  • Maintains backwards compatibility
  • Update documentation

@provinzkraut provinzkraut linked a pull request Oct 26, 2022 that will close this issue
4 tasks
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.

5 participants