ge-aerospace/shopdb-flask
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Decide guest grants explicitly instead of by permission name

Browse Source 
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
(cherry picked from commit 347402896c)
This commit is contained in:
cproudlock
2026-08-31 07:01:52 -04:00
parent 5f27f81e88
commit 85e8a7b877
10 changed files with 192 additions and 41 deletions
Download Patch File Download Diff File Expand all files Collapse all files

19
CHANGELOG.md
View File

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

17
plugins/computers/plugin.py
View File

@@ -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']

24
plugins/endpoints/plugin.py
View File

@@ -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.

8
plugins/warranty/plugin.py
View File

@@ -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})")

6
shopdb/core/api/applications.py
View File

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

6
shopdb/core/api/scripts.py
View File

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

15
shopdb/core/api/users.py
View File

@@ -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]

63
shopdb/core/models/user.py
View File

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

21
shopdb/plugins/base.py
View File

@@ -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 []

54
tests/test_core/test_guest_role.py
View File

@@ -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: