diff --git a/CHANGELOG.md b/CHANGELOG.md index 061d8a2e..f9f83525 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/plugins/computers/plugin.py b/plugins/computers/plugin.py index ba77db0f..3e6ed2ab 100644 --- a/plugins/computers/plugin.py +++ b/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'] diff --git a/plugins/endpoints/plugin.py b/plugins/endpoints/plugin.py index 245eef0e..71526442 100644 --- a/plugins/endpoints/plugin.py +++ b/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 - # opens any '.view' or '.read' permission to the guest role, so this - # name is what keeps the fleet roster off the public surface while - # per-asset telemetry can still be granted. + # The name avoids '.view' for readability, but the NAME no longer + # decides anything: guest_grantable_permissions is a declared list + # now, and this plugin blesses endpoints.view alone below. Adding + # 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. diff --git a/plugins/warranty/plugin.py b/plugins/warranty/plugin.py index 520c5e36..bb2f9e4c 100644 --- a/plugins/warranty/plugin.py +++ b/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})") diff --git a/shopdb/core/api/applications.py b/shopdb/core/api/applications.py index 6712a2d3..023a12dd 100644 --- a/shopdb/core/api/applications.py +++ b/shopdb/core/api/applications.py @@ -656,13 +656,17 @@ def upload_application_package(app_id: int): @applications_bp.route('/package/', 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) diff --git a/shopdb/core/api/scripts.py b/shopdb/core/api/scripts.py index 3206835c..8b606d41 100644 --- a/shopdb/core/api/scripts.py +++ b/shopdb/core/api/scripts.py @@ -196,11 +196,15 @@ def upload_script_body(script_id: int): @scripts_bp.route('/body/', 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) diff --git a/shopdb/core/api/users.py b/shopdb/core/api/users.py index 54492075..64cc06e0 100644 --- a/shopdb/core/api/users.py +++ b/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 - # declared safe to do anonymously. Refused at the door rather than - # warned about - a checkbox that quietly makes deleting an asset - # anonymous is not a warning anybody reads twice. + # bounded to what core or a plugin has explicitly declared grantable. + # Refused at the door rather than warned about - a checkbox that + # quietly makes deleting an asset anonymous is not a warning anybody + # 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 ' - 'view permission, and only writes a plugin has declared ' - 'safe to do without an account.'.format(', '.join(refused))) + 'The guest role cannot hold {}. A guest may only be ' + 'granted permissions core or the owning plugin has ' + '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] diff --git a/shopdb/core/models/user.py b/shopdb/core/models/user.py index 6f7561e2..9ce0e300 100644 --- a/shopdb/core/models/user.py +++ b/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 - open up: any `.view` or `.read` permission may be handed to guests. + Every entry is DECLARED, by core below or by a plugin through + get_guest_grantable_permissions(). Nothing qualifies by being spelled a + particular way. - A write is different - it lets somebody with no account change this - database, and the audit trail can then say when and from where but never - who. So a write has to be BLESSED by whoever owns it, through the plugin's - get_guest_grantable_permissions(). Secure Send is the case that exists: - creating a one-view link for somebody is the whole point of the page, and - the page has always been usable by people with no account here. + It used to: any name ending `.view` or `.read` was grantable, on the + reasoning that reading is the safe default. That is true of a permission + that gates a list of asset numbers and false of one that gates bytes, and + the suffix cannot tell them apart. `applications.view` reached the + SYSTEM-context PowerShell bodies and the installer binaries; + `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 - checkbox that quietly makes deleting an asset anonymous is not a warning - anybody reads twice. + So the rule is inverted: a permission is grantable when somebody wrote it + down, having looked at what it actually serves. Adding one is a code + 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 - if name.endswith('.view') or name.endswith('.read')} + known = {name for name, _, _ in catalog} + 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 diff --git a/shopdb/plugins/base.py b/shopdb/plugins/base.py index 6114b80a..850f412a 100644 --- a/shopdb/plugins/base.py +++ b/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 - grantable without asking a plugin - reading is the safe default. This - hook is for the exception: a WRITE that is genuinely meant to be usable - by somebody who never signs in. + A guest is anyone with no account. EVERY permission of this plugin's + that a site may grant one has to be named here, reads included. + + 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 - be defensible in the audit log? If not, leave it out. + The bar for a read: could an anonymous visitor hold this, forever, and + 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 [] diff --git a/tests/test_core/test_guest_role.py b/tests/test_core/test_guest_role.py index a59c8b7f..50313325 100644 --- a/tests/test_core/test_guest_role.py +++ b/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, - guestrole): +def test_the_api_allows_granting_a_guest_a_declared_read(client, db, + 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 [p.name for p in guestrole.permissions] == ['audit.view'] + assert response.status_code == 400 + 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: