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

REST requests should mirror Alyx user group permissions #882

Open
5 tasks
k1o0 opened this issue Nov 20, 2024 · 0 comments
Open
5 tasks

REST requests should mirror Alyx user group permissions #882

k1o0 opened this issue Nov 20, 2024 · 0 comments

Comments

@k1o0
Copy link
Collaborator

k1o0 commented Nov 20, 2024

Most (but maybe not all) REST views use permissions defined in alyx.base.BestRestPublicPermission:

alyx/alyx/alyx/base.py

Lines 648 to 664 in 95c2e40

class BaseRestPublicPermission(permissions.BasePermission):
"""
The purpose is to prevent public users from interfering in any way using writable methods
"""
def has_permission(self, request, view):
if request.method == 'GET':
return True
elif request.user.is_public_user:
return False
else:
return True
def rest_permission_classes():
permission_classes = (permissions.IsAuthenticated & BaseRestPublicPermission,)
return permission_classes
, while most (but maybe not all) model admin instances use the alyx.base.BaseAdmin.has_change_permission method:

alyx/alyx/alyx/base.py

Lines 374 to 413 in 95c2e40

def has_change_permission(self, request, obj=None):
if request.user.is_public_user:
return False
if not obj:
return True
if request.user.is_superuser:
return True
# [CR 2024-03-12]
# HACK: following a request by Charu R from cortexlab, we authorize all users in the
# special Husbandry group to edit litters.
husbandry = 'husbandry' in ', '.join(_.name.lower() for _ in request.user.groups.all())
if husbandry:
if obj.__class__.__name__ in ('Litter', 'Subject', 'BreedingPair'):
return True
# Find subject associated to the object.
if hasattr(obj, 'responsible_user'):
subj = obj
elif getattr(obj, 'session', None):
subj = obj.session.subject
elif getattr(obj, 'subject', None):
subj = obj.subject
else:
return False
resp_user = getattr(subj, 'responsible_user', None)
# List of allowed users for the subject.
allowed = getattr(resp_user, 'allowed_users', None)
allowed = set(allowed.all() if allowed else [])
if resp_user:
allowed.add(resp_user)
# Add the responsible user or user(s) to the list of allowed users.
if hasattr(obj, 'responsible_user'):
allowed.add(obj.responsible_user)
if hasattr(obj, 'user'):
allowed.add(obj.user)
if hasattr(obj, 'users'):
for user in obj.users.all():
allowed.add(user)
return request.user in allowed

This means that users have far more permissions when making REST queries than via the admin interface which is extremely insecure and confusing to users. Below are some suggested improvements:

  • Ensure all REST views are using a shared base permissions set (i.e. permission_classes = rest_permission_classes() is present everywhere)
  • Ensure all model admin classes use the same basic permissions system (i.e. super class call to BaseAdmin.has_change_permission)
  • Consolidate base permissions between APIs (the above two methods can call a common base permissions function)
  • Permit test Alyx to allow all REST permissions for testing purposes - check library tests still pass after changes
  • Add method to BaseTests to check that REST API permissions are suitable. This can be a generic test for all apps.
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