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

* changes:
  OAuth: Simplify protocol implementation
  Allow to link user identity to another OAuth provider

3 files changed, 84 insertions, 61 deletions

diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/MyIdentitiesScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/MyIdentitiesScreen.java
index e0a6d8f9b8..1f4d7ed20f 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/MyIdentitiesScreen.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/MyIdentitiesScreen.java
@@ -59,7 +59,8 @@ public class MyIdentitiesScreen extends SettingsScreen {
     });
     add(deleteIdentity);
 
-    if (Gerrit.getConfig().getAuthType() == AuthType.OPENID) {
+    if (Gerrit.getConfig().getAuthType() == AuthType.OPENID
+        || Gerrit.getConfig().getAuthType() == AuthType.OAUTH) {
       Button linkIdentity = new Button(Util.C.buttonLinkIdentity());
       linkIdentity.addClickHandler(new ClickHandler() {
         @Override
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 739dffe52a..d24c8a0017 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
@@ -26,11 +26,14 @@ import com.google.gerrit.extensions.restapi.Url;
 import com.google.gerrit.httpd.CanonicalWebUrl;
 import com.google.gerrit.httpd.WebSession;
 import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.account.AccountException;
 import com.google.gerrit.server.account.AccountManager;
+import com.google.gerrit.server.account.AuthRequest;
 import com.google.gerrit.server.account.AuthResult;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
+import com.google.inject.Provider;
 import com.google.inject.servlet.SessionScoped;
 
 import org.apache.commons.codec.binary.Base64;
@@ -52,18 +55,22 @@ class OAuthSession {
   private static final SecureRandom randomState = newRandomGenerator();
   private final String state;
   private final DynamicItem<WebSession> webSession;
+  private final Provider<IdentifiedUser> identifiedUser;
   private final AccountManager accountManager;
   private final CanonicalWebUrl urlProvider;
   private OAuthServiceProvider serviceProvider;
   private OAuthToken token;
   private OAuthUserInfo user;
   private String redirectToken;
+  private boolean linkMode;
 
   @Inject
   OAuthSession(DynamicItem<WebSession> webSession,
+      Provider<IdentifiedUser> identifiedUser,
       AccountManager accountManager,
       CanonicalWebUrl urlProvider) {
     this.state = generateRandomState();
+    this.identifiedUser = identifiedUser;
     this.webSession = webSession;
     this.accountManager = accountManager;
     this.urlProvider = urlProvider;
@@ -79,10 +86,6 @@ class OAuthSession {
 
   boolean login(HttpServletRequest request, HttpServletResponse response,
       OAuthServiceProvider oauth) throws IOException {
-    if (isLoggedIn()) {
-      return true;
-    }
-
     log.debug("Login " + this);
 
     if (isOAuthFinal(request)) {
@@ -122,46 +125,19 @@ class OAuthSession {
 
   private void authenticateAndRedirect(HttpServletRequest req,
       HttpServletResponse rsp) throws IOException {
-    com.google.gerrit.server.account.AuthRequest areq =
-        new com.google.gerrit.server.account.AuthRequest(user.getExternalId());
+    AuthRequest areq = new AuthRequest(user.getExternalId());
     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;
-          }
+        if (!authenticateWithIdentityClaimedDuringHandshake(areq, rsp,
+            claimedIdentifier)) {
+          return;
+        }
+      } else if (linkMode) {
+        if (!authenticateWithLinkedIdentity(areq, rsp)) {
+          return;
         }
-      } else {
-        log.debug("OAuth2: claimed identity is empty");
       }
       areq.setUserName(user.getUserName());
       areq.setEmailAddress(user.getEmailAddress());
@@ -181,6 +157,59 @@ class OAuthSession {
     rsp.sendRedirect(rdr.toString());
   }
 
+  private boolean authenticateWithIdentityClaimedDuringHandshake(
+      AuthRequest req, HttpServletResponse rsp, String claimedIdentifier)
+      throws AccountException, IOException {
+    Account.Id claimedId = accountManager.lookup(claimedIdentifier);
+    Account.Id actualId = accountManager.lookup(user.getExternalId());
+    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 false;
+      }
+    } 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, req);
+      } catch (OrmException e) {
+        log.error("Cannot link: " +  user.getExternalId()
+            + " to user identity:\n"
+            + "  Claimed ID: " + claimedId + " is " + claimedIdentifier);
+        rsp.sendError(HttpServletResponse.SC_FORBIDDEN);
+        return false;
+      }
+    }
+    return true;
+  }
+
+  private boolean authenticateWithLinkedIdentity(AuthRequest areq,
+      HttpServletResponse rsp) throws AccountException, IOException {
+    try {
+      accountManager.link(identifiedUser.get().getAccountId(), areq);
+    } catch (OrmException e) {
+      log.error("Cannot link: " + user.getExternalId()
+          + " to user identity: " + identifiedUser.get().getAccountId());
+      rsp.sendError(HttpServletResponse.SC_FORBIDDEN);
+      return false;
+    } finally {
+      linkMode = false;
+    }
+    return true;
+  }
+
   void logout() {
     token = null;
     user = null;
@@ -224,4 +253,12 @@ class OAuthSession {
   public OAuthServiceProvider getServiceProvider() {
     return serviceProvider;
   }
+
+  public void setLinkMode(boolean linkMode) {
+    this.linkMode = linkMode;
+  }
+
+  public boolean isLinkMode() {
+    return linkMode;
+  }
 }
diff --git a/gerrit-oauth/src/main/java/com/google/gerrit/httpd/auth/oauth/OAuthWebFilter.java b/gerrit-oauth/src/main/java/com/google/gerrit/httpd/auth/oauth/OAuthWebFilter.java
index 48963a6e51..91c3e33b7f 100644
--- a/gerrit-oauth/src/main/java/com/google/gerrit/httpd/auth/oauth/OAuthWebFilter.java
+++ b/gerrit-oauth/src/main/java/com/google/gerrit/httpd/auth/oauth/OAuthWebFilter.java
@@ -23,7 +23,6 @@ import com.google.gerrit.extensions.registration.DynamicMap;
 import com.google.gerrit.httpd.HtmlDomUtil;
 import com.google.gerrit.httpd.LoginUrlToken;
 import com.google.gerrit.httpd.template.SiteHeaderFooter;
-import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.config.CanonicalWebUrl;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
@@ -48,7 +47,6 @@ import javax.servlet.ServletRequest;
 import javax.servlet.ServletResponse;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
-import javax.servlet.http.HttpSession;
 
 @Singleton
 /* OAuth web filter uses active OAuth session to perform OAuth requests */
@@ -56,7 +54,6 @@ class OAuthWebFilter implements Filter {
   static final String GERRIT_LOGIN = "/login";
 
   private final Provider<String> urlProvider;
-  private final Provider<CurrentUser> currentUserProvider;
   private final Provider<OAuthSession> oauthSessionProvider;
   private final DynamicMap<OAuthServiceProvider> oauthServiceProviders;
   private final SiteHeaderFooter header;
@@ -64,12 +61,10 @@ class OAuthWebFilter implements Filter {
 
   @Inject
   OAuthWebFilter(@CanonicalWebUrl @Nullable Provider<String> urlProvider,
-      Provider<CurrentUser> currentUserProvider,
       DynamicMap<OAuthServiceProvider> oauthServiceProviders,
       Provider<OAuthSession> oauthSessionProvider,
       SiteHeaderFooter header) {
     this.urlProvider = urlProvider;
-    this.currentUserProvider = currentUserProvider;
     this.oauthServiceProviders = oauthServiceProviders;
     this.oauthSessionProvider = oauthSessionProvider;
     this.header = header;
@@ -88,30 +83,20 @@ class OAuthWebFilter implements Filter {
   public void doFilter(ServletRequest request, ServletResponse response,
       FilterChain chain) throws IOException, ServletException {
     HttpServletRequest httpRequest = (HttpServletRequest) request;
-    HttpSession httpSession = ((HttpServletRequest) request).getSession(false);
+    HttpServletResponse httpResponse = (HttpServletResponse) response;
+
     OAuthSession oauthSession = oauthSessionProvider.get();
-    if (currentUserProvider.get().isIdentifiedUser()) {
-      if (httpSession != null) {
-        httpSession.invalidate();
-      }
-      chain.doFilter(request, response);
-      return;
-    } else {
-      if (oauthSession.isLoggedIn()) {
-        oauthSession.logout();
-      }
+    if (request.getParameter("link") != null) {
+      oauthSession.setLinkMode(true);
+      oauthSession.setServiceProvider(null);
     }
 
-    HttpServletResponse httpResponse = (HttpServletResponse) response;
-
     String provider = httpRequest.getParameter("provider");
     OAuthServiceProvider service = ssoProvider == null
         ? oauthSession.getServiceProvider()
         : ssoProvider;
 
-    if ((isGerritLogin(httpRequest)
-        || oauthSession.isOAuthFinal(httpRequest))
-        && !oauthSession.isLoggedIn()) {
+    if (isGerritLogin(httpRequest) || oauthSession.isOAuthFinal(httpRequest)) {
       if (service == null && Strings.isNullOrEmpty(provider)) {
         selectProvider(httpRequest, httpResponse, null);
         return;