Merge changes from topic 'oauth-authentication-provider' into stable-2.10

* changes:
  Add log messages to troubleshoot OAuth/OpenID linking
  OAuth: Allow to link claimed identity to existing accounts
  OAuth: Allow to change username

5 files changed, 54 insertions, 6 deletions

diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/auth/oauth/OAuthUserInfo.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/auth/oauth/OAuthUserInfo.java
index 23a7bec4b4..388ce361e5 100644
--- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/auth/oauth/OAuthUserInfo.java
+++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/auth/oauth/OAuthUserInfo.java
@@ -20,15 +20,18 @@ public class OAuthUserInfo {
   private final String userName;
   private final String emailAddress;
   private final String displayName;
+  private final String claimedIdentity;
 
   public OAuthUserInfo(String externalId,
       String userName,
       String emailAddress,
-      String displayName) {
+      String displayName,
+      String claimedIdentity) {
     this.externalId = externalId;
     this.userName = userName;
     this.emailAddress = emailAddress;
     this.displayName = displayName;
+    this.claimedIdentity = claimedIdentity;
   }
 
   public String getExternalId() {
@@ -46,4 +49,8 @@ public class OAuthUserInfo {
   public String getDisplayName() {
     return displayName;
   }
+
+  public String getClaimedIdentity() {
+    return claimedIdentity;
+  }
 }
diff --git a/gerrit-oauth/BUCK b/gerrit-oauth/BUCK
index 4641e8168e..fa5a8e2cb1 100644
--- a/gerrit-oauth/BUCK
+++ b/gerrit-oauth/BUCK
@@ -11,9 +11,11 @@ java_library(
     '//gerrit-common:annotations',
     '//gerrit-extension-api:api',
     '//gerrit-httpd:httpd',
+    '//gerrit-reviewdb:server',
     '//gerrit-server:server',
     '//lib:gson',
     '//lib:guava',
+    '//lib:gwtorm',
     '//lib/commons:codec',
     '//lib/guice:guice',
     '//lib/guice:guice-servlet',
diff --git a/gerrit-oauth/src/main/java/com/google/gerrit/httpd/auth/oauth/OAuthSession.java b/gerrit-oauth/src/main/java/com/google/gerrit/httpd/auth/oauth/OAuthSession.java
index d625e02abd..6e3ea7aad6 100644
--- a/gerrit-oauth/src/main/java/com/google/gerrit/httpd/auth/oauth/OAuthSession.java
+++ b/gerrit-oauth/src/main/java/com/google/gerrit/httpd/auth/oauth/OAuthSession.java
@@ -23,9 +23,11 @@ import com.google.gerrit.extensions.auth.oauth.OAuthUserInfo;
 import com.google.gerrit.extensions.auth.oauth.OAuthVerifier;
 import com.google.gerrit.extensions.registration.DynamicItem;
 import com.google.gerrit.httpd.WebSession;
+import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.server.account.AccountException;
 import com.google.gerrit.server.account.AccountManager;
 import com.google.gerrit.server.account.AuthResult;
+import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.servlet.SessionScoped;
 
@@ -113,11 +115,48 @@ class OAuthSession {
       throws IOException {
     com.google.gerrit.server.account.AuthRequest areq =
         new com.google.gerrit.server.account.AuthRequest(user.getExternalId());
-    areq.setUserName(user.getUserName());
-    areq.setEmailAddress(user.getEmailAddress());
-    areq.setDisplayName(user.getDisplayName());
     AuthResult arsp;
     try {
+      String claimedIdentifier = user.getClaimedIdentity();
+      Account.Id actualId = accountManager.lookup(user.getExternalId());
+      if (!Strings.isNullOrEmpty(claimedIdentifier)) {
+        Account.Id claimedId = accountManager.lookup(claimedIdentifier);
+        if (claimedId != null && actualId != null) {
+          if (claimedId.equals(actualId)) {
+            // Both link to the same account, that's what we expected.
+            log.debug("OAuth2: claimed identity equals current id");
+          } else {
+            // This is (for now) a fatal error. There are two records
+            // for what might be the same user.
+            //
+            log.error("OAuth accounts disagree over user identity:\n"
+                + "  Claimed ID: " + claimedId + " is " + claimedIdentifier
+                + "\n" + "  Delgate ID: " + actualId + " is "
+                + user.getExternalId());
+            rsp.sendError(HttpServletResponse.SC_FORBIDDEN);
+            return;
+          }
+        } else if (claimedId != null && actualId == null) {
+          // Claimed account already exists: link to it.
+          //
+          log.info("OAuth2: linking claimed identity to {}",
+              claimedId.toString());
+          try {
+            accountManager.link(claimedId, areq);
+          } catch (OrmException e) {
+            log.error("Cannot link: " +  user.getExternalId()
+                + " to user identity:\n"
+                + "  Claimed ID: " + claimedId + " is " + claimedIdentifier);
+            rsp.sendError(HttpServletResponse.SC_FORBIDDEN);
+            return;
+          }
+        }
+      } else {
+        log.debug("OAuth2: claimed identity is empty");
+      }
+      areq.setUserName(user.getUserName());
+      areq.setEmailAddress(user.getEmailAddress());
+      areq.setDisplayName(user.getDisplayName());
       arsp = accountManager.authenticate(areq);
     } catch (AccountException e) {
       log.error("Unable to authenticate user \"" + user + "\"", e);
diff --git a/gerrit-openid/src/main/java/com/google/gerrit/httpd/auth/openid/OpenIdServiceImpl.java b/gerrit-openid/src/main/java/com/google/gerrit/httpd/auth/openid/OpenIdServiceImpl.java
index 1681bc248e..44b58c39ed 100644
--- a/gerrit-openid/src/main/java/com/google/gerrit/httpd/auth/openid/OpenIdServiceImpl.java
+++ b/gerrit-openid/src/main/java/com/google/gerrit/httpd/auth/openid/OpenIdServiceImpl.java
@@ -157,6 +157,7 @@ class OpenIdServiceImpl {
     final AuthRequest aReq;
     try {
       aReq = manager.authenticate(state.discovered, state.retTo.toString());
+      log.debug("OpenID: openid-realm={}", state.contextUrl);
       aReq.setRealm(state.contextUrl);
 
       if (requestRegistration(aReq)) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/DefaultRealm.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/DefaultRealm.java
index 4b8f0b4b1e..938d940ac6 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/DefaultRealm.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/DefaultRealm.java
@@ -40,8 +40,7 @@ public class DefaultRealm implements Realm {
 
   @Override
   public boolean allowsEdit(final Account.FieldName field) {
-    if (authConfig.getAuthType() == AuthType.HTTP
-        || authConfig.getAuthType() == AuthType.OAUTH) {
+    if (authConfig.getAuthType() == AuthType.HTTP) {
       switch (field) {
         case USER_NAME:
           return false;