Decide guest grants explicitly instead of by permission name

guest_grantable_permissions() opened any permission whose name ended
'.view' or '.read' to the guest role, on the reasoning that reading is
the safe default. That is true of a permission gating a list of asset
numbers and false of one gating bytes, and a suffix cannot tell them
apart. Twenty-four of eighty-five qualified and none had been decided:

  applications.view  reached the SYSTEM-context PowerShell bodies and
                     the installer binaries pushed to bays
  backups.view       returned the full payload that the separate
                     backups.download permission exists to protect
  assets.view        gated a global search that queries the staff
                     directory, USB history and network inventory with
                     no per-domain check of its own
  audit.view         usernames, addresses, the before/after of every
  settings.view      change, and every setting row - both admin
                     category, opened by naming alone
  users.view         gates nothing at all; list_users is
                     require_role('admin')

Every entry is now declared, by CORE_GUEST_GRANTABLE or by the plugin
that owns the permission, having read what its routes actually return.
Adding one is a code change, which is the point: it puts the decision
in a diff rather than in a suffix.

The two routes serving script bodies and installer packages move to a
new applications.download permission, which is not grantable, so a site
can still open the catalogue metadata to its floor without the bytes
going with it.

The declared set preserves the grants this site had already chosen:
applications.view, computers.view, endpoints.view, reports.view,
tools.secrets, warranty.view.

Two known gaps are recorded on the hooks that expose them rather than
silently carried: the computer serializer publishes every column plus
resolved rdp:// and vnc:// links, and the per-asset endpoint panels are
keyed on a small integer so walking ids reconstructs much of the roster
that endpoints.fleet exists to protect. Neither is widened here.

A shipped test was granting guests audit.view and passing.

Scope: product

CHANGELOG.md

@@ -128,6 +128,25 @@ ADR-007 and ADR-002.
 
 ### Fixed
 
+- **A permission became grantable to anonymous visitors because of how it was
+  spelled.** `guest_grantable_permissions()` opened any permission whose name
+  ended `.view` or `.read` to the guest role, on the reasoning that reading is
+  the safe default. That is true of a permission gating a list of asset numbers
+  and false of one gating bytes, and a suffix cannot tell them apart. Twenty-four
+  of eighty-five qualified, and nobody had decided any of them:
+  `applications.view` reached the SYSTEM-context PowerShell bodies and the
+  installer binaries a site pushes to its bays; `backups.view` returned the full
+  payload that the separate `backups.download` permission exists to protect;
+  `assets.view` gated a global search that queries the staff directory, the USB
+  history and the network inventory with no per-domain check of its own;
+  `audit.view` and `settings.view` are admin-category and were opened by naming
+  alone. Every entry is now DECLARED, by core or by the plugin that owns it,
+  having read what its routes actually return. Adding one is a code change,
+  which is the point - it puts the decision in a diff rather than in a suffix.
+  The two routes serving script bodies and installer packages moved to a new
+  `applications.download` permission, which is not grantable, so the catalogue
+  metadata can still be opened to a site's floor without the bytes going with it.
+
 - **The published site's API documentation page had no spec to show.**
   `shopdb/core/api/docs.py` reads `docs/openapi.json` at runtime to serve
   `/api/docs`, and publication excluded the whole of `docs/` because the prose

plugins/computers/plugin.py

@@ -2013,3 +2013,20 @@ class ComputersPlugin(BasePlugin):
             ('computers.edit', 'Edit computers', 'computers'),
             ('computers.delete', 'Delete computers', 'computers'),
         ]
+
+    def get_guest_grantable_permissions(self) -> List[str]:
+        """Declared because a site chose it: the floor looks PCs up by number.
+
+        KNOWN GAP, tracked separately. The computer serializer is
+        BaseModel.to_dict(), so it publishes every column - serialnumber, the
+        Azure device and object ids - and the detail route resolves
+        accessmethods into working rdp:// and vnc:// links plus the full
+        Communication row. A guest granted this gets all of it, on the LIST
+        route as well, so it is one request rather than one per asset.
+
+        The fix is a field allowlist on Computer.to_dict() plus suppressing the
+        resolved links unless the caller holds a remote-access permission. Do
+        not widen this hook until that lands; a site enabling guest access
+        today should weigh it.
+        """
+        return ['computers.view']

plugins/endpoints/plugin.py

@@ -145,10 +145,11 @@ class EndpointsPlugin(BasePlugin):
             # including every logged-in username on the floor, because the
             # panels and the list were gated on the same string.
             #
-            # It is deliberately NOT named '.view'. guest_grantable_permissions
+            # The name avoids '.view' for readability, but the NAME no longer
-            # opens any '.view' or '.read' permission to the guest role, so this
+            # decides anything: guest_grantable_permissions is a declared list
-            # name is what keeps the fleet roster off the public surface while
+            # now, and this plugin blesses endpoints.view alone below. Adding
-            # per-asset telemetry can still be granted.
+            # endpoints.fleet there would be what opens the roster, and the
+            # docstring on that hook says why not to.
             ('endpoints.fleet',
              'View the fleet-wide endpoint list and dashboards', 'endpoints'),
             # Issue an allowlisted command to an endpoint agent (for example the
@@ -187,6 +188,21 @@ class EndpointsPlugin(BasePlugin):
              'Submit endpoint agent telemetry (fleet reporting)', 'collector'),
         ]
 
+    def get_guest_grantable_permissions(self) -> List[str]:
+        """Declared because a site chose it: live status on a PC page.
+
+        endpoints.view only. NOT endpoints.fleet - that is the roster, and the
+        whole reason the two were split.
+
+        KNOWN GAP, tracked separately: the per-asset panels this opens are
+        keyed on a small integer asset id and the status panel emits the
+        logged-in username, so walking ids reconstructs most of the roster the
+        split exists to protect. Bounding or rate-limiting the per-asset panels
+        is the fix. Until then a site enabling guest access should know the
+        split is a speed bump here, not a wall.
+        """
+        return ['endpoints.view']
+
     def get_settings_defaults(self) -> List[Dict]:
         """Settings owned by the endpoints agent-key kill switch.
 

plugins/warranty/plugin.py

@@ -174,5 +174,13 @@ class WarrantyPlugin(BasePlugin):
             ('warranty.delete', 'Delete warranties', 'warranty'),
         ]
 
+    def get_guest_grantable_permissions(self) -> List[str]:
+        """Coverage dates and vendor for an asset somebody is standing at.
+
+        Nothing here is a credential and nothing reaches a supplier portal; the
+        provider lookups run server-side with keys the caller never sees.
+        """
+        return ['warranty.view']
+
     def init_app(self, app: Flask, db_instance) -> None:
         logger.info(f"Warranty plugin initialized (v{self.meta.version})")

shopdb/core/api/applications.py

@@ -656,13 +656,17 @@ def upload_application_package(app_id: int):
 
 
 @applications_bp.route('/package/<path:filename>', methods=['GET'])
-@require_permission('applications.view')
+@require_permission('applications.download')
 def serve_application_package(filename):
     """Download an uploaded installer.
 
     Authenticated, unlike the image: this is licensed vendor software, and an
     open URL would publish it to anything that can reach the site. Always sent
     as an attachment so a browser saves it rather than trying to render it.
+
+    Gated on applications.download, not applications.view: a guest may be
+    granted the metadata permission, and "an open URL would publish it" is just
+    as true of a URL any anonymous visitor can reach.
     """
     return send_upload(_apppackage_dir(), filename, as_attachment=True)
 

shopdb/core/api/scripts.py

@@ -196,11 +196,15 @@ def upload_script_body(script_id: int):
 
 
 @scripts_bp.route('/body/<path:filename>', methods=['GET'])
-@require_permission('applications.view')
+@require_permission('applications.download')
 def download_script_body(filename: str):
     """Operator-facing script body download (JWT/RBAC).
 
     The endpoint agent uses the collector-authenticated plugin route instead;
     this one is for humans reviewing what a script does.
+
+    Gated on applications.download, not applications.view: this serves the
+    SYSTEM-context PowerShell a site pushes to its bays, and a guest may be
+    granted the metadata permission.
     """
     return send_upload(_script_dir(), filename, as_attachment=True)

shopdb/core/api/users.py

@@ -351,19 +351,20 @@ def update_role(roleid: int):
     # Update permissions
     if 'permissions' in data and role.rolename != 'admin':
         # The guest role is anyone with no account, so what it may hold is
-        # bounded: reads, plus the handful of writes a plugin has explicitly
+        # bounded to what core or a plugin has explicitly declared grantable.
-        # declared safe to do anonymously. Refused at the door rather than
+        # Refused at the door rather than warned about - a checkbox that
-        # warned about - a checkbox that quietly makes deleting an asset
+        # quietly makes deleting an asset anonymous is not a warning anybody
-        # anonymous is not a warning anybody reads twice.
+        # reads twice.
         if role.rolename == Role.GUEST:
             allowed = guest_grantable_permissions()
             refused = sorted(set(data['permissions']) - allowed)
             if refused:
                 return error_response(
                     ErrorCodes.VALIDATION_ERROR,
-                    'The guest role cannot hold {}. A guest may be granted any '
+                    'The guest role cannot hold {}. A guest may only be '
-                    'view permission, and only writes a plugin has declared '
+                    'granted permissions core or the owning plugin has '
-                    'safe to do without an account.'.format(', '.join(refused)))
+                    'declared grantable, which is a code change rather than a '
+                    'setting.'.format(', '.join(refused)))
         old_perms = [p.name for p in role.permissions]
         perms = Permission.query.filter(Permission.name.in_(data['permissions'])).all()
         new_perms = [p.name for p in perms]

shopdb/core/models/user.py

@@ -46,6 +46,12 @@ class Permission(db.Model):
         ('assets.delete', 'Delete assets', 'assets'),
         # Applications
         ('applications.view', 'View applications', 'applications'),
+        # Deliberately NOT '.view': reading the catalogue and pulling the bytes
+        # are different acts. A script body is SYSTEM-context PowerShell and an
+        # installer is a vendor binary, so the routes that serve them are gated
+        # here rather than on the metadata permission a site may hand to guests.
+        ('applications.download',
+         'Download application installers and script bodies', 'applications'),
         ('applications.create', 'Create applications', 'applications'),
         ('applications.edit', 'Edit applications', 'applications'),
         ('applications.delete', 'Delete applications', 'applications'),
@@ -152,28 +158,58 @@ def full_permission_catalog(include_disabled=False):
     return catalog
 
 
+# Core permissions a site MAY grant the guest role. Declared, not derived.
+# 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')
+CORE_GUEST_GRANTABLE = {
+    # Aggregate counts only; no row-level data reaches the caller.
+    'reports.view',
+    # Catalogue metadata. Safe only because the bytes moved to
+    # applications.download, which is not grantable.
+    'applications.view',
+}
+
+
 def guest_grantable_permissions():
     """Which permissions the guest role may be granted.
 
-    Reading is the safe default and covers almost everything a site wants to
+    Every entry is DECLARED, by core below or by a plugin through
-    open up: any `.view` or `.read` permission may be handed to guests.
+    get_guest_grantable_permissions(). Nothing qualifies by being spelled a
+    particular way.
 
-    A write is different - it lets somebody with no account change this
+    It used to: any name ending `.view` or `.read` was grantable, on the
-    database, and the audit trail can then say when and from where but never
+    reasoning that reading is the safe default. That is true of a permission
-    who. So a write has to be BLESSED by whoever owns it, through the plugin's
+    that gates a list of asset numbers and false of one that gates bytes, and
-    get_guest_grantable_permissions(). Secure Send is the case that exists:
+    the suffix cannot tell them apart. `applications.view` reached the
-    creating a one-view link for somebody is the whole point of the page, and
+    SYSTEM-context PowerShell bodies and the installer binaries;
-    the page has always been usable by people with no account here.
+    `backups.view` returned the full payload the separate backups.download
+    permission exists to protect; `assets.view` gated a global search that
+    queries the staff directory. None of those were decided. They were named.
 
-    Everything else is refused at the door rather than warned about, because a
+    So the rule is inverted: a permission is grantable when somebody wrote it
-    checkbox that quietly makes deleting an asset anonymous is not a warning
+    down, having looked at what it actually serves. Adding one is a code
-    anybody reads twice.
+    change, which is the point - it puts the decision in a diff rather than in
+    a suffix.
+
+    Naming one here does not open it. It makes the checkbox available in the
+    role grid; a site still has to tick it.
+
+    The bar, for a read: could an anonymous visitor hold this, forever, and
+    would the site be content with everything it returns? For a write, add
+    that the act can never be attributed to anybody.
     """
     from flask import current_app, has_app_context
 
     catalog = full_permission_catalog()
-    grantable = {name for name, _, _ in catalog
+    known = {name for name, _, _ in catalog}
-                 if name.endswith('.view') or name.endswith('.read')}
+    grantable = {name for name in CORE_GUEST_GRANTABLE if name in known}
 
     if not has_app_context():
         return grantable
@@ -181,7 +217,6 @@ def guest_grantable_permissions():
     if not pm:
         return grantable
 
-    known = {name for name, _, _ in catalog}
     for name, plugin in pm.get_all_plugins().items():
         if not pm.registry.is_enabled(name):
             continue

shopdb/plugins/base.py

@@ -107,17 +107,24 @@ class BasePlugin(ABC):
     def get_guest_grantable_permissions(self) -> List[str]:
         """Permissions this plugin allows a site to grant to the GUEST role.
 
-        A guest is anyone with no account. Any `.view` or `.read` permission is
+        A guest is anyone with no account. EVERY permission of this plugin's
-        grantable without asking a plugin - reading is the safe default. This
+        that a site may grant one has to be named here, reads included.
-        hook is for the exception: a WRITE that is genuinely meant to be usable
+
-        by somebody who never signs in.
+        Reads used to be automatic: anything ending `.view` or `.read` was
+        grantable. That treats a name as a decision, and it went wrong in the
+        obvious way - permissions that gated installer bytes, backup payloads
+        and a search across the staff directory were all anonymous-grantable
+        because of how they were spelled. So the plugin that owns a permission
+        now says, having looked at what its routes return.
 
         Naming one here does not open it. It makes the checkbox available in
         the role grid; a site still has to tick it. Return [] (the default) and
-        none of this plugin's writes can ever be made anonymous.
+        nothing of this plugin's can be made anonymous.
 
-        The bar: could this action be attributed to nobody, forever, and still
+        The bar for a read: could an anonymous visitor hold this, forever, and
-        be defensible in the audit log? If not, leave it out.
+        would the site be content with everything the routes return - every
+        column of every serializer, not just the ones the UI happens to show?
+        For a write, add that the act can never be attributed to anybody.
         """
         return []
 

tests/test_core/test_guest_role.py

@@ -98,12 +98,35 @@ def test_a_site_with_no_guest_role_behaves_as_it_always_did(client, db):
 # The guardrail
 # ---------------------------------------------------------------------------
 
-def test_a_view_permission_is_grantable(app):
+def test_a_declared_permission_is_grantable(app):
     from shopdb.core.models import guest_grantable_permissions
     with app.app_context():
         grantable = guest_grantable_permissions()
-    assert 'assets.view' in grantable
     assert 'reports.view' in grantable
+    assert 'applications.view' in grantable
+
+
+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
+    change; users.view nothing at all. Each was anonymous-grantable purely
+    because of how it was spelled.
+    """
+    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'):
+        assert name not in grantable, name
+
+
+def test_the_bytes_behind_applications_are_not_grantable(app):
+    """applications.view is grantable only because the bytes moved off it."""
+    from shopdb.core.models import guest_grantable_permissions
+    with app.app_context():
+        grantable = guest_grantable_permissions()
+    assert 'applications.download' not in grantable
 
 
 def test_a_write_permission_is_not_grantable(app):
@@ -127,13 +150,29 @@ def test_the_api_refuses_to_grant_a_guest_a_write(client, db, auth_headers,
     assert guestrole.permissions == []
 
 
-def test_the_api_allows_granting_a_guest_a_view(client, db, auth_headers,
+def test_the_api_allows_granting_a_guest_a_declared_read(client, db,
-                                                guestrole):
+                                                         auth_headers,
+                                                         guestrole):
+    response = client.put('/api/users/roles/{}'.format(guestrole.roleid),
+                          headers=auth_headers,
+                          json={'permissions': ['reports.view']})
+    assert response.status_code == 200
+    assert [p.name for p in guestrole.permissions] == ['reports.view']
+
+
+def test_the_api_refuses_an_undeclared_read(client, db, auth_headers,
+                                            guestrole):
+    """This test used to grant guests audit.view and pass, which is the bug.
+
+    audit.view ends in '.view' and gates usernames, addresses and the
+    before/after of every change in the database.
+    """
     response = client.put('/api/users/roles/{}'.format(guestrole.roleid),
                           headers=auth_headers,
                           json={'permissions': ['audit.view']})
-    assert response.status_code == 200
+    assert response.status_code == 400
-    assert [p.name for p in guestrole.permissions] == ['audit.view']
+    assert 'audit.view' in response.get_json()['data']['error']['message']
+    assert guestrole.permissions == []
 
 
 def test_the_guest_role_cannot_be_deleted(client, auth_headers, guestrole):
@@ -155,8 +194,9 @@ def test_the_role_list_says_what_a_guest_may_hold(client, auth_headers,
     rows = client.get('/api/users/roles', headers=auth_headers).get_json()['data']
     guest = next(r for r in rows if r['rolename'] == 'guest')
     assert guest['isguest'] is True
-    assert 'assets.view' in guest['grantable']
+    assert 'reports.view' in guest['grantable']
     assert 'assets.delete' not in guest['grantable']
+    assert 'assets.view' not in guest['grantable']
 
     other = next((r for r in rows if r['rolename'] != 'guest'), None)
     if other is not None: