Make session cookie use ASGI root path

[mahmoudhossam]

Fixes #233

[JayH5]

Thanks, this seems like a reasonable improvement. It does need a test, I think, though.

[lovelydinosaur]

Once the final remaining suggestion is addressed I'm probably okay with pulling this in.

It could potentially use a test case, but it's probably also okay enough without.

[mahmoudhossam]

I'm going to add a test and then we can merge this, thanks for the quick response!

[mahmoudhossam]

I couldn't find a way to set the root path directly on the app so I had to mount another app on the main one and tested that, seems to work as expected.

[lovelydinosaur]

Ah fantastic, yup.

[devsetgo]

@mahmoudhossam or @tomchristie This update broke my session checking function to ensure users are logged in. I mount all the sub routes (users, etc..). The request.session has data in it when set, but when I redirect back to the start page then session is now null. I see no documentation on how to have the session across Mounts?

Below is example of what I am doing...

# Routes
routes = [
    Route("/", main_pages.homepage, name="dashboard", methods=["GET", "POST"]),
    Route("/about", main_pages.about_page, methods=["GET"]),
    Mount("/user", routes=
    [
        Route(
            "/forgot", routes=user_pages.forgot_password, methods=["GET", "POST"]
        ),
        Route("/login", routes=user_pages.login, methods=["GET", "POST"]),
        Route("/logout", routes=user_pages.logout, methods=["GET", "POST"]),
        Route(
            "/password-change",
            routes=user_pages.password_change,
            methods=["GET", "POST"],
        ),
        Route("/profile", routes=user_pages.profile, methods=["GET"]),
        Route("/register", routes=user_pages.register, methods=["GET", "POST"]),
    ],
    name="user",
    ),

# Session Checking
def require_login(endpoint: Callable) -> Callable:
    async def check_login(request: Request) -> Response:
        if "user_name" not in request.session:
            logger.error(
                f"user page access without being logged in from {request.client.host}"
            )
            return RedirectResponse(url="/user/login", status_code=303)

        else:
            one_twenty = datetime.now() - timedelta(
                minutes=config_settings.login_timeout
            )
            current: bool = one_twenty < datetime.strptime(
                request.session["updated"], "%Y-%m-%d %H:%M:%S.%f"
            )

            if current == False:
                logger.error(
                    f"user {request.session['user_name']} outside window: {current}"
                )
                return RedirectResponse(url="/user/login", status_code=303)

            # update datetime of last use
            logger.info(
                f"user {request.session['id']} within timeout window: {current}"
            )
            request.session["updated"] = str(datetime.now())
        return await endpoint(request)

    return check_login

[mahmoudhossam]

I think there should be a difference between mounting routes and mounting whole applications.

I'd expect routes to share everything with the parent application, but submounted applications should have their own session.

@tomchristie thoughts?

[devsetgo]

I think there should be a difference between mounting routes and mounting whole applications.

I'd expect routes to share everything with the parent application, but submounted applications should have their own session.

@tomchristie thoughts?

I agree, the documentation shows using Mount with a sub app (Mount("/static", app=StaticFiles(directory="static"), name="static"), but without defining "app=x" it is just Submounting routes. For the session, it should check if submounted as an app or route.

[devsetgo]

@tomchristie ?

[mahmoudhossam]

@devsetgo I'd say open a new issue describing your use case and make sure to mention that this PR broke your flow so it'd be easy to track for the maintainers.

This PR is already closed and I don't think anyone will be checking it anymore.

[lovelydinosaur]

Okay folks - got a resolution to this here... #1512