It seems like previously we were doing a best effort to match the account. Can you explain more to me the reason for behavior change to return nil, nil all the time instead? Just curious...

Ah, yes, so FetchAccount is used in cases like Bitbucket Server or the previous attempt at implementing Gerrit user permissions, where we don't have a way of connecting to a user account. So in those cases, we take a Sourcegraph property, like the user's email or username, and fetch user accounts on the Code Host using the code host PAT, and try to match them up. And then we sync permissions for that user using Repo-based permissions syncing

But because we're adding user-based permissions syncing now with a user providing HTTP credentials, this is no longer required.

IIRC, if we do not have the AuthData, it means, that we do not have credentials. Should we instead say that in the error, since that seems to be the more important error than not having the external account metadata?

Also, in what kind of scenario can this actually happen? If we add the account through addExternalAccount mutation, there should always be data, right? Otherwise the mutation itself fails, as far as I understand.

Yeah I was contemplating this scenario myself, but kinda just followed the existing patterns of the other auth providers.

I almost think this is only likely to occur in unit testing, where the programmer can forget to add AuthData when providing an account to test this 🤷‍♂️

This shouldn't occur naturally in a production environment.

Similarly, the error above it where it checks for the right account type, that also shouldn't occur naturally, since the perms syncer where this is used already matches the accounts to the providers.

Feel like there is probably a balance to maintain here between overly cautious checking vs. clarity/readability

@mrnugget lately commented on my PR, that using global mocks is not a good practice and we should strive to not use them anymore. Is there another, cleaner way to mock VerifyAccount? For example by injecting it into the addGerritExternalAccount function?

In that case there was a struct on which to "inject" the dependency, here it's just a free-standing function. In that case I've also resorted to using global mock vars (but don't like it). If it makes sense to create a struct (gerritAccountVerifier struct { verifyFn func(...) }) then it's better to inject the method, I think

This makes me sad 😭

Very much 😭

It might be worth putting all of this pagination complexity into another function. The code might look cleaner afterwards, with smaller functions. But feel free to disagree.

lol just read this after replying to your comment above. But yes I agree, I think I'm just going to split these two scenarios altogether