From ae7618f30ac49b6c06f1b5210527912ed1b4741e Mon Sep 17 00:00:00 2001 From: Jussi Arpalahti Date: Wed, 4 Dec 2019 14:28:34 +0200 Subject: [PATCH 1/2] Can bypass payment when making a reservation (#651) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Update OpenAPI spec (#647) * Update OpenAPI spec Update OpenAPI specification to include test server url * fix url typo * RESPA-177 | Enable Unit Admins and Unit Managers to bypass payment (#649) * Add permissions for unit manager * Make changes according to review * Create can_bypass_payments permission * Minor changes to payment bypass and add tests * Add documentation for can_bypass_payment * Add translations * Fill in tests for manager level as well * Change user permissions test to actually test ignoring payment --- docs/permissions.rst | 43 ++++--- locale/fi/LC_MESSAGES/django.po | 3 + locale/sv/LC_MESSAGES/django.po | 3 + openapi.yaml | 3 +- payments/api/reservation.py | 10 +- payments/tests/test_reservation_api.py | 28 +++++ resources/api/reservation.py | 5 +- resources/api/resource.py | 1 + .../0083_create_can_bypass_payment_perm.py | 21 ++++ resources/models/permissions.py | 1 + resources/models/resource.py | 27 ++++- resources/tests/test_reservation_api.py | 111 ++++++++++++++++++ resources/tests/test_resource_api.py | 58 +++++++-- 13 files changed, 282 insertions(+), 32 deletions(-) create mode 100644 resources/migrations/0083_create_can_bypass_payment_perm.py diff --git a/docs/permissions.rst b/docs/permissions.rst index b5fecfa8b..08892d915 100644 --- a/docs/permissions.rst +++ b/docs/permissions.rst @@ -121,6 +121,9 @@ can_view_reservation_access_code can_view_reservation_extra_fields Can view reservation extra fields +can_bypass_payment + Can bypass payment when making a reservation + can_access_reservation_comments Can access reservation comments @@ -149,22 +152,30 @@ with the scope of authorization and the authorized roles. General Administrator role is not bound to any Unit or Unit Group and so their permissions are unscoped. -================================ ============ ====== ======= ====== ====== -**Permission** **Scope** **GA** **UGA** **UA** **UM** --------------------------------- ------------ ------ ------- ------ ------ -can_login_to_respa_admin General X X X X -can_modify_resources Unit X X X X -can_modify_unit Unit X X X X -can_access_permissions_view General X X X -can_search_users General X X X -can_manage_resource_perms Unit X X X -can_manage_auth_of_unit Unit X X X -can_create_resource_to_unit Unit X X X -can_delete_resource_of_unit Unit X X X -can_manage_auth_of_unit_group Unit Group X X -can_create_unit_to_group Unit Group X X -can_delete_unit_of_group Unit Group X X -================================ ============ ====== ======= ====== ====== +====================================== ============ ====== ======= ====== ====== +**Permission** **Scope** **GA** **UGA** **UA** **UM** +-------------------------------------- ------------ ------ ------- ------ ------ +can_login_to_respa_admin General X X X X +can_modify_resources Unit X X X X +can_modify_unit Unit X X X X +can_bypass_payment Unit X X X X +can_access_permissions_view General X X X +can_search_users General X X X +can_manage_resource_perms Unit X X X +can_manage_auth_of_unit Unit X X X +can_create_resource_to_unit Unit X X X +can_delete_resource_of_unit Unit X X X +can_make_reservations Unit X +can_modify_reservations Unit X +can_ignore_opening_hours Unit X +can_view_reservation_access_code Unit X +can_view_reservation_extra_fields Unit X +can_access_reservation_comments Unit X +can_view_reservation_catering_orders Unit X +can_manage_auth_of_unit_group Unit Group X X +can_create_unit_to_group Unit Group X X +can_delete_unit_of_group Unit Group X X +====================================== ============ ====== ======= ====== ====== Definitions of the permissions: diff --git a/locale/fi/LC_MESSAGES/django.po b/locale/fi/LC_MESSAGES/django.po index 3f822fdc5..8363e7980 100644 --- a/locale/fi/LC_MESSAGES/django.po +++ b/locale/fi/LC_MESSAGES/django.po @@ -718,6 +718,9 @@ msgstr "Voi nähdä varauksen tuotetilaukset" msgid "Can modify paid reservations" msgstr "Voi muokata maksettuja varauksia" +msgid "Can bypass payment for paid reservations" +msgstr "Voi ohittaa varauksen maksamisen" + msgid "created" msgstr "luoja" diff --git a/locale/sv/LC_MESSAGES/django.po b/locale/sv/LC_MESSAGES/django.po index cb50e2e73..ea2835049 100644 --- a/locale/sv/LC_MESSAGES/django.po +++ b/locale/sv/LC_MESSAGES/django.po @@ -803,6 +803,9 @@ msgstr "Kan visa reservationer catering beställningar" msgid "Can modify paid reservations" msgstr "Kan modifiera reservationer" +msgid "Can bypass payment for paid reservations" +msgstr "Kan åsidosätta betalningen av bokningen" + msgid "created" msgstr "skapad" diff --git a/openapi.yaml b/openapi.yaml index 89bb4d57f..184395b80 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -3,9 +3,10 @@ info: title: Respa description: The Respa API provides categorized data on resources available for reservation within a city or metropolitan area and enables the reservation of these resources. The API provides data in the JSON format, in a RESTful fashion. - version: 1.5.1 + version: 1.6.1 servers: - url: https://api.hel.fi/respa/v1 +- url: https://respa.koe.hel.ninja/v1 tags: - name: resource description: Look for available resources diff --git a/payments/api/reservation.py b/payments/api/reservation.py index 3de3eaa25..986f39d9f 100644 --- a/payments/api/reservation.py +++ b/payments/api/reservation.py @@ -89,8 +89,16 @@ def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) if self.context['view'].action == 'create': + request = self.context.get('request') resource = self.context.get('resource') - order_required = resource.has_rent() if resource else True + + if resource and request: + order_required = resource.has_rent() and not resource.can_bypass_payment(request.user) + elif resource: + order_required = resource.has_rent() + else: + order_required = True + self.fields['order'] = ReservationEndpointOrderSerializer(required=order_required) elif 'order_detail' in self.context['includes']: self.fields['order'] = ReservationEndpointOrderSerializer(read_only=True) diff --git a/payments/tests/test_reservation_api.py b/payments/tests/test_reservation_api.py index 2045d2acf..ab7ea96cf 100644 --- a/payments/tests/test_reservation_api.py +++ b/payments/tests/test_reservation_api.py @@ -5,7 +5,9 @@ from guardian.shortcuts import assign_perm from rest_framework.reverse import reverse +from resources.enums import UnitAuthorizationLevel from resources.models import Reservation +from resources.models.unit import UnitAuthorization from resources.tests.conftest import resource_in_unit, user_api_client # noqa from resources.tests.test_reservation_api import day_and_period # noqa @@ -290,3 +292,29 @@ def test_order_must_include_rent_if_one_exists(user_api_client, resource_in_unit response = user_api_client.post(LIST_URL, reservation_data) assert response.status_code == 400 + + +def test_unit_admin_and_unit_manager_may_bypass_payment(user_api_client, resource_in_unit, user): + reservation_data = build_reservation_data(resource_in_unit) + ProductFactory(type=Product.RENT, resources=[resource_in_unit]) + + # Order required for normal user + response = user_api_client.post(LIST_URL, reservation_data) + assert response.status_code == 400 + assert 'order' in response.data + + # Order not required for admin user + UnitAuthorization.objects.create(subject=resource_in_unit.unit, level=UnitAuthorizationLevel.admin, authorized=user) + response = user_api_client.post(LIST_URL, reservation_data) + assert response.status_code == 201 + new_reservation = Reservation.objects.last() + assert new_reservation.state == Reservation.CONFIRMED + UnitAuthorization.objects.all().delete() + Reservation.objects.all().delete() + + # Order not required for manager user + UnitAuthorization.objects.create(subject=resource_in_unit.unit, level=UnitAuthorizationLevel.manager, authorized=user) + response = user_api_client.post(LIST_URL, reservation_data) + assert response.status_code == 201 + new_reservation = Reservation.objects.last() + assert new_reservation.state == Reservation.CONFIRMED \ No newline at end of file diff --git a/resources/api/reservation.py b/resources/api/reservation.py index 69ea3a6bd..132245524 100644 --- a/resources/api/reservation.py +++ b/resources/api/reservation.py @@ -273,9 +273,10 @@ def to_representation(self, instance): 'created_at': instance.created_at }) - # Show the comments field and the user object only for staff - if not resource.is_admin(user): + if not resource.is_admin(user) and not resource.can_access_reservation_comments(user): del data['comments'] + + if not resource.is_admin(user): del data['user'] if instance.are_extra_fields_visible(user): diff --git a/resources/api/resource.py b/resources/api/resource.py index 312f6383f..74b35cd6b 100644 --- a/resources/api/resource.py +++ b/resources/api/resource.py @@ -206,6 +206,7 @@ def get_user_permissions(self, obj): 'can_make_reservations': obj.can_make_reservations(request.user) if request else False, 'can_ignore_opening_hours': obj.can_ignore_opening_hours(request.user) if request else False, 'is_admin': obj.is_admin(request.user) if request else False, + 'can_bypass_payment': obj.can_bypass_payment(request.user) if request else False, } def get_is_favorite(self, obj): diff --git a/resources/migrations/0083_create_can_bypass_payment_perm.py b/resources/migrations/0083_create_can_bypass_payment_perm.py new file mode 100644 index 000000000..2113f089d --- /dev/null +++ b/resources/migrations/0083_create_can_bypass_payment_perm.py @@ -0,0 +1,21 @@ +# Generated by Django 2.2.5 on 2019-11-21 07:39 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('resources', '0082_unit_data_sources'), + ] + + operations = [ + migrations.AlterModelOptions( + name='resourcegroup', + options={'ordering': ('name',), 'permissions': [('group:can_approve_reservation', 'Can approve reservation'), ('group:can_make_reservations', 'Can make reservations'), ('group:can_modify_reservations', 'Can modify reservations'), ('group:can_ignore_opening_hours', 'Can make reservations outside opening hours'), ('group:can_view_reservation_access_code', 'Can view reservation access code'), ('group:can_view_reservation_extra_fields', 'Can view reservation extra fields'), ('group:can_access_reservation_comments', 'Can access reservation comments'), ('group:can_view_reservation_catering_orders', 'Can view reservation catering orders'), ('group:can_modify_reservation_catering_orders', 'Can modify reservation catering orders'), ('group:can_view_reservation_product_orders', 'Can view reservation product orders'), ('group:can_modify_paid_reservations', 'Can modify paid reservations'), ('group:can_bypass_payment', 'Can bypass payment for paid reservations')], 'verbose_name': 'Resource group', 'verbose_name_plural': 'Resource groups'}, + ), + migrations.AlterModelOptions( + name='unit', + options={'ordering': ('name',), 'permissions': [('unit:can_approve_reservation', 'Can approve reservation'), ('unit:can_make_reservations', 'Can make reservations'), ('unit:can_modify_reservations', 'Can modify reservations'), ('unit:can_ignore_opening_hours', 'Can make reservations outside opening hours'), ('unit:can_view_reservation_access_code', 'Can view reservation access code'), ('unit:can_view_reservation_extra_fields', 'Can view reservation extra fields'), ('unit:can_access_reservation_comments', 'Can access reservation comments'), ('unit:can_view_reservation_catering_orders', 'Can view reservation catering orders'), ('unit:can_modify_reservation_catering_orders', 'Can modify reservation catering orders'), ('unit:can_view_reservation_product_orders', 'Can view reservation product orders'), ('unit:can_modify_paid_reservations', 'Can modify paid reservations'), ('unit:can_bypass_payment', 'Can bypass payment for paid reservations')], 'verbose_name': 'unit', 'verbose_name_plural': 'units'}, + ), + ] diff --git a/resources/models/permissions.py b/resources/models/permissions.py index 5432e457a..7bc5810f2 100644 --- a/resources/models/permissions.py +++ b/resources/models/permissions.py @@ -12,6 +12,7 @@ ('can_modify_reservation_catering_orders', _('Can modify reservation catering orders')), ('can_view_reservation_product_orders', _('Can view reservation product orders')), ('can_modify_paid_reservations', _('Can modify paid reservations')), + ('can_bypass_payment', _('Can bypass payment for paid reservations')), ) UNIT_PERMISSIONS = [ diff --git a/resources/models/resource.py b/resources/models/resource.py index b80191f61..7ce836505 100644 --- a/resources/models/resource.py +++ b/resources/models/resource.py @@ -299,10 +299,10 @@ def validate_reservation_period(self, reservation, user, data=None): if begin.date() != end.date(): raise ValidationError(_("You cannot make a multi day reservation")) - opening_hours = self.get_opening_hours(begin.date(), end.date()) - days = opening_hours.get(begin.date(), None) - if days is None or not any(day['opens'] and begin >= day['opens'] and end <= day['closes'] for day in days): - if not self._has_perm(user, 'can_ignore_opening_hours'): + if not self.can_ignore_opening_hours(user): + opening_hours = self.get_opening_hours(begin.date(), end.date()) + days = opening_hours.get(begin.date(), None) + if days is None or not any(day['opens'] and begin >= day['opens'] and end <= day['closes'] for day in days): raise ValidationError(_("You must start and end the reservation during opening hours")) if self.max_period and (end - begin) > self.max_period: @@ -559,21 +559,33 @@ def get_users_with_perm(self, perm): return users def can_make_reservations(self, user): + if self.is_manager(user): + return True return self.reservable or self._has_perm(user, 'can_make_reservations') def can_modify_reservations(self, user): + if self.is_manager(user): + return True return self._has_perm(user, 'can_modify_reservations') def can_ignore_opening_hours(self, user): + if self.is_manager(user): + return True return self._has_perm(user, 'can_ignore_opening_hours') def can_view_reservation_extra_fields(self, user): + if self.is_manager(user): + return True return self._has_perm(user, 'can_view_reservation_extra_fields') def can_access_reservation_comments(self, user): + if self.is_manager(user): + return True return self._has_perm(user, 'can_access_reservation_comments') def can_view_catering_orders(self, user): + if self.is_manager(user): + return True return self._has_perm(user, 'can_view_reservation_catering_orders') def can_modify_catering_orders(self, user): @@ -589,8 +601,15 @@ def can_approve_reservations(self, user): return self._has_perm(user, 'can_approve_reservation', allow_admin=False) def can_view_access_codes(self, user): + if self.is_manager(user): + return True return self._has_perm(user, 'can_view_reservation_access_code') + def can_bypass_payment(self, user): + if self.is_manager(user) or self.is_admin(user): + return True + return self._has_perm(user, 'can_bypass_payment') + def is_access_code_enabled(self): return self.access_code_type != Resource.ACCESS_CODE_TYPE_NONE diff --git a/resources/tests/test_reservation_api.py b/resources/tests/test_reservation_api.py index a2c2dd75e..3d26a82bb 100644 --- a/resources/tests/test_reservation_api.py +++ b/resources/tests/test_reservation_api.py @@ -2065,3 +2065,114 @@ def test_reservation_cannot_add_bogus_type(resource_in_unit, reservation_data, a reservation_data['type'] = 'foobar' response = api_client.post(list_url, data=reservation_data) assert response.status_code == 400 + + +@pytest.mark.django_db +def test_manager_can_make_reservations( + resource_in_unit, + resource_in_unit2, + api_client, unit_manager_user, + reservation_data, + list_url +): + # unit_manager_user is manager of resource_in_unit + resource_in_unit.reservable = False + resource_in_unit.save() + api_client.force_authenticate(user=unit_manager_user) + response = api_client.post(list_url, data=reservation_data) + assert response.status_code == 201 + + # unit_manager_user is NOT manager of resource_in_unit2 + resource_in_unit2.reservable = False + resource_in_unit2.save() + reservation_data['resource'] = resource_in_unit2.pk + response = api_client.post(list_url, data=reservation_data) + assert response.status_code == 403 + + +@pytest.mark.django_db +def test_manager_can_modify_reservations(resource_in_unit, resource_in_unit2, api_client, unit_manager_user, reservation_data, list_url, detail_url): + # unit_manager_user is manager of resource_in_unit (resource of reservation_data) + api_client.force_authenticate(user=unit_manager_user) + response = api_client.put(detail_url, data=reservation_data) + assert response.status_code == 200 + + # unit_manager_user is NOT manager of resource_in_unit2 + reservation_data['resource'] = resource_in_unit2.pk + response = api_client.put(detail_url, data=reservation_data) + assert response.status_code == 400 + + +@freeze_time('2115-04-02') +@pytest.mark.django_db +def test_manager_can_ignore_opening_hours(api_client, list_url, reservation_data, unit_manager_user, user): + # regular user should not be aple to ignore opening hours + api_client.force_authenticate(user=user) + reservation_data['begin'] = '2115-04-04T06:00:00+02:00' + reservation_data['end'] = '2115-04-04T08:00:00+02:00' + response = api_client.post(list_url, data=reservation_data, HTTP_ACCEPT_LANGUAGE='en') + assert response.status_code == 400 + + api_client.force_authenticate(user=unit_manager_user) + reservation_data['begin'] = '2115-04-04T06:00:00+02:00' + reservation_data['end'] = '2115-04-04T08:00:00+02:00' + response = api_client.post(list_url, data=reservation_data, HTTP_ACCEPT_LANGUAGE='en') + assert response.status_code == 201 + + +@pytest.mark.django_db +def test_manager_can_view_reservation_access_code(api_client, resource_in_unit, reservation, unit_manager_user): + resource_in_unit.access_code_type = Resource.ACCESS_CODE_TYPE_PIN6 + resource_in_unit.save() + reservation.access_code = '123456' + reservation.save() + detail_url = reverse('reservation-detail', kwargs={'pk': reservation.pk}) + + api_client.force_authenticate(user=unit_manager_user) + response = api_client.get(detail_url) + assert response.status_code == 200 + assert response.data['access_code'] == '123456' + + +@pytest.mark.django_db +def test_manager_can_view_reservation_extra_fields(api_client, resource_in_unit, reservation, unit_manager_user): + resource_in_unit.reservation_metadata_set = ReservationMetadataSet.objects.get(name='default') + resource_in_unit.save() + detail_url = reverse('reservation-detail', kwargs={'pk': reservation.pk}) + api_client.force_authenticate(user=unit_manager_user) + + response = api_client.get(detail_url) + assert response.status_code == 200 + for field_name in DEFAULT_RESERVATION_EXTRA_FIELDS: + assert (field_name in response.data) is True + + +@pytest.mark.django_db +def test_manager_can_access_reservation_comments(api_client, resource_in_unit, reservation, unit_manager_user): + reservation.comments = 'To be a foo or not to be a foo, that is the question' + reservation.save() + detail_url = reverse('reservation-detail', kwargs={'pk': reservation.pk}) + api_client.force_authenticate(user=unit_manager_user) + + response = api_client.get(detail_url) + assert response.status_code == 200 + assert response.data['comments'] == 'To be a foo or not to be a foo, that is the question' + + +@pytest.mark.django_db +def test_manager_can_view_reservation_catering_orders(api_client, reservation, unit_manager_user): + provider = CateringProvider.objects.create( + name='Ihan Hyva Catering Oy', + price_list_url_fi='www.ihanhyva.foobar/hinnasto/', + ) + CateringOrder.objects.create( + reservation=reservation, + provider=provider + ) + + detail_url = reverse('reservation-detail', kwargs={'pk': reservation.pk}) + api_client.force_authenticate(user=unit_manager_user) + + response = api_client.get(detail_url) + assert response.status_code == 200 + assert response.data['has_catering_order'] == True diff --git a/resources/tests/test_resource_api.py b/resources/tests/test_resource_api.py index c312cd2d7..4c3544bbc 100644 --- a/resources/tests/test_resource_api.py +++ b/resources/tests/test_resource_api.py @@ -29,7 +29,7 @@ def detail_url(resource_in_unit): def _check_permissions_dict(api_client, resource, is_admin, can_make_reservations, - can_ignore_opening_hours): + can_ignore_opening_hours, can_bypass_payment): """ Check that user permissions returned from resource endpoint contain correct values for given user and resource. api_client should have the user authenticated. @@ -39,10 +39,11 @@ def _check_permissions_dict(api_client, resource, is_admin, can_make_reservation response = api_client.get(url) assert response.status_code == 200 permissions = response.data['user_permissions'] - assert len(permissions) == 3 + assert len(permissions) == 4 assert permissions['is_admin'] == is_admin assert permissions['can_make_reservations'] == can_make_reservations assert permissions['can_ignore_opening_hours'] == can_ignore_opening_hours + assert permissions['can_bypass_payment'] == can_bypass_payment @pytest.mark.django_db @@ -62,20 +63,23 @@ def test_user_permissions_in_resource_endpoint(api_client, resource_in_unit, use # normal user, reservable = True _check_permissions_dict(api_client, resource_in_unit, is_admin=False, - can_make_reservations=True, can_ignore_opening_hours=False) + can_make_reservations=True, can_ignore_opening_hours=False, + can_bypass_payment=False) # normal user, reservable = False resource_in_unit.reservable = False resource_in_unit.save() _check_permissions_dict(api_client, resource_in_unit, is_admin=False, - can_make_reservations=False, can_ignore_opening_hours=False) + can_make_reservations=False, can_ignore_opening_hours=False, + can_bypass_payment=False) # admin, reservable = False user.is_general_admin = True user.save() api_client.force_authenticate(user=user) _check_permissions_dict(api_client, resource_in_unit, is_admin=True, - can_make_reservations=True, can_ignore_opening_hours=True) + can_make_reservations=True, can_ignore_opening_hours=True, + can_bypass_payment=True) user.is_general_admin = False user.save() @@ -84,19 +88,57 @@ def test_user_permissions_in_resource_endpoint(api_client, resource_in_unit, use assign_perm('unit:can_make_reservations', group, resource_in_unit.unit) api_client.force_authenticate(user=user) _check_permissions_dict(api_client, resource_in_unit, is_admin=False, - can_make_reservations=True, can_ignore_opening_hours=False) + can_make_reservations=True, can_ignore_opening_hours=False, + can_bypass_payment=False) remove_perm('unit:can_make_reservations', group, resource_in_unit.unit) resource_group = resource_in_unit.groups.create(name='rg1') assign_perm('group:can_make_reservations', group, resource_group) api_client.force_authenticate(user=user) _check_permissions_dict(api_client, resource_in_unit, is_admin=False, - can_make_reservations=True, can_ignore_opening_hours=False) + can_make_reservations=True, can_ignore_opening_hours=False, + can_bypass_payment=False) assign_perm('unit:can_ignore_opening_hours', group, resource_in_unit.unit) api_client.force_authenticate(user=user) _check_permissions_dict(api_client, resource_in_unit, is_admin=False, - can_make_reservations=True, can_ignore_opening_hours=True) + can_make_reservations=True, can_ignore_opening_hours=True, + can_bypass_payment=False) + + # user has explicit permission to bypass payment + assign_perm('unit:can_bypass_payment', group, resource_in_unit.unit) + api_client.force_authenticate(user=user) + _check_permissions_dict(api_client, resource_in_unit, is_admin=False, + can_make_reservations=True, can_ignore_opening_hours=True, + can_bypass_payment=True) + remove_perm('unit:can_bypass_payment', group, resource_in_unit.unit) + + # unit admins can ignore opening hours + user.is_general_admin = False + user.save() + user.unit_authorizations.create( + authorized=user, + level=UnitAuthorizationLevel.admin, + subject=resource_in_unit.unit + ) + user.save() + api_client.force_authenticate(user=user) + _check_permissions_dict(api_client, resource_in_unit, is_admin=True, + can_make_reservations=True, can_ignore_opening_hours=True, + can_bypass_payment=True) + user.unit_authorizations.all().delete() + + # unit managers can ignore opening hours + user.unit_authorizations.create( + authorized=user, + level=UnitAuthorizationLevel.manager, + subject=resource_in_unit.unit + ) + user.save() + api_client.force_authenticate(user=user) + _check_permissions_dict(api_client, resource_in_unit, is_admin=False, + can_make_reservations=True, can_ignore_opening_hours=True, + can_bypass_payment=True) @pytest.mark.django_db From 1be1dc8ecdf662380ce9ce2225ca2c909f107414 Mon Sep 17 00:00:00 2001 From: tommimanbytes <56429082+tommimanbytes@users.noreply.github.com> Date: Fri, 14 Feb 2020 12:46:11 +0200 Subject: [PATCH 2/2] Release/v0.9.1 (#681) * Some fixes and additions to permissions improvements (#679) * Fixed bug where anonymizing users didn't anonymize data in related SocialAccount and EmailAddress models causing errors (#680) --- docs/permissions.rst | 16 ++++ resources/api/reservation.py | 22 +++-- .../0086_more_additions_to_permissions.py | 21 +++++ resources/models/permissions.py | 23 +++++ resources/models/resource.py | 12 +++ resources/tests/conftest.py | 15 ++++ resources/tests/test_admin_actions.py | 8 ++ resources/tests/test_reservation_api.py | 86 +++++++++++++++++++ users/admin.py | 4 + 9 files changed, 195 insertions(+), 12 deletions(-) create mode 100644 resources/migrations/0086_more_additions_to_permissions.py diff --git a/docs/permissions.rst b/docs/permissions.rst index bd2c71204..e99d8f2fe 100644 --- a/docs/permissions.rst +++ b/docs/permissions.rst @@ -133,6 +133,10 @@ can_modify_reservation_catering_orders can_view_reservation_product_orders can_modify_paid_reservations can_bypass_payment X X X X +can_create_staff_event X X X X +can_create_special_type_reservation X X X X +can_bypass_manual_confirmation X X X X +can_create_reservations_for_other_users X X X ====================================== ====== ======= ====== ====== ====== @@ -180,6 +184,18 @@ can_modify_paid_reservations can_bypass_payment Can bypass payment when making a reservation +can_create_staff_event + Can create a reservation that is a staff event + +can_create_special_type_reservation + Can create reservations of a non-normal type + +can_bypass_manual_confirmation + Can bypass manual confirmation requirement for resources + +can_create_reservations_for_other_users + Can create reservations for other registered users + Respa Admin Permissions ~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/resources/api/reservation.py b/resources/api/reservation.py index 19f5dc94c..ea4b468e0 100644 --- a/resources/api/reservation.py +++ b/resources/api/reservation.py @@ -108,9 +108,10 @@ def __init__(self, *args, **kwargs): required = resource.get_required_reservation_extra_field_names(cache=cache) # staff events have less requirements + request_user = self.context['request'].user is_staff_event = data.get('staff_event', False) - is_resource_manager = resource.is_manager(self.context['request'].user) - if is_staff_event and is_resource_manager: + + if is_staff_event and resource.can_create_staff_event(request_user): required = {'reserver_name', 'event_description'} # we don't need to remove a field here if it isn't supported, as it will be read-only and will be more @@ -167,11 +168,7 @@ def validate(self, data): if data['end'] < timezone.now(): raise ValidationError(_('You cannot make a reservation in the past')) - is_resource_admin = resource.is_admin(request_user) - is_resource_manager = resource.is_manager(request_user) - is_resource_viewer = resource.is_viewer(request_user) - - if not is_resource_admin: + if not resource.can_ignore_opening_hours(request_user): reservable_before = resource.get_reservable_before() if reservable_before and data['begin'] >= reservable_before: raise ValidationError(_('The resource is reservable only before %(datetime)s' % @@ -182,18 +179,19 @@ def validate(self, data): {'datetime': reservable_after})) # normal users cannot make reservations for other people - if not is_resource_admin: + if not resource.can_create_reservations_for_other_users(request_user): data.pop('user', None) # Check user specific reservation restrictions relating to given period. resource.validate_reservation_period(reservation, request_user, data=data) if data.get('staff_event', False): - if not is_resource_manager: + if not resource.can_create_staff_event(request_user): raise ValidationError(dict(staff_event=_('Only allowed to be set by resource managers'))) if 'type' in data: - if data['type'] != Reservation.TYPE_NORMAL and not (is_resource_admin or is_resource_manager): + if (data['type'] != Reservation.TYPE_NORMAL and + not resource.can_create_special_type_reservation(request_user)): raise ValidationError({'type': _('You are not allowed to make a reservation of this type')}) if 'comments' in data: @@ -631,8 +629,8 @@ def perform_create(self, serializer): instance = serializer.save(**override_data) resource = serializer.validated_data['resource'] - is_resource_manager = resource.is_manager(self.request.user) - if resource.need_manual_confirmation and not is_resource_manager: + + if resource.need_manual_confirmation and not resource.can_bypass_manual_confirmation(self.request.user): new_state = Reservation.REQUESTED else: if instance.get_order(): diff --git a/resources/migrations/0086_more_additions_to_permissions.py b/resources/migrations/0086_more_additions_to_permissions.py new file mode 100644 index 000000000..d7a61c034 --- /dev/null +++ b/resources/migrations/0086_more_additions_to_permissions.py @@ -0,0 +1,21 @@ +# Generated by Django 2.2.9 on 2020-02-13 15:16 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('resources', '0085_additions_to_permissions'), + ] + + operations = [ + migrations.AlterModelOptions( + name='resourcegroup', + options={'ordering': ('name',), 'permissions': [('group:can_approve_reservation', 'Can approve reservation'), ('group:can_make_reservations', 'Can make reservations'), ('group:can_modify_reservations', 'Can modify reservations'), ('group:can_ignore_opening_hours', 'Can make reservations outside opening hours'), ('group:can_view_reservation_access_code', 'Can view reservation access code'), ('group:can_view_reservation_extra_fields', 'Can view reservation extra fields'), ('group:can_view_reservation_user', 'Can view reservation user'), ('group:can_access_reservation_comments', 'Can access reservation comments'), ('group:can_comment_reservations', 'Can create comments for a reservation'), ('group:can_view_reservation_catering_orders', 'Can view reservation catering orders'), ('group:can_modify_reservation_catering_orders', 'Can modify reservation catering orders'), ('group:can_view_reservation_product_orders', 'Can view reservation product orders'), ('group:can_modify_paid_reservations', 'Can modify paid reservations'), ('group:can_bypass_payment', 'Can bypass payment for paid reservations'), ('group:can_create_staff_event', 'Can create a reservation that is a staff event'), ('group:can_create_special_type_reservation', 'Can create reservations of a non-normal type'), ('group:can_bypass_manual_confirmation', 'Can bypass manual confirmation requirement for resources'), ('group:can_create_reservations_for_other_users', 'Can create reservations for other registered users')], 'verbose_name': 'Resource group', 'verbose_name_plural': 'Resource groups'}, + ), + migrations.AlterModelOptions( + name='unit', + options={'ordering': ('name',), 'permissions': [('unit:can_approve_reservation', 'Can approve reservation'), ('unit:can_make_reservations', 'Can make reservations'), ('unit:can_modify_reservations', 'Can modify reservations'), ('unit:can_ignore_opening_hours', 'Can make reservations outside opening hours'), ('unit:can_view_reservation_access_code', 'Can view reservation access code'), ('unit:can_view_reservation_extra_fields', 'Can view reservation extra fields'), ('unit:can_view_reservation_user', 'Can view reservation user'), ('unit:can_access_reservation_comments', 'Can access reservation comments'), ('unit:can_comment_reservations', 'Can create comments for a reservation'), ('unit:can_view_reservation_catering_orders', 'Can view reservation catering orders'), ('unit:can_modify_reservation_catering_orders', 'Can modify reservation catering orders'), ('unit:can_view_reservation_product_orders', 'Can view reservation product orders'), ('unit:can_modify_paid_reservations', 'Can modify paid reservations'), ('unit:can_bypass_payment', 'Can bypass payment for paid reservations'), ('unit:can_create_staff_event', 'Can create a reservation that is a staff event'), ('unit:can_create_special_type_reservation', 'Can create reservations of a non-normal type'), ('unit:can_bypass_manual_confirmation', 'Can bypass manual confirmation requirement for resources'), ('unit:can_create_reservations_for_other_users', 'Can create reservations for other registered users')], 'verbose_name': 'unit', 'verbose_name_plural': 'units'}, + ), + ] diff --git a/resources/models/permissions.py b/resources/models/permissions.py index c05cd0565..514f2a95f 100644 --- a/resources/models/permissions.py +++ b/resources/models/permissions.py @@ -18,6 +18,10 @@ ('can_view_reservation_product_orders', _('Can view reservation product orders')), ('can_modify_paid_reservations', _('Can modify paid reservations')), ('can_bypass_payment', _('Can bypass payment for paid reservations')), + ('can_create_staff_event', _('Can create a reservation that is a staff event')), + ('can_create_special_type_reservation', _('Can create reservations of a non-normal type')), + ('can_bypass_manual_confirmation', _('Can bypass manual confirmation requirement for resources')), + ('can_create_reservations_for_other_users', _('Can create reservations for other registered users')) ) UNIT_ROLE_PERMISSIONS = { @@ -80,6 +84,25 @@ UnitGroupAuthorizationLevel.admin, UnitAuthorizationLevel.admin, UnitAuthorizationLevel.manager + ], + 'can_create_staff_event': [ + UnitGroupAuthorizationLevel.admin, + UnitAuthorizationLevel.admin, + UnitAuthorizationLevel.manager + ], + 'can_create_special_type_reservation': [ + UnitGroupAuthorizationLevel.admin, + UnitAuthorizationLevel.admin, + UnitAuthorizationLevel.manager + ], + 'can_bypass_manual_confirmation': [ + UnitGroupAuthorizationLevel.admin, + UnitAuthorizationLevel.admin, + UnitAuthorizationLevel.manager + ], + 'can_create_reservations_for_other_users': [ + UnitGroupAuthorizationLevel.admin, + UnitAuthorizationLevel.admin ] } diff --git a/resources/models/resource.py b/resources/models/resource.py index c6e24a4a3..55a178618 100644 --- a/resources/models/resource.py +++ b/resources/models/resource.py @@ -636,6 +636,18 @@ def can_view_reservation_access_code(self, user): def can_bypass_payment(self, user): return self._has_perm(user, 'can_bypass_payment') + def can_create_staff_event(self, user): + return self._has_perm(user, 'can_create_staff_event') + + def can_create_special_type_reservation(self, user): + return self._has_perm(user, 'can_create_special_type_reservation') + + def can_bypass_manual_confirmation(self, user): + return self._has_perm(user, 'can_bypass_manual_confirmation') + + def can_create_reservations_for_other_users(self, user): + return self._has_perm(user, 'can_create_reservations_for_other_users') + def is_access_code_enabled(self): return self.access_code_type != Resource.ACCESS_CODE_TYPE_NONE diff --git a/resources/tests/conftest.py b/resources/tests/conftest.py index d040ef267..f7a10694a 100644 --- a/resources/tests/conftest.py +++ b/resources/tests/conftest.py @@ -235,6 +235,21 @@ def staff_user(): ) +@pytest.mark.django_db +@pytest.fixture +def unit_admin_user(resource_in_unit): + user = get_user_model().objects.create( + username='test_admin_user', + first_name='Inspector', + last_name='Lestrade', + email='lestrade@scotlandyard.co.uk', + is_staff=True, + preferred_language='en' + ) + user.unit_authorizations.create(subject=resource_in_unit.unit, level=UnitAuthorizationLevel.admin) + return user + + @pytest.mark.django_db @pytest.fixture def unit_manager_user(resource_in_unit): diff --git a/resources/tests/test_admin_actions.py b/resources/tests/test_admin_actions.py index 0012eae54..0377ab2ea 100644 --- a/resources/tests/test_admin_actions.py +++ b/resources/tests/test_admin_actions.py @@ -3,6 +3,7 @@ from django.contrib.auth import get_user_model from resources.models import Reservation +from allauth.socialaccount.models import SocialAccount, EmailAddress from users.admin import anonymize_user_data @@ -14,7 +15,12 @@ def test_anonymize_user_data(api_client, resource_in_unit, user): user.first_name = 'testi_ukkeli' user.save() original_uuid = user.uuid + original_email = user.email user_pk = user.pk + + SocialAccount.objects.create(user=user, uid=original_uuid, provider='helsinki') + EmailAddress.objects.create(user=user, email=original_email) + Reservation.objects.create( resource=resource_in_unit, begin='2015-04-04T09:00:00+02:00', @@ -32,3 +38,5 @@ def test_anonymize_user_data(api_client, resource_in_unit, user): assert reservation.event_description == 'Sensitive data of this reservation has been anonymized by a script.' changed_user = get_user_model().objects.get(pk=user_pk) assert changed_user.uuid != original_uuid + assert not SocialAccount.objects.filter(user=user, uid=original_uuid).exists() + assert not EmailAddress.objects.filter(user=user, email=original_email).exists() diff --git a/resources/tests/test_reservation_api.py b/resources/tests/test_reservation_api.py index bcd221bb4..f79fc038c 100644 --- a/resources/tests/test_reservation_api.py +++ b/resources/tests/test_reservation_api.py @@ -2247,6 +2247,92 @@ def test_viewer_can_comment_reservations(resource_in_unit, api_client, unit_view assert response.status_code == 200 +@pytest.mark.django_db +def test_admin_can_make_staff_reservation( + resource_in_unit, list_url, reservation_data, unit_admin_user, api_client): + """ + User with admin status on the resource should be able to make staff event reservations. + """ + reservation_data['staff_event'] = True + reservation_data['reserver_name'] = 'herra huu' + reservation_data['event_description'] = 'herra huun bileet' + + api_client.force_authenticate(user=unit_admin_user) + response = api_client.post(list_url, data=reservation_data) + assert response.status_code == 201, "Request failed with: %s" % (str(response.content, 'utf8')) + assert response.data.get('staff_event', False) is True + reservation = Reservation.objects.get(id=response.data['id']) + assert reservation.staff_event is True + + +@pytest.mark.django_db +def test_admin_can_create_special_type_reservation( + resource_in_unit, list_url, reservation_data, unit_admin_user, api_client): + """ + User with admin status on the resource should be able to make special type reservations. + """ + reservation_data['type'] = Reservation.TYPE_BLOCKED + + api_client.force_authenticate(user=unit_admin_user) + response = api_client.post(list_url, data=reservation_data) + + assert response.status_code == 201 + reservation = Reservation.objects.get(id=response.data['id']) + assert reservation.type == Reservation.TYPE_BLOCKED + + +@pytest.mark.django_db +def test_manager_can_create_special_type_reservation( + resource_in_unit, list_url, reservation_data, unit_manager_user, api_client): + """ + User with manager status on the resource should be able to make special type reservations. + """ + reservation_data['type'] = Reservation.TYPE_BLOCKED + + api_client.force_authenticate(user=unit_manager_user) + response = api_client.post(list_url, data=reservation_data) + + assert response.status_code == 201 + reservation = Reservation.objects.get(id=response.data['id']) + assert reservation.type == Reservation.TYPE_BLOCKED + + +@pytest.mark.django_db +def test_admin_can_bypass_manual_confirmation( + resource_in_unit, list_url, reservation_data, unit_admin_user, api_client): + """ + User with admin status on the resource should be able to bypass manual confirmation. + """ + + resource_in_unit.need_manual_confirmation = True + resource_in_unit.save() + + api_client.force_authenticate(user=unit_admin_user) + response = api_client.post(list_url, data=reservation_data) + + assert response.status_code == 201 + reservation = Reservation.objects.get(id=response.data['id']) + assert reservation.state != Reservation.REQUESTED + + +@pytest.mark.django_db +def test_manager_can_bypass_manual_confirmation( + resource_in_unit, list_url, reservation_data, unit_manager_user, api_client): + """ + User with manager status on the resource should be able to bypass manual confirmation. + """ + + resource_in_unit.need_manual_confirmation = True + resource_in_unit.save() + + api_client.force_authenticate(user=unit_manager_user) + response = api_client.post(list_url, data=reservation_data) + + assert response.status_code == 201 + reservation = Reservation.objects.get(id=response.data['id']) + assert reservation.state != Reservation.REQUESTED + + @pytest.mark.django_db def test_query_counts(user_api_client, staff_api_client, list_url, django_assert_max_num_queries): """ diff --git a/users/admin.py b/users/admin.py index bba97475c..700a30ac6 100644 --- a/users/admin.py +++ b/users/admin.py @@ -4,6 +4,7 @@ from django.contrib.auth.admin import UserAdmin as DjangoUserAdmin from django.contrib import admin from resources.models import Reservation +from allauth.socialaccount.models import SocialAccount, EmailAddress User = get_user_model() @@ -78,6 +79,9 @@ def anonymize_user_data(modeladmin, request, queryset): user.uuid = uuid.uuid4() user.save() + SocialAccount.objects.filter(user=user).update(uid=user.uuid, extra_data='{}') + EmailAddress.objects.filter(user=user).update(email=user.email) + user_reservations = Reservation.objects.filter(user=user) user_reservations.update( event_subject='Removed',