Add prebuilt user scopes and guard invalid Reporter grants
A newly created user held no roles and no permissions, so the sidebar filtered every nav item away and the account appeared broken. Adds three prebuilt scopes (Reporter, Operator, Technician) derived from the live permission catalog by verb rather than hand-listed, so a new plugin permission lands in the right scope automatically instead of silently going ungranted. users.view is granted to all three since a user who cannot see the directory cannot collaborate; users.create/edit/delete stay admin-only. Role.seedscopes() is additive and idempotent, seeded via flask seed permissions, and preserves custom grants on rerun. Reporter guard: the auth model is additive across admin, direct grants and roles, so Reporter cannot be expressed as merely fewer grants - a direct endpoint-action grant would silently re-arm a read-only account. Rather than change the global permission predicate and alter every route's semantics, _check_scope_grants() rejects the invalid combination at write time on both create and update. The update path validates the effective post-update state, so demoting a Technician to Reporter is refused while a stale endpoint grant remains rather than quietly leaving a read-only user with write authority. Validation runs before the User object is constructed. Assigning user.roles first put the relationship into the session autoflush path while the User was still unattached; nothing broke because the refusal returns before db.session.add(), but it left a trap where any query added below that point would autoflush a half-built user and surface later as an orphan row. Verified with -W error::sqlalchemy.exc.SAWarning: 25 passed.
This commit is contained in:
benjamin.baker
@@ -310,6 +310,62 @@ def test_scope_ceiling_rejects_permission_owner_lacks(client, db):
|
||||
assert 'applications.create' in response.get_json()['data']['error']['message']
|
||||
|
||||
|
||||
def test_a_direct_grant_raises_the_scope_ceiling(client, db):
|
||||
"""A direct per-user grant is an ordinary permission: it lifts the token
|
||||
ceiling exactly as a role-held one does. If getpermissions() and
|
||||
haspermission() ever disagree about direct grants, this fails."""
|
||||
from shopdb.core.models import Permission, User
|
||||
|
||||
_user, headers = _user_with_perms(client, db, 'directmaker',
|
||||
['apitokens.create'])
|
||||
|
||||
# Not held any way yet: the ceiling must refuse it.
|
||||
refused = _create_token(client, headers, scopes=['applications.create'])
|
||||
assert refused.status_code == 400
|
||||
assert 'applications.create' in refused.get_json()['data']['error']['message']
|
||||
|
||||
user = User.query.filter_by(username='directmaker').one()
|
||||
user.directpermissions = [
|
||||
Permission.query.filter_by(name='applications.create').one()]
|
||||
db.session.commit()
|
||||
|
||||
allowed = _create_token(client, headers, name='direct token',
|
||||
scopes=['applications.create'])
|
||||
assert allowed.status_code == 201, allowed.get_json()
|
||||
|
||||
secret = allowed.get_json()['data']['secret']
|
||||
used = client.post('/api/applications', json={'appname': 'Direct App'},
|
||||
headers=_pat_headers(secret))
|
||||
assert used.status_code == 201, used.get_json()
|
||||
|
||||
|
||||
def test_revoking_a_direct_grant_shrinks_an_existing_token(client, db):
|
||||
"""Ceilings are computed live precisely so revocation bites immediately.
|
||||
Pulling the direct grant must disarm a token already minted against it."""
|
||||
from shopdb.core.models import Permission, User
|
||||
|
||||
_user, headers = _user_with_perms(client, db, 'revokee',
|
||||
['apitokens.create'])
|
||||
user = User.query.filter_by(username='revokee').one()
|
||||
user.directpermissions = [
|
||||
Permission.query.filter_by(name='applications.create').one()]
|
||||
db.session.commit()
|
||||
|
||||
secret = _create_token(client, headers, name='doomed token',
|
||||
scopes=['applications.create']
|
||||
).get_json()['data']['secret']
|
||||
assert client.post('/api/applications', json={'appname': 'Before Revoke'},
|
||||
headers=_pat_headers(secret)).status_code == 201
|
||||
|
||||
user = User.query.filter_by(username='revokee').one()
|
||||
user.directpermissions = []
|
||||
db.session.commit()
|
||||
|
||||
after = client.post('/api/applications', json={'appname': 'After Revoke'},
|
||||
headers=_pat_headers(secret))
|
||||
assert after.status_code == 403, after.get_json()
|
||||
|
||||
|
||||
def test_non_admin_scoped_token_capped_at_own_permissions(client, db):
|
||||
"""A non-admin holding applications.create + apitokens.create may mint a
|
||||
token scoped to applications.create and use it."""
|
||||
|
||||
267
tests/test_core/test_user_scopes.py
Normal file
267
tests/test_core/test_user_scopes.py
Normal file
@@ -0,0 +1,267 @@
|
||||
"""Prebuilt user scopes: derived grants, seeding, and the Reporter guard.
|
||||
|
||||
The guard tests are the important ones. The scope dropdown greys out endpoint
|
||||
actions for a Reporter, but a greyed checkbox stops nobody with an HTTP client,
|
||||
and Reporter was specified as holding no action capability even if expressly
|
||||
permitted. These assert that the promise holds against the API rather than
|
||||
against the UI.
|
||||
"""
|
||||
|
||||
import pytest
|
||||
|
||||
from shopdb.core.models import (
|
||||
Role, Permission, scope_permission_names, PREBUILT_SCOPES,
|
||||
SCOPE_ENDPOINT_ACTIONS, SCOPE_NEVER_GRANT)
|
||||
|
||||
|
||||
def _core_catalog():
|
||||
return list(Permission.CORE_PERMISSIONS)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Derived grant lists
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
@pytest.mark.parametrize('scope', ['operator', 'technician', 'reporter'])
|
||||
def test_scope_never_grants_the_excluded_permissions(scope, app):
|
||||
"""No scope may leak an excluded permission, whatever the catalog holds.
|
||||
|
||||
Guards the derivation itself: the lists are computed by verb, so a new
|
||||
permission named with an unexpected verb could otherwise slip into a scope
|
||||
silently.
|
||||
"""
|
||||
with app.app_context():
|
||||
granted = set(scope_permission_names(scope))
|
||||
assert not (granted & SCOPE_NEVER_GRANT)
|
||||
|
||||
|
||||
@pytest.mark.parametrize('scope', ['operator', 'technician', 'reporter'])
|
||||
def test_no_scope_can_act_on_endpoints_through_its_role(scope, app):
|
||||
"""Endpoint actions are per-user grants, never role grants.
|
||||
|
||||
If a scope ever carried one, the per-user checkbox would stop being the
|
||||
control point and the Reporter guard could be bypassed by role alone.
|
||||
"""
|
||||
with app.app_context():
|
||||
granted = set(scope_permission_names(scope))
|
||||
assert not (granted & set(SCOPE_ENDPOINT_ACTIONS))
|
||||
|
||||
|
||||
def test_operator_can_edit_but_never_delete(app):
|
||||
"""Deletion stays with admins - the defining line for Operator."""
|
||||
with app.app_context():
|
||||
granted = scope_permission_names('operator', catalog=_core_catalog())
|
||||
assert 'assets.create' in granted
|
||||
assert 'assets.edit' in granted
|
||||
assert not [n for n in granted if n.endswith('.delete')]
|
||||
|
||||
|
||||
def test_technician_holds_no_record_mutation(app):
|
||||
"""Technician reads records; endpoint work comes from per-user grants."""
|
||||
with app.app_context():
|
||||
granted = scope_permission_names('technician', catalog=_core_catalog())
|
||||
assert 'assets.view' in granted
|
||||
assert 'assets.create' not in granted
|
||||
assert 'assets.edit' not in granted
|
||||
|
||||
|
||||
def test_reporter_reads_and_exports(app):
|
||||
with app.app_context():
|
||||
granted = scope_permission_names('reporter', catalog=_core_catalog())
|
||||
assert 'reports.view' in granted
|
||||
assert 'reports.export' in granted
|
||||
|
||||
|
||||
def test_every_scope_sees_the_user_directory_but_cannot_manage_it(app):
|
||||
"""users.view is granted; creating and editing accounts is admin-only."""
|
||||
with app.app_context():
|
||||
for scope in ('operator', 'technician', 'reporter'):
|
||||
granted = scope_permission_names(scope, catalog=_core_catalog())
|
||||
assert 'users.view' in granted, scope
|
||||
assert 'users.create' not in granted, scope
|
||||
assert 'users.edit' not in granted, scope
|
||||
assert 'users.delete' not in granted, scope
|
||||
|
||||
|
||||
def test_scope_permission_names_rejects_unknown_scope(app):
|
||||
with app.app_context():
|
||||
with pytest.raises(ValueError):
|
||||
scope_permission_names('wizard')
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Seeding
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
def test_seedscopes_creates_roles_and_is_idempotent(app, db):
|
||||
"""A second run creates nothing and grants nothing new."""
|
||||
with app.app_context():
|
||||
Permission.seed()
|
||||
db.session.commit()
|
||||
|
||||
first = Role.seedscopes()
|
||||
db.session.commit()
|
||||
assert set(first['created']) == {n for n, _, _ in PREBUILT_SCOPES}
|
||||
|
||||
second = Role.seedscopes()
|
||||
db.session.commit()
|
||||
assert second['created'] == []
|
||||
assert second['granted'] == {}
|
||||
|
||||
|
||||
def test_seedscopes_does_not_revoke_custom_grants(app, db):
|
||||
"""Re-running must not undo a deliberate site customization.
|
||||
|
||||
The seed is additive by design: a revoking seed would silently strip an
|
||||
extra grant an admin had chosen, and it would do so on deploy, where nobody
|
||||
is watching.
|
||||
"""
|
||||
with app.app_context():
|
||||
Permission.seed()
|
||||
db.session.commit()
|
||||
Role.seedscopes()
|
||||
db.session.commit()
|
||||
|
||||
technician = Role.query.filter_by(rolename='technician').one()
|
||||
extra = Permission.query.filter_by(name='assets.edit').one()
|
||||
technician.permissions.append(extra)
|
||||
db.session.commit()
|
||||
|
||||
Role.seedscopes()
|
||||
db.session.commit()
|
||||
|
||||
technician = Role.query.filter_by(rolename='technician').one()
|
||||
assert 'assets.edit' in {p.name for p in technician.permissions}
|
||||
|
||||
|
||||
def test_seedscopes_grants_survive_a_reporter_role(app, db):
|
||||
with app.app_context():
|
||||
Permission.seed()
|
||||
db.session.commit()
|
||||
Role.seedscopes()
|
||||
db.session.commit()
|
||||
|
||||
reporter = Role.query.filter_by(rolename='reporter').one()
|
||||
held = {p.name for p in reporter.permissions}
|
||||
assert 'reports.export' in held
|
||||
assert not (held & set(SCOPE_ENDPOINT_ACTIONS))
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# The write-path guard
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
def _seed_scopes(app, db):
|
||||
with app.app_context():
|
||||
Permission.seed()
|
||||
db.session.commit()
|
||||
Role.seedscopes()
|
||||
db.session.commit()
|
||||
return {
|
||||
name: Role.query.filter_by(rolename=name).one().roleid
|
||||
for name, _, _ in PREBUILT_SCOPES
|
||||
}
|
||||
|
||||
|
||||
def test_create_reporter_with_endpoint_action_is_refused(client, auth_headers,
|
||||
app, db):
|
||||
"""The whole point: a Reporter cannot be created holding a restart grant."""
|
||||
ids = _seed_scopes(app, db)
|
||||
response = client.post('/api/users', headers=auth_headers, json={
|
||||
'username': 'scopereporter', 'email': 'scopereporter@test.local',
|
||||
'password': 'TestPass123!', 'roles': [ids['reporter']],
|
||||
'directpermissions': ['endpoints.restart'],
|
||||
})
|
||||
assert response.status_code == 400, response.get_json()
|
||||
message = response.get_json()['data']['error']['message']
|
||||
assert 'reporter' in message.lower()
|
||||
|
||||
from shopdb.core.models import User
|
||||
with app.app_context():
|
||||
assert User.query.filter_by(username='scopereporter').first() is None
|
||||
|
||||
|
||||
def test_create_technician_with_endpoint_action_is_allowed(client, auth_headers,
|
||||
app, db):
|
||||
ids = _seed_scopes(app, db)
|
||||
response = client.post('/api/users', headers=auth_headers, json={
|
||||
'username': 'scopetech', 'email': 'scopetech@test.local',
|
||||
'password': 'TestPass123!', 'roles': [ids['technician']],
|
||||
'directpermissions': ['endpoints.restart'],
|
||||
})
|
||||
assert response.status_code == 201, response.get_json()
|
||||
|
||||
|
||||
def test_create_reporter_without_actions_is_allowed(client, auth_headers,
|
||||
app, db):
|
||||
ids = _seed_scopes(app, db)
|
||||
response = client.post('/api/users', headers=auth_headers, json={
|
||||
'username': 'plainreporter', 'email': 'plainreporter@test.local',
|
||||
'password': 'TestPass123!', 'roles': [ids['reporter']],
|
||||
'directpermissions': [],
|
||||
})
|
||||
assert response.status_code == 201, response.get_json()
|
||||
|
||||
|
||||
def test_demoting_a_technician_to_reporter_keeps_no_stale_grant(
|
||||
client, auth_headers, app, db):
|
||||
"""The partial-payload case, and the easiest one to get wrong.
|
||||
|
||||
The request changes ROLES only and never mentions directpermissions, so a
|
||||
guard that inspected just the supplied fields would see no endpoint action
|
||||
and allow it - leaving a Reporter holding restart, the exact state the
|
||||
scope promises cannot exist. The guard checks effective state instead.
|
||||
"""
|
||||
ids = _seed_scopes(app, db)
|
||||
created = client.post('/api/users', headers=auth_headers, json={
|
||||
'username': 'demoteme', 'email': 'demoteme@test.local',
|
||||
'password': 'TestPass123!', 'roles': [ids['technician']],
|
||||
'directpermissions': ['endpoints.restart'],
|
||||
})
|
||||
assert created.status_code == 201, created.get_json()
|
||||
userid = created.get_json()['data']['userid']
|
||||
|
||||
response = client.put(f'/api/users/{userid}', headers=auth_headers,
|
||||
json={'roles': [ids['reporter']]})
|
||||
assert response.status_code == 400, response.get_json()
|
||||
|
||||
from shopdb.core.models import User
|
||||
with app.app_context():
|
||||
user = db.session.get(User, userid)
|
||||
assert [r.rolename for r in user.roles] == ['technician']
|
||||
assert 'endpoints.restart' in user.getdirectpermissionnames()
|
||||
|
||||
|
||||
def test_adding_an_action_to_an_existing_reporter_is_refused(
|
||||
client, auth_headers, app, db):
|
||||
"""The mirror case: grants change, roles unmentioned."""
|
||||
ids = _seed_scopes(app, db)
|
||||
created = client.post('/api/users', headers=auth_headers, json={
|
||||
'username': 'staticreporter', 'email': 'staticreporter@test.local',
|
||||
'password': 'TestPass123!', 'roles': [ids['reporter']],
|
||||
})
|
||||
assert created.status_code == 201, created.get_json()
|
||||
userid = created.get_json()['data']['userid']
|
||||
|
||||
response = client.put(
|
||||
f'/api/users/{userid}', headers=auth_headers,
|
||||
json={'directpermissions': ['endpoints.runscript']})
|
||||
assert response.status_code == 400, response.get_json()
|
||||
|
||||
from shopdb.core.models import User
|
||||
with app.app_context():
|
||||
user = db.session.get(User, userid)
|
||||
assert user.getdirectpermissionnames() == []
|
||||
|
||||
|
||||
def test_stacking_technician_onto_reporter_cannot_launder_a_grant(
|
||||
client, auth_headers, app, db):
|
||||
"""Reporter denies actions even when a permissive scope sits beside it."""
|
||||
ids = _seed_scopes(app, db)
|
||||
response = client.post('/api/users', headers=auth_headers, json={
|
||||
'username': 'stacked', 'email': 'stacked@test.local',
|
||||
'password': 'TestPass123!',
|
||||
'roles': [ids['reporter'], ids['technician']],
|
||||
'directpermissions': ['endpoints.corptools'],
|
||||
})
|
||||
assert response.status_code == 400, response.get_json()
|
||||
@@ -47,3 +47,85 @@ def test_role_color_create_list_update(client, auth_headers):
|
||||
json={'color': '#00aa55'})
|
||||
assert updated.status_code == 200, updated.get_json()
|
||||
assert updated.get_json()['data']['color'] == '#00aa55'
|
||||
|
||||
|
||||
def test_directpermissions_granted_without_a_role(client, auth_headers, app, db):
|
||||
"""A permission may be granted to one user without minting a role for it,
|
||||
and it must reach every surface that answers "may this person do X"."""
|
||||
from shopdb.core.models import Permission, User
|
||||
|
||||
with app.app_context():
|
||||
Permission.seed_entries([
|
||||
('endpoints.runscript', 'Push scripts', 'endpoints'),
|
||||
('endpoints.corptools', 'Corporate tool use', 'endpoints'),
|
||||
])
|
||||
db.session.commit()
|
||||
|
||||
created = client.post('/api/users', headers=auth_headers, json={
|
||||
'username': 'bob', 'email': 'bob@test.local', 'password': 'hunter2!!',
|
||||
'roles': [], 'directpermissions': ['endpoints.runscript'],
|
||||
'sendwelcome': False,
|
||||
})
|
||||
assert created.status_code in (200, 201), created.get_json()
|
||||
payload = created.get_json()['data']
|
||||
userid = payload['userid']
|
||||
|
||||
assert payload['directpermissions'] == ['endpoints.runscript']
|
||||
assert 'endpoints.runscript' in payload['permissions']
|
||||
|
||||
with app.app_context():
|
||||
user = db.session.get(User, userid)
|
||||
assert user.haspermission('endpoints.runscript')
|
||||
assert not user.haspermission('endpoints.corptools')
|
||||
assert 'endpoints.runscript' in user.getpermissions()
|
||||
|
||||
revoked = client.put(f'/api/users/{userid}', headers=auth_headers,
|
||||
json={'directpermissions': []})
|
||||
assert revoked.status_code == 200, revoked.get_json()
|
||||
assert revoked.get_json()['data']['directpermissions'] == []
|
||||
|
||||
with app.app_context():
|
||||
user = db.session.get(User, userid)
|
||||
assert not user.haspermission('endpoints.runscript')
|
||||
|
||||
|
||||
def test_a_direct_grant_is_not_duplicated_when_a_role_supplies_it_too(
|
||||
client, auth_headers, app, db):
|
||||
"""Holding a permission both ways is one capability, not two. getpermissions()
|
||||
feeds the API token scope ceiling, so a duplicate would be a visible defect."""
|
||||
from shopdb.core.models import Permission, Role, User
|
||||
|
||||
with app.app_context():
|
||||
Permission.seed_entries([
|
||||
('endpoints.updateapp', 'Update applications', 'endpoints'),
|
||||
])
|
||||
permission = Permission.query.filter_by(
|
||||
name='endpoints.updateapp').one()
|
||||
role = Role(rolename='updaters', description='app updaters')
|
||||
role.permissions = [permission]
|
||||
user = User(username='dana', email='dana@test.local',
|
||||
passwordhash='x', isactive=True)
|
||||
user.roles = [role]
|
||||
user.directpermissions = [permission]
|
||||
db.session.add_all([role, user])
|
||||
db.session.commit()
|
||||
userid = user.userid
|
||||
|
||||
with app.app_context():
|
||||
user = db.session.get(User, userid)
|
||||
granted = user.getpermissions()
|
||||
assert sorted(granted).count('endpoints.updateapp') == 1
|
||||
assert user.haspermission('endpoints.updateapp')
|
||||
|
||||
|
||||
def test_directpermissions_rejects_an_unknown_name(client, auth_headers):
|
||||
"""A typo must fail loudly. Dropping it would report success while granting
|
||||
nothing, and the admin would believe the capability was handed out."""
|
||||
created = client.post('/api/users', headers=auth_headers, json={
|
||||
'username': 'carl', 'email': 'carl@test.local', 'password': 'hunter2!!',
|
||||
'directpermissions': ['endpoints.runscripts'], 'sendwelcome': False,
|
||||
})
|
||||
assert created.status_code == 400, created.get_json()
|
||||
message = created.get_json()['data']['error']['message']
|
||||
assert 'endpoints.runscripts' in message
|
||||
|
||||
|
||||
Reference in New Issue
Block a user