From c60c0967c5b00724a0c92d6d4f82b47277aa1ccd Mon Sep 17 00:00:00 2001 From: Renaud Michotte Date: Mon, 15 Jun 2020 14:16:11 +0200 Subject: [PATCH] authorization: create role management API Creates an API to expose which roles could be managed by the current logged user. This commit also introduces a restriction to disallow the current user to delete itself. Co-Authored-by: Renaud Michotte --- rero_ils/modules/patrons/api.py | 31 ++++++++++++- .../jsonschemas/patrons/patron-v0.0.1.json | 6 ++- rero_ils/modules/patrons/permissions.py | 20 +++++++++ rero_ils/modules/patrons/views.py | 10 +++++ tests/api/test_patrons_rest.py | 11 +++-- tests/api/test_permissions_librarian.py | 8 ++++ tests/api/test_permissions_patron.py | 5 +++ tests/api/test_permissions_sys_librarian.py | 7 +++ tests/api/test_record_permissions.py | 6 ++- tests/data/data.json | 20 +++++++++ tests/fixtures/circulation.py | 45 +++++++++++++++++++ tests/ui/patrons/test_patrons_api.py | 28 ++++++++++++ 12 files changed, 190 insertions(+), 7 deletions(-) diff --git a/rero_ils/modules/patrons/api.py b/rero_ils/modules/patrons/api.py index 86ea5d3aad..525a4f0e55 100644 --- a/rero_ils/modules/patrons/api.py +++ b/rero_ils/modules/patrons/api.py @@ -71,11 +71,22 @@ class Meta: class Patron(IlsRecord): """Define API for patrons mixing.""" + ROLE_PATRON = 'patron' + ROLE_LIBRARIAN = 'librarian' + ROLE_SYSTEM_LIBRARIAN = 'system_librarian' + + ROLES_HIERARCHY = { + ROLE_PATRON: [], + ROLE_LIBRARIAN: [], + ROLE_SYSTEM_LIBRARIAN: [ROLE_LIBRARIAN] + } + minter = patron_id_minter fetcher = patron_id_fetcher provider = PatronProvider model_cls = PatronMetadata - available_roles = ['system_librarian', 'librarian', 'patron'] + + available_roles = [ROLE_SYSTEM_LIBRARIAN, ROLE_LIBRARIAN, ROLE_PATRON] def validate(self, **kwargs): """Validate record against schema. @@ -209,6 +220,24 @@ def _remove_roles(self): if role in db_roles: self.remove_role(role) + @classmethod + def get_reachable_roles(cls, role): + """Get list of roles depending on role hierarchy.""" + if role not in Patron.ROLES_HIERARCHY: + return [] + roles = Patron.ROLES_HIERARCHY[role].copy() + roles.append(role) + return roles + + @classmethod + def get_all_roles_for_role(cls, role): + """The list of roles covering given role based on the hierarchy.""" + roles = [role] + for key in Patron.ROLES_HIERARCHY: + if role in Patron.ROLES_HIERARCHY[key] and key not in roles: + roles.append(key) + return roles + @classmethod def get_patron_by_user(cls, user): """Get patron by user.""" diff --git a/rero_ils/modules/patrons/jsonschemas/patrons/patron-v0.0.1.json b/rero_ils/modules/patrons/jsonschemas/patrons/patron-v0.0.1.json index bf5eae178e..485a84f129 100644 --- a/rero_ils/modules/patrons/jsonschemas/patrons/patron-v0.0.1.json +++ b/rero_ils/modules/patrons/jsonschemas/patrons/patron-v0.0.1.json @@ -189,7 +189,8 @@ "hideExpression": "!field.parent.model.roles.some(v => (v === 'librarian' || v === 'system_librarian'))", "expressionProperties": { "templateOptions.required": "field.parent.model.roles.some(v => (v === 'librarian' || v === 'system_librarian'))" - } + }, + "fieldMap": "library" } }, "roles": { @@ -225,6 +226,9 @@ } ] } + }, + "form": { + "fieldMap": "roles" } }, "communication_channel": { diff --git a/rero_ils/modules/patrons/permissions.py b/rero_ils/modules/patrons/permissions.py index a04c8c8a32..79908c0a30 100644 --- a/rero_ils/modules/patrons/permissions.py +++ b/rero_ils/modules/patrons/permissions.py @@ -19,6 +19,7 @@ from flask import request +from .api import Patron from ...permissions import staffer_is_authenticated @@ -58,6 +59,9 @@ def can_delete_patron_factory(record, *args, **kwargs): """ def can(self): patron = staffer_is_authenticated() + # It should be not possible to remove itself ! + if patron and patron.pid == record.pid: + return False if patron and patron.organisation_pid == record.organisation_pid: if patron.is_system_librarian: return True @@ -71,3 +75,19 @@ def can(self): return True return False return type('Check', (), {'can': can})() + + +def get_allowed_roles_management(): + """Get the roles that current logged user could manage. + + :return An array of allowed role management. + """ + allowed_roles = [] + patron = staffer_is_authenticated() + if patron: + if patron.is_librarian: + allowed_roles.append(Patron.ROLE_PATRON) + allowed_roles.append(Patron.ROLE_LIBRARIAN) + if patron.is_system_librarian: + allowed_roles.append(Patron.ROLE_SYSTEM_LIBRARIAN) + return allowed_roles diff --git a/rero_ils/modules/patrons/views.py b/rero_ils/modules/patrons/views.py index 1e410ffe72..908d2ff94f 100644 --- a/rero_ils/modules/patrons/views.py +++ b/rero_ils/modules/patrons/views.py @@ -32,6 +32,7 @@ from werkzeug.exceptions import NotFound from .api import Patron, PatronsSearch +from .permissions import get_allowed_roles_management from .utils import user_has_patron from ..items.api import Item from ..libraries.api import Library @@ -215,6 +216,15 @@ def profile(viewcode): ) +@api_blueprint.route('/roles_management_permissions', methods=['GET']) +@check_permission +def get_roles_management_permissions(): + """Get the roles that current logged user could manage.""" + return jsonify({ + 'allowed_roles': get_allowed_roles_management() + }) + + @blueprint.app_template_filter('get_patron_from_barcode') def get_patron_from_barcode(value): """Get patron from barcode.""" diff --git a/tests/api/test_patrons_rest.py b/tests/api/test_patrons_rest.py index 4b8bc1b723..05c018f07c 100644 --- a/tests/api/test_patrons_rest.py +++ b/tests/api/test_patrons_rest.py @@ -356,10 +356,9 @@ def test_patron_secure_api(client, json_header, # assert res.status_code == 403 -def test_patron_secure_api_create(client, json_header, +def test_patron_secure_api_create(client, patron_martigny_data, - librarian_martigny_no_email, - librarian_sion_no_email): + librarian_martigny_no_email): """Test patron secure api create.""" # Martigny login_user_via_session(client, librarian_martigny_no_email.user) @@ -427,6 +426,12 @@ def test_patron_secure_api_delete(client, json_header, res = client.delete(record_url) assert res.status_code == 204 + # try to delete itself + record_url = url_for('invenio_records_rest.ptrn_item', + pid_value=librarian_martigny_no_email.pid) + res = client.delete(record_url) + assert res.status_code == 403 + # Sion # login_user_via_session(client, librarian_sion_no_email.user) diff --git a/tests/api/test_permissions_librarian.py b/tests/api/test_permissions_librarian.py index 3699c8941d..6eec14141e 100644 --- a/tests/api/test_permissions_librarian.py +++ b/tests/api/test_permissions_librarian.py @@ -70,6 +70,14 @@ def test_librarian_permissions( data = get_json(res) assert data['hits']['total'] == 3 + # can manage all types of patron roles + role_url = url_for('api_patrons.get_roles_management_permissions') + res = client.get(role_url) + assert res.status_code == 200 + data = get_json(res) + assert 'librarian' in data['allowed_roles'] + assert 'system_librarian' not in data['allowed_roles'] + # can create all type of users except system_librarians post_entrypoint = 'invenio_records_rest.ptrn_list' system_librarian = deepcopy(record) diff --git a/tests/api/test_permissions_patron.py b/tests/api/test_permissions_patron.py index eb678c9541..453b2975ac 100644 --- a/tests/api/test_permissions_patron.py +++ b/tests/api/test_permissions_patron.py @@ -50,6 +50,11 @@ def test_patron_permissions( res = client.get(list_url) assert res.status_code == 403 + # can not manage any types of patron roles + role_url = url_for('api_patrons.get_roles_management_permissions') + res = client.get(role_url) + assert res.status_code == 403 + # can not create any type of users. system_librarian = deepcopy(record) librarian = deepcopy(record) diff --git a/tests/api/test_permissions_sys_librarian.py b/tests/api/test_permissions_sys_librarian.py index c5d70d038c..f8843654be 100644 --- a/tests/api/test_permissions_sys_librarian.py +++ b/tests/api/test_permissions_sys_librarian.py @@ -52,6 +52,13 @@ def test_system_librarian_permissions( data = get_json(res) assert data['hits']['total'] == 3 + # can manage all types of patron roles + role_url = url_for('api_patrons.get_roles_management_permissions') + res = client.get(role_url) + assert res.status_code == 200 + data = get_json(res) + assert 'system_librarian' in data['allowed_roles'] + # can create all type of users. system_librarian = deepcopy(record) librarian = deepcopy(record) diff --git a/tests/api/test_record_permissions.py b/tests/api/test_record_permissions.py index a8d4d8e120..43b29a9e8b 100644 --- a/tests/api/test_record_permissions.py +++ b/tests/api/test_record_permissions.py @@ -92,10 +92,11 @@ def test_patrons_permissions( librarian2_martigny_no_email, librarian_saxon_no_email, system_librarian_martigny_no_email, + system_librarian_martigny2_no_email, system_librarian_sion_no_email, librarian_sion_no_email ): - """Test serializers for patrons.""" + """Test permissions for patrons.""" # simple librarian ----------------------------------------------- login_user(client, librarian_martigny_no_email) @@ -130,8 +131,9 @@ def test_patrons_permissions( assert data['update']['can'] # should update and delete a system librarian of the same organisation + # but not itself data = call_api_permissions(client, 'patrons', - system_librarian_martigny_no_email.pid) + system_librarian_martigny2_no_email.pid) assert data['delete']['can'] assert data['update']['can'] diff --git a/tests/data/data.json b/tests/data/data.json index e05634f31d..57af7d8f85 100644 --- a/tests/data/data.json +++ b/tests/data/data.json @@ -2340,6 +2340,26 @@ "blocked": true, "blocked_note": "Lost card" }, + "ptrn12": { + "$schema": "https://ils.rero.ch/schema/patrons/patron-v0.0.1.json", + "pid": "ptrn12", + "first_name": "Joella", + "last_name": "Dosimonta", + "street": "Grand place, 123", + "postal_code": "1920", + "city": "Martigny", + "birth_date": "1980-06-07", + "library": { + "$ref": "https://ils.rero.ch/api/libraries/lib1" + }, + "email": "joellalibri07@gmail.com", + "phone": "+32324993585", + "roles": [ + "system_librarian", + "librarian" + ], + "barcode": "sys_ptrn12" + }, "dummy_notif": { "$schema": "https://ils.rero.ch/schemas/notifications/notification-v0.0.1.json", "pid": "notif1", diff --git a/tests/fixtures/circulation.py b/tests/fixtures/circulation.py index af94b97955..70bf29553c 100644 --- a/tests/fixtures/circulation.py +++ b/tests/fixtures/circulation.py @@ -89,6 +89,51 @@ def system_librarian_martigny_no_email( return ptrn +@pytest.fixture(scope="module") +def system_librarian_martigny2_data(data): + """Load Martigny system librarian data.""" + return deepcopy(data.get('ptrn12')) + + +@pytest.fixture(scope="function") +def system_librarian_martigny2_data_tmp(data): + """Load Martigny system librarian data scope function.""" + return deepcopy(data.get('ptrn12')) + + +@pytest.fixture(scope="module") +def system_librarian_martigny2( + app, + roles, + lib_martigny, + system_librarian_martigny2_data): + """Create Martigny system librarian record.""" + ptrn = Patron.create( + data=system_librarian_martigny2_data, + delete_pid=False, + dbcommit=True, + reindex=True) + flush_index(PatronsSearch.Meta.index) + return ptrn + + +@pytest.fixture(scope="module") +@mock.patch('rero_ils.modules.patrons.api.send_reset_password_instructions') +def system_librarian_martigny2_no_email( + app, + roles, + lib_martigny, + system_librarian_martigny2_data): + """Create Martigny system librarian without sending reset password.""" + ptrn = Patron.create( + data=system_librarian_martigny2_data, + delete_pid=False, + dbcommit=True, + reindex=True) + flush_index(PatronsSearch.Meta.index) + return ptrn + + # ------------ Org: Martigny, Lib: Martigny, Librarian 1 ---------- @pytest.fixture(scope="module") def librarian_martigny_data(data): diff --git a/tests/ui/patrons/test_patrons_api.py b/tests/ui/patrons/test_patrons_api.py index 16dd5eb77e..f0aad1eec6 100644 --- a/tests/ui/patrons/test_patrons_api.py +++ b/tests/ui/patrons/test_patrons_api.py @@ -175,3 +175,31 @@ def test_get_patron_blocked_field_absent(patron2_martigny_no_email): """Test patron blocked field retrieval.""" patron = Patron.get_patron_by_email(patron2_martigny_no_email.get('email')) assert 'blocked' not in patron + + +def test_get_reachable_roles(): + """Test get roles covered by the given role.""" + roles = Patron.get_reachable_roles(Patron.ROLE_SYSTEM_LIBRARIAN) + assert len(roles) == 2 + assert Patron.ROLE_LIBRARIAN in roles + assert Patron.ROLE_SYSTEM_LIBRARIAN in roles + + roles = Patron.get_reachable_roles('unknown_role') + assert not roles + + +def test_get_all_roles_for_role(): + """Test get roles covering by roles hierarchy.""" + roles = Patron.get_all_roles_for_role(Patron.ROLE_PATRON) + assert len(roles) == 1 + assert Patron.ROLE_PATRON in roles + + roles = Patron.get_all_roles_for_role(Patron.ROLE_SYSTEM_LIBRARIAN) + assert len(roles) == 1 + assert Patron.ROLE_SYSTEM_LIBRARIAN in roles + + roles = Patron.get_all_roles_for_role(Patron.ROLE_LIBRARIAN) + assert len(roles) == 2 + assert Patron.ROLE_LIBRARIAN in roles + assert Patron.ROLE_SYSTEM_LIBRARIAN in roles +