pywb icon indicating copy to clipboard operation
pywb copied to clipboard

Add support for user based default access

Open krakan opened this issue 2 years ago • 4 comments

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)
  • [x] 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.
  • [x] I have updated the documentation accordingly.
  • [x] I have added or updated tests to cover my changes.
  • [x] All new and existing tests passed.

krakan avatar Feb 10 '23 14:02 krakan

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?

ikreymer avatar Feb 16 '23 16:02 ikreymer

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

krakan avatar Feb 17 '23 17:02 krakan

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

default_access:
   default: block
   member: allow

krakan avatar Feb 23 '23 10:02 krakan

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

krakan avatar Mar 02 '23 09:03 krakan