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

check_visibility can now take multiple permissions into account #1842

Merged
merged 7 commits into from
Oct 24, 2022

Conversation

simonw
Copy link
Owner

@simonw simonw commented Oct 14, 2022

Refs #1829

  • Fix table page
  • Fix database page
  • Fix query page
  • Fix row page
  • Tests
  • Updated documentation for check_visibility method, to cover the new permissions= keyword argument

Also this fix is currently only applied on the table page - needs to be applied on database, row and query pages too.


📚 Documentation preview 📚: https://datasette--1842.org.readthedocs.build/en/1842/

@codecov
Copy link

codecov bot commented Oct 14, 2022

Codecov Report

Base: 92.52% // Head: 92.54% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (3623475) compared to base (79aa0de).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1842      +/-   ##
==========================================
+ Coverage   92.52%   92.54%   +0.02%     
==========================================
  Files          35       35              
  Lines        4415     4428      +13     
==========================================
+ Hits         4085     4098      +13     
  Misses        330      330              
Impacted Files Coverage Δ
datasette/app.py 94.28% <100.00%> (+0.05%) ⬆️
datasette/views/database.py 95.29% <100.00%> (+0.06%) ⬆️
datasette/views/index.py 96.49% <100.00%> (ø)
datasette/views/row.py 88.70% <100.00%> (+0.37%) ⬆️
datasette/views/table.py 95.20% <100.00%> (+0.01%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@simonw simonw marked this pull request as ready for review October 24, 2022 01:53
@simonw simonw changed the title check_visibility can take multiple permissions into account check_visibility can now take multiple permissions into account Oct 24, 2022
@simonw
Copy link
Owner Author

simonw commented Oct 24, 2022

I need to do one last round of manual testing before I merge this.

@simonw
Copy link
Owner Author

simonw commented Oct 24, 2022

I'm going to construct a metadata.yml which makes various databases and tables visible or invisible, then browse them using the root user.

block-instance.yml:

allow:
  id: root

block-database.yml:

databases:
  fixtures:
    allow:
      id: root

block-table.yml:

databases:
  fixtures:
    tables:
      searchable:
        allow:
          id: root

block-query.yml:

databases:
  fixtures:
    queries:
      two:
        sql: select 1 + 1
        allow:
          id: root

https://gist.github.com/simonw/2d007ebe43de46d44499c77a2a291756 - checkout that Gist to get all four.

I manually tested all four scenarios with root and non-root users and confirmed that they worked correctly and padlocks were shown in the right places.

@simonw simonw merged commit 78dad23 into main Oct 24, 2022
@simonw simonw deleted the issue-1829 branch October 24, 2022 02:11
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.

1 participant