opendev/gerrit
Code Issues Proposed changes

OAuth-Linking: Don't create new account when claimed identity unknown

Browse Source 
Claimed Identity feature was enabled to support old Google OpenID
accounts, that cannot be activated anymore. In some corner cases,
when for example the URL is not from the production Gerrit site,
like it's always the case on staging instance, the OpenID identity
may deviate from the original one. In case of mismatch, the lookup
for real user for the claimed identity would fail, and the linking
mode is lost, and as the consequence a new account is created.

Creating new account, when user asked for linking is always the
worst option we have, as this cannot be easily undone. Detect this
case, preserve the linking mode and keep trying to link instead of
create new account. Note that in case this account already exist,
the linking would fail with the sane message.

Test Plan I:

1. Create gerrit site gerrit.example.org in year 2010
2. Configure the site to use OpenId (non SSO mode)
3. Observe that 85% user base using Google OpeID
4. After Google's OpenID shutdown in May 2015, nobody is able
   to login anymore using their account and link their identity
   primary to Launchpad
5. Swap production site to staging site with entire database
   and Git repository
6. Install gerrit-oauth-provider plugin and activate automagically
   Google OpenID old token discovery and linking option
7. Configure new OAuth application on Google developer console, but
   route it to gerrit-test.example.org. Note that this deviation breaks
   the old OpenID tokens!
8. Test with old user, that has OpenID Google account, that was
   additionally linked to Launchpad OpenID provider
9. Login with Launchpad OpenID iendtity for this user
10. Profile=>Setting=>Link another identity
11. Select Google OAuth provider offered by the gerrit-oauth-provider
    plugin
12. Intead of linking to the existing account (or linking error) new
    account is created
13. This diff fixed this. Error is issued, that the account already
    exist and linking is not possible

Note that when all this would be done on real production site, this
error wouldn't happen, because the URL wouldn't deviate and there
wouldn't be token mismatch between OpenID token in the database and the
token discovered by Google's OAuth OpenID scope. But still, we could
easily prevent in this very specific corner case the creation of new
account.

Note that it's still possible with this setting to create duplicate
account for this user by signing in directly with Google OAuth provider
without linking mode. However, this would also work as expected on the
real production site, because old OpenID token would match with the
existing Gerrit account and linking would happen automagically. That is
exactly why the option link-to-existing-openid-accounts = true was
invented. Unfortunately there is no way to test that this work as
expected already on staging Gerrit instance.

Test Plan II:

1. User register first time after Google's OpenID 2.0 shut down with
   OpenID provider, say Launchpad
2. Login with Launchpad
3. Profile=>Setting=>Link another identity
4. Link with Google OAuth2 provider

Expected: The OAuth2 identity is linked to the existing account.
Actual: New account is created.

This diff fixed it. The problem is that I apparently misunderstood
the migration spec: [1] and assumed that the OpenID token is provided
only when a user was already associated with this site. This is
not true. OpenID token also returned for new users, that were never
registered with this site before. To rectify it, and still to work
for both, known and unknown users, we apply the check. If the OpenID
token is known use it for linking. If it is not known, ignore it, but
preserve linking mode.

* [1] https://developers.google.com/identity/protocols/OpenID2Migration#map-identifiers

Change-Id: Icf70cde5fd96cd72aa383218e1d143107a551b45
This commit is contained in:
David Ostrovsky 2015-12-08 23:48:58 +01:00 committed by David Ostrovsky
parent c9f5a5ab76
commit e6ac4ad532
1 changed files with 22 additions and 6 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

28
gerrit-openid/src/main/java/com/google/gerrit/httpd/auth/openid/OAuthSessionOverOpenID.java
View File

@ -125,16 +125,32 @@ class OAuthSessionOverOpenID {
try {
String claimedIdentifier = user.getClaimedIdentity();
Account.Id actualId = accountManager.lookup(user.getExternalId());
// Use case 1: claimed identity was provided during handshake phase
Account.Id claimedId = null;
// We try to retrieve claimed identity.
// For some reason, for example staging instance
// it may deviate from the really old OpenID identity.
// What we want to avoid in any event is to create new
// account instead of linking to the existing one.
// That why we query it here, not to lose linking mode.
if (!Strings.isNullOrEmpty(claimedIdentifier)) {
Account.Id claimedId = accountManager.lookup(claimedIdentifier);
if (claimedId != null && actualId != null) {
claimedId = accountManager.lookup(claimedIdentifier);
if (claimedId == null) {
log.debug("Claimed identity is unknown");
}
}
// Use case 1: claimed identity was provided during handshake phase
// and user account exists for this identity
if (claimedId != null) {
log.debug("Claimed identity is set and is known");
if (actualId != null) {
if (claimedId.equals(actualId)) {
// Both link to the same account, that's what we expected.
} else {
// This is (for now) a fatal error. There are two records
// for what might be the same user.
//
// for what might be the same user. The admin would have to
// link the accounts manually.
log.error("OAuth accounts disagree over user identity:\n"
+ " Claimed ID: " + claimedId + " is " + claimedIdentifier
+ "\n" + " Delgate ID: " + actualId + " is "
@ -142,7 +158,7 @@ class OAuthSessionOverOpenID {
rsp.sendError(HttpServletResponse.SC_FORBIDDEN);
return;
}
} else if (claimedId != null && actualId == null) {
} else {
// Claimed account already exists: link to it.
//
try {