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

Support "allow" block on root, databases and tables, not just queries #811

Closed
15 tasks done
simonw opened this issue Jun 7, 2020 · 16 comments
Closed
15 tasks done

Comments

@simonw
Copy link
Owner

simonw commented Jun 7, 2020

No reason not to expand the "allow" mechanism described here to the root of metadata.json plus to databases and tables.

Refs #810 and #800.

{
    "databases": {
        "mydatabase": {
            "allow": {
                "id": ["root"]
            }
        }
    }
}

TODO:

  • Instance level
  • Database level
  • Table level
  • Query level
  • Affects list of queries
  • Affects list of tables on database page
  • Affects truncated list of tables on index page
  • Affects list of SQL views on database page
  • Affects list of databases on index page
  • Show 🔒 in header on index page for private instances
  • Show 🔒 in header on private database page
  • Show 🔒 in header on private table page
  • Show 🔒 in header on private query page
  • Move assert_permissions_checked() calls from test_html.py to test_permissions.py
  • Update documentation
@simonw
Copy link
Owner Author

simonw commented Jun 7, 2020

If the allow block at the database level forbids access this needs to cascade down to the table, query and row levels as well.

@simonw
Copy link
Owner Author

simonw commented Jun 7, 2020

I'll need a neat testing pattern for this.

@simonw
Copy link
Owner Author

simonw commented Jun 7, 2020

Testing pattern:

def test_canned_query_with_custom_metadata(app_client):
    response = app_client.get("/fixtures/neighborhood_search?text=town")
    assert_permissions_checked(
        app_client.ds,
        [
            "view-instance",
            ("view-database", "database", "fixtures"),
            ("view-query", "query", ("fixtures", "neighborhood_search")),
        ],
    )

@simonw
Copy link
Owner Author

simonw commented Jun 7, 2020

I'm going to add a test_permissions.py module that checks for 403 errors against different patterns of the actors block at different levels in metadata.json.

@simonw
Copy link
Owner Author

simonw commented Jun 7, 2020

Next step: fix this

-            # TODO: fix this to use that permission check
-            if not actor_matches_allow(
-                request.scope.get("actor", None), metadata.get("allow")
-            ):
-                return Response("Permission denied", status=403)

@simonw
Copy link
Owner Author

simonw commented Jun 7, 2020

The tests in test_permissions.py could check the .json variants and assert that permission checks were carried out too.

@simonw
Copy link
Owner Author

simonw commented Jun 8, 2020

I'd like to be able to apply permissions for the ability to run a SQL query - but I'm not sure where the best place for that "allow" block to live would be.

@simonw
Copy link
Owner Author

simonw commented Jun 8, 2020

Also need to expand the docs on https://datasette.readthedocs.io/en/latest/authentication.html to explain where you can put allow blocks to control access to the instance, database or table.

@simonw
Copy link
Owner Author

simonw commented Jun 8, 2020

Do row-level permissions even make sense? Might be a good idea to remove those until I have a good use-case for them.

@simonw
Copy link
Owner Author

simonw commented Jun 8, 2020

I should take these permissions into account when displaying a list of tables or a list of databases (like I do right now when displaying a list of queries).

@simonw
Copy link
Owner Author

simonw commented Jun 8, 2020

Oh this is a bit awkward - should I be running per-table permission checks for every table that might be shown on the index page?

Datasette__fixtures__data

@simonw
Copy link
Owner Author

simonw commented Jun 8, 2020

Per-table permissions is pretty interesting for large installations though - an organization might have hundreds of CSV files imported into Datasette and then allow users to specify which exact users within that organization are allowed to see which CSV.

simonw added a commit that referenced this issue Jun 8, 2020
Also now using 🔒 to indicate private resources - resources that
would not be available to the anonymous user. Refs #811
@simonw
Copy link
Owner Author

simonw commented Jun 8, 2020

New convention: the 🔒 icon is now shown next to resources that are private - that are visible to you now, but would not be visible to the anonymous user.

Datasette__fixtures__data

@simonw
Copy link
Owner Author

simonw commented Jun 8, 2020

I'm finding myself repeating this pattern a lot:

        for table in table_counts:
            allowed = await self.ds.permission_allowed(
                request.scope.get("actor"),
                "view-table",
                resource_type="table",
                resource_identifier=(database, table),
                default=True,
            )
            if not allowed:
                continue
            private = not await self.ds.permission_allowed(
                None,
                "view-table",
                resource_type="table",
                resource_identifier=(database, table),
            )

I use a similar pattern for lists of databases and lists of queries, and I'll be doing the same thing for lists of SQL views too.

An abstraction around this would be useful.

Idea:

visible, private = await check_visibility(
    self.ds, actor, "view-table", "table", (database, table)
)

simonw added a commit that referenced this issue Jun 8, 2020
@simonw
Copy link
Owner Author

simonw commented Jun 8, 2020

I really like the padlocks. I should include a screenshot in the documentation that illustrates them.

data

Maybe I should figure out a way to have the https://latest.datasette.io/ demo illustrate both a logged-in and a logged-out state.

@simonw
Copy link
Owner Author

simonw commented Jun 8, 2020

Should the padlock show up on tables that are private only because they inherited their privacy from their parent database or even the parent instance?

Interesting question. If an instance is private, I'm not sure it makes sense to show padlocks on absolutely everything.

Likewise, a list of tables shown on the database table with a padlock next to every single table (when the database itself is private) doesn't seem to add any useful information.

I think "Show 🔒 in header on private database page" will resolve this for me. I'll always show the padlock in the header of a database/table page even if that privacy is inherited - but I won't do that for padlocks shown in the list of tables or list of databases.

simonw added a commit that referenced this issue Jun 8, 2020
simonw added a commit that referenced this issue Jun 8, 2020
simonw added a commit that referenced this issue Jun 8, 2020
simonw added a commit that referenced this issue Jun 8, 2020
simonw added a commit that referenced this issue Jun 8, 2020
Also makes a small change to the /fixtures.json JSON:

    "views": ["view_name"]

Is now:

    "views": [{"name": "view_name", "private": true}]
@simonw simonw unpinned this issue Jun 8, 2020
@simonw simonw closed this as completed in 5437085 Jun 8, 2020
@simonw simonw added the large label Jun 8, 2020
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