commit 8367b862f36529a487f12f9c0b45e94e38d0a9b8
author Edwin Kempin <ekempin@google.com> Wed Feb 01 15:05:35 2023 +0100
committer Edwin Kempin <ekempin@google.com> Thu Feb 02 12:21:52 2023 +0100
tree bd0cdf864cc1c91ab8fc2d926ddf3a9debdcd9c0
parent 7110c26ed1387fbe1ec1aee608811ecb34fc2355

AccountResolver: Fix account visibility checking

The ByFullName and ByDefaultSearch searchers have an
allowSkippingVisibilityCheck option that controls whether the visibility
check for the returned results can be skipped. This option has been
added in change I5dd345826 based on a misunderstanding of why these
searchers hard-coded true as the return value for the
callerMayAssumeCandidatesAreVisible() method. When adding the
allowSkippingVisibilityCheck option it was assumed that these searchers
skip the visibility check because they return true from the
callerMayAssumeCandidatesAreVisible() and hence to enforce a visibility
check we would need a new option to make this method return false.

In fact both searches already did an account visibility check and now
account visibility is checked twice if allowSkippingVisibilityCheck is
false:

1. Once the account visibility is checked when executing the account
   query in the tryParse/search method by using enforceVisibility(true)
   on InternalAccountQuery that is used to do the account query.

2. Once the account visibility is checked in the searchImpl method when
   searcher.callerMayAssumeCandidatesAreVisible() returns false.

The account visibility check at 1. is done for the current user while
the account visibility check at 2. is done for the user on whom's behalf
the accounts should be resolved (this is either the current user if the
resolve method is used, or the provided user if the resolveAsUser method
is used).

This means the check done at 1. is wrong when the resolveAsUser method
is used to resolve accounts on behalf of a user that is not the current
user.

To fix this we remove the usage of enforceVisibility(true) which always
checks the visibility for the current user and instead we always perform
the visibility check through 2. For this we remove the overrides of the
callerMayAssumeCandidatesAreVisible() for these searches so that the
default implementation of this method is used that always return false.

Doing this requires converting the ByFullName searcher into a
StringSearcher:

* old behaviour:
  tryParse was querying visible accounts by fullname and disregarded the
  result if there was not exactly one match. When searching, the search
  method simply returned that one match without any further check.

* new behaviour:
  When searching, the account index is queried by fullname without
  checking visibility and all results are returned. The visibility check
  is performed afterwards in searchImpl due to
  callerMayAssumeCandidatesAreVisible returning false now. In contrast
  to the old behaviour it may happen now that we find multiple visible
  accounts with the same full name. This is OK, since callers decide
  after retrieving the result if they require a unique match (see
  AccountResolver.Result#asUnique()) or if they are fine with multiple
  matches (see AccountResolver.Result#asList()). It might have actually
  been a bug with the old behaviour that finding mutliple accounts with
  the same full name did not work even when the caller was fine with
  getting mutltiple results.

Since the allowSkippingVisibilityCheck option is removed and
callerMayAssumeCandidatesAreVisible() always returns false for these
searchers now, we can remove the forcedVisibilitySearchers list of
searchers and the forceVisibilityCheck flag from the resolveAsUser
method, which simplifies AccountResolver.

Alternatively we considered dropping the visibility check at 2. (always
return true from callerMayAssumeCandidatesAreVisible()) and fix the
visibility checks at 1. to take the user on whom's behalf accounts are
resolved into account (e.g. replace the enforceVisibility(true) call
with an enforceVisibilityFor(user) call) but doing this turned out to be
quite tricky (e.g. QueryProcessor to which the call is delegated cannot
depend on CurrentUser without introducing a cyclic dependency in Guice
and fixing this would require us to rethink about how our build modules
should be structured). Hence we didn't take this alternative.

Release-Notes: skip
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I90c2cd6245929eb98687bb4e70e803b88d467f22