Get rid of HTTP passwords use on GitHub plugin
GitHub plugin cannot use anymore the "trick" of reading a user's
HTTP password for managing Git/HTTP authentication with GitHub OAuth
persistent tokens.
Change-Id: Ib059e065c312c5077fb2e5012a7ffaa6929e952f
diff --git a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/IdentifiedUserGitHubLoginProvider.java b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/IdentifiedUserGitHubLoginProvider.java
index ebaae7b..4c92e2e 100644
--- a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/IdentifiedUserGitHubLoginProvider.java
+++ b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/IdentifiedUserGitHubLoginProvider.java
@@ -25,6 +25,7 @@
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.AccountState;
+import com.google.gerrit.server.account.ExternalId;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
@@ -35,8 +36,8 @@
UserScopedProvider<GitHubLogin> {
private static final Logger log = LoggerFactory
.getLogger(IdentifiedUserGitHubLoginProvider.class);
- private static final String EXTERNAL_ID_PREFIX = "external:"
- + OAuthWebFilter.GITHUB_EXT_ID;
+ public static final String EXTERNAL_ID_PREFIX =
+ AccountExternalId.SCHEME_EXTERNAL + OAuthWebFilter.GITHUB_EXT_ID;
private final Provider<IdentifiedUser> userProvider;
private final GitHubOAuthConfig config;
@@ -80,9 +81,9 @@
private AccessToken newAccessTokenFromUser(String username) {
AccountState account = accountCache.getByUsername(username);
- Collection<AccountExternalId> externalIds = account.getExternalIds();
- for (AccountExternalId accountExternalId : externalIds) {
- String key = accountExternalId.getKey().get();
+ Collection<ExternalId> externalIds = account.getExternalIds();
+ for (ExternalId accountExternalId : externalIds) {
+ String key = accountExternalId.asAccountExternalId().getKey().get();
if (key.startsWith(EXTERNAL_ID_PREFIX)) {
return new AccessToken(key.substring(EXTERNAL_ID_PREFIX.length()));
}
diff --git a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthFilter.java b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthFilter.java
index b1426f8..1e816d2 100644
--- a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthFilter.java
+++ b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthFilter.java
@@ -50,7 +50,6 @@
.newHashSet("scope.html");
private final GitHubOAuthConfig config;
- private final OAuthGitFilter gitFilter;
private final OAuthWebFilter webFilter;
@Inject
@@ -58,13 +57,10 @@
OAuthWebFilter webFilter, Injector injector) {
this.config = config;
this.webFilter = webFilter;
- Injector childInjector = injector.createChildInjector(OAuthCache.module());
- this.gitFilter = childInjector.getInstance(OAuthGitFilter.class);
}
@Override
public void init(FilterConfig filterConfig) throws ServletException {
- gitFilter.init(filterConfig);
webFilter.init(filterConfig);
}
@@ -80,7 +76,7 @@
} else {
if (GIT_HTTP_REQUEST_PATTERN.matcher(requestUrl).matches()) {
- gitFilter.doFilter(request, response, chain);
+ chain.doFilter(request, response);
} else {
System.out.println("Authorising " + requestUrl);
webFilter.doFilter(request, response, chain);
@@ -127,7 +123,6 @@
@Override
public void destroy() {
log.info("Destroy");
- gitFilter.destroy();
webFilter.destroy();
}
}
diff --git a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthGitFilter.java b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthGitFilter.java
deleted file mode 100644
index 02d6020..0000000
--- a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthGitFilter.java
+++ /dev/null
@@ -1,300 +0,0 @@
-// Copyright (C) 2013 The Android Open Source Project
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-package com.googlesource.gerrit.plugins.github.oauth;
-
-import static javax.servlet.http.HttpServletResponse.SC_FORBIDDEN;
-import static javax.servlet.http.HttpServletResponse.SC_UNAUTHORIZED;
-
-import com.google.common.base.MoreObjects;
-import com.google.common.base.Strings;
-import com.google.common.collect.Iterators;
-import com.google.gerrit.httpd.XGerritAuth;
-import com.google.gerrit.server.account.AccountCache;
-import com.google.inject.Inject;
-import com.google.inject.Provider;
-import com.google.inject.Singleton;
-import com.googlesource.gerrit.plugins.github.oauth.OAuthProtocol.AccessToken;
-
-import org.apache.commons.codec.binary.Base64;
-import org.apache.commons.lang.StringUtils;
-import org.apache.http.HttpResponse;
-import org.apache.http.HttpStatus;
-import org.apache.http.client.HttpClient;
-import org.apache.http.client.methods.HttpPut;
-import org.apache.http.entity.ContentType;
-import org.apache.http.entity.StringEntity;
-import org.slf4j.LoggerFactory;
-
-import java.io.IOException;
-import java.io.UnsupportedEncodingException;
-import java.net.MalformedURLException;
-import java.net.URI;
-import java.net.URL;
-import java.util.Enumeration;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.concurrent.ExecutionException;
-
-import javax.servlet.Filter;
-import javax.servlet.FilterChain;
-import javax.servlet.FilterConfig;
-import javax.servlet.ServletException;
-import javax.servlet.ServletRequest;
-import javax.servlet.ServletResponse;
-import javax.servlet.http.Cookie;
-import javax.servlet.http.HttpServletRequest;
-import javax.servlet.http.HttpServletRequestWrapper;
-import javax.servlet.http.HttpServletResponse;
-
-@Singleton
-public class OAuthGitFilter implements Filter {
- private static final String GITHUB_X_OAUTH_BASIC = "x-oauth-basic";
- private static final org.slf4j.Logger log = LoggerFactory
- .getLogger(OAuthGitFilter.class);
- public static final String GIT_REALM_NAME =
- "GitHub authentication for Gerrit Code Review";
- private static final String GIT_AUTHORIZATION_HEADER = "Authorization";
- private static final String GIT_AUTHENTICATION_BASIC = "Basic ";
-
- private final OAuthCache oauthCache;
- private final AccountCache accountCache;
- private final Provider<HttpClient> httpClientProvider;
- private final GitHubOAuthConfig config;
- private final XGerritAuth xGerritAuth;
- private ScopedProvider<GitHubLogin> ghLoginProvider;
-
- public static class BasicAuthHttpRequest extends HttpServletRequestWrapper {
- private HashMap<String, String> headers = new HashMap<>();
-
- public BasicAuthHttpRequest(HttpServletRequest request, String username,
- String password) {
- super(request);
-
- try {
- headers.put(
- GIT_AUTHORIZATION_HEADER,
- GIT_AUTHENTICATION_BASIC
- + Base64.encodeBase64String((username + ":" + password)
- .getBytes(OAuthGitFilter.encoding(request))));
- } catch (UnsupportedEncodingException e) {
- // This cannot really happen as we have already used the encoding for
- // decoding the request
- }
- }
-
- @Override
- public Enumeration<String> getHeaderNames() {
- final Enumeration<String> wrappedHeaderNames = super.getHeaderNames();
- HashSet<String> headerNames = new HashSet<>(headers.keySet());
- while (wrappedHeaderNames.hasMoreElements()) {
- headerNames.add(wrappedHeaderNames.nextElement());
- }
- return Iterators.asEnumeration(headerNames.iterator());
- }
-
- @Override
- public String getHeader(String name) {
- String headerValue = headers.get(name);
- if (headerValue != null) {
- return headerValue;
- }
- return super.getHeader(name);
- }
- }
-
- @Inject
- public OAuthGitFilter(OAuthCache oauthCache, AccountCache accountCache,
- PooledHttpClientProvider httpClientProvider, GitHubOAuthConfig config,
- XGerritAuth xGerritAuth, GitHubLogin.Provider ghLoginProvider) {
- this.oauthCache = oauthCache;
- this.accountCache = accountCache;
- this.httpClientProvider = httpClientProvider;
- this.config = config;
- this.xGerritAuth = xGerritAuth;
- this.ghLoginProvider = ghLoginProvider;
- }
-
- @Override
- public void init(FilterConfig filterConfig) throws ServletException {
- }
-
- @Override
- public void doFilter(ServletRequest request, ServletResponse response,
- FilterChain chain) throws IOException, ServletException {
-
- HttpServletRequest httpRequest = (HttpServletRequest) request;
- HttpServletResponse httpResponse =
- new OAuthGitWrappedResponse((HttpServletResponse) response);
- GitHubLogin ghLogin = ghLoginProvider.get(httpRequest);
- log.debug("OAuthGitFilter(" + httpRequest.getRequestURL() + ") ghLogin="
- + ghLogin);
-
- String username =
- getAuthenticatedUserFromGitRequestUsingOAuthToken(httpRequest,
- httpResponse);
- if (username != null) {
- String gerritPassword =
- accountCache.getByUsername(username).getPassword(username);
-
- if (gerritPassword == null) {
- gerritPassword =
- generateRandomGerritPassword(username, httpRequest, httpResponse,
- chain);
- if (Strings.isNullOrEmpty(gerritPassword)) {
- httpResponse.sendError(SC_FORBIDDEN,
- "Unable to generate Gerrit password for Git Basic-Auth");
- } else {
- httpResponse.sendRedirect(getRequestPathWithQueryString(httpRequest));
- }
- return;
- }
-
- httpRequest =
- new BasicAuthHttpRequest(httpRequest, username, gerritPassword);
- }
-
- chain.doFilter(httpRequest, httpResponse);
- }
-
- private String getRequestPathWithQueryString(HttpServletRequest httpRequest) {
- String requestPathWithQueryString =
- httpRequest.getContextPath() + httpRequest.getServletPath()
- + Strings.nullToEmpty(httpRequest.getPathInfo()) + "?"
- + httpRequest.getQueryString();
- return requestPathWithQueryString;
- }
-
- private String generateRandomGerritPassword(String username,
- HttpServletRequest httpRequest, HttpServletResponse httpResponse,
- FilterChain chain) throws IOException, ServletException {
- log.warn("User " + username + " has not a Gerrit HTTP password: "
- + "generating a random one in order to be able to use Git over HTTP");
- Cookie gerritCookie =
- getGerritLoginCookie(username, httpRequest, httpResponse, chain);
- String xGerritAuthValue = xGerritAuth.getAuthValue(gerritCookie);
-
- HttpPut putRequest =
- new HttpPut(getRequestUrlWithAlternatePath(httpRequest,
- "/accounts/self/password.http"));
- putRequest.setHeader("Cookie",
- gerritCookie.getName() + "=" + gerritCookie.getValue());
- putRequest.setHeader(XGerritAuth.X_GERRIT_AUTH, xGerritAuthValue);
-
- putRequest.setEntity(new StringEntity("{\"generate\":true}",
- ContentType.APPLICATION_JSON));
- HttpResponse putResponse = httpClientProvider.get().execute(putRequest);
- if (putResponse.getStatusLine().getStatusCode() != HttpStatus.SC_OK) {
- throw new ServletException(
- "Cannot generate HTTP password for authenticating user " + username);
- }
-
- return accountCache.getByUsername(username).getPassword(username);
- }
-
- private URI getRequestUrlWithAlternatePath(HttpServletRequest httpRequest,
- String alternatePath) throws MalformedURLException {
- URL originalUrl = new URL(httpRequest.getRequestURL().toString());
- String contextPath = httpRequest.getContextPath();
- return URI.create(originalUrl.getProtocol() + "://" + originalUrl.getHost()
- + ":" + getPort(originalUrl) + contextPath + alternatePath);
- }
-
- private int getPort(URL originalUrl) {
- String protocol = originalUrl.getProtocol().toLowerCase();
- int port = originalUrl.getPort();
- if (port == -1) {
- return protocol.equals("https") ? 443 : 80;
- }
- return port;
- }
-
- private Cookie getGerritLoginCookie(String username,
- HttpServletRequest httpRequest, HttpServletResponse httpResponse,
- FilterChain chain) throws IOException, ServletException {
- AuthenticatedPathHttpRequest loginRequest =
- new AuthenticatedLoginHttpRequest(httpRequest, config.httpHeader,
- username);
- AuthenticatedLoginHttpResponse loginResponse =
- new AuthenticatedLoginHttpResponse(httpResponse);
- chain.doFilter(loginRequest, loginResponse);
- return loginResponse.getGerritCookie();
- }
-
- private String getAuthenticatedUserFromGitRequestUsingOAuthToken(
- HttpServletRequest req, HttpServletResponse rsp) throws IOException {
- final String httpBasicAuth = getHttpBasicAuthenticationHeader(req);
- if (httpBasicAuth == null) {
- return null;
- }
-
- if (isInvalidHttpAuthenticationHeader(httpBasicAuth)) {
- rsp.sendError(SC_UNAUTHORIZED);
- return null;
- }
-
- String oauthToken = StringUtils.substringBefore(httpBasicAuth, ":");
- String oauthKeyword = StringUtils.substringAfter(httpBasicAuth, ":");
- if (Strings.isNullOrEmpty(oauthToken)
- || Strings.isNullOrEmpty(oauthKeyword)) {
- rsp.sendError(SC_UNAUTHORIZED);
- return null;
- }
-
- if (!oauthKeyword.equalsIgnoreCase(GITHUB_X_OAUTH_BASIC)) {
- return null;
- }
-
- boolean loginSuccessful = false;
- String oauthLogin = null;
- try {
- oauthLogin =
- oauthCache.getLoginByAccessToken(new AccessToken(oauthToken));
- loginSuccessful = !Strings.isNullOrEmpty(oauthLogin);
- } catch (ExecutionException e) {
- log.warn("Login failed for OAuth token " + oauthToken, e);
- loginSuccessful = false;
- }
-
- if (!loginSuccessful) {
- rsp.sendError(SC_FORBIDDEN);
- return null;
- }
-
- return oauthLogin;
- }
-
-
- private boolean isInvalidHttpAuthenticationHeader(String usernamePassword) {
- return usernamePassword.indexOf(':') < 1;
- }
-
- static String encoding(HttpServletRequest req) {
- return MoreObjects.firstNonNull(req.getCharacterEncoding(), "UTF-8");
- }
-
- private String getHttpBasicAuthenticationHeader(final HttpServletRequest req)
- throws UnsupportedEncodingException {
- String hdr = req.getHeader(GIT_AUTHORIZATION_HEADER);
- if (hdr == null || !hdr.startsWith(GIT_AUTHENTICATION_BASIC)) {
- return null;
- }
- return new String(Base64.decodeBase64(hdr
- .substring(GIT_AUTHENTICATION_BASIC.length())), encoding(req));
- }
-
- @Override
- public void destroy() {
- log.info("Destroy");
- }
-}
diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/filters/GitHubOAuthFilter.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/filters/GitHubOAuthFilter.java
index 03010b4..c143cb1 100644
--- a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/filters/GitHubOAuthFilter.java
+++ b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/filters/GitHubOAuthFilter.java
@@ -19,10 +19,12 @@
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.AccountCache;
+import com.google.gerrit.server.account.ExternalId;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
import com.googlesource.gerrit.plugins.github.oauth.GitHubLogin;
+import com.googlesource.gerrit.plugins.github.oauth.IdentifiedUserGitHubLoginProvider;
import com.googlesource.gerrit.plugins.github.oauth.OAuthFilter;
import com.googlesource.gerrit.plugins.github.oauth.OAuthProtocol.AccessToken;
import com.googlesource.gerrit.plugins.github.oauth.OAuthWebFilter;
@@ -84,17 +86,19 @@
}
private AccountExternalId getGitHubExternalId(CurrentUser user) {
- Collection<AccountExternalId> accountExtIds =
+ Collection<ExternalId> accountExtIds =
accountCache.get(((IdentifiedUser) user).getAccountId())
.getExternalIds();
- Collection<AccountExternalId> gitHubExtId =
+ Collection<ExternalId> gitHubExtId =
Collections2.filter(accountExtIds,
- new Predicate<AccountExternalId>() {
+ new Predicate<ExternalId>() {
@Override
- public boolean apply(AccountExternalId externalId) {
- return externalId.isScheme(AccountExternalId.SCHEME_EXTERNAL)
- && externalId.getSchemeRest().startsWith(
- OAuthWebFilter.GITHUB_EXT_ID);
+ public boolean apply(ExternalId externalId) {
+ return externalId
+ .key()
+ .get()
+ .startsWith(
+ IdentifiedUserGitHubLoginProvider.EXTERNAL_ID_PREFIX);
}
});
@@ -102,7 +106,7 @@
throw new IllegalStateException("Current Gerrit user "
+ user.getUserName() + " has no GitHub OAuth external ID");
}
- return gitHubExtId.iterator().next();
+ return gitHubExtId.iterator().next().asAccountExternalId();
}
@Override
diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/wizard/AccountController.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/wizard/AccountController.java
index 882ded3..13085c4 100644
--- a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/wizard/AccountController.java
+++ b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/wizard/AccountController.java
@@ -36,6 +36,7 @@
import com.googlesource.gerrit.plugins.github.oauth.GitHubLogin;
import org.apache.commons.lang.StringUtils;
+import org.eclipse.jgit.errors.ConfigInvalidException;
import org.kohsuke.github.GHKey;
import org.kohsuke.github.GHMyself;
import org.kohsuke.github.GHVerifiedKey;
@@ -83,13 +84,13 @@
setAccoutPublicKeys(user, hubLogin, req);
log.info("Created account '" + user.getUserName() + "'");
- } catch (IOException e) {
+ } catch (IOException | ConfigInvalidException e) {
log.error("Account '" + user.getUserName() + "' creation failed", e);
- throw e;
+ throw new IOException(e);
}
}
- private void setAccountIdentity(IdentifiedUser user, HttpServletRequest req) throws ServletException {
+ private void setAccountIdentity(IdentifiedUser user, HttpServletRequest req) throws ServletException, ConfigInvalidException {
String fullName = req.getParameter("fullname");
String email = req.getParameter("email");
try {
