diff --git a/migrations/versions/7d41_drop_guest_tools_secrets.py b/migrations/versions/7d41_drop_guest_tools_secrets.py new file mode 100644 index 00000000..50ac629d --- /dev/null +++ b/migrations/versions/7d41_drop_guest_tools_secrets.py @@ -0,0 +1,56 @@ +"""Revoke a guest grant that never did anything, before it locks the role. + +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 the securesend_allow_guests setting, which fails closed and carries +the rate limit. A site that ticked the box got no change in behaviour, in either +direction. + +The plugin no longer declares it grantable. That alone would repeat the deadlock +fixed in 723a4ff: update_role refuses the WHOLE request when a submitted +permission is not grantable, and the role grid resubmits what the role already +holds - so a site whose guest role holds tools.secrets could no longer save the +guest role at all, including to change something else entirely. + +So the grant is removed here, from the one role that could hold it. Nothing is +lost: it granted nothing. Guest secret-link creation stays exactly where it was, +under securesend_allow_guests. + +Only the GUEST role is touched. A signed-in role holding tools.secrets is +unaffected - that permission works normally for an account. + +Revision ID: 7d41_drop_guest_tools_secrets +Revises: 7d40_machine_maintenance_dates +""" +from alembic import op + +import sqlalchemy as sa + + +revision = '7d41_drop_guest_tools_secrets' +down_revision = '7d40_machine_maintenance_dates' +branch_labels = None +depends_on = None + + +def upgrade(): + bind = op.get_bind() + inspector = sa.inspect(bind) + tables = set(inspector.get_table_names()) + if not {'roles', 'permissions', 'rolepermissions'} <= tables: + return + + bind.execute(sa.text( + 'DELETE FROM rolepermissions WHERE roleid IN ' + '(SELECT roleid FROM roles WHERE rolename = :guest) ' + 'AND permissionid IN ' + '(SELECT permissionid FROM permissions WHERE name = :permission)'), + {'guest': 'guest', 'permission': 'tools.secrets'}) + + +def downgrade(): + """Deliberately a no-op. + + Re-granting a permission that nothing reads would restore the confusion + rather than any capability. + """ diff --git a/plugins/tools/plugin.py b/plugins/tools/plugin.py index 0dd105af..3cb97642 100644 --- a/plugins/tools/plugin.py +++ b/plugins/tools/plugin.py @@ -86,13 +86,21 @@ class ToolsPlugin(BasePlugin): from where, and never who. That is the cost, and it is why this is the only write in the product offered to guests at all. - tools.view rides along because tools.secrets without it is a page a - guest holds permission to use and no way to reach: it gates the nav - entry, and the browser tools behind it (codes, conversions) run client - side and touch no data at all. Declared explicitly rather than inferred - from the .view suffix, which is what the grantable rule now asks for. + tools.secrets is NOT here, and that is the fix for a grant that did + nothing. Whether a guest may create a secret link is decided by the + securesend_allow_guests setting - which fails closed, carries the rate + limit, and is read straight from the row rather than the cache. The + route never consulted the guest ROLE, so ticking tools.secrets for + guests in the role grid was accepted, stored, and ignored: the site + looked open and was not, or looked closed and was not. Two mechanisms + for one decision, and the setting is the one with the safeguards. + + tools.view stays grantable: it gates the nav entry, and the browser + tools behind it (codes, conversions) run client side and touch no data + at all. Declared explicitly rather than inferred from the .view suffix, + which is what the grantable rule now asks for. """ - return ['tools.secrets', 'tools.view'] + return ['tools.view'] def get_settings_defaults(self) -> List[dict]: """The one switch this plugin owns. diff --git a/shopdb/core/api/apitokens.py b/shopdb/core/api/apitokens.py index 51a60148..da45d984 100644 --- a/shopdb/core/api/apitokens.py +++ b/shopdb/core/api/apitokens.py @@ -163,7 +163,18 @@ def update_apitoken(tokenid: int): return error_response(ErrorCodes.VALIDATION_ERROR, 'name cannot be empty') token.name = newname if 'isactive' in data: - token.isactive = bool(data['isactive']) + # Revocation is ONE WAY. A revoked token's secret is out there - that + # is why it was revoked - and re-activating the row made the same + # secret work again, so "revoked" meant "paused until somebody flips it + # back". Deactivating through this route is still allowed; bringing one + # back is not, and the answer says to mint a new one. + wanted = bool(data['isactive']) + if wanted and not token.isactive: + return error_response( + ErrorCodes.VALIDATION_ERROR, + 'A revoked token cannot be re-activated - its secret is ' + 'already out. Create a new token instead.') + token.isactive = wanted if 'scopes' in data: scopelist, scope_error = _validate_scopes(data.get('scopes'), token.user) if scope_error is not None: diff --git a/shopdb/core/api/users.py b/shopdb/core/api/users.py index 64cc06e0..b2688317 100644 --- a/shopdb/core/api/users.py +++ b/shopdb/core/api/users.py @@ -187,9 +187,28 @@ def update_user(userid: int): user.failedlogins = 0 changes['unlocked'] = {'old': True, 'new': False} - # Password change + # Password change. + # + # An ADMIN may set one here - that is how a temporary password is issued, + # and mustchangepassword forces the holder to replace it at next login. + # Somebody changing their OWN password may not: this route asks for no + # current password and enforces no length, so it was a way around + # /api/auth/change-password, which requires both. The two rules have to + # agree or the weaker one is the real one. if 'password' in data and data['password']: + if not current_user.hasrole('admin'): + return error_response( + ErrorCodes.FORBIDDEN, + 'Change your own password at /api/auth/change-password, which ' + 'requires your current one.', + http_code=403) + if len(data['password']) < 8: + return error_response( + ErrorCodes.VALIDATION_ERROR, + 'Password must be at least 8 characters') user.passwordhash = generate_password_hash(data['password']) + # Issued by somebody else, so it is temporary by definition. + user.mustchangepassword = True changes['password'] = {'old': '***', 'new': '***'} if changes: diff --git a/tests/test_core/test_credential_paths.py b/tests/test_core/test_credential_paths.py new file mode 100644 index 00000000..61122614 --- /dev/null +++ b/tests/test_core/test_credential_paths.py @@ -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/ 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/ {"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 diff --git a/tests/test_core/test_guest_role.py b/tests/test_core/test_guest_role.py index 5d21dad9..941254f6 100644 --- a/tests/test_core/test_guest_role.py +++ b/tests/test_core/test_guest_role.py @@ -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()