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

Restricted accounts #788

Merged
merged 7 commits into from
Nov 26, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ rest_framework = ["rest_framework"]
[tool.pytest.ini_options]
DJANGO_SETTINGS_MODULE = "config.settings"
testpaths = ["rdmo"]
python_files = "test_*[!.txt].py"
python_files = "test_*.py"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MyPyDavid @afuetterer for some reason I still don't understand, this change removed a substantial part of our tests from the CI. The tests run, but they are not included in just running pytest. This concerned import and export. I should have investigated when the coverage dropped after 2.0.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, oops 🙈 , strange but good to know

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which one was problematic?
test_.py or test_[!.txt].py?

What is the meaning of test_*[!.txt].py?

Copy link
Member Author

@jochenklar jochenklar Nov 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test_*[!.txt].py. I think @MyPyDavid wanted to exclude files like test_foo_bar.txt.py, but apparently it does not work that way. It did exclude, e.g. test_view_project_create_import.py.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I got it, all the missing tests got a t at the end:

test_validator_parent.py
test_viewset_import.py
test_export.py
test_validator_conflict.py
test_view_project.py
test_view_project_create_import.py
test_view_project_update_import.py
test_view_snapshot.py
test_viewset_project.py
test_viewset_project_snapshot.py
test_viewset_snapshot.py
test_viewset_questionset.py

I got the list with pytest --co with and without the pattern and some grepping.

The pattern means that the file must not end with .tx before the .py.

pythonpath = [".", "testing"]
addopts = '-p no:randomly -m "not e2e"'
markers = [
Expand Down
19 changes: 19 additions & 0 deletions rdmo/accounts/adapter.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from django.conf import settings
from django.contrib.auth.models import Group

from allauth.account.adapter import DefaultAccountAdapter
from allauth.socialaccount.adapter import DefaultSocialAccountAdapter
Expand All @@ -9,8 +10,26 @@ class AccountAdapter(DefaultAccountAdapter):
def is_open_for_signup(self, request):
return settings.ACCOUNT_SIGNUP

def save_user(self, request, user, form, commit=True):
user = super().save_user(request, user, form, commit)

if settings.ACCOUNT_GROUPS:
groups = Group.objects.filter(name__in=settings.ACCOUNT_GROUPS)
user.groups.set(groups)

return user

class SocialAccountAdapter(DefaultSocialAccountAdapter):

def is_open_for_signup(self, request, sociallogin):
return settings.SOCIALACCOUNT_SIGNUP

def save_user(self, request, sociallogin, form=None):
user = super().save_user(request, sociallogin, form)

if settings.SOCIALACCOUNT_GROUPS:
provider = str(sociallogin.account.provider)
groups = Group.objects.filter(name__in=settings.SOCIALACCOUNT_GROUPS.get(provider, []))
user.groups.set(groups)

return user
24 changes: 13 additions & 11 deletions rdmo/core/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -98,15 +98,9 @@

ACCOUNT = False
ACCOUNT_SIGNUP = False
ACCOUNT_GROUPS = []
ACCOUNT_TERMS_OF_USE = False

SOCIALACCOUNT = False

SHIBBOLETH = False
SHIBBOLETH_LOGIN_URL = '/Shibboleth.sso/Login'
SHIBBOLETH_LOGOUT_URL = '/Shibboleth.sso/Logout'
SHIBBOLETH_USERNAME_PATTERN = None

ACCOUNT_ADAPTER = 'rdmo.accounts.adapter.AccountAdapter'
ACCOUNT_SIGNUP_FORM_CLASS = 'rdmo.accounts.forms.SignupForm'
ACCOUNT_USER_DISPLAY = 'rdmo.accounts.utils.get_full_name'
ACCOUNT_EMAIL_REQUIRED = True
Expand All @@ -120,11 +114,16 @@
ACCOUNT_PREVENT_ENUMERATION = False
ACCOUNT_ALLOW_USER_TOKEN = False

ACCOUNT_ADAPTER = 'rdmo.accounts.adapter.AccountAdapter'

SOCIALACCOUNT_ADAPTER = 'rdmo.accounts.adapter.SocialAccountAdapter'
SOCIALACCOUNT = False
SOCIALACCOUNT_SIGNUP = False
SOCIALACCOUNT_GROUPS = []
SOCIALACCOUNT_AUTO_SIGNUP = False
SOCIALACCOUNT_ADAPTER = 'rdmo.accounts.adapter.SocialAccountAdapter'

SHIBBOLETH = False
SHIBBOLETH_LOGIN_URL = '/Shibboleth.sso/Login'
SHIBBOLETH_LOGOUT_URL = '/Shibboleth.sso/Logout'
SHIBBOLETH_USERNAME_PATTERN = None

LANGUAGE_CODE = 'en-us'

Expand Down Expand Up @@ -306,6 +305,9 @@

PROJECT_REMOVE_VIEWS = True

PROJECT_CREATE_RESTRICTED = False
PROJECT_CREATE_GROUPS = []

NESTED_PROJECTS = True

OPTIONSET_PROVIDERS = []
Expand Down
39 changes: 28 additions & 11 deletions rdmo/projects/permissions.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from rdmo.core.permissions import HasObjectPermission, log_result
from rdmo.core.permissions import HasModelPermission, HasObjectPermission, log_result


class HasProjectsPermission(HasObjectPermission):
Expand All @@ -8,11 +8,21 @@ def has_permission(self, request, view):
if not (request.user and request.user.is_authenticated):
return False

# always return True:
# for retrieve, update, partial_update, the permission will be checked on the
# object level (in the next step), list and create is allowed for every user since
# the filtering is done in the queryset
return True
if view.detail:
# for retrieve, update, partial_update, the permission will be checked on the
MyPyDavid marked this conversation as resolved.
Show resolved Hide resolved
# object level (in the next step)
return True

if view.action == 'list':
# list is allowed for every user since the filtering is done in the queryset
return True

if 'create' in view.action_map.values():
# for create, check the permission (from rules.py),
# but only if it is not a ReadOnlyValueSet (i.e. only for ProjectViewSet)
return super().has_permission(request, view)
else:
return True

@log_result
def has_object_permission(self, request, view, obj):
Expand Down Expand Up @@ -64,12 +74,19 @@ def get_required_object_permissions(self, method, model_cls):
return ('projects.view_page_object', )


class HasProjectProgressPermission(HasProjectPermission):
class HasProjectProgressModelPermission(HasModelPermission):

def get_required_permissions(self, method, model_cls):
if method == 'POST':
return ('projects.change_project', )
else:
return ('projects.view_project', )


class HasProjectProgressObjectPermission(HasProjectPermission):

def get_required_object_permissions(self, method, model_cls):
if method == 'GET':
return ('projects.view_project_object', )
elif method == 'POST':
if method == 'POST':
return ('projects.change_project_progress_object', )
else:
raise RuntimeError('Unsupported method for HasProjectProgressPermission')
return ('projects.view_project_object', )
14 changes: 13 additions & 1 deletion rdmo/projects/rules.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,21 @@
from django.conf import settings
from django.contrib.sites.shortcuts import get_current_site

import rules
from rules.predicates import is_superuser


@rules.predicate
def can_add_project(user):
MyPyDavid marked this conversation as resolved.
Show resolved Hide resolved
if not settings.PROJECT_CREATE_RESTRICTED:
return True

if settings.PROJECT_CREATE_GROUPS:
return user.groups.filter(name__in=settings.PROJECT_CREATE_GROUPS).exists()
else:
return False


@rules.predicate
def is_project_member(user, project):
return user in project.member or (project.parent and is_project_member(user, project.parent))
Expand Down Expand Up @@ -54,7 +66,7 @@ def is_site_manager_for_current_site(user, request):
# Add rule for check in template
rules.add_rule('projects.can_view_all_projects', is_site_manager_for_current_site | is_superuser)


rules.add_perm('projects.add_project', can_add_project)
rules.add_perm('projects.view_project_object', is_project_member | is_site_manager)
rules.add_perm('projects.change_project_object', is_project_manager | is_project_owner | is_site_manager)
rules.add_perm('projects.change_project_progress_object', is_project_author | is_project_manager | is_project_owner | is_site_manager) # noqa: E501
Expand Down
12 changes: 11 additions & 1 deletion rdmo/projects/templates/projects/projects.html
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,13 @@
{% endblock %}

{% block sidebar %}
{% has_perm 'projects.add_project' request.user as can_add_project %}
{% test_rule 'projects.can_view_all_projects' request.user request as can_view_all_projects %}

{% if can_add_project or can_view_all_projects %}
<h2>{% trans 'Options' %}</h2>

{% if can_add_project %}
<ul class="list-unstyled">
<li>
<strong>
Expand All @@ -33,8 +38,8 @@ <h2>{% trans 'Options' %}</h2>
</strong>
</li>
</ul>
{% endif %}

{% test_rule 'projects.can_view_all_projects' request.user request as can_view_all_projects %}
{% if can_view_all_projects %}
<ul class="list-unstyled">
<li>
Expand All @@ -44,6 +49,7 @@ <h2>{% trans 'Options' %}</h2>
</li>
</ul>
{% endif %}
{% endif %}

<h2>{% trans 'Filter projects' %}</h2>

Expand All @@ -62,6 +68,8 @@ <h2>{% trans 'Filter projects' %}</h2>
</p>
</form>

{% if can_add_project %}

<h2>{% trans 'Import existing project' %}</h2>

<ul class="list-unstyled">
Expand Down Expand Up @@ -91,6 +99,8 @@ <h2>{% trans 'Import existing project' %}</h2>
{% endif %}
</ul>

{% endif %}

{% if invites %}

<h2>{% trans 'Pending invitations' %}</h2>
Expand Down
65 changes: 65 additions & 0 deletions rdmo/projects/tests/test_view_project.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import pytest

from django.contrib.auth.models import Group, User
from django.urls import reverse

from pytest_django.asserts import assertContains, assertNotContains, assertTemplateUsed
Expand Down Expand Up @@ -145,6 +146,33 @@ def test_project_create_get(db, client, username, password):
assert response.status_code == 302


def test_project_create_restricted_get(db, client, settings):
settings.PROJECT_CREATE_RESTRICTED = True
settings.PROJECT_CREATE_GROUPS = ['projects']

group = Group.objects.create(name='projects')
user = User.objects.get(username='user')
user.groups.add(group)

client.login(username='user', password='user')

url = reverse('project_create')
response = client.get(url)

assert response.status_code == 200


def test_project_create_forbidden_get(db, client, settings):
settings.PROJECT_CREATE_RESTRICTED = True

client.login(username='user', password='user')

url = reverse('project_create')
response = client.get(url)

assert response.status_code == 403


@pytest.mark.parametrize('username,password', users)
def test_project_create_get_for_extra_users_and_unavailable_catalogs(db, client, username, password):
client.login(username=username, password=password)
Expand Down Expand Up @@ -215,6 +243,43 @@ def test_project_create_post(db, client, username, password):
assert Project.objects.count() == project_count


def test_project_create_post_restricted(db, client, settings):
settings.PROJECT_CREATE_RESTRICTED = True
settings.PROJECT_CREATE_GROUPS = ['projects']

group = Group.objects.create(name='projects')
user = User.objects.get(username='user')
user.groups.add(group)

client.login(username='user', password='user')

url = reverse('project_create')
data = {
'title': 'A new project',
'description': 'Some description',
'catalog': catalog_id
}
response = client.post(url, data)

assert response.status_code == 302


def test_project_create_post_forbidden(db, client, settings):
settings.PROJECT_CREATE_RESTRICTED = True

client.login(username='user', password='user')

url = reverse('project_create')
data = {
'title': 'A new project',
'description': 'Some description',
'catalog': catalog_id
}
response = client.post(url, data)

assert response.status_code == 403


@pytest.mark.parametrize('username,password', users)
def test_project_create_parent_post(db, client, username, password):
client.login(username=username, password=password)
Expand Down
61 changes: 61 additions & 0 deletions rdmo/projects/tests/test_view_project_create_import.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import pytest

from django.contrib.auth.models import Group, User
from django.urls import reverse

from rdmo.core.constants import VALUE_TYPE_FILE
Expand Down Expand Up @@ -43,6 +44,31 @@ def test_project_create_import_get(db, client, username, password):
assert response.url.startswith('/account/login/')


def test_project_create_import_get_restricted(db, client, settings):
settings.PROJECT_CREATE_RESTRICTED = True
settings.PROJECT_CREATE_GROUPS = ['projects']

group = Group.objects.create(name='projects')
user = User.objects.get(username='user')
user.groups.add(group)

client.login(username='user', password='user')

url = reverse('project_create_import')
response = client.get(url)
assert response.status_code == 400


def test_project_create_import_get_forbidden(db, client, settings):
settings.PROJECT_CREATE_RESTRICTED = True

client.login(username='user', password='user')

url = reverse('project_create_import')
response = client.get(url)
assert response.status_code == 403


@pytest.mark.parametrize('username,password', users)
def test_project_create_import_post_empty(db, settings, client, username, password):
client.login(username=username, password=password)
Expand Down Expand Up @@ -122,6 +148,41 @@ def test_project_create_import_post_upload_file_empty(db, client, username, pass
assert response.url.startswith('/account/login/')


def test_project_create_import_post_upload_file_restricted(db, client, settings):
settings.PROJECT_CREATE_RESTRICTED = True
settings.PROJECT_CREATE_GROUPS = ['projects']

group = Group.objects.create(name='projects')
user = User.objects.get(username='user')
user.groups.add(group)

client.login(username='user', password='user')

url = reverse('project_create_import')
xml_file = os.path.join(settings.BASE_DIR, 'xml', 'project.xml')
with open(xml_file, encoding='utf8') as f:
response = client.post(url, {
'method': 'upload_file',
'uploaded_file': f
})
assert response.status_code == 302


def test_project_create_import_post_upload_file_forbidden(db, client, settings):
settings.PROJECT_CREATE_RESTRICTED = True

client.login(username='user', password='user')

url = reverse('project_create_import')
xml_file = os.path.join(settings.BASE_DIR, 'xml', 'project.xml')
with open(xml_file, encoding='utf8') as f:
response = client.post(url, {
'method': 'upload_file',
'uploaded_file': f
})
assert response.status_code == 403


@pytest.mark.parametrize('username,password', users)
def test_project_create_import_post_import_file(db, settings, client, files, username, password):
client.login(username=username, password=password)
Expand Down
Loading
Loading