simonw / datasette-graphql

Datasette plugin providing an automatic GraphQL API for your SQLite databases
https://datasette-graphql-demo.datasette.io/
Apache License 2.0
101 stars 6 forks source link

Doesn't work with authenticated Datasette instances #97

Open simonw opened 1 year ago

simonw commented 1 year ago

The GraphQL explorer at least doesn't - you get this error:

CleanShot 2023-09-07 at 10 07 27@2x

Interesting that the API introspection DOES work, but the actual query execution does not.

simonw commented 1 year ago

I set that instance up with https://datasette.io/datasette-auth-tokens and this config:

plugins:
  datasette-auth-tokens:
    tokens:
    - token: secret-token
      actor:
        id: bot

allow:
  id:
  - bot
  - root
simonw commented 1 year ago

Made this change:

diff --git a/datasette_graphql/utils.py b/datasette_graphql/utils.py
index 9eeb358..2218e8a 100644
--- a/datasette_graphql/utils.py
+++ b/datasette_graphql/utils.py
@@ -594,7 +594,10 @@ def make_table_resolver(
                     path_with_query_string,
                 )

-        data = (await datasette.client.get(path_with_query_string)).json()
+        response = await datasette.client.get(path_with_query_string)
+        if response.status_code != 200:
+            raise Exception(str(response.status_code) + response.text)
+        data = response.json()
         # If any cells are $base64, decode them into bytes objects
         for row in data["rows"]:
             for key, value in row.items():

And now:

You do not have permission to view this table

CleanShot 2023-09-07 at 10 10 32@2x
simonw commented 1 year ago

Fixing this is actually quite hard. The datasette.client.get() call is going to need access to the cookies and Authorization header from the incoming request object - but getting that object to it requires digging through a few levels of abstraction.

The main problem is that it's part of the schema which is created in here:

https://github.com/simonw/datasette-graphql/blob/c0ab66bc34a2cc65df2585cf9894d6f4c6db6186/datasette_graphql/utils.py#L92-L94

Specifically this bit: https://github.com/simonw/datasette-graphql/blob/c0ab66bc34a2cc65df2585cf9894d6f4c6db6186/datasette_graphql/utils.py#L180-L187

And the whole point of schema_for_database is that it can be cached once and used for multiple requests!

simonw commented 1 year ago

Maybe the answer lies here: https://github.com/simonw/datasette-graphql/blob/c0ab66bc34a2cc65df2585cf9894d6f4c6db6186/datasette_graphql/__init__.py#L110-L124

I could inject the request information into that context.

simonw commented 1 year ago

OK, this seems to fix things:

diff --git a/datasette_graphql/__init__.py b/datasette_graphql/__init__.py
index d02262f..ac18ce6 100644
--- a/datasette_graphql/__init__.py
+++ b/datasette_graphql/__init__.py
@@ -113,6 +113,7 @@ async def view_graphql(request, datasette):
         "num_queries_executed": 0,
         "num_queries_limit": config.get("num_queries_limit")
         or DEFAULT_NUM_QUERIES_LIMIT,
+        "request": request,  # For authentication headers
     }

     result = await schema.execute_async(
diff --git a/datasette_graphql/utils.py b/datasette_graphql/utils.py
index 9eeb358..c346ded 100644
--- a/datasette_graphql/utils.py
+++ b/datasette_graphql/utils.py
@@ -594,7 +594,15 @@ def make_table_resolver(
                     path_with_query_string,
                 )

-        data = (await datasette.client.get(path_with_query_string)).json()
+        headers = context["request"].headers
+        cookies = context["request"].cookies
+
+        response = await datasette.client.get(
+            path_with_query_string, headers=headers, cookies=cookies
+        )
+        if response.status_code != 200:
+            raise Exception(str(response.status_code) + response.text)
+        data = response.json()
         # If any cells are $base64, decode them into bytes objects
         for row in data["rows"]:
             for key, value in row.items():

I need to do a bunch of testing around this though. Including checking that the schema isn't being linked to users who should not be able to view it.