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

Add support for user based default access #812

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

krakan
Copy link
Contributor

@krakan krakan commented Feb 10, 2023

Setting a different default access for a specific user didn't previously work because

  • the ACL must be sorted in reversed alphabetic order which means that an empty URL (ie. a default rule) must be last, and

  • the search for matching ACL rules stops when the top level domain of the a rule is alphabetically lower than the one searched for.

Ie. given default_access: allow and the ACL:

com,example)/ - {"access": "block"}
 - {"access": "block", "user": "unknown"}

and searching for http://iana.org, the last line would not be found so that the unknown user would still be allowed.

Description

Don't stop searching for rules based on top level domain

Motivation and Context

Having a way to specify a default access rule for a specific user would be useful. In our case we would give external users access to the index but not to the data. This was previously possible only if the rule with an empty URL was the only one.

Screenshots (if appropriate):

Types of changes

  • Replay fix (fixes a replay specific issue)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added or updated tests to cover my changes.
  • All new and existing tests passed.

@tw4l tw4l self-requested a review February 15, 2023 18:08
@ikreymer ikreymer self-requested a review February 16, 2023 16:19
@ikreymer
Copy link
Member

This is a neat solution for default rules by user, the system wasn't really designed for that as is.
My main concern is continuing the search just to find the blank entry, if there are a lot of exclusion rules, it would slow down the search.

It may be clearer to disallow empty URLs, but instead support a list of default rules by user:

default_access:
   # default user
   - user: ""
      access: block
      
   - user: member
      access: allow

and then just check from this list instead?

@krakan
Copy link
Contributor Author

krakan commented Feb 17, 2023

Agreed, that is indeed clearer - I'll look into changing the PR.

@krakan
Copy link
Contributor Author

krakan commented Feb 23, 2023

I opted for a dict layout of the default_access entry instead of the proposed list:

default_access:
   default: block
   member: allow

This enables us to skip youtube-dl tests in GitHub Actions by
ensuring that the "CI" env var is passed to tox.
@krakan krakan changed the title Make empty URL:s work in ACL:s Add support for user based default access Mar 2, 2023
@krakan
Copy link
Contributor Author

krakan commented Mar 2, 2023

There! I had forgotten to add a file (again 🤦) - and I also rebased on the origin/dev/skip-youtube-dl-test-in-ci branch

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.

3 participants