commit 7e4b43c9c66e162587dfaaf714fb3753f0b38cd1
author Edwin Kempin <ekempin@google.com> Wed Feb 15 17:19:45 2023 +0100
committer Edwin Kempin <ekempin@google.com> Mon Feb 20 12:36:47 2023 +0100
tree 5af3d56758211aed7e4eeb249226b918ad0ce194
parent 7e5c52a588e9106e00c422c3f6d995a265fbb3c9

Skip account visibility checks when querying changes

On the change screen users can see who participates in the change (the
change owner, reviewers, CCs etc.). The names of the participating users
are linkified and clicking on them leads to a dashboard with all changes
that are owned by this user. Due to Gerrit's account visibility setting
it can happen that the accounts of some of these users are not visible.
In this case clicking on the user link results in an error that is
unintelligible and confuses users (the error message is: "An error
occurred Error 400: Account [REDACTED_EMAIL_ADDRESS]' not found").

To solve this problem we skip the account visibility check when querying
changes (except if the user is referenced by a secondary email, see
reasoning below). This way clicking on a participating user on the
change screen will work even if their account is not visible.

Skipping the account visibility check when querying changes of
participating users is okay because:

* It doesn't reveal the existence of any account:
  The calling user already knows that this account exists since they can
  see it as a participating user on the change.

* The change query only returns changes that are visible to the calling
  user: The calling user could have found the same changes by other
  means (e.g. iterate over all changes that are visible to them and then
  check which of these changes have the participating user as the change
  owner).

Skipping the account visibility check for change queries in general is
okay because:

* Users can confirm the existence of non-visible accounts that
  participated on any visible change by other means: The calling user
  can iterate over all changes that are visible to them and collect all
  users that participate in them.

To avoid that users can probe whether an account exists, we will no
longer return an error if an account doesn't exist, but instead we
return an empty query result. Otherwise users could deduce from the
response whether an arbitrary account exists (error -> account doesn't
exist, empty result -> account exists but didn't take part in any
visible changes).

Secondary emails are usually not visible to other users (e.g. to avoid
exposing dead names) and can only be seen by the user themselves, admin
users and special privileged users that have the Modify Account
capability assigned to them.

Secondary emails of participating users are not visible on the change
screen. In other words if a non-visible account takes part in a change,
all their account properties are visible to the users that can see the
change, except their secondary emails.

To avoid that users can probe for secondary emails by doing a change
query (e.g. by doing a 'owner:<secondary-email>' query) we still check
whether the calling user has permission to see secondary emails when the
given user identifier is resolved to a secondary email and return an
empty result if not (we can't return an error as this would reveal to
the calling user that this email exists).

Skipping the account visibility check for change queries affects several
search predicates:

* owner / uploader / label + user / reviewer / attention / commentby:
  - Account visibility is not checked and matching changes are returned
    if they are visible.
  - For non-visible accounts that didn't participate in visible changes
    and non-existing accounts an empty result is returned. Note that
    the reviewer predicate already returned an empty result in this case
    (if the specified account doesn't exist or is not visible the
    reviewer predicate falls back to searching with the
    reviewerByEmailPredicate which is for matching reviewers by email
    that have no account). This means with this change we are making the
    behaviour of these predicates consistent again.

The following predicates which are also based on users still check
account visibility:

* visibleto:
  Account visibility is still checked. Whether a change is visible to an
  account cannot be seen on the change. This means users that can see
  the change, don't know about other accounts that can see the change.
  Skipping the account visibility check for this predicate could reveal
  the existence of non-visible accounts even if they didn't participate
  in any visible changes (if the query returns results users can deduce
  that the account exists).

* destination / query:
  Account visibility is still checked. These predicates read a resource
  of the account (a destination/query) that determines which changes
  should be matched. Accessing this resource should only be allowed if
  the account is visible, even if the account participated in any
  visible changes as these resources are not exposed via the visible
  changes.

Release-Notes: Account visibility is no longer checked when querying changes
Signed-off-by: Edwin Kempin <ekempin@google.com>
Bug: Google b/254824035
Change-Id: I022c6a13cd8df9284f526c9262a417cc757578dc
Signed-off-by: Edwin Kempin <ekempin@google.com>