fix: a restore cannot hand an account to somebody else - #886
Merged
blaipr merged 1 commit intoAug 26, 2026
Conversation
`update()` excludes `userId` and `userGroupId` from the columns it writes and puts them back only when `userCanChangeOwner()` and `userCanChangeGroup()` say the caller may change them — an application admin, an accounts admin, or the owner holding `accPermission`. `restoreModified()` excluded neither, so it wrote every column of the historical snapshot back, those two among them. Restoring an old version therefore reverted who owns the account and which group it belongs to. Those are exactly the columns `AccountAcl` compares the signed-in user against, so it changes who can see the account — and the door needs only `ACCOUNT_EDIT_RESTORE`, which `AccountPermission` buckets with plain `ACCOUNT_EDIT`. So anybody the account was merely shared with for editing could hand it back to a previous owner and group, which is the thing `userCanChangeOwner()` exists to refuse them two screens away. The restore asks the same two questions the edit asks, and writes those columns only if the answer is yes. Everything else in the snapshot is restored exactly as before. Checked by putting the two columns back in the exclusion list: the new test fails, asserting on the bound values of the emitted statement rather than on a row, because what matters is that the UPDATE does not carry them at all.
blaipr
deleted the
fix/a-restore-cannot-hand-an-account-to-someone-else
branch
August 26, 2026 20:45
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
update()excludesuserIdanduserGroupIdfrom the columns it writes and puts themback only when
userCanChangeOwner()anduserCanChangeGroup()say the caller may changethem — an application admin, an accounts admin, or the owner holding
accPermission.restoreModified()excluded neither, so it wrote every column of the historical snapshotback, those two among them.
Restoring an old version therefore reverted who owns the account and which group it
belongs to. Those are exactly the columns
AccountAclcompares the signed-in useragainst, so it changes who can see the account — and the door needs only
ACCOUNT_EDIT_RESTORE, whichAccountPermissionbuckets with plainACCOUNT_EDIT. Soanybody the account was merely shared with for editing could hand it back to a previous
owner and group, which is the thing
userCanChangeOwner()exists to refuse them twoscreens away.
The restore asks the same two questions the edit asks, and writes those columns only if
the answer is yes. Everything else in the snapshot is restored exactly as before.
Checked by putting the two columns back in the exclusion list: the new test fails,
asserting on the bound values of the emitted statement rather than on a row, because
what matters is that the UPDATE does not carry them at all.