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

Attaching to a FastAPI instance with another storage_secret breaks session storage #2578

Open
denniswittich opened this issue Feb 20, 2024 · 2 comments
Assignees
Labels
enhancement New feature or request

Comments

@denniswittich
Copy link

denniswittich commented Feb 20, 2024

Description

I noticed that when attaching to a FastAPI instance using ui.run_with one has to use the same secret like the FastAPI SessionMiddleware - otherwise the session data is not persistence across page reloads. This seems to be problematic in two ways:

  1. Setting a storage_secret seems to be redundant as it is already set in the middleware.
  2. Setting another secret does not result in an error but instead results in non-persistent sessions.

Debugging, took me a while, so I would recommend to improve the feedback if the user tries to set another password and/or to improve the web examples for attaching to an existing FastAPI instance (e.g. show the example how to attach to an app with a installed middleware).

When writing the MRCE I also noticed, that initializing NiceGUI will break all endpoints defined later in the code via FastAPI. I don't know if this is problematic, but it seems weird. You may have a look at this.

MRCE:

File 1: main.py

import uvicorn
from fastapi import FastAPI, Request
from ng_page import storage
from nicegui import ui
from starlette.middleware.sessions import SessionMiddleware

# /test1 : http://localhost:8080/test1
# /test2 : http://localhost:8080/test2
# /testR : http://localhost:8080/testR
# /storage : http://localhost:8080/storage


app = FastAPI()
refresh_count = 0


@app.get("/testR")
def tR(request: Request):
    global refresh_count
    request.session['refresh_count'] = refresh_count
    refresh_count += 1
    return f"refresh to increase counter: {refresh_count}"


@app.get("/test1")
def t1():
    return "test1"


case = 3

if case == 0:  # ----- No key set -----
    pass
    # /test1 is accessible
    # /test2 is accessible
    # /testR gives an error (because the SessionMiddleware is not installed)
    # /storage is not accessible (because nicegui is not running)

if case == 1:  # ----- Setting a key only for the fastapi app -----
    app.add_middleware(SessionMiddleware, secret_key='themagickey')

    # /test1 is accessible
    # /test2 is accessible
    # /testR is accessible and the session is working
    # /storage is not accessible (because nicegui is not running)

if case == 2:  # ----- Setting a key only for the storage -----
    ui.run_with(app, storage_secret='aregularkey')

    # /test1 is accessible
    # /test2 is not accessible (initializing nicegui breraks any later defined access points in the fastapi app)
    # /testR gives an error (because the SessionMiddleware is not installed)
    # /storage is accessible and the storage is working

if case == 3:  # ----- Setting key for the fastapi app but not for NiceGUI
    app.add_middleware(SessionMiddleware, secret_key='themagickey')
    ui.run_with(app)

    # /test1 is accessible
    # /test2 is accessible
    # /testR is accessible and the session is working
    # /storage gives hint to set the storage_secret

if case == 4:  # ----- Setting different keys for the app and the storage -----
    app.add_middleware(SessionMiddleware, secret_key='themagickey')
    ui.run_with(app, storage_secret='aregularkey')

    # /test1 is accessible
    # /test2 is not accessible (see case 2)
    # /testR is accessible and the session is working
    # /storage is accessible but the storage is not working

if case == 5:  # ----- Setting the same key for both the app and the storage -----
    app.add_middleware(SessionMiddleware, secret_key='themagickey')
    ui.run_with(app, storage_secret='themagickey')

    # /test1 is accessible
    # /test2 is not accessible (see case 2)
    # /testR is accessible and the session is working
    # /storage is accessible and the storage is working


@app.get("/test2")
def t2():
    return "test2"


if __name__ == "__main__":
    uvicorn.run("main:app", host="0.0.0.0", port=8080, lifespan='on', use_colors=True, reload=True,)

File 2: ng_page.py

from nicegui import app, ui


@ui.page("/storage")
def storage():

    status_label = ui.label('')
    detail_label = ui.label('')

    def update_labels():
        status_label.text = f'User is authenticated: {app.storage.user.get("authenticated", False)}'
        detail_label.text = f'"authenticated" in storage: {"authenticated" in app.storage.user.keys()}'

    def on_authenticate():
        app.storage.user['authenticated'] = True
        update_labels()

    def on_deauthenticate():
        app.storage.user['authenticated'] = False
        update_labels()

    def clear_storage():
        app.storage.clear()
        update_labels()

    ui.button('Authenticate', on_click=on_authenticate)
    ui.button('Deauthenticate', on_click=on_deauthenticate)
    ui.button('Clear storage', on_click=clear_storage)
    ui.button('Reload', on_click=lambda: ui.run_javascript('window.location.reload()'))

    update_labels()

Expected/ideal behavior:

  • case 2: I would expect '/test2' to work.
  • case 3: I would expect '/storage' to work, because I set a secret_key in the middleware
  • case 4: I would expect an error because I used two different keys
@kwmartin
Copy link

kwmartin commented Feb 20, 2024

I've been having similar issues. I haven't figured it out yet, but so far: I need to add TLS to server (i.e. https using Let's Encrypt). I can't find any documentation under NiceGui on hosting a https site which is required these days so I have been trying to run nicegui under FastAPI using ui.run_with. It breaks when I try to add AuthMiddleware to core.api. As an aside: there are two api's one for fastapi and one for core in ui. In addition, middleware can be added to either fastapi or ui (or both?). I can't find documentation dealing with this and it's somewhat confusing and/or convoluted to follow what is going on. Irrespective, when adding AuthMiddleware, dispatch of BaseHTTPMiddleware needs to be replaced. This calls:

        if not app.storage.user.get('authenticated', False):

right at the beginning which calls (from the storage module):

    @property
    def browser(self) -> Union[ReadOnlyDict, Dict]:
        """Small storage that is saved directly within the user's browser (encrypted cookie).

        The data is shared between all browser tabs and can only be modified before the initial request has been submitted.
        Therefore it is normally better to use `app.storage.user` instead,
        which can be modified anytime, reduces overall payload, improves security and has larger storage capacity.
        """
        request: Optional[Request] = request_contextvar.get()
        if request is None:
            if self._is_in_auto_index_context():
                raise RuntimeError('app.storage.browser can only be used with page builder functions '
                                   '(https://nicegui.io/documentation/page)')
            raise RuntimeError('app.storage.browser needs a storage_secret passed in ui.run()')
        if request.state.responded:
            return ReadOnlyDict(
                request.session,
                'the response to the browser has already been built, so modifications cannot be sent back anymore'
            )
        return request.session

This breaks in the very first line as request is None (it shouldn't be None). request_contextvar is declared at the beginning of storage.py:

request_contextvar: contextvars.ContextVar[Optional[Request]] = contextvars.ContextVar('request_var', default=None)

and it shouldn't be None. In the nicegui authentication example, which uses run(), not run_with(), it is not None at the same location. So far, I can't figure out why it is None when run_with() is used. I tried checking if it might be a different thread, but so far it doesn't seem to be. Still working on it, but getting close to giving up on nicegui because of the difficult getting TLS working (it's alway a pain), but it seems starlette-admin might have it working? I'd prefer to stay with nicegui, but I am running out of time trying to get TLS without documentation or even an example? Any suggestions are appreciated as I'm running out of hair!

@falkoschindler falkoschindler added the enhancement New feature or request label Feb 21, 2024
@MaiHoangViet1809
Copy link

MaiHoangViet1809 commented May 28, 2024

I have met the similar situation, and this might be a bug in concept design not support Processing pool (worker > 1) when the cache request_contextvar set by RequestTrackingMiddleware only share within same thread, while each process have it own thread, then each workers of Uvicorn in startup process messing up together I guess.

Update:
after 3 hours debug into core code, I observe abnormally with middleware's execution order:

  1. although set_storage_secret add (RequestTrackingMiddleware and SessionMiddleware) to core.app in module storage, I don't know it add with correct order, but when I do app.add_middleware(AuthMiddleware) in the main.py, it alway called before any other 2 middleware needed for storage (RequestTrackingMiddleware and SessionMiddleware), so the request_contextvar will be None and Storage.user will "raise RuntimeError('app.storage.user needs a storage_secret passed in ui.run()')"
  2. to solve above problem, I saw the only way is to apply logic of set_storage_secret to main app fast api, not the NiceGUI core.app:
def run_with(...) -> None:
    ...
    storage.set_storage_secret(storage_secret=storage_secret, target_app=app)
    ...

and change set_storage_secret to something like this:

def set_storage_secret(storage_secret: Optional[str] = None, target_app = None) -> None:
    if not target_app:
        from nicegui import app
        target_app = app
   
    # this logic prevent repeat add middleware to user_middleware
    if any(m.cls in [SessionMiddleware, RequestTrackingMiddleware] for m in target_app.user_middleware):
        return

    """Set storage_secret and add request tracking middleware."""
    if any(m.cls == SessionMiddleware for m in target_app.user_middleware):
        # NOTE not using "add_middleware" because it would be the wrong order
        target_app.user_middleware.append(RequestTrackingMiddleware)
    elif storage_secret is not None:
        target_app.add_middleware(RequestTrackingMiddleware)
        target_app.add_middleware(SessionMiddleware, secret_key=storage_secret)
    

and in main app should apply ui.run_with before app.add_middleware too:

ui.run_with(app_fastapi, storage_secret="some secret")
app.add_middleware(AuthMiddleware)

and voila those middleware will execute in correct order in the outside main fast api app.

@falkoschindler please take a look, thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants