sbp commented on issue #731: URL: https://github.com/apache/tooling-trusted-releases/issues/731#issuecomment-4121228729
In [`base.py`](https://github.com/apache/infrastructure-asfquart/blob/788bbbcca5552644ed669cd0bbd4bee295da8f9a/src/asfquart/base.py#L104-L105) there is already a precedent for callback properties on the app: ```python # token handler callback for PATs - see docs/sessions.md self.token_handler = None # Default to no PAT handler available. ``` This one is handled in [`session.py`](https://github.com/apache/infrastructure-asfquart/blob/788bbbcca5552644ed669cd0bbd4bee295da8f9a/src/asfquart/session.py#L70C1-L83C59) where it checks for it being async or sync using `asyncio.iscoroutinefunction` and `callable`. (We don't use this in ATR, as is [already documented](https://github.com/apache/tooling-trusted-releases/blob/41294d6c5b803047641f2a83a0418aa72614fa6f/atr/docs/asfquart-usage.md?plain=1#L141)). We could add similar handler callbacks for login, request, and logout events. Quart itself (not ASFQuart) has a [`SessionInterface`](https://github.com/pallets/quart/blob/5817e983d0b586889337a596d674c0c246d68878/src/quart/sessions.py#L25) class which is the basis for all sessions. We could extend it with a `ServerSideSessionInterface` class, which, _prima facie_, looks like the right approach. Sessions in Quart are, however, also used for things like flash messages and CSRF token storage, and a new `ServerSideSessionInterface` would have to take over those responsibilities too. Since CSRF state is stored there, any public sessions would require sessions if they carry any POST forms. Perhaps it's possible to use different sessions just for public routes? But either way, it's extra complexity. Another problem is that [as a comment in the flask source code makes clear](https://github.com/pallets/flask/blob/7ef2946fb5151b745df30201b8c27790cac53875/src/flask/sessions.py#L127-L132) in its version of `SessionInterface`, requests are not serialised; as far as I can tell, the same is true in quart too, though it does not have the same comment. This means that there can be race conditions between requests at the session level, which have to be prevented by careful synchronisation in sessions (separate to database serialisation, which does nothing to help with session serialisation). This is, again, extra complexity. One much more lightweight alternative would be to store a server side session key in the `metadata` extension field of the existing ASFQuart client cookie. In this design, ASFQuart does not even know that server side validation is taking place, but it does still need to have the aforementioned handler callbacks. We would keep things as they are, as much as possible, but add a way to check that the session is known by the server for each request, which would be the repsonsibility of the request callback handler. To keep the naming consistent with `token_handler`, we could call the handlers `session_login_handler`, `session_request_handler`, and `session_logout_handler`. These would all be defined in `base.py` next to `token_handler`. The calls to these handlers would be located as follows: 1. `session_login_handler` is called from `async oauth_endpoint` in `setup_oauth` in `generics.py`, before `session.write`, because that's where OAuth is handled. It returns the potentially modified session for ASFQuart to write. 2. `session_request_handler` is called from the cookie path in `async read` in `session.py`, because that's where the cookie is fetched. We should do the expiry checks first because then we might not need to read from the database. 3. `session_logout_handler` is called from `async oauth_endpoint` in `setup_oauth` in `generics.py`, before `session.clear`, because that's where OAuth is handled. (We could also put it in `clear` itself in `session.py`, but that function isn't async.) This handler receives the session as it was before logout, so that it can potentially act upon it; in ATR we would delete the server side session representation. Because we can't put the `session_logout_handler` in `session.clear`, which would be the more natural location, we'd have to add session revocation calls to all of the places where ATR itself calls `session.clear`. Any exception in the login and request handlers must prevent ASFQuart from proceeding entirely. Maybe the logout handler should still remove the cookie? Otherwise the user would not be able to log out if there is a problem. Request handler validation failure should also clear the cookie in addition to failing the request, because the user is counted as logged out when the cookie fails to validate, so they should not have a cookie set. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
