Skip to content

Add revision history for user account and permission changes - #396

Open
level09 wants to merge 5 commits into
mainfrom
feat/user-revision-history
Open

Add revision history for user account and permission changes#396
level09 wants to merge 5 commits into
mainfrom
feat/user-revision-history

Conversation

@level09

@level09 level09 commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

Closes BYNT-1550.

Records a snapshot of a user's account and permission state on every create and update, so an admin can answer "when did this user get this permission, and who granted it". Salvaged from the closed #217 and rebuilt on current main.

The existing activity table only stores {"id": N, "class": "user"} as the subject, so it records that an edit happened but not what changed. It is also skipped entirely when the action is not in ACTIVITIES_LIST, so it cannot serve as the permission audit trail.

Changes

  • UserHistory model following the existing *History pattern, with target_user_id (subject) and user_id (who made the change).
  • User.to_history_dict() as the snapshot serializer. Deliberately not to_dict(): that one masks names through the secure_* properties based on the viewing user, and carries force_reset (the live password reset key). A stored revision must be neither viewer-dependent nor hold secrets.
  • User.create_revision(), called from api_user_create and api_user_update.
  • GET /admin/api/userhistory/<id>, Admin only. Not gated by the view_history permissions used for content items, since these revisions carry account and permission state.
  • Alembic migration, single head.

On deletion

Users are hard deleted and every user now carries at least one revision, so a plain FK would have made api_user_delete fail permanently. target_user_id is therefore ON DELETE CASCADE and user_id is ON DELETE SET NULL, so deleting an admin does not destroy the trail of what they did to other accounts. Covered by test_delete_cascades_revisions.

Verification

  • 895 passed, 4 skipped (full suite), including 9 new tests.
  • Migration applied for real against a scratch DB (stamp down, flask db upgrade), resulting schema matches the model, flask doctor reports "Migrations up to date" and "Schema aligned with models".

Not included

  • Frontend. The endpoint returns the same {items: [...]} shape as the other history endpoints, so it can hang off the existing history drawer.
  • Revisions for password reset, forced reset, and 2FA revoke (Daniel's note on Rebuilding Roles / Users / Permissions  #217). Those are actions rather than account state; they belong in activity, where they are currently not logged at all. Worth its own small ticket.
  • The pyproject.toml line is a ruff per-file-ignore for the model package's re-export __init__.py, needed because adding one import made the hook lint the whole pre-existing file.

@level09
level09 requested a review from apodacaduron as a code owner August 11, 2026 11:12
@level09 level09 self-assigned this Aug 11, 2026
@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 15310ed0-3b58-4659-aad5-4e0b4ad1a4f1

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Comment thread tests/test_user_history.py Fixed
@level09

level09 commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author

Added a baseline backfill to the migration (b6ecf45) after a review pass flagged the gap.

Without it, a revision has nothing to diff against until the account is edited twice. Bulletins get edited constantly so their history closes that gap immediately, but user accounts are edited rarely, which would have left the first edit of every existing account undiffable for as long as that account went untouched.

The migration now inserts one snapshot per existing user, with user_id null since no acting user made that change. Verified against a seeded database covering single role, multiple roles, no roles, null name/email, and both active states: the backfilled JSON is byte-identical to User.to_history_dict() in every case, and flask doctor still reports schema aligned.

@level09

level09 commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author

Changed the deletion behavior (16e3826) after review: revisions now survive account deletion instead of cascading with it.

This is an evidence platform, so erasing the record of what an account was allowed to do is the wrong default even when the account itself goes away. target_user_id is now nullable with ON DELETE SET NULL, and since each snapshot already stores id, username, name and email, a detached row still identifies its subject. Deletion is still not blocked by the revisions, which was the original reason for the cascade.

Verified through the ORM delete path against the migrated schema: the account goes, the revision stays with target_user_id=NULL, data.id=1, data.username='doomed', data.can_export=true, and user_id still pointing at the admin who made the change.

Also fixed in 5c08259: to_history_dict() now sorts roles by id. The relationship has no order_by, so two snapshots of identical roles could serialize in different orders and read as a change on diff. The backfill SQL already sorted, so this makes the two agree.

Follow-up worth its own ticket: soft-delete users rather than hard-delete them at all. One hazard for whoever picks it up: do not reuse the global filter in enferno/utils/soft_delete.py. with_loader_criteria applies to relationship loads, and there are 36 FKs attributing work to users (bulletin.assigned_to_id, actor.first_peer_reviewer_id, activity.user_id, every *_history.user_id), so a soft-deleted user would silently disappear from every item they ever touched. That needs targeted filtering at the user-list and auth layers only.

revision_id = UserHistory.query.filter_by(target_user_id=user_id).one().id

# deletion still works, the revision does not block it
assert user.delete() is True
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant