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

Include ASGI scope root_path in url used to validate mAuth signature #46

Merged
merged 2 commits into from
Oct 31, 2023

Conversation

pburke-mdsol
Copy link
Contributor

This fixes an issue encountered with a FastAPI setup similar to the the example provided below. When using a sub application mounted into a parent application the value of scope["path"] that the middleware attached to the sub application sees is the path without the prefix the application was mounted at. The route prefix is stored in scope["root_path"]. Both need to be combined when creating the URL used for mAuth signature verification.

For example using the application below, for a request to "/subapp/with_auth": scope["path"] will be "/with_auth" and scope["root_path"] will be "/subapp".

from fastapi import FastAPI
from mauth_client.middlewares import MAuthASGIMiddleware

app = FastAPI()


@app.get("/no_auth")
async def no_auth():
    return {"message": "no auth"}


subapp = FastAPI()
subapp.add_middleware(MAuthASGIMiddleware)


@subapp.get("/with_auth")
async def with_auth():
    return {"message": "with auth"}


app.mount("/subapp", subapp)

@pburke-mdsol pburke-mdsol marked this pull request as ready for review October 30, 2023 18:14
@ejinotti-mdsol
Copy link
Contributor

Looks good, thx for contributing. Just needs: version bump, changelog, and a test case 👍.

@pburke-mdsol
Copy link
Contributor Author

Looks good, thx for contributing. Just needs: version bump, changelog, and a test case 👍.

Should I bump the version to 1.6.2?

@ejinotti-mdsol
Copy link
Contributor

yea, 1.6.2 seems correct. patch version for bug fixes: https://semver.org/

@pburke-mdsol
Copy link
Contributor Author

OK, should hopefully be ready for re-review.

@ejinotti-mdsol ejinotti-mdsol merged commit 7381959 into main Oct 31, 2023
14 checks passed
@ejinotti-mdsol ejinotti-mdsol deleted the include-asgi-root-path branch October 31, 2023 16:32
@ejinotti-mdsol
Copy link
Contributor

created new tag/release @pburke-mdsol

@pburke-mdsol
Copy link
Contributor Author

created new tag/release @pburke-mdsol

Thanks!

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants