Make revoking final, keep password rules in one place, drop a grant nobody read

Three credential and authz findings, all the same shape: a second path to a
decision, with weaker rules than the path built for it.

PUT /api/apitokens/<id> {"isactive": true} un-revoked a token, and the ORIGINAL
secret worked again - so "revoked" meant "paused until somebody flips it back",
for a credential whose secret is assumed to be out by the act of revoking it.
Revocation is one way now. Deactivating through the same route still works;
bringing one back does not, and the refusal says to mint a new token.

PUT /api/users/<self> accepted a `password` field with no current-password
proof and no length rule, while /api/auth/change-password requires both. When
two routes reach the same state the weaker one is the real rule, so the stronger
one was decoration. Changing your own password through the users route is
refused and points at the right endpoint. An ADMIN may still set one - that is
how a temporary password is issued - with the same 8-character floor, and
mustchangepassword is now set, because a password somebody else chose is
temporary by definition.

tools.secrets was declared guest-grantable, so the role grid offered it and
stored it, and the secure-send route never consulted the guest role at all: it
decides on securesend_allow_guests, which fails closed and carries the rate
limit. Ticking the box changed nothing in either direction, so the screen and
the behaviour disagreed. The declaration is gone; tools.view stays, because it
really does gate the nav entry.

Removing it needed care: update_role refuses the WHOLE request when a submitted
permission is not grantable, and the grid resubmits what the role already holds,
so a site whose guest role held tools.secrets could no longer save that role at
all - the deadlock fixed in 723a4ff, re-created from the other end. Migration
7d41 revokes the grant from the guest role, and nothing is lost because it
granted nothing.

Eight tests. The first two verified by planting: remove either guard and three
fail.

Scope: product
This commit is contained in:
cproudlock
2026-09-03 16:03:47 -04:00
parent f6e5f6ca47
commit 1a8f426e93
6 changed files with 214 additions and 8 deletions

View File

@@ -0,0 +1,80 @@
"""Two ways to change a credential must not disagree.
Both of these were found by the 2026-09-03 run, and both are the same shape: a
second route that reached the same state with fewer rules than the route
designed for it.
* PUT /api/users/<self> accepted a `password` field with no current-password
proof and no length rule, while /api/auth/change-password requires both.
The weaker route is the real rule, so the stronger one was decoration.
* PUT /api/apitokens/<id> {"isactive": true} un-revoked a token, and the
ORIGINAL secret worked again. "Revoked" meant "paused until somebody flips
it back" - for a credential whose secret is, by the act of revoking it,
assumed to be out.
"""
def _token(client, headers, name='cred-test'):
created = client.post('/api/apitokens', headers=headers,
json={'name': name})
assert created.status_code in (200, 201), created.get_json()
return created.get_json()['data']
def test_a_user_cannot_change_their_own_password_through_the_users_route(
client, db, member_headers, member_user):
"""The route that asks for no current password must refuse the change, not
perform it."""
response = client.put('/api/users/%d' % member_user.userid,
headers=member_headers, json={'password': 'brandnew1'})
assert response.status_code == 403, response.get_json()
assert 'change-password' in response.get_json()['data']['error']['message']
def test_an_admin_may_still_issue_a_temporary_password(client, db,
auth_headers,
member_user):
"""Issuing one is how a locked-out user gets back in - and it is temporary
by definition, so the holder must replace it at next login."""
response = client.put('/api/users/%d' % member_user.userid,
headers=auth_headers, json={'password': 'issued12345'})
assert response.status_code == 200, response.get_json()
db.session.refresh(member_user)
assert member_user.mustchangepassword is True
def test_an_admin_issued_password_still_has_to_be_long_enough(client, db,
auth_headers,
member_user):
response = client.put('/api/users/%d' % member_user.userid,
headers=auth_headers, json={'password': 'short'})
assert response.status_code == 400, response.get_json()
def test_a_revoked_token_cannot_be_brought_back(client, db, auth_headers):
token = _token(client, auth_headers, name='cred-revoke')
tokenid = token['tokenid']
secret = token['secret']
assert client.delete('/api/apitokens/%d' % tokenid,
headers=auth_headers).status_code == 200
refused = client.put('/api/apitokens/%d' % tokenid, headers=auth_headers,
json={'isactive': True})
assert refused.status_code == 400, refused.get_json()
assert 'new token' in refused.get_json()['data']['error']['message']
# And the original secret stays dead, which is the point.
denied = client.get('/api/assets',
headers={'Authorization': 'Bearer %s' % secret})
assert denied.status_code == 401
def test_a_live_token_can_still_be_deactivated_through_the_same_route(
client, db, auth_headers):
"""One-way, not frozen: turning one OFF here still works."""
token = _token(client, auth_headers, name='cred-deactivate')
response = client.put('/api/apitokens/%d' % token['tokenid'],
headers=auth_headers, json={'isactive': False})
assert response.status_code == 200, response.get_json()
assert response.get_json()['data']['isactive'] is False

View File

@@ -290,3 +290,35 @@ def test_capabilities_does_not_report_what_the_guest_role_holds(client, db,
grant(db, guestrole, 'assets.view')
body = client.get('/api/dashboard/capabilities').get_json()['data']
assert set(body) == {'cansearch'}
# --- a grant nobody reads is worse than no grant ----------------------------
#
# tools.secrets was declared guest-grantable, so the role grid offered it and
# stored it, and the secure-send route never consulted the guest ROLE - it
# decides on securesend_allow_guests, which fails closed and carries the rate
# limit. Ticking the box changed nothing in either direction, so the screen and
# the behaviour disagreed. One decision, one mechanism.
def test_tools_secrets_is_not_offered_as_a_guest_grant(app):
from shopdb.core.models import guest_grantable_permissions
with app.app_context():
grantable = guest_grantable_permissions()
assert 'tools.secrets' not in grantable
assert 'tools.view' in grantable, \
'the nav entry gate is a real guest grant and must stay'
def test_the_guest_role_still_saves_when_it_holds_only_grantable_permissions(
client, db, auth_headers, guestrole):
"""The deadlock guard: removing something from the grantable set must not
stop the guest role being saved (see 723a4ff)."""
from shopdb.core.models import Role
guest = Role.query.filter_by(rolename=Role.GUEST).first()
assert guest is not None
saved = client.put('/api/users/roles/%d' % guest.roleid, headers=auth_headers,
json={'permissions': ['tools.view']})
assert saved.status_code == 200, saved.get_json()