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 · 1 comment
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
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

4 participants