add `excluded_paths` to `JWTMiddleware` #226

sinisaos · 2023-04-13T14:25:30Z

codecov-commenter · 2023-04-15T06:56:30Z

Codecov Report

Merging #226 (0d6b825) into master (92ea0ac) will increase coverage by 0.03%.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master     #226      +/-   ##
==========================================
+ Coverage   92.55%   92.58%   +0.03%     
==========================================
  Files          33       33              
  Lines        2027     2036       +9     
==========================================
+ Hits         1876     1885       +9     
  Misses        151      151

Impacted Files	Coverage Δ
piccolo_api/jwt_auth/middleware.py	`90.80% <100.00%> (+1.06%)`	⬆️

dantownsend · 2023-04-15T07:05:45Z

piccolo_api/jwt_auth/middleware.py


        headers = dict(scope["headers"])
        token = self.get_token(headers)
        if not token:
            error = JWTError.token_not_found.value
-            if allow_unauthenticated:
+            if allow_unauthenticated or visible_paths:


You need to be careful here because this is checking to the presence of visible_paths, and not whether it matches the current path.

Do you think scope["path"] in visible_paths is a better option?

I think checking presence of visible_paths (if is empty list is or not specified we got Token not found error) enough because we can make a GET request only for Swagger docs from browser on routes /docs and /openapi.json where we have to authorize the user if we want to communicate with the endpoints. All other routes must have a token in the header.

Do you think scope["path"] in visible_paths is a better option?

Yes, something like that.

OK, I'll change that even though I think it might not be necessary (see previous comment) because if for example we pass a /bands route to visible_paths, we can't make a request to that route directly from the browser (we'll get Token not found). Basically we can only pass the Swagger docs routes /docs and /openapi.json. Sorry if I'm wrong.

async def __call__(self, scope, receive, send): """ Add the user_id to the scope if a JWT token is available, and the user is recognised, otherwise raise a 403 HTTP error. """ allow_unauthenticated = self.allow_unauthenticated if scope['path'] in self.visible_paths: await self.asgi(scope, receive, send) return # then all of the existing code

I'd maybe change visible_paths to something like excluded_paths. Meaning it's excluded from the checks.

Yeah. All the time I tested with

auth_header = APIKeyHeader(name="Authorization") private_app = FastAPI(dependencies=[Depends(auth_header)])

and with auth_header as dependency to the whole private_app only Swagger endpoints work in visible_paths because the authorization header dependency expected header on every request (that's why the /bands endpoint requested a token). I'm sorry.
I will make the changes you suggested. If that's not enough, feel free to change anything.

Ok, interesting. I didn't consider the interaction with APIKeyHeader.

It might be best to attach APIKeyHeader to an APIRouter rather than the app.

Yes. We can do this at APIRouter level or if we use FastAPIWrapper we can do this

auth_header = APIKeyHeader(name="Authorization") private_app = FastAPI() FastAPIWrapper( "/bands/", fastapi_app=private_app, piccolo_crud=PiccoloCRUD(Band, read_only=False), fastapi_kwargs=FastAPIKwargs( all_routes={"tags": ["Band"]}, post={"dependencies": [Depends(auth_header)]}, # protected route put={"dependencies": [Depends(auth_header)]}, # protected route patch={"dependencies": [Depends(auth_header)]}, # protected route delete_single={ "dependencies": [Depends(auth_header)] }, # protected route ), )

But to be honest, I like it best at the application level, as we do in Piccolo Admin, because a protected (private) application should be private, and I think the best option is that only Swagger docs can be seen (for frontend developers as requested in issue) but in order to communicate with the endpoints, the user must be authorized which is provided with APIKeyHeader.
But it depends on the user how they want to implement the protection, and with excluded_paths we provide an option for that.

dantownsend · 2023-04-16T15:31:40Z

piccolo_api/jwt_auth/middleware.py

@@ -130,6 +137,10 @@ async def __call__(self, scope, receive, send):
        """
        allow_unauthenticated = self.allow_unauthenticated

+        if scope["path"] in self.excluded_paths:


We can enhance this quite easily to allow *. For example foo/*.

for excluded_path in self.excluded_paths: if excluded_path.endswith('*'): if scope['path'].startswith(excluded_path.rstrip('*')): await self.asgi(scope, receive, send) return else: if scope['path'] == excluded_path: await self.asgi(scope, receive, send) return

It would be a really useful feature.

Great idea. With this we can exclude the entire paths of the app, for example for private_app we can exclude all paths like private/*. I only need to change this line

if scope['path'].startswith(excluded_path.rstrip('*')):

to

if excluded_path.startswith(excluded_path.rstrip("*")):

to have an effect.

dantownsend · 2023-04-16T15:33:02Z

tests/jwt_auth/test_jwt_middleware.py

@@ -199,3 +205,13 @@ def test_token_without_user_id(self):
            response.json(),
            {"user_id": None, "jwt_error": JWTError.user_not_found.value},
        )
+
+    def test_visible_paths(self):


We should update this to test_excluded_paths.

I forgot to change that. Thanks.

dantownsend · 2023-04-16T17:52:09Z

piccolo_api/jwt_auth/middleware.py

-            return
+        for excluded_path in self.excluded_paths:
+            if excluded_path.endswith("*"):
+                if excluded_path.startswith(excluded_path.rstrip("*")):


This isn't quite right - because it's comparing excluded_paths with excluded_paths.

But

if scope['path'].startswith(excluded_path.rstrip('*')):

does nothing. If we have /foo/* we need check to root_path like this

if scope["root_path"].startswith(excluded_path.rstrip("/*")):

to take some effect. Sorry if I don't understand well.

if scope['path'].startswith(excluded_path.rstrip('*')):

work only if we specified excluded_paths=["*"] not excluded_paths=["/foo/*"]. Did you think so?

I think the confusion is to do with how FastAPI splits the path between path and root_path.

For example, if I have the an APIRouter mounted at /private, and an endpoint mounted to that router at /blog, then root_path is /private and path is /blog.

I think we need to combine them for it to work properly. So rather than just checking against path it's something like urllib.parse.urljoin(request.scope['root_path'], request.scope['path']).

There's also raw_path:

https://asgi.readthedocs.io/en/latest/specs/www.html#http-connection-scope

This discussion gives some more context django/asgiref#229

raw_path did the trick. If we use raw_path like this

if scope["raw_path"].decode("utf-8").startswith(excluded_path.rstrip("*")):

both excluded_paths=["*"] and excluded_paths=["/foo/*"] works.

dantownsend · 2023-04-17T09:23:23Z

tests/jwt_auth/test_jwt_middleware.py

@@ -39,7 +39,7 @@ def wildcard_route():
    asgi=ECHO_APP, secret="SECRET", allow_unauthenticated=True
 )
 APP_EXCLUDED_PATHS = JWTMiddleware(
-    asgi=fastapi_app, secret="SECRET", excluded_paths=["/docs", "/*"]
+    asgi=fastapi_app, secret="SECRET", excluded_paths=["/docs", "*"]


We should remove "*" as this lets everything through.

That will decrease the coverage. If we remove that, we need to add another FastAPI application to test the route with wildcards like this

fastapi_app_wildcard = FastAPI() @fastapi_app_wildcard.get("/") def wildcard_route(): return "Wildcard route test" APP_EXCLUDED_PATHS_WILDCARD = JWTMiddleware( asgi=fastapi_app_wildcard, secret="SECRET", excluded_paths=["*"], ) def test_excluded_paths_wildcards(self): client = TestClient(APP_EXCLUDED_PATHS_WILDCARD) response = client.get("/") self.assertEqual(response.status_code, 200) self.assertIn( b"Wildcard route test", response.content, )

Is that OK?

I was thinking something like this:

fastapi_app_wildcard = FastAPI() @fastapi_app_wildcard.get("/") def home_root(): return "Root" @fastapi_app_wildcard.get("/path/a/") def sub_root(): return "Sub route" APP_EXCLUDED_PATHS_WILDCARD = JWTMiddleware( asgi=fastapi_app_wildcard, secret="SECRET", excluded_paths=["/path/*"], ) def test_excluded_paths_wildcards(self): client = TestClient(APP_EXCLUDED_PATHS_WILDCARD) response = client.get("/") # Requires a token self.assertEqual(response.status_code, 403) # Is an excluded path, so doesn't need a token response = client.get("/path/a/") self.assertEqual(response.status_code, 200)

OK. I will add your tests with one change

def test_excluded_paths_wildcards(self): client = TestClient(APP_EXCLUDED_PATHS_WILDCARD) with self.assertRaises(HTTPException): response = client.get("/") # Requires a token self.assertEqual(response.status_code, 403) # Is an excluded path, so doesn't need a token response = client.get("/path/a/") self.assertEqual(response.status_code, 200)

sinisaos · 2023-05-07T06:45:10Z

@dantownsend What is the problem that this is not merged, since it is practically the same as TokenAuth excluded_paths which is merged? Do I need to change something?

dantownsend · 2023-05-07T10:50:51Z

@sinisaos I think it just slipped through the cracks. I can't remember if we tweaked the docs or something on the other PR. Will have a look later on.

sinisaos · 2023-05-07T11:05:54Z

@dantownsend Great. Thanks

sinisaos added 3 commits April 13, 2023 16:23

add visible_paths to JWTMiddleware

504c973

fix visible_paths

f1c3eab

update tests

9239fd6

dantownsend reviewed Apr 15, 2023

View reviewed changes

make suggested changes

c6b5542

dantownsend reviewed Apr 16, 2023

View reviewed changes

support for wildcard routes

42560a8

dantownsend reviewed Apr 16, 2023

View reviewed changes

include raw_path check

b24d719

dantownsend reviewed Apr 17, 2023

View reviewed changes

sinisaos added 2 commits April 17, 2023 14:41

update tests

b18e2f6

change List to Sequence

7e903ca

dantownsend changed the title ~~add visible_paths to JWTMiddleware~~ add excluded_paths to JWTMiddleware Apr 20, 2023

Merge branch 'piccolo-orm:master' into jwt_visible_paths

08c74fe

Merge branch 'piccolo-orm:master' into jwt_visible_paths

0d6b825

sinisaos closed this Sep 1, 2023

sinisaos deleted the jwt_visible_paths branch September 1, 2023 12:37

sinisaos mentioned this pull request Oct 11, 2023

Add post save, edit, delete hooks #210

Open

2 tasks

dantownsend mentioned this pull request Apr 10, 2024

Add excluded_paths to SessionsAuthBackend #282

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add `excluded_paths` to `JWTMiddleware` #226

add `excluded_paths` to `JWTMiddleware` #226

sinisaos commented Apr 13, 2023

codecov-commenter commented Apr 15, 2023 •

edited

Loading

dantownsend Apr 15, 2023

sinisaos Apr 15, 2023

sinisaos Apr 15, 2023

dantownsend Apr 15, 2023

sinisaos Apr 15, 2023

dantownsend Apr 15, 2023 •

edited

Loading

dantownsend Apr 15, 2023

sinisaos Apr 15, 2023

dantownsend Apr 15, 2023

sinisaos Apr 15, 2023 •

edited

Loading

dantownsend Apr 16, 2023 •

edited

Loading

sinisaos Apr 16, 2023

dantownsend Apr 16, 2023

sinisaos Apr 16, 2023

dantownsend Apr 16, 2023

sinisaos Apr 16, 2023

sinisaos Apr 16, 2023

dantownsend Apr 16, 2023 •

edited

Loading

dantownsend Apr 16, 2023

dantownsend Apr 16, 2023

sinisaos Apr 16, 2023

dantownsend Apr 17, 2023

sinisaos Apr 17, 2023

dantownsend Apr 17, 2023

sinisaos Apr 17, 2023

sinisaos commented May 7, 2023

dantownsend commented May 7, 2023

sinisaos commented May 7, 2023

add excluded_paths to JWTMiddleware #226

add excluded_paths to JWTMiddleware #226

Conversation

sinisaos commented Apr 13, 2023

codecov-commenter commented Apr 15, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dantownsend Apr 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sinisaos Apr 15, 2023 • edited Loading

Choose a reason for hiding this comment

dantownsend Apr 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dantownsend Apr 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sinisaos commented May 7, 2023

dantownsend commented May 7, 2023

sinisaos commented May 7, 2023

add `excluded_paths` to `JWTMiddleware` #226

add `excluded_paths` to `JWTMiddleware` #226

codecov-commenter commented Apr 15, 2023 •

edited

Loading

dantownsend Apr 15, 2023 •

edited

Loading

sinisaos Apr 15, 2023 •

edited

Loading

dantownsend Apr 16, 2023 •

edited

Loading

dantownsend Apr 16, 2023 •

edited

Loading