diff --git a/CHANGELOG.md b/CHANGELOG.md index c7f54a08..2b6b5691 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -128,6 +128,23 @@ ADR-007 and ADR-002. ### Fixed +- **Global search answered twelve domains behind one permission.** The route is + gated on `assets.view`, and that was the entire check: looking up an asset + number also read out of the staff directory, the USB checkout history, the + notification log and the network inventory, none of which consult their own + permission. The `SEARCH_DOMAINS` settings are site feature toggles, not + authorization, and were never doing that job. Each domain now answers only to + a caller holding its permission - `employees.view` for the directory, + `usb.view` for checkouts, `network.view` for address and subnet matches, + `computers.view` for hostnames. Asset, custom-field and vendor/model results + stay on `assets.view`, since all three emit asset rows. The check is + `caller_holds` rather than `require_permission`: a search that refused the + whole request because one domain was off-limits would be useless, so a domain + the caller cannot see simply contributes nothing. This is also what makes + `assets.view` safe to hand to the guest role again, which is what lets a + signed-out visitor look up an asset number without also getting the staff + directory. + - **The printer supplies card was ordered by printer name, so the emptiest cartridge was not first and sometimes was not shown at all.** Rows sorted on "is critical" and then alphabetically, and critical is everything at or below diff --git a/shopdb/core/api/search.py b/shopdb/core/api/search.py index 11218a9b..3f9d7c3d 100644 --- a/shopdb/core/api/search.py +++ b/shopdb/core/api/search.py @@ -18,7 +18,7 @@ from shopdb.core.models import ( from shopdb.core.api.settings import get_cached_settings from shopdb.core.services.spellfix import Vocabulary from shopdb.utils.responses import success_response -from shopdb.utils.authz import require_permission +from shopdb.utils.authz import require_permission, caller_holds logger = logging.getLogger(__name__) @@ -1233,22 +1233,42 @@ def global_search(): results = [] search_term = f'%{query}%' - # Run all search domains - results.extend(_search_applications(query, search_term)) - results.extend(_search_knowledgebase(query, search_term)) - results.extend(_search_guides(query, search_term)) - results.extend(_search_employees(query, search_term)) - results.extend(_search_assets(query, search_term)) - results.extend(_search_measuringtools(query, search_term)) - results.extend(_search_usbdevices(query, search_term)) - results.extend(_search_printeditems(query, search_term)) - results.extend(_search_customfields(query, search_term)) - results.extend(_search_notifications(query, search_term)) - results.extend(_search_hostnames(query, search_term)) - results.extend(_search_vendor_model_type(query, search_term)) + # Every domain answers only to a caller who holds ITS permission. + # + # The route is gated on assets.view alone, and that used to be the whole + # check: one permission then queried twelve domains, so it read out of the + # staff directory, the USB checkout history, the notification log and the + # network inventory to anybody who could look up an asset number. The + # SEARCH_DOMAINS settings are feature toggles a site sets, not authorization, + # and were never doing this job. + # + # caller_holds rather than require_permission: a search that refuses the + # whole request because one domain is off-limits is useless. Each domain + # simply contributes nothing. + domains = [ + ('assets.view', _search_assets), + # Emit asset results routed to an asset page, so they belong to the + # same grant as the assets themselves. + ('assets.view', _search_customfields), + ('assets.view', _search_vendor_model_type), + ('applications.view', _search_applications), + ('kb.view', _search_knowledgebase), + ('guides.view', _search_guides), + ('employees.view', _search_employees), + ('measuringtools.view', _search_measuringtools), + ('usb.view', _search_usbdevices), + ('printedparts.view', _search_printeditems), + ('notifications.view', _search_notifications), + ('computers.view', _search_hostnames), + ] + for permission, search in domains: + if caller_holds(permission): + results.extend(search(query, search_term)) - # IP-specific searches - if classification['is_ip']: + # IP-specific searches. An address and the subnet it sits in are network + # inventory, and knowing what answers on 10.20.x is a different question + # from looking up an asset number. + if classification['is_ip'] and caller_holds('network.view'): results.extend(_search_by_ip(query, search_term)) results.extend(_search_subnets(query)) diff --git a/shopdb/core/models/user.py b/shopdb/core/models/user.py index 9ce0e300..ba10489a 100644 --- a/shopdb/core/models/user.py +++ b/shopdb/core/models/user.py @@ -162,9 +162,6 @@ def full_permission_catalog(include_disabled=False): # Each one has been read against what its routes actually return. # # Deliberately absent, with the reason, so nobody re-adds them by reflex: -# assets.view - gates the global search, which queries the staff directory -# and the USB, notification and network domains with no -# per-domain check of its own # settings.view - every setting row, and secret masking is a substring test # audit.view - usernames, addresses, and the before/after of every change # users.view - gates nothing; list_users is require_role('admin') @@ -174,6 +171,11 @@ CORE_GUEST_GRANTABLE = { # Catalogue metadata. Safe only because the bytes moved to # applications.download, which is not grantable. 'applications.view', + # Assets, and the global search over them. Safe only because search now + # gates each domain on its own permission: holding this reaches assets and + # nothing else, where it used to reach the staff directory and the USB, + # notification and network domains too. + 'assets.view', } diff --git a/tests/test_core/test_guest_role.py b/tests/test_core/test_guest_role.py index 50313325..3a37d511 100644 --- a/tests/test_core/test_guest_role.py +++ b/tests/test_core/test_guest_role.py @@ -109,15 +109,19 @@ def test_a_declared_permission_is_grantable(app): def test_a_view_permission_is_not_grantable_merely_for_being_a_view(app): """The suffix stopped deciding. These are all '.view' and none is declared. - assets.view gates a global search that reaches the staff directory; - settings.view every setting row; audit.view the before/after of every + settings.view gates every setting row; audit.view the before/after of every change; users.view nothing at all. Each was anonymous-grantable purely because of how it was spelled. + + assets.view is deliberately NOT in this list. It was, while global search + answered twelve domains behind it; now that search gates each domain on its + own permission the grant reaches assets alone, and the floor needs it to + look an asset number up without signing in. """ from shopdb.core.models import guest_grantable_permissions with app.app_context(): grantable = guest_grantable_permissions() - for name in ('assets.view', 'settings.view', 'audit.view', 'users.view'): + for name in ('settings.view', 'audit.view', 'users.view'): assert name not in grantable, name @@ -196,7 +200,7 @@ def test_the_role_list_says_what_a_guest_may_hold(client, auth_headers, assert guest['isguest'] is True assert 'reports.view' in guest['grantable'] assert 'assets.delete' not in guest['grantable'] - assert 'assets.view' not in guest['grantable'] + assert 'audit.view' not in guest['grantable'] other = next((r for r in rows if r['rolename'] != 'guest'), None) if other is not None: diff --git a/tests/test_core/test_search_domain_permissions.py b/tests/test_core/test_search_domain_permissions.py new file mode 100644 index 00000000..9e249a0e --- /dev/null +++ b/tests/test_core/test_search_domain_permissions.py @@ -0,0 +1,94 @@ +"""Global search answers each domain only to a caller who holds ITS permission. + +The route is gated on assets.view, and that used to be the entire check. One +permission then queried twelve domains, so anyone who could look up an asset +number also read out of the staff directory, the USB checkout history, the +notification log and the network inventory. + +That is why assets.view could not be handed to guests, which cost the floor its +search. Gating each domain separately is what makes the grant safe again: it +reaches assets and nothing else. +""" +from unittest.mock import patch + +import pytest + +from shopdb.core.models import Permission, Role + + +def _grant_guest(db, *names): + guest = Role.guest() + guest.permissions = [p for p in Permission.query.filter( + Permission.name.in_(names)).all()] + db.session.commit() + + +@pytest.fixture +def guestrole(db): + """The seeded guest role, holding nothing.""" + Permission.seed() + Role.seedguest() + db.session.commit() + return Role.guest() + + +def _search(client, term='a'): + response = client.get('/api/search?q={}'.format(term)) + assert response.status_code == 200, response.get_json() + return response.get_json()['data'] + + +FAKE_EMPLOYEE = [{ + 'First_Name': 'Ada', 'Last_Name': 'Smith', 'SSO': 508123, + 'Team': 'Machining', 'Role': 'Operator', +}] + + +def _with_employee_directory(): + """The HR directory is an external database and is unreachable in tests, so + the domain silently returned nothing and a leak test over it proved nothing. + Patched here so there is something real to leak.""" + return patch('shopdb.core.api.search._employee_rows', + return_value=FAKE_EMPLOYEE) + + +def test_assets_view_alone_does_not_reach_the_staff_directory(client, db, + guestrole): + """The finding this exists for: ?q=smith returned employee records to a + signed-out visitor holding only assets.view.""" + _grant_guest(db, 'assets.view') + with _with_employee_directory(): + data = _search(client, 'smith') + assert 'employee' not in {row['type'] for row in data['results']} + + +def test_employees_view_does_reach_it(client, db, guestrole): + """The other direction, so the gate is not passing by refusing everything.""" + _grant_guest(db, 'assets.view', 'employees.view') + with _with_employee_directory(): + data = _search(client, 'smith') + employees = [row for row in data['results'] if row['type'] == 'employee'] + assert len(employees) == 1 + assert employees[0]['title'] == 'Ada Smith' + + +def test_search_still_answers_a_guest_with_assets_view(client, db, guestrole): + """The regression that prompted this: guests lost search entirely. + + assets.view left the grantable set because of the leak above, which took + the floor's asset lookup with it. + """ + _grant_guest(db, 'assets.view') + response = client.get('/api/search?q=press') + assert response.status_code == 200 + + +def test_a_guest_with_nothing_granted_is_refused(client, db, guestrole): + _grant_guest(db) + assert client.get('/api/search?q=press').status_code in (401, 403) + + +def test_assets_view_is_grantable_again(app): + from shopdb.core.models import guest_grantable_permissions + with app.app_context(): + assert 'assets.view' in guest_grantable_permissions()