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 {