fix: a private account is private in the account manager too - #872
Merged
Merged
Conversation
`AccountFilter::buildFilter()` puts the private rule outside the block the admin flags skip, so it holds for `isAdminApp` and `isAdminAcc` alike: a private account is its owner's, a private-group account its group's, and nothing else in the application shows it to anybody else. `AccountAcl` agrees, checking `isWithheldAsPrivate()` before the admin bypass. The account manager's grid applied neither, because it applies no filter at all — `Account::search()` selects from the search view with nothing but the LIKE for the search box. So the one screen in the application where a private account was visible to somebody who is not its owner was the manager, which listed its name, client, category and owning user to any holder of `mgmAccounts`. That the grid is unscoped by *ownership* is deliberate and already recorded — #748 wrote it down precisely so a later change scoping it would be a decision rather than an accident, and there is a test holding it. This does not touch that. It applies the private rule and only the private rule, so an account that is merely somebody else's is still listed exactly as before, and the test that pins that still passes. The rule now has one definition rather than two. `buildFilterPrivate()` is what `buildFilter()` ends with, and what the manager's query asks for on its own — with a column prefix, because the manager reads a view where the account's columns are unqualified. Two tests, both against a real database in the file that already switches who is signed in: a stranger holding the manager permission does not see the private account but does still see the non-private one, and the owner still sees their own. Without that second assertion a fix that hid everything would satisfy the first, and without the third the rule would have become "nobody manages a private account" rather than "only its owner does". Checked by removing the filter: the first test fails.
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
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.
AccountFilter::buildFilter()puts the private rule outside the block the admin flagsskip, so it holds for
isAdminAppandisAdminAccalike: a private account is itsowner's, a private-group account its group's, and nothing else in the application shows
it to anybody else.
AccountAclagrees, checkingisWithheldAsPrivate()before the adminbypass. The account manager's grid applied neither, because it applies no filter at all —
Account::search()selects from the search view with nothing but the LIKE for the searchbox.
So the one screen in the application where a private account was visible to somebody who
is not its owner was the manager, which listed its name, client, category and owning user
to any holder of
mgmAccounts.That the grid is unscoped by ownership is deliberate and already recorded — #748 wrote
it down precisely so a later change scoping it would be a decision rather than an
accident, and there is a test holding it. This does not touch that. It applies the private
rule and only the private rule, so an account that is merely somebody else's is still
listed exactly as before, and the test that pins that still passes.
The rule now has one definition rather than two.
buildFilterPrivate()is whatbuildFilter()ends with, and what the manager's query asks for on its own — with acolumn prefix, because the manager reads a view where the account's columns are
unqualified.
Two tests, both against a real database in the file that already switches who is signed
in: a stranger holding the manager permission does not see the private account but does
still see the non-private one, and the owner still sees their own. Without that second
assertion a fix that hid everything would satisfy the first, and without the third the
rule would have become "nobody manages a private account" rather than "only its owner
does".
Checked by removing the filter: the first test fails.