Gate each search domain on its own permission

Global search is gated on assets.view, and that was the whole check.
Behind it the route queried twelve domains: applications, the knowledge
base, guides, the staff directory, assets, measuring tools, USB devices,
printed parts, custom fields, notifications, hostnames, and vendor and
model reference data, plus addresses and subnets for an IP-shaped query.
None of them consulted their own permission.

So anyone who could look up an asset number could also read the staff
directory out of the HR database, the USB checkout history and who holds
which drive, and the network inventory. The SEARCH_DOMAINS settings look
like they bound this but they are site feature toggles, not
authorization.

Each domain now answers only to a caller who holds its permission.
Assets, custom fields and vendor/model stay on assets.view because all
three emit asset rows routed to asset pages.

caller_holds rather than require_permission: refusing the whole request
because one domain is off-limits would make search useless to anyone
holding less than everything. A domain the caller cannot see contributes
nothing and the rest still answers.

This is what makes assets.view safe to grant the guest role again. It
left the grantable set in 3474028 because of the leak above, and that
took the shop floor's asset lookup with it - a signed-out visitor at a
machine could no longer search for the asset number in front of them.
Now the grant reaches assets and nothing else.

The first version of the leak test passed with the gates removed. The
staff directory is an external HR database and is unreachable under
test, so the domain silently returned nothing and the assertion proved
nothing. It patches _employee_rows now, and fails when the gate goes.

Scope: product
(cherry picked from commit 0a01ddcf25)

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

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))
 

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',
 }
 
 

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:

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()