It is expected that this block will handle the case you describe, note the comment about rebinding with anonymous credentials and the actual rebind lower down.

$result = $this->searchLDAPForRecord($search_query);
if (!$result) {
  // This is unusual (since the bind succeeded) but we've seen it at least
  // once in the wild, where the anonymous user is allowed to search but
  // the credentialed user is not.

  // If we don't have anonymous credentials, raise an explicit exception
  // here since we'll fail a typehint if we don't return an array anyway
  // and this is a more useful error.

  // If we do have anonymous credentials, we'll rebind and try the search
  // again below. Doing this automatically means things work correctly more
  // often without requiring additional configuration.
  if (!$this->shouldBindWithoutIdentity()) {
    // No anonymous credentials, so we just fail here.
    throw new Exception(
      pht(
        'LDAP: Failed to retrieve record for user "%s" when searching. '.
        'Credentialed users may not be able to search your LDAP server. '.
        'Try configuring anonymous credentials or fully anonymous binds.',
        $login_user));
  } else {
    // Rebind as anonymous and try the search again.
    $user = $this->anonymousUsername;
    $pass = $this->anonymousPassword;
    $this->bindLDAP($conn, $user, $pass);

    $result = $this->searchLDAPForRecord($search_query);
    if (!$result) {
      throw new Exception(
        pht(
          'LDAP: Failed to retrieve record for user "%s" when searching '.
          'with both user and anonymous credentials.',
          $login_user));
    }
  }
}

What behavior are you seeing without your patch?

In my authentication module I have e.g. this code for my PhutilAuthAdapter:

public function getAccountRealName() {
  $username = explode("@", $this->getAccountID(), 2)[0];
  $provider = PhabricatorLDAPAuthProvider::getLDAPProvider();

  $adapter = $provider->getAdapter()
    ->setLoginUsername($username);

  return $adapter->getAccountRealName();
}

The idea is to get the (in my installation there is only one) LDAP data source and ask it for the data of the user my module authenticated. Since I do not know the user's password, I can only provide the username. However, PhutilLDAPAuthAdapter will crash (with some cryptic error message), since it assumes that if $login_user is provided, so is $login_pass (which is wrong in my case). Thus I need it to not try binding as the user if that is bound to fail.

I assume my patch does not change current behaviour for others, or significantly increase your maintenance burden and thus does not hurt anyone. But it helps me reduce the amount of patches necessary for T7641#103110.

Atleast you might want to improve handling of the case where loginUsername is set, but loginPassword is not.

In T7692#104265, @epriestley wrote:

In some setups, your patch allows any attacker to log in to any LDAP account by just not typing a password.

Oh, I assume that was handled by the password envelope never being null, even if it contains an empty string.

Please don't file issues which depend on local patches. Even if they're real bugs, we probably won't fix them if no users can actually encounter them. This is a very poor use of our time in general, and often makes the software more fragile because the behavior won't be tested regularly since it's unreachable.

I.e. you will not fix the weird exception when loginUsername is set, but loginPassword is not, because PhabricatorLDAPAuthProvider never calls PhutilAuthAdapter in a way where that might happen? (src/applications/auth/provider/PhabricatorLDAPAuthProvider.php:152 and 161/173)

I assume that was handled by the password envelope

Ah, I think you're right. However, this interaction is not obvious and might change in the future.

I.e. you will not fix the weird exception when loginUsername is set, but loginPassword is not, because PhabricatorLDAPAuthProvider never calls PhutilAuthAdapter in a way where that might happen?

Correct.