How to safely update account external-ids in notedb

618 views
Skip to first unread message

Clark Boylan

unread,
Jan 20, 2021, 3:15:10 PM1/20/21
to Repo and Gerrit Discussion
We've run into a situation where a user modified their preferred
gerrit email address then modified their email address in our openid
provider. This initially hit a conflict within Gerrit because logging
in with the openid provider at that point tried to create a new
account (I suspect the openid provider updated the openid when the
email address changed), but this failed because the preferred email
address belonged to an existing account.

Eventually the user reverted the openid provider email changes setting
email back to a different value. This seems to have reverted the
openid url value as well, but since the email address didn't conflict
Gerrit updated the external id for that openid url with a new email
address. This effectively forced Gerrit to create a new account
because the email addresses for the existing account and the email
address associated with the openid no longer overlapped. Also the old
account is no longer accessible via openid login because no openid
external id is associated with it.

I'm not sure how well I've described this with words so here are the
current external-ids:

Old Account
[{"identity":"username:oldaccount","trusted":true},{"identity":"mailto:oldac...@foo.com","email_address":"oldac...@foo.com","trusted":true,"can_delete":true},{"identity":"mailto:diff...@bar.com","email_address":"diff...@bar.com","trusted":true,"can_delete":true}]

New Account
[{"identity":"https://login.somewhere.com/+id/stuff","email_address":"thir...@foo.com","trusted":true,"can_delete":true}]

Prior to notedb we would have addressed this with some straightforward
SQL table updates. The process was roughly to do an `UPDATE
account_external_ids SET account_id=12345 WHERE
external_id="https://login.foo.bar/+baz";` Then disable the account
that was created in error via the SSH or HTTP API and maybe flush
caches.

With notedb this no longer seems straightforward as the Gerrit api [0]
does not have methods for updating external ids other than to delete
them. One thought was that we could delete the openid external-id and
disable the new account then have the user login with the desired
email address. This would create a new external-id for the openid that
maps to the existing account's email addresses. But I'm fairly certain
Gerrit would throw an error claiming that there is an email address
conflict.

The external id notedb situation is documented [1] and that doc says
we can manually push an update to All-Users:refs/meta/external-ids.
Gerrit will sanity check this push and reject it if we've got
something fundamentally wrong. What I don't understand is how are we
supposed to avoid races between new users being created and pushing an
update to refs/meta/external-ids? It seems that this process is
inherently flawed, but I may be missing something obvious. We could
disable all external access to our server temporarily but that is less
than desirable. Maybe we rebase and retry until it is a fast
forwardable update?

Also, to make sure I've understood what the docs are telling us: I
would update the external-id for
"identity":"https://login.somewhere.com/+id/stuff" to set
"email_address":"oldac...@foo.com". Having the email address map to
the old existing account is what will tie them together? Do we need to
reindex accounts after doing this?

Any help on how to properly fix this with the least amount of risk to
All-Users external-id consistency would be appreciated.

Finally, it would be nice if we could avoid this problem in the future
(perhaps this points to a bug in openid external-id updates? eg don't
update them if it would detach an existing user account from its
openid). And exposing external-id management through the api if
possible would likely simplify this greatly for us.

[0] https://gerrit-review.googlesource.com/Documentation/rest-api-accounts.html#get-account-external-ids
[1] https://gerrit-review.googlesource.com/Documentation/config-accounts.html#external-ids

Clark

thomasmu...@yahoo.com

unread,
Jan 22, 2021, 1:13:10 PM1/22/21
to Repo and Gerrit Discussion
We've had similar issues see https://phabricator.wikimedia.org/T197192. I think that being able to edit external-id via rest api should be added/and being able to when pushing ignore certain validations.

Clark Boylan

unread,
Jan 22, 2021, 2:26:00 PM1/22/21
to Repo and Gerrit Discussion
On Wed, Jan 20, 2021 at 12:14 PM Clark Boylan <clark....@gmail.com> wrote:
>

snip
We have been testing things with a test server and have learned some
stuff but also have new questions. Our process was roughly this:

$ git clone ssh://admin@gerrit-test:29418/All-Users && cd All-Users
$ git fetch refs/meta/external-ids && git checkout FETCH_HEAD
$ edit `echo -n "https://login.somewhere.com/+id/stuff" | shasum` to
set accountId to the value present in `echo -n "username:oldaccount" |
shasum`
$ git add $file && git commit -m "Move openid from account 321 to 123"
$ git push origin HEAD:refs/meta/external-ids

Does this process look correct? It doesn't feel very git note'y, and I
want to make sure we are approaching this in a reasonable manner.

The first time we tried this push Gerrit rejected the push as it
included non fast forwardable changes. The answer to my earlier
question about races is that it is racy and you may have to rebase and
push until it goes through. Once we had rebased and pushed again,
Gerrit spent a bit of time processing changes then spit out 642 email
address conflicts and it rejected our openid edits. Some of these
conflicts exist between different openid external ids and others
between openid and mailto external ids. It looks like wikimedia ran
into this as well and filed some bugs about it [2][3].

This comment [4] in particular suggests that it is a bug to reject
updates to external ids that don't have conflicts. It appears that
this bug still exists in 3.2.5.1, does anyone know if this is getting
fixed?

That comment [4] also suggests that we can work around this by pushing
to All-Users directly while Gerrit is offline, then reindex all
accounts (does this need to be offline as well?) and flush account
caches. It also mentions that we should be very careful when modifying
All-Users directly like this to avoid introducing new errors. Do you
know if the 642 errors reported by Gerrit on our initial push is a
complete set of errors? If so we can likely test our pushes first with
gerrit online, then stop gerrit and push with a reasonable degree of
confidence we aren't making things worse. If Gerrit does fail and
report early before checking all conflicts is that another thing we
might be able to change in order to give users the most complete set
of info?

Assuming that all makes sense for today, do we think this can be
improved for the future? In particular it sounds like Gerrit shouldn't
reject pushes if they don't introduce new errors. After working
through this a bit I also think it would be helpful to have a REST API
method for modifying external ids, that way Gerrit can handle
sequencing and avoid the races on push. But I'm not sure what all the
implications of such a method are.

Finally, I'm happy to push changes to improve the external-id notedb
docs [5]. I just need a bit of direction, so any help here will
hopefully end up in the docs.

>
> Any help on how to properly fix this with the least amount of risk to
> All-Users external-id consistency would be appreciated.
>
> Finally, it would be nice if we could avoid this problem in the future
> (perhaps this points to a bug in openid external-id updates? eg don't
> update them if it would detach an existing user account from its
> openid). And exposing external-id management through the api if
> possible would likely simplify this greatly for us.
>
> [0] https://gerrit-review.googlesource.com/Documentation/rest-api-accounts.html#get-account-external-ids
> [1] https://gerrit-review.googlesource.com/Documentation/config-accounts.html#external-ids

[2] https://bugs.chromium.org/p/gerrit/issues/detail?id=9256
[3] https://phabricator.wikimedia.org/T197192
[4] https://bugs.chromium.org/p/gerrit/issues/detail?id=9256#c16
[5] https://gerrit-review.googlesource.com/Documentation/config-accounts.html#external-ids

>
> Clark

Jakub Sokół

unread,
Jan 22, 2021, 2:44:42 PM1/22/21
to Clark Boylan, Repo and Gerrit Discussion
Hi!


On Fri, 22 Jan 2021, 20:25 Clark Boylan, <clark....@gmail.com> wrote:
On Wed, Jan 20, 2021 at 12:14 PM Clark Boylan <clark....@gmail.com> wrote:
>

snip

> I'm not sure how well I've described this with words so here are the
> current external-ids:
>
> Old Account
> [{"identity":"username:oldaccount","trusted":true},{"identity":"mailto:oldac...@foo.com","email_address":"oldac...@foo.com","trusted":true,"can_delete":true},{"identity":"mailto:diff...@bar.com","email_address":"diff...@bar.com","trusted":true,"can_delete":true}]
>
> New Account
> [{"identity":"https://login.somewhere.com/+id/stuff","email_address":"thir...@foo.com","trusted":true,"can_delete":true}]
>
> Prior to notedb we would have addressed this with some straightforward
> SQL table updates. The process was roughly to do an `UPDATE
> account_external_ids SET account_id=12345 WHERE
> external_id="https://login.foo.bar/+baz";` Then disable the account
> that was created in error via the SSH or HTTP API and maybe flush
> caches.

While a bit less common/intuitive, I don't think that manipulating git branches is less risky than doing raw updates on database - I'd even risk saying that by making backups by creating a new ref and restoring backup by doing `git update-ref <old-sha1>` is easier to do right :-)

>
> With notedb this no longer seems straightforward as the Gerrit api [0]
> does not have methods for updating external ids other than to delete
> them. One thought was that we could delete the openid external-id and
> disable the new account then have the user login with the desired
> email address. This would create a new external-id for the openid that
> maps to the existing account's email addresses. But I'm fairly certain
> Gerrit would throw an error claiming that there is an email address
> conflict.
>
> The external id notedb situation is documented [1] and that doc says
> we can manually push an update to All-Users:refs/meta/external-ids.
> Gerrit will sanity check this push and reject it if we've got
> something fundamentally wrong. What I don't understand is how are we
> supposed to avoid races between new users being created and pushing an
> update to refs/meta/external-ids? It seems that this process is
> inherently flawed, but I may be missing something obvious. We could
> disable all external access to our server temporarily but that is less
> than desirable. Maybe we rebase and retry until it is a fast
> forwardable update?

I think that's exactly what you should do - if someone does update in between, push will simply be rejected, so there's no risk of race introducing unexpected changes. 

>
> Also, to make sure I've understood what the docs are telling us: I
> would update the external-id for
> "identity":"https://login.somewhere.com/+id/stuff" to set
> "email_address":"oldac...@foo.com". Having the email address map to
> the old existing account is what will tie them together? Do we need to
> reindex accounts after doing this?

We have been testing things with a test server and have learned some
stuff but also have new questions. Our process was roughly this:

  $ git clone ssh://admin@gerrit-test:29418/All-Users && cd All-Users
  $ git fetch refs/meta/external-ids && git checkout FETCH_HEAD
  $ edit `echo -n "https://login.somewhere.com/+id/stuff" | shasum` to
set accountId to the value present in `echo -n "username:oldaccount" |
shasum`
  $ git add $file && git commit -m "Move openid from account 321 to 123"
  $ git push origin HEAD:refs/meta/external-ids

Does this process look correct? It doesn't feel very git note'y, and I
want to make sure we are approaching this in a reasonable manner.

It sounds to me that what you're trying to achieve is `git revert` of commits that are now problematic i.e one that created new account and one that changed external-id. Wouldn't it be enough?

--
--
To unsubscribe, email repo-discuss...@googlegroups.com
More info at http://groups.google.com/group/repo-discuss?hl=en

---
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/repo-discuss/CAAcH74iEW40yPW-CZVMoQ4toV5UWNUXkYRjPRY%2BweMGOJ6S25A%40mail.gmail.com.

Clark Boylan

unread,
Jan 22, 2021, 3:27:46 PM1/22/21
to Repo and Gerrit Discussion
On Fri, Jan 22, 2021 at 11:44 AM Jakub Sokół <jso...@vewd.com> wrote:
>
> Hi!
>
> On Fri, 22 Jan 2021, 20:25 Clark Boylan, <clark....@gmail.com> wrote:
>>
>> On Wed, Jan 20, 2021 at 12:14 PM Clark Boylan <clark....@gmail.com> wrote:
>> >
>>
>> snip
>>
>> > I'm not sure how well I've described this with words so here are the
>> > current external-ids:
>> >
>> > Old Account
>> > [{"identity":"username:oldaccount","trusted":true},{"identity":"mailto:oldac...@foo.com","email_address":"oldac...@foo.com","trusted":true,"can_delete":true},{"identity":"mailto:diff...@bar.com","email_address":"diff...@bar.com","trusted":true,"can_delete":true}]
>> >
>> > New Account
>> > [{"identity":"https://login.somewhere.com/+id/stuff","email_address":"thir...@foo.com","trusted":true,"can_delete":true}]
>> >
>> > Prior to notedb we would have addressed this with some straightforward
>> > SQL table updates. The process was roughly to do an `UPDATE
>> > account_external_ids SET account_id=12345 WHERE
>> > external_id="https://login.foo.bar/+baz";` Then disable the account
>> > that was created in error via the SSH or HTTP API and maybe flush
>> > caches.
>
>
> While a bit less common/intuitive, I don't think that manipulating git branches is less risky than doing raw updates on database - I'd even risk saying that by making backups by creating a new ref and restoring backup by doing `git update-ref <old-sha1>` is easier to do right :-)

I agree that this gives you a nice recovery method as well as logging
which is excellent. I was more trying to call out that a sql database
and its tables describe themselves and have a well known language for
making changes to them. That additional information helps operators in
understanding what is necessary to create the desired changes in the
system. The notedb system seems to lack that (based on my vast number
of questions). As mentioned below I'm happy to help address that by
improving the docs once I've better understood the setup here.

>
>> >
>> > With notedb this no longer seems straightforward as the Gerrit api [0]
>> > does not have methods for updating external ids other than to delete
>> > them. One thought was that we could delete the openid external-id and
>> > disable the new account then have the user login with the desired
>> > email address. This would create a new external-id for the openid that
>> > maps to the existing account's email addresses. But I'm fairly certain
>> > Gerrit would throw an error claiming that there is an email address
>> > conflict.
>>
>> >
>> > The external id notedb situation is documented [1] and that doc says
>> > we can manually push an update to All-Users:refs/meta/external-ids.
>> > Gerrit will sanity check this push and reject it if we've got
>> > something fundamentally wrong. What I don't understand is how are we
>> > supposed to avoid races between new users being created and pushing an
>> > update to refs/meta/external-ids? It seems that this process is
>> > inherently flawed, but I may be missing something obvious. We could
>> > disable all external access to our server temporarily but that is less
>> > than desirable. Maybe we rebase and retry until it is a fast
>> > forwardable update?
>
>
> I think that's exactly what you should do - if someone does update in between, push will simply be rejected, so there's no risk of race introducing unexpected changes.

Yes, we've confirmed this is the case through testing.

>
>> >
>> > Also, to make sure I've understood what the docs are telling us: I
>> > would update the external-id for
>> > "identity":"https://login.somewhere.com/+id/stuff" to set
>> > "email_address":"oldac...@foo.com". Having the email address map to
>> > the old existing account is what will tie them together? Do we need to
>> > reindex accounts after doing this?
>>
>> We have been testing things with a test server and have learned some
>> stuff but also have new questions. Our process was roughly this:
>>
>> $ git clone ssh://admin@gerrit-test:29418/All-Users && cd All-Users
>> $ git fetch refs/meta/external-ids && git checkout FETCH_HEAD
>> $ edit `echo -n "https://login.somewhere.com/+id/stuff" | shasum` to
>> set accountId to the value present in `echo -n "username:oldaccount" |
>> shasum`
>> $ git add $file && git commit -m "Move openid from account 321 to 123"
>> $ git push origin HEAD:refs/meta/external-ids
>>
>> Does this process look correct? It doesn't feel very git note'y, and I
>> want to make sure we are approaching this in a reasonable manner.
>
>
> It sounds to me that what you're trying to achieve is `git revert` of commits that are now problematic i.e one that created new account and one that changed external-id. Wouldn't it be enough?

I don't think so because then we would be in the old state which means
the next time the user logs in we will end up right back where we
started. I believe we need to update the external-ids such that the
openids and emails all belong to the same account. Then when they
login again gerrit shouldn't shift the openid over to a new account
due to a change in email addresses.

David Ostrovsky

unread,
Jan 23, 2021, 11:45:48 AM1/23/21
to Repo and Gerrit Discussion
clark....@gmail.com schrieb am Mittwoch, 20. Januar 2021 um 21:15:10 UTC+1:
We've run into a situation where a user modified their preferred
gerrit email address then modified their email address in our openid
provider. This initially hit a conflict within Gerrit because logging
in with the openid provider at that point tried to create a new
account (I suspect the openid provider updated the openid when the
email address changed), but this failed because the preferred email
address belonged to an existing account.

Clark Boylan

unread,
Jan 25, 2021, 6:17:48 PM1/25/21
to Repo and Gerrit Discussion
This was super helpful, thank you for pointing me to that. Andrew, do
you mind if I try and upstream some of this into the Gerrit docs? I
think some practical user management processes in the docs would go a
long way to making this less painful.

Clark

Andrew Grimberg

unread,
Jan 26, 2021, 11:40:10 AM1/26/21
to Clark Boylan, Repo and Gerrit Discussion
Clark,

Be my guest. I wrote up that because my team services 10 different
Gerrit systems, our oldest having started back in March 2013 and given
time and upgrades we had built up some issues that the transition to
noteDB raised ;)

That oldest system has actually had 3 different authentication backends
over its lifetime so it didn't surprise me that my having to fix all the
problems it had ended up causing me to do some major documentation. like
that.

I would be more than happy to work through a review of documentation
that is added as it's so firmly stuck in my head at this point that
making it more available to people (outside of just the mailing list
post I did) would be good!

-Andy-
OpenPGP_0x3360FFB703A9DA1F_and_old_rev.asc
OpenPGP_signature

Doug Luedtke

unread,
Jan 27, 2021, 4:00:41 PM1/27/21
to Repo and Gerrit Discussion
An option in the UI to update account usernames and emails would be helpful. I'm on my second account to fix this month. Both were returning users where IT gave them new usernames, but still the same email address.

Andrew Grimberg

unread,
Jan 27, 2021, 4:18:46 PM1/27/21
to Doug Luedtke, Repo and Gerrit Discussion
I would love for something like this, but I can also say that there is
(AFAIK) no administrative UI for manipulating any user account
configuration outside of group management.

-Andy-
OpenPGP_0x3360FFB703A9DA1F_and_old_rev.asc
OpenPGP_signature
Reply all
Reply to author
Forward
0 new messages