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

UI to create reduced scope tokens from the /-/create-token page #1947

Closed
simonw opened this issue Dec 13, 2022 · 22 comments
Closed

UI to create reduced scope tokens from the /-/create-token page #1947

simonw opened this issue Dec 13, 2022 · 22 comments

Comments

@simonw
Copy link
Owner

simonw commented Dec 13, 2022

Split from:

@simonw
Copy link
Owner Author

simonw commented Dec 13, 2022

For the UI: I think I'm going to dump a whole bunch of form elements on the page (so you can set up to 3 of each category of limit without any JavaScript), then add JavaScript that hides all but one of the options and gives you a "add another" widget that adds multiple more.

@simonw simonw changed the title Ability to create reduced scope tokens from the /-/create-token page UI to create reduced scope tokens from the /-/create-token page Dec 13, 2022
@simonw simonw pinned this issue Dec 13, 2022
@simonw
Copy link
Owner Author

simonw commented Dec 13, 2022

https://latest.datasette.io/-/create-token currently looks like this:

Image

As a reminder, the CLI options that this needs to provide an alternative to are:

Usage: datasette create-token [OPTIONS] ID
Create a signed API token for the specified actor ID
Example:
datasette create-token root --secret mysecret
To only allow create-table:
datasette create-token root --secret mysecret \
--all create-table
Or to only allow insert-row against a specific table:
datasette create-token root --secret myscret \
--resource mydb mytable insert-row
Restricted actions can be specified multiple times using multiple --all,
--database, and --resource options.

@simonw
Copy link
Owner Author

simonw commented Dec 13, 2022

  • I should add a --database example to that help text.

@simonw simonw added this to the Datasette 1.0a2 milestone Dec 13, 2022
@simonw
Copy link
Owner Author

simonw commented Dec 13, 2022

I'm going to hide the options for reducing the scope of the token inside a details/summary element.

@simonw
Copy link
Owner Author

simonw commented Dec 13, 2022

I'm experimenting with a <select multiple> for this.

The usability for keyboards is still pretty awful, but it's a niche enough feature that maybe that's OK for the moment?

var select = document.querySelector('select');
var selected = Array.from(temp0.options).filter(o => o.selected).map(o => o.value) 

@simonw
Copy link
Owner Author

simonw commented Dec 13, 2022

Got an option group thing working:

image

But... it strikes me that any time you're considering a <select multiple> like this a nested list of checkboxes would actually be better - easier for people to use.

@simonw
Copy link
Owner Author

simonw commented Dec 13, 2022

My <select multiple> prototype:

diff --git a/datasette/templates/create_token.html b/datasette/templates/create_token.html
index a94881ed..5bd641cc 100644
--- a/datasette/templates/create_token.html
+++ b/datasette/templates/create_token.html
@@ -6,7 +6,7 @@
 
 <h1>Create an API token</h1>
 
-<p>This token will allow API access with the same abilities as your current user.</p>
+<p>This token will allow API access with the same abilities as your current user, <strong>{{ request.actor.id }}</strong></p>
 
 {% if errors %}
   {% for error in errors %}
@@ -28,6 +28,36 @@
     <input type="hidden" name="csrftoken" value="{{ csrftoken() }}">
     <input type="submit" value="Create token">
   </div>
+
+  <details style="margin-top: 1em" open>
+    <summary style="cursor: pointer;">Restrict actions that can be performed using this token</summary>
+    <p style="margin-top: 1em">Restrict actions that can be performed using this token:</p>
+    <p><label="all_permissions">All databases and tables:</label></p>
+    <p><select multiple id="all_permissions" size="{{ all_permissions|length * 4 }}">
+      <optgroup label="All databases and tables">
+      {% for permission in all_permissions %}
+        <option value="all:{{ permission }}">{{ permission }}</option>
+      {% endfor %}
+    </optgroup>
+    {% for database in databases %}
+      <optgroup label="All tables in database: {{ database }}">
+        {% for permission in database_permissions %}
+          <option value="db:{{ database }}:{{ permission }}">{{ permission }}</option>
+        {% endfor %}
+      </optgroup>
+    {% endfor %}
+    {% for dbt in database_with_tables %}
+      {% for table in dbt.tables %}
+        <optgroup label="Table {{ dbt.database }}.{{ table }}">
+          {% for permission in table_permissions %}
+            <option value="table:{{ dbt.database }}:{{ permission }}">{{ permission }}</option>
+          {% endfor %}
+        </optgroup>
+      {% endfor %}
+    {% endfor %}
+  </select></p>
+  </details>
+
 </form>
 
 {% if token %}
diff --git a/datasette/views/special.py b/datasette/views/special.py
index 30345d14..9d0fcd31 100644
--- a/datasette/views/special.py
+++ b/datasette/views/special.py
@@ -231,7 +231,17 @@ class CreateTokenView(BaseView):
         return await self.render(
             ["create_token.html"],
             request,
-            {"actor": request.actor},
+            {
+                "actor": request.actor,
+                "all_permissions": self.ds.permissions.keys(),
+                "database_permissions": [key for key, value in self.ds.permissions.items() if value.takes_database],
+                "table_permissions": [key for key, value in self.ds.permissions.items() if value.takes_resource],
+                "databases": self.ds.databases.keys(),
+                "database_with_tables": [{
+                    "database": db.name,
+                    "tables": await db.table_names(),
+                } for db in self.ds.databases.values()],
+            },
         )
 
     async def post(self, request):

@simonw
Copy link
Owner Author

simonw commented Dec 13, 2022

I think checkboxes will work well.

Here's the data I get back from them (as post_vars()):

{'all:debug-menu': 'on',
 'all:insert-row': 'on',
 'expire_duration': '',
 'expire_type': '',
 'table:fixtures:delete-row': 'on',
 'table:fixtures:drop-table': 'on',
 'table:fixtures:view-query': 'on'}

@simonw
Copy link
Owner Author

simonw commented Dec 13, 2022

Checkbox interface looks like this. It's not beautiful but it's good enough for the moment:

image

@simonw
Copy link
Owner Author

simonw commented Dec 13, 2022

Slightly tricky thing here is that it should only show permissions that the user themselves has - on databases and tables that they have permission to access.

I have a nasty feeling this may require looping through everything and running every permission check, which could get very expensive if there are plugins involved that do their own storage check to resolve a permission.

It's that classic permission system problem: how to efficiently iterate through everything the user has permission to do in one go?

Might be that I have to punt on that, and show the user a list of permissions to select that they might not actually have ability for.

@simonw
Copy link
Owner Author

simonw commented Dec 13, 2022

Here's the checkbox prototype:

diff --git a/datasette/templates/create_token.html b/datasette/templates/create_token.html
index a94881ed..1795ebaf 100644
--- a/datasette/templates/create_token.html
+++ b/datasette/templates/create_token.html
@@ -2,11 +2,20 @@
 
 {% block title %}Create an API token{% endblock %}
 
+{% block extra_head %}
+<style type="text/css">
+#restrict-permissions label {
+  display: inline;
+  width: 90%;
+}
+</style>
+{% endblock %}
+
 {% block content %}
 
 <h1>Create an API token</h1>
 
-<p>This token will allow API access with the same abilities as your current user.</p>
+<p>This token will allow API access with the same abilities as your current user, <strong>{{ request.actor.id }}</strong></p>
 
 {% if errors %}
   {% for error in errors %}
@@ -27,8 +36,39 @@
     <input type="text" name="expire_duration" style="width: 10%">
     <input type="hidden" name="csrftoken" value="{{ csrftoken() }}">
     <input type="submit" value="Create token">
-  </div>
+
+  <details style="margin-top: 1em" open id="restrict-permissions">
+    <summary style="cursor: pointer;">Restrict actions that can be performed using this token</summary>
+    <h2>All databases and tables</h2>
+    <ul>
+      {% for permission in all_permissions %}
+        <li><label><input type="checkbox" name="all:{{ permission }}"> {{ permission }}</label></li>
+      {% endfor %}
+    </ul>
+
+    {% for database in databases %}
+      <h2>All tables in database: {{ database }}</h2>
+      <ul>
+        {% for permission in database_permissions %}
+          <li><label><input type="checkbox" name="db:{{ database }}:{{ permission }}"> {{ permission }}</label></li>
+        {% endfor %}
+      </ul>
+    {% endfor %}
+    <h2>Specific tables</h2>
+    {% for dbt in database_with_tables %}
+      {% for table in dbt.tables %}
+        <h3>{{ dbt.database }}: {{ table }}</h3>
+        <ul>
+          {% for permission in table_permissions %}
+            <li><label><input type="checkbox" name="table:{{ dbt.database }}:{{ permission }}"> {{ permission }}</label></li>
+          {% endfor %}
+        </ul>
+      {% endfor %}
+    {% endfor %}
+  </details>
+
 </form>
+</div>
 
 {% if token %}
   <div>
diff --git a/datasette/views/special.py b/datasette/views/special.py
index 30345d14..48357f87 100644
--- a/datasette/views/special.py
+++ b/datasette/views/special.py
@@ -231,12 +231,37 @@ class CreateTokenView(BaseView):
         return await self.render(
             ["create_token.html"],
             request,
-            {"actor": request.actor},
+            {
+                "actor": request.actor,
+                "all_permissions": self.ds.permissions.keys(),
+                "database_permissions": [
+                    key
+                    for key, value in self.ds.permissions.items()
+                    if value.takes_database
+                ],
+                "table_permissions": [
+                    key
+                    for key, value in self.ds.permissions.items()
+                    if value.takes_resource
+                ],
+                "databases": [k for k in self.ds.databases.keys() if k != "_internal"],
+                "database_with_tables": [
+                    {
+                        "database": db.name,
+                        "tables": await db.table_names(),
+                    }
+                    for db in self.ds.databases.values()
+                    if db.name != "_internal"
+                ],
+            },
         )
 
     async def post(self, request):
         self.check_permission(request)
         post = await request.post_vars()
+        from pprint import pprint
+
+        pprint(post)
         errors = []
         duration = None
         if post.get("expire_type"):

@simonw
Copy link
Owner Author

simonw commented Dec 13, 2022

I'm going to ignore the permissions issue for the moment - I'll allow people to select any permissions they like in any of the databases or tables that are visible to them (don't want to leak the existence of databases/tables to users who shouldn't be able to see them).

I think the value of getting this working outweights any potential confusion from not using finely grained permission checks to decide if the user should be able to apply a permission or not.

The tokens themselves won't be able to perform insert-row or similar if the user doesn't have the ability to do that, even if they selected that checkbox.

@simonw
Copy link
Owner Author

simonw commented Dec 14, 2022

Another thing to consider in the future: once Datasette can support thousands of tables (see #417) the list on this page will turn into multiple MBs of HTML, which may cause all kinds of problems - not to mention the overhead of all of those table visibility permission checks.

Hopefully by then I'll have a good fix for the permission listings problem:

And I can apply the same mechanism here.

@simonw
Copy link
Owner Author

simonw commented Dec 14, 2022

Also: don't show hidden tables.

@simonw
Copy link
Owner Author

simonw commented Dec 14, 2022

Another option: I could set a time limit - say 200ms - on how long I'm willing to spend calculating permissions before displaying this form

First calculate view permissions for tables and databases (and maybe views and canned queries too).

Then see if I can check every permission that I'm going to show as a checkbox on this page. If I get that done within the time limit use that to show the options.

If I run out of time show all options and maybe include a note saying that some of them may not actually be available.

@simonw
Copy link
Owner Author

simonw commented Dec 14, 2022

With tilde-encoding for database and table names the HTML looks like this:

<input type="checkbox" name="table:weird:foo~3Abar:view-table">

@simonw
Copy link
Owner Author

simonw commented Dec 14, 2022

I shipped a working interface. Could still do with some extra tests.

@simonw
Copy link
Owner Author

simonw commented Dec 14, 2022

Tests can go here:

(
{"expire_type": "hours", "expire_duration": "10"},
[],
10 * 60 * 60,
),
(
{"expire_type": "days", "expire_duration": "3"},
[],
60 * 60 * 24 * 3,
),
),
)
def test_auth_create_token(app_client, post_data, errors, expected_duration):
assert app_client.get("/-/create-token").status == 403
ds_actor = app_client.actor_cookie({"id": "test"})
response = app_client.get("/-/create-token", cookies={"ds_actor": ds_actor})
assert response.status == 200
assert ">Create an API token<" in response.text

@simonw
Copy link
Owner Author

simonw commented Dec 14, 2022

New interface now live at https://latest.datasette.io/-/create-token

image

It shouldn't be showing _memory though.

@simonw
Copy link
Owner Author

simonw commented Dec 14, 2022

Maybe I should have kept _memory listed for instances that are running with --crossdb enabled?

Yeah I think I should.

@simonw
Copy link
Owner Author

simonw commented Dec 14, 2022

It would be neat not to show write permissions against immutable databases too - and not hard from a performance perspective since it doesn't involve hundreds more permission checks.

That will need permissions to grow a flag for if they need a mutable database though, which is a bigger job.

@simonw
Copy link
Owner Author

simonw commented Dec 14, 2022

I think the next big step for this feature is for me to actually use it to build a few things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant