Improvements of feedback from review I86fb8fab3 GitHub authentication has been submitted 9 months ago to Gerrit as extra authentication backend: a lot of suggestions and after 57 (!) patch-sets many improvements have been added to the code and are now incorporated into GitHub master: - Introduction of random state to prevent OAuth code injection and replay attacks - Upgrade to HTTPClient 4.4 and connection pooling - Introduction of HTTP proxy for outbound connections when installed within company firewalls - Removal of heavy non serialisable objects from the HTTP Session - Early release of HTTP connections to the pool to improve scalability - Simplified set-up during init - Improved documentation Many thanks to all the people that participated in the Code-Review and contributions: as Git doesn’t allow to have more than one author, I will mention them below. Thanks to: Edwin Kempin, David Pursehouse, Thomas Broyer, David Ostrovsky, Sasa Zivkov, Andre Saddler, Dariusz Luksza, Derek Hunter, Will DeBerry, Tom Hughes, Shawn Pearce Change-Id: I19e761485692d9b4bc0ade8d0f3ad761b0fa5369
diff --git a/github-oauth/pom.xml b/github-oauth/pom.xml index c0ab78f..e6d70e7 100644 --- a/github-oauth/pom.xml +++ b/github-oauth/pom.xml
@@ -112,7 +112,7 @@ <dependency> <groupId>org.apache.httpcomponents</groupId> <artifactId>httpclient</artifactId> - <version>4.2.5</version> + <version>4.4</version> </dependency> </dependencies> </project>
diff --git a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/GitHubHttpProvider.java b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/GitHubHttpProvider.java deleted file mode 100644 index 367a390..0000000 --- a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/GitHubHttpProvider.java +++ /dev/null
@@ -1,56 +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 com.google.inject.Provider; -import com.google.inject.Singleton; - -import org.apache.http.HttpHost; -import org.apache.http.client.HttpClient; -import org.apache.http.conn.routing.HttpRoute; -import org.apache.http.conn.scheme.PlainSocketFactory; -import org.apache.http.conn.scheme.Scheme; -import org.apache.http.conn.scheme.SchemeRegistry; -import org.apache.http.conn.ssl.SSLSocketFactory; -import org.apache.http.impl.client.DefaultHttpClient; -import org.apache.http.impl.conn.PoolingClientConnectionManager; - -@Singleton -public class GitHubHttpProvider implements Provider<HttpClient> { - private static final int MAX_TOTAL_CONN = 1024; - private static final int MAX_CONN_PER_ROUTE = 10; - private static final int MAX_LOCALHOST_CONN = 512; - private final PoolingClientConnectionManager connectionManager; - - public GitHubHttpProvider() { - SchemeRegistry schemeRegistry = new SchemeRegistry(); - schemeRegistry.register(new Scheme("http", 80, PlainSocketFactory - .getSocketFactory())); - schemeRegistry.register(new Scheme("https", 443, SSLSocketFactory - .getSocketFactory())); - - connectionManager = new PoolingClientConnectionManager(schemeRegistry); - connectionManager.setMaxTotal(MAX_TOTAL_CONN); - connectionManager.setDefaultMaxPerRoute(MAX_CONN_PER_ROUTE); - HttpHost localhost = new HttpHost("locahost", 80); - connectionManager.setMaxPerRoute(new HttpRoute(localhost), - MAX_LOCALHOST_CONN); - } - - @Override - public HttpClient get() { - return new DefaultHttpClient(connectionManager); - } - -}
diff --git a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/GitHubLogin.java b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/GitHubLogin.java index 2950996..32f751a 100644 --- a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/GitHubLogin.java +++ b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/GitHubLogin.java
@@ -16,20 +16,8 @@ import static java.util.concurrent.TimeUnit.DAYS; -import com.google.common.base.MoreObjects; -import com.google.inject.Inject; -import com.google.inject.Singleton; - -import com.googlesource.gerrit.plugins.github.oauth.OAuthProtocol.AccessToken; -import com.googlesource.gerrit.plugins.github.oauth.OAuthProtocol.Scope; - -import org.apache.http.HttpStatus; -import org.kohsuke.github.GHMyself; -import org.kohsuke.github.GitHub; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - import java.io.IOException; +import java.io.Serializable; import java.util.Arrays; import java.util.Collections; import java.util.HashSet; @@ -38,21 +26,30 @@ import java.util.SortedSet; import java.util.TreeSet; -import javax.servlet.ServletRequest; -import javax.servlet.ServletResponse; import javax.servlet.http.Cookie; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import lombok.Getter; -public class GitHubLogin { +import org.kohsuke.github.GHMyself; +import org.kohsuke.github.GitHub; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import com.google.common.base.MoreObjects; +import com.google.inject.Inject; +import com.google.inject.Singleton; +import com.googlesource.gerrit.plugins.github.oauth.OAuthProtocol.AccessToken; +import com.googlesource.gerrit.plugins.github.oauth.OAuthProtocol.Scope; + +public class GitHubLogin implements Serializable { + private static final long serialVersionUID = 1L; private static final Logger log = LoggerFactory.getLogger(GitHubLogin.class); private static final List<Scope> DEFAULT_SCOPES = Arrays.asList( Scope.PUBLIC_REPO, Scope.USER_EMAIL); - private static final int YEARS = 365; private static final long SCOPE_COOKIE_NEVER_EXPIRES = DAYS - .toSeconds(50 * YEARS); + .toSeconds(50 * 365); @Singleton public static class Provider extends HttpSessionProvider<GitHubLogin> { @@ -63,21 +60,16 @@ } @Getter - protected AccessToken token; + private AccessToken token; - @Getter - protected GitHub hub; - - protected GHMyself myself; - - private transient OAuthProtocol oauth; + private String state; private SortedSet<Scope> loginScopes; private final GitHubOAuthConfig config; - public GHMyself getMyself() { + public GHMyself getMyself() throws IOException { if (isLoggedIn()) { - return myself; + return getHub().getMyself(); } else { return null; } @@ -85,92 +77,68 @@ public Set<String> getMyOrganisationsLogins() throws IOException { if (isLoggedIn()) { - return hub.getMyOrganizations().keySet(); + return getHub().getMyOrganizations().keySet(); } else { return Collections.emptySet(); } } @Inject - public GitHubLogin(final OAuthProtocol oauth, final GitHubOAuthConfig config) { - this.oauth = oauth; + public GitHubLogin(final GitHubOAuthConfig config) { this.config = config; } public boolean isLoggedIn() { - boolean loggedIn = token != null && hub != null; - if (loggedIn && myself == null) { - try { - myself = hub.getMyself(); - } catch (Throwable e) { - log.error("Connection to GitHub broken: logging out", e); - logout(); - loggedIn = false; - } - } - return loggedIn; - } - - public boolean login(ServletRequest request, ServletResponse response, - Scope... scopes) throws IOException { - return login((HttpServletRequest) request, (HttpServletResponse) response, - scopes); + return token != null; } public boolean login(HttpServletRequest request, - HttpServletResponse response, Scope... scopes) throws IOException { + HttpServletResponse response, OAuthProtocol oauth, Scope... scopes) throws IOException { if (isLoggedIn()) { return true; } log.debug("Login " + this); - if (OAuthProtocol.isOAuthFinal(request)) { log.debug("Login-FINAL " + this); - AccessToken loginAccessToken = oauth.loginPhase2(request, response); - if(loginAccessToken != null && !loginAccessToken.isError()) { - login(loginAccessToken); - } + login(oauth.loginPhase2(request, state)); + this.state = ""; // Make sure state is used only once if (isLoggedIn()) { log.debug("Login-SUCCESS " + this); response.sendRedirect(OAuthProtocol.getTargetUrl(request)); return true; } else { - response.sendError(HttpStatus.SC_UNAUTHORIZED); return false; } } else { this.loginScopes = getScopes(getScopesKey(request, response), scopes); log.debug("Login-PHASE1 " + this); - oauth.loginPhase1(request, response, loginScopes); + state = oauth.loginPhase1(request, response, loginScopes); return false; } } public void logout() { - hub = null; token = null; } - public OAuthProtocol getOAuthProtocol() { - return oauth; - } - public GitHub login(AccessToken authToken) throws IOException { log.debug("Logging in using access token {}", authToken.accessToken); this.token = authToken; - this.hub = GitHub.connectUsingOAuth(authToken.accessToken); - this.myself = hub.getMyself(); - return this.hub; + return getHub(); } @Override public String toString() { - return "GitHubLogin [token=" + token + ", myself=" + myself + ", scopes=" + return "GitHubLogin [token=" + token + ", scopes=" + loginScopes + "]"; } + public GitHub getHub() throws IOException { + return GitHub.connectUsingOAuth(this.token.accessToken); + } + private String getScopesKey(HttpServletRequest request, HttpServletResponse response) { String scopeRequested = request.getParameter("scope");
diff --git a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/GitHubOAuthConfig.java b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/GitHubOAuthConfig.java index c41be26..7d30d19 100644 --- a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/GitHubOAuthConfig.java +++ b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/GitHubOAuthConfig.java
@@ -14,36 +14,41 @@ package com.googlesource.gerrit.plugins.github.oauth; import com.google.common.base.MoreObjects; +import com.google.common.base.CharMatcher; +import com.google.common.base.Preconditions; import com.google.common.base.Strings; import com.google.common.collect.Maps; import com.google.gerrit.reviewdb.client.AuthType; +import com.google.gerrit.server.config.AuthConfig; +import com.google.gerrit.server.config.CanonicalWebUrl; import com.google.gerrit.server.config.GerritServerConfig; import com.google.inject.Inject; import com.google.inject.Singleton; - import com.googlesource.gerrit.plugins.github.oauth.OAuthProtocol.Scope; import org.eclipse.jgit.lib.Config; -import java.net.MalformedURLException; -import java.net.URL; import java.util.ArrayList; import java.util.List; import java.util.Map; import java.util.Set; @Singleton -public class GitHubOAuthConfig { - protected static final String CONF_SECTION = "github"; - private static final String LOGIN_OAUTH_AUTHORIZE = "/login/oauth/authorize"; - private static final String GITHUB_URL = "https://github.com"; - public static final String OAUTH_FINAL = "oauth"; - public static final String LOGIN_OAUTH_ACCESS_TOKEN = +public +class GitHubOAuthConfig { + public static final String CONF_SECTION = "github"; + public static final String GITHUB_OAUTH_AUTHORIZE = "/login/oauth/authorize"; + public static final String GITHUB_OAUTH_ACCESS_TOKEN = "/login/oauth/access_token"; - public static final String OAUTH_LOGIN = "/login"; - public static final String OAUTH_LOGOUT = "/logout"; + public static final String GITHUB_GET_USER = "/user"; + public static final String GERRIT_OAUTH_FINAL = "/oauth"; + public static final String GITHUB_URL_DEFAULT = "https://github.com"; + public static final String GITHUB_API_URL_DEFAULT = "https://api.github.com"; + public static final String GERRIT_LOGIN = "/login"; + public static final String GERRIT_LOGOUT = "/logout"; public final String gitHubUrl; + public final String gitHubApiUrl; public final String gitHubClientId; public final String gitHubClientSecret; public final String logoutRedirectUrl; @@ -55,34 +60,49 @@ public final Map<String, List<OAuthProtocol.Scope>> scopes; public final int fileUpdateMaxRetryCount; public final int fileUpdateMaxRetryIntervalMsec; - public final Config gerritConfig; public final String oauthHttpHeader; @Inject - public GitHubOAuthConfig(@GerritServerConfig Config config) - throws MalformedURLException { - this.gerritConfig = config; + protected + GitHubOAuthConfig(@GerritServerConfig Config config, + @CanonicalWebUrl String canonicalWebUrl, AuthConfig authConfig) { + httpHeader = + Preconditions.checkNotNull( + config.getString("auth", null, "httpHeader"), + "HTTP Header for GitHub user must be provided"); + gitHubUrl = + trimTrailingSlash(MoreObjects.firstNonNull( + config.getString(CONF_SECTION, null, "url"), GITHUB_URL_DEFAULT)); + gitHubApiUrl = + trimTrailingSlash(MoreObjects.firstNonNull( + config.getString(CONF_SECTION, null, "apiUrl"), + GITHUB_API_URL_DEFAULT)); + gitHubClientId = + Preconditions.checkNotNull( + config.getString(CONF_SECTION, null, "clientId"), + "GitHub `clientId` must be provided"); + gitHubClientSecret = + Preconditions.checkNotNull( + config.getString(CONF_SECTION, null, "clientSecret"), + "GitHub `clientSecret` must be provided"); - httpHeader = config.getString("auth", null, "httpHeader"); oauthHttpHeader = config.getString("auth", null, "httpExternalIdHeader"); - gitHubUrl = dropTrailingSlash( - MoreObjects.firstNonNull(config.getString(CONF_SECTION, null, "url"), - GITHUB_URL)); - gitHubClientId = config.getString(CONF_SECTION, null, "clientId"); - gitHubClientSecret = config.getString(CONF_SECTION, null, "clientSecret"); - gitHubOAuthUrl = getUrl(gitHubUrl, LOGIN_OAUTH_AUTHORIZE); - gitHubOAuthAccessTokenUrl = getUrl(gitHubUrl, LOGIN_OAUTH_ACCESS_TOKEN); - logoutRedirectUrl = config.getString(CONF_SECTION, null, "logoutRedirectUrl"); + gitHubOAuthUrl = gitHubUrl + GITHUB_OAUTH_AUTHORIZE; + gitHubOAuthAccessTokenUrl = gitHubUrl + GITHUB_OAUTH_ACCESS_TOKEN; + logoutRedirectUrl = + config.getString(CONF_SECTION, null, "logoutRedirectUrl"); oAuthFinalRedirectUrl = - getUrl(config.getString("gerrit", null, "canonicalWebUrl"), OAUTH_FINAL); + trimTrailingSlash(canonicalWebUrl) + GERRIT_OAUTH_FINAL; enabled = config.getString("auth", null, "type").equalsIgnoreCase( AuthType.HTTP.toString()); scopes = getScopes(config); - fileUpdateMaxRetryCount = config.getInt(CONF_SECTION, "fileUpdateMaxRetryCount", 3); - fileUpdateMaxRetryIntervalMsec = config.getInt(CONF_SECTION, "fileUpdateMaxRetryIntervalMsec", 3000); + fileUpdateMaxRetryCount = + config.getInt(CONF_SECTION, "fileUpdateMaxRetryCount", 3); + fileUpdateMaxRetryIntervalMsec = + config.getInt(CONF_SECTION, "fileUpdateMaxRetryIntervalMsec", 3000); } private Map<String, List<Scope>> getScopes(Config config) { @@ -97,13 +117,13 @@ return scopes; } - private String dropTrailingSlash(String url) { - return (url.endsWith("/") ? url.substring(0, url.length()-1):url); + private String trimTrailingSlash(String url) { + return CharMatcher.is('/').trimTrailingFrom(url); } private List<Scope> parseScopesString(String scopesString) { ArrayList<Scope> scopes = new ArrayList<OAuthProtocol.Scope>(); - if(Strings.emptyToNull(scopesString) != null) { + if (Strings.emptyToNull(scopesString) != null) { String[] scopesStrings = scopesString.split(","); for (String scope : scopesStrings) { scopes.add(Enum.valueOf(Scope.class, scope)); @@ -113,11 +133,6 @@ return scopes; } - private static String getUrl(String baseUrl, String path) - throws MalformedURLException { - return new URL(new URL(baseUrl), path).toExternalForm(); - } - public Scope[] getDefaultScopes() { if (scopes == null || scopes.get("scopes") == null) { return new Scope[0];
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 29d8840..646b602 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
@@ -14,6 +14,12 @@ package com.googlesource.gerrit.plugins.github.oauth; +import java.io.IOException; +import java.util.Collection; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + import com.google.gerrit.common.Nullable; import com.google.gerrit.reviewdb.client.AccountExternalId; import com.google.gerrit.server.IdentifiedUser; @@ -22,34 +28,25 @@ 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.slf4j.Logger; -import org.slf4j.LoggerFactory; - -import java.io.IOException; -import java.util.Collection; - @Singleton public class IdentifiedUserGitHubLoginProvider implements UserScopedProvider<GitHubLogin> { - private static final Logger log = LoggerFactory.getLogger(IdentifiedUserGitHubLoginProvider.class); + private static final Logger log = LoggerFactory + .getLogger(IdentifiedUserGitHubLoginProvider.class); private static final String EXTERNAL_ID_PREFIX = "external:" + OAuthWebFilter.GITHUB_EXT_ID; private final Provider<IdentifiedUser> userProvider; - private OAuthProtocol oauth; private GitHubOAuthConfig config; private AccountCache accountCache; @Inject public IdentifiedUserGitHubLoginProvider( final Provider<IdentifiedUser> identifiedUserProvider, - final OAuthProtocol oauth, final GitHubOAuthConfig config, - final AccountCache accountCache) { + final GitHubOAuthConfig config, final AccountCache accountCache) { this.userProvider = identifiedUserProvider; - this.oauth = oauth; this.config = config; this.accountCache = accountCache; } @@ -66,15 +63,14 @@ try { AccessToken accessToken = newAccessTokenFromUser(username); if (accessToken != null) { - GitHubLogin login = new GitHubLogin(oauth, config); + GitHubLogin login = new GitHubLogin(config); login.login(accessToken); return login; } else { return null; } } catch (IOException e) { - log.error("Cannot login to GitHub as '" + username - + "'", e); + log.error("Cannot login to GitHub as '" + username + "'", e); return null; } }
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 index e2a8f26..c0f3897 100644 --- 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
@@ -22,14 +22,15 @@ 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; @@ -68,7 +69,7 @@ private final OAuthCache oauthCache; private final AccountCache accountCache; - private final GitHubHttpProvider httpClientProvider; + private final Provider<HttpClient> httpClientProvider; private final GitHubOAuthConfig config; private final XGerritAuth xGerritAuth; private ScopedProvider<GitHubLogin> ghLoginProvider; @@ -115,7 +116,7 @@ @Inject public OAuthGitFilter(OAuthCache oauthCache, AccountCache accountCache, - GitHubHttpProvider httpClientProvider, GitHubOAuthConfig config, + PooledHttpClientProvider httpClientProvider, GitHubOAuthConfig config, XGerritAuth xGerritAuth, GitHubLogin.Provider ghLoginProvider) { this.oauthCache = oauthCache; this.accountCache = accountCache;
diff --git a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthProtocol.java b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthProtocol.java index 563a71e..4358c0d 100644 --- a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthProtocol.java +++ b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthProtocol.java
@@ -2,9 +2,11 @@ import com.google.common.base.Charsets; import com.google.common.base.Strings; +import com.google.common.io.BaseEncoding; import com.google.common.io.CharStreams; import com.google.gson.Gson; import com.google.inject.Inject; +import com.google.inject.Provider; import com.google.inject.Singleton; import org.apache.http.HttpResponse; @@ -24,8 +26,11 @@ import java.net.HttpURLConnection; import java.net.URLEncoder; import java.nio.charset.StandardCharsets; +import java.security.NoSuchAlgorithmException; +import java.security.SecureRandom; import java.util.ArrayList; import java.util.List; +import java.util.Objects; import java.util.Set; import javax.servlet.ServletRequest; @@ -58,12 +63,13 @@ } } private static final String ME_SEPARATOR = ","; - private static final Logger LOG = LoggerFactory + private static final Logger log = LoggerFactory .getLogger(OAuthProtocol.class); + private static SecureRandom randomState = newRandomGenerator(); private final GitHubOAuthConfig config; - private final HttpClient http; private final Gson gson; + private final Provider<HttpClient> httpProvider; public static class AccessToken { public String accessToken; @@ -98,29 +104,13 @@ @Override public int hashCode() { - final int prime = 31; - int result = 1; - result = - prime * result - + ((accessToken == null) ? 0 : accessToken.hashCode()); - result = - prime * result + ((tokenType == null) ? 0 : tokenType.hashCode()); - return result; + return Objects.hash(accessToken, tokenType, error, errorDescription, + errorUri); } @Override public boolean equals(Object obj) { - if (this == obj) return true; - if (obj == null) return false; - if (getClass() != obj.getClass()) return false; - AccessToken other = (AccessToken) obj; - if (accessToken == null) { - if (other.accessToken != null) return false; - } else if (!accessToken.equals(other.accessToken)) return false; - if (tokenType == null) { - if (other.tokenType != null) return false; - } else if (!tokenType.equals(other.tokenType)) return false; - return true; + return Objects.deepEquals(this, obj); } public boolean isError() { @@ -129,36 +119,49 @@ } @Inject - public OAuthProtocol(GitHubOAuthConfig config, GitHubHttpProvider httpClientProvider, - /* We need to explicitly tell Guice which Provider<> we need as this class may be - instantiated outside the standard Guice Module set-up (e.g. initial Servlet login - filter) */ + public OAuthProtocol(GitHubOAuthConfig config, + PooledHttpClientProvider httpClientProvider, + /* + * We need to explicitly tell Guice which Provider<> we need as this class + * may be instantiated outside the standard Guice Module set-up (e.g. + * initial Servlet login filter) + */ GsonProvider gsonProvider) { this.config = config; - this.http = httpClientProvider.get(); + this.httpProvider = httpClientProvider; this.gson = gsonProvider.get(); } - public void loginPhase1(HttpServletRequest request, - HttpServletResponse response, Set<Scope> scopes) throws IOException { + private static SecureRandom newRandomGenerator() { + try { + return SecureRandom.getInstance("SHA1PRNG"); + } catch (NoSuchAlgorithmException e) { + throw new IllegalArgumentException( + "No SecureRandom available for GitHub authentication", e); + } + } + public String loginPhase1(HttpServletRequest request, + HttpServletResponse response, Set<Scope> scopes) throws IOException { String scopesString = getScope(scopes); - LOG.debug("Initiating GitHub Login for ClientId=" + config.gitHubClientId + " Scopes=" + scopesString); + log.debug("Initiating GitHub Login for ClientId=" + config.gitHubClientId + + " Scopes=" + scopesString); + String state = newRandomState(request.getRequestURI().toString()); response.sendRedirect(String.format( - "%s?client_id=%s%s&redirect_uri=%s&state=%s%s", config.gitHubOAuthUrl, - config.gitHubClientId, scopesString, - getURLEncoded(config.oAuthFinalRedirectUrl), - me(), getURLEncoded(request.getRequestURI().toString()))); + "%s?client_id=%s%s&redirect_uri=%s&state=%s", + config.gitHubOAuthUrl, config.gitHubClientId, scopesString, + getURLEncoded(config.oAuthFinalRedirectUrl), getURLEncoded(state))); + return state; } private String getScope(Set<Scope> scopes) { - if(scopes.size() <= 0) { + if (scopes.size() <= 0) { return ""; } - + StringBuilder out = new StringBuilder(); for (Scope scope : scopes) { - if(out.length() > 0) { + if (out.length() > 0) { out.append(","); } out.append(scope.getValue()); @@ -179,28 +182,33 @@ return Strings.emptyToNull(request.getParameter("code")) != null; } - public String me() { - return "" + hashCode() + ME_SEPARATOR; + public String newRandomState(String redirectUrl) { + byte[] stateBin = new byte[20]; // SHA-1 size + randomState.nextBytes(stateBin); + return BaseEncoding.base64Url().encode(stateBin) + ME_SEPARATOR + redirectUrl; } public static boolean isOAuthLogin(HttpServletRequest request) { - return request.getRequestURI().indexOf(GitHubOAuthConfig.OAUTH_LOGIN) >= 0; + return request.getRequestURI().indexOf(GitHubOAuthConfig.GERRIT_LOGIN) >= 0; } public static boolean isOAuthLogout(HttpServletRequest request) { - return request.getRequestURI().indexOf(GitHubOAuthConfig.OAUTH_LOGOUT) >= 0; + return request.getRequestURI().indexOf(GitHubOAuthConfig.GERRIT_LOGOUT) >= 0; } public static boolean isOAuthRequest(HttpServletRequest httpRequest) { return OAuthProtocol.isOAuthLogin(httpRequest) || OAuthProtocol.isOAuthFinal(httpRequest); } - public AccessToken loginPhase2(HttpServletRequest request, - HttpServletResponse response) throws IOException { + public AccessToken loginPhase2(HttpServletRequest request, String state) + throws IOException { - HttpPost post = null; + String requestState = request.getParameter("state"); + if (!Objects.equals(state, requestState)) { + throw new IOException("Invalid authentication state"); + } - post = new HttpPost(config.gitHubOAuthAccessTokenUrl); + HttpPost post = new HttpPost(config.gitHubOAuthAccessTokenUrl); post.setHeader("Accept", "application/json"); List<NameValuePair> nvps = new ArrayList<NameValuePair>(); nvps.add(new BasicNameValuePair("client_id", config.gitHubClientId)); @@ -208,46 +216,40 @@ nvps.add(new BasicNameValuePair("code", request.getParameter("code"))); post.setEntity(new UrlEncodedFormEntity(nvps, Charsets.UTF_8)); - try { - HttpResponse postResponse = http.execute(post); - if (postResponse.getStatusLine().getStatusCode() != HttpURLConnection.HTTP_OK) { - LOG.error("POST " + config.gitHubOAuthAccessTokenUrl - + " request for access token failed with status " - + postResponse.getStatusLine()); - EntityUtils.consume(postResponse.getEntity()); - return null; - } - - InputStream content = postResponse.getEntity().getContent(); - String tokenJsonString = - CharStreams.toString(new InputStreamReader(content, - StandardCharsets.UTF_8)); - AccessToken token = gson.fromJson(tokenJsonString, AccessToken.class); - if (token.isError()) { - LOG.error("POST " + config.gitHubOAuthAccessTokenUrl - + " returned an error token: " + token); - } - return token; - } catch (IOException e) { - LOG.error("POST " + config.gitHubOAuthAccessTokenUrl - + " request for access token failed", e); - return null; + HttpResponse postResponse = httpProvider.get().execute(post); + if (postResponse.getStatusLine().getStatusCode() != HttpURLConnection.HTTP_OK) { + log.error("POST " + config.gitHubOAuthAccessTokenUrl + + " request for access token failed with status " + + postResponse.getStatusLine()); + EntityUtils.consume(postResponse.getEntity()); + throw new IOException("GitHub OAuth request failed"); } + + InputStream content = postResponse.getEntity().getContent(); + String tokenJsonString = + CharStreams.toString(new InputStreamReader(content, + StandardCharsets.UTF_8)); + AccessToken token = gson.fromJson(tokenJsonString, AccessToken.class); + if (token.isError()) { + log.error("POST " + config.gitHubOAuthAccessTokenUrl + + " returned an error token: " + token); + throw new IOException("Invalid GitHub OAuth token"); + } + return token; } private static String getURLEncoded(String url) { try { return URLEncoder.encode(url, "UTF-8"); } catch (UnsupportedEncodingException e) { - // UTF-8 is hardcoded, cannot fail - return null; + throw new IllegalStateException("Cannot find UTF-8 encoding", e); } } public static String getTargetUrl(ServletRequest request) { int meEnd = state(request).indexOf(ME_SEPARATOR); if (meEnd > 0) { - return state(request).substring(meEnd+1); + return state(request).substring(meEnd + 1); } else { return ""; }
diff --git a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthWebFilter.java b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthWebFilter.java index bf0a4b1..6929d16 100644 --- a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthWebFilter.java +++ b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthWebFilter.java
@@ -51,16 +51,20 @@ private final GitHubOAuthConfig config; private final Random retryRandom = new Random(System.currentTimeMillis()); - private SitePaths sites; - private ScopedProvider<GitHubLogin> loginProvider; + private final SitePaths sites; + private final ScopedProvider<GitHubLogin> loginProvider; + private final OAuthProtocol oauth; @Inject - public OAuthWebFilter(GitHubOAuthConfig config, SitePaths sites, + public OAuthWebFilter(GitHubOAuthConfig config, + SitePaths sites, + OAuthProtocol oauth, // We need to explicitly tell Guice the correct implementation // as this filter is instantiated with a standard Gerrit WebModule GitHubLogin.Provider loginProvider) { this.config = config; this.sites = sites; + this.oauth = oauth; this.loginProvider = loginProvider; } @@ -120,12 +124,12 @@ private void login(ServletRequest request, HttpServletRequest httpRequest, HttpServletResponse httpResponse, GitHubLogin ghLogin) throws IOException { - if (ghLogin.login(httpRequest, httpResponse)) { + if (ghLogin.login(httpRequest, httpResponse, oauth)) { GHMyself myself = ghLogin.getMyself(); String user = myself.getLogin(); - updateSecureConfigWithRetry(ghLogin.hub.getMyOrganizations().keySet(), - user, ghLogin.token.accessToken); + updateSecureConfigWithRetry(ghLogin.getHub().getMyOrganizations().keySet(), + user, ghLogin.getToken().accessToken); } }
diff --git a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/PooledHttpClientProvider.java b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/PooledHttpClientProvider.java new file mode 100644 index 0000000..0263877 --- /dev/null +++ b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/PooledHttpClientProvider.java
@@ -0,0 +1,75 @@ +// 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 com.google.common.base.Strings; +import com.google.gerrit.httpd.ProxyProperties; +import com.google.gerrit.server.config.GerritServerConfig; +import com.google.inject.Inject; +import com.google.inject.Provider; + +import org.apache.http.HttpHost; +import org.apache.http.auth.AuthScope; +import org.apache.http.auth.UsernamePasswordCredentials; +import org.apache.http.client.CredentialsProvider; +import org.apache.http.client.HttpClient; +import org.apache.http.impl.client.BasicCredentialsProvider; +import org.apache.http.impl.client.HttpClientBuilder; +import org.apache.http.impl.client.ProxyAuthenticationStrategy; +import org.eclipse.jgit.lib.Config; + +import java.net.URL; + +public class PooledHttpClientProvider implements Provider<HttpClient> { + private final int maxTotalConnection; + private final int maxConnectionPerRoute; + private final ProxyProperties proxy; + + @Inject + PooledHttpClientProvider(@GerritServerConfig Config config, + ProxyProperties proxyProperties) { + this.proxy = proxyProperties; + maxConnectionPerRoute = config.getInt("http", null, + "pooledMaxConnectionsPerRoute", 16); + maxTotalConnection = config.getInt("http", null, + "pooledMaxTotalConnections", 32); + } + + @Override + public HttpClient get() { + HttpClientBuilder builder = HttpClientBuilder + .create() + .setMaxConnPerRoute(maxConnectionPerRoute) + .setMaxConnTotal(maxTotalConnection); + + if (proxy.getProxyUrl() != null) { + URL url = proxy.getProxyUrl(); + builder.setProxy(new HttpHost(url.getHost(), url.getPort())); + if (!Strings.isNullOrEmpty(proxy.getUsername()) + && !Strings.isNullOrEmpty(proxy.getPassword())) { + UsernamePasswordCredentials creds = new UsernamePasswordCredentials( + proxy.getUsername(), proxy.getPassword()); + CredentialsProvider credsProvider = new BasicCredentialsProvider(); + credsProvider.setCredentials(new AuthScope(url.getHost(), + url.getPort()), creds); + builder.setDefaultCredentialsProvider(credsProvider); + builder.setProxyAuthenticationStrategy( + new ProxyAuthenticationStrategy()); + } + } + + return builder.build(); + } +}
diff --git a/github-plugin/pom.xml b/github-plugin/pom.xml index fcf52a5..259f559 100644 --- a/github-plugin/pom.xml +++ b/github-plugin/pom.xml
@@ -136,7 +136,7 @@ <dependency> <groupId>org.apache.httpcomponents</groupId> <artifactId>httpclient</artifactId> - <version>4.0</version> + <version>4.4</version> </dependency> <dependency> <groupId>javax.mail</groupId>
diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/GitHubConfig.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/GitHubConfig.java index f4d070d..17c3b9e 100644 --- a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/GitHubConfig.java +++ b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/GitHubConfig.java
@@ -16,11 +16,12 @@ import com.google.common.base.MoreObjects; import com.google.common.collect.Maps; import com.google.gerrit.server.config.AllProjectsNameProvider; +import com.google.gerrit.server.config.AuthConfig; +import com.google.gerrit.server.config.CanonicalWebUrl; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.config.SitePaths; import com.google.inject.Inject; import com.google.inject.Singleton; - import com.googlesource.gerrit.plugins.github.oauth.GitHubOAuthConfig; import org.eclipse.jgit.lib.Config; @@ -69,9 +70,13 @@ @Inject - public GitHubConfig(@GerritServerConfig Config config, final SitePaths site, AllProjectsNameProvider allProjectsNameProvider) + public GitHubConfig(@GerritServerConfig Config config, + final SitePaths site, + AllProjectsNameProvider allProjectsNameProvider, + @CanonicalWebUrl String canonicalWebUrl, + AuthConfig authConfig) throws MalformedURLException { - super(config); + super(config, canonicalWebUrl, authConfig); String[] wizardFlows = config.getStringList(CONF_SECTION, null, CONF_WIZARD_FLOW); for (String fromTo : wizardFlows) {
diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/GuiceHttpModule.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/GuiceHttpModule.java index 29727a4..6bfbb16 100644 --- a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/GuiceHttpModule.java +++ b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/GuiceHttpModule.java
@@ -17,7 +17,6 @@ import com.google.inject.assistedinject.FactoryModuleBuilder; import com.google.inject.name.Names; import com.google.inject.servlet.ServletModule; - import com.googlesource.gerrit.plugins.github.filters.GitHubOAuthFilter; import com.googlesource.gerrit.plugins.github.git.CreateProjectStep; import com.googlesource.gerrit.plugins.github.git.GitCloneStep; @@ -25,8 +24,8 @@ import com.googlesource.gerrit.plugins.github.git.GitImporter; import com.googlesource.gerrit.plugins.github.git.PullRequestImportJob; import com.googlesource.gerrit.plugins.github.git.ReplicateProjectStep; -import com.googlesource.gerrit.plugins.github.oauth.GitHubHttpProvider; import com.googlesource.gerrit.plugins.github.oauth.GitHubLogin; +import com.googlesource.gerrit.plugins.github.oauth.PooledHttpClientProvider; import com.googlesource.gerrit.plugins.github.oauth.ScopedProvider; import com.googlesource.gerrit.plugins.github.replication.RemoteSiteUser; import com.googlesource.gerrit.plugins.github.velocity.PluginVelocityRuntimeProvider; @@ -41,7 +40,7 @@ @Override protected void configureServlets() { - bind(HttpClient.class).toProvider(GitHubHttpProvider.class); + bind(HttpClient.class).toProvider(PooledHttpClientProvider.class); bind(new TypeLiteral<ScopedProvider<GitHubLogin>>() {}).to(GitHubLogin.Provider.class); bind(new TypeLiteral<ScopedProvider<GitImporter>>() {}).to(GitImporter.Provider.class); @@ -68,9 +67,9 @@ serve("*.css", "*.js", "*.png", "*.jpg", "*.woff", "*.gif", "*.ttf").with( VelocityStaticServlet.class); - serve("*.html").with(VelocityViewServlet.class); serve("*.gh").with(VelocityControllerServlet.class); + serve("/static/*").with(VelocityViewServlet.class); filter("*").through(GitHubOAuthFilter.class); } }
diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/InitGitHub.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/InitGitHub.java index 261587f..564fd3e 100644 --- a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/InitGitHub.java +++ b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/InitGitHub.java
@@ -13,16 +13,25 @@ // limitations under the License. package com.googlesource.gerrit.plugins.github; +import java.net.URISyntaxException; + import com.google.gerrit.pgm.init.api.ConsoleUI; import com.google.gerrit.pgm.init.api.InitStep; +import com.google.gerrit.pgm.init.api.InitUtil; import com.google.gerrit.pgm.init.api.Section; import com.google.inject.Inject; public class InitGitHub implements InitStep { + private static final String GITHUB_URL = "https://github.com"; + private static final String GITHUB_API_URL = "https://api.github.com"; + private static final String GITHUB_REGISTER_APPLICATION_PATH = "/settings/applications/new"; + private static final String GERRIT_OAUTH_CALLBACK_PATH = "oauth"; + private final ConsoleUI ui; private final Section auth; private final Section httpd; - private Section github; + private final Section github; + private final Section gerrit; @Inject InitGitHub(final ConsoleUI ui, final Section.Factory sections) { @@ -30,29 +39,60 @@ this.github = sections.get("github", null); this.httpd = sections.get("httpd", null); this.auth = sections.get("auth", null); + this.gerrit = sections.get("gerrit", null); } @Override public void run() throws Exception { ui.header("GitHub Integration"); - github.string("GitHub URL", "url", "https://github.com"); + auth.set("httpHeader", "GITHUB_USER"); + auth.set("httpExternalIdHeader", "GITHUB_OAUTH_TOKEN"); + auth.set("loginUrl","/login"); + auth.set("loginText", "Sign-in with GitHub"); + auth.set("registerPageUrl", "/#/register"); - boolean gitHubAuth = ui.yesno(true, "Use GitHub for Gerrit login ?"); - if(gitHubAuth) { - configureAuth(); - } - } + github.string("GitHub URL", "url", GITHUB_URL); + github.string("GitHub API URL", "apiUrl", GITHUB_API_URL); + ui.message("\nNOTE: You might need to configure a proxy using http.proxy" + + " if you run Gerrit behind a firewall.\n"); - private void configureAuth() { - github.string("ClientId", "clientId", null); - github.string("ClientSecret", "clientSecret", null); + String gerritUrl = getAssumedCanonicalWebUrl(); + ui.header("GitHub OAuth registration and credentials"); + ui.message( + "Register Gerrit as GitHub application on:\n" + + "%s%s\n\n", + github.get("url"), GITHUB_REGISTER_APPLICATION_PATH); + ui.message("Settings (assumed Gerrit URL: %s)\n", gerritUrl); + ui.message("* Application name: Gerrit Code Review\n"); + ui.message("* Homepage URL: %s\n", gerritUrl); + ui.message("* Authorization callback URL: %s%s\n\n", gerritUrl, GERRIT_OAUTH_CALLBACK_PATH); + ui.message("After registration is complete, enter the generated OAuth credentials:\n"); + github.string("GitHub Client ID", "clientId", null); + github.passwordForKey("GitHub Client Secret", "clientSecret"); auth.string("HTTP Authentication Header", "httpHeader", "GITHUB_USER"); auth.set("type", "HTTP"); httpd.set("filterClass", "com.googlesource.gerrit.plugins.github.oauth.OAuthFilter"); } + private String getAssumedCanonicalWebUrl() { + String url = gerrit.get("canonicalWebUrl"); + if (url != null) { + return url; + } + + String httpListen = httpd.get("listenUrl"); + if (httpListen != null) { + try { + return InitUtil.toURI(httpListen).toString(); + } catch (URISyntaxException e) { + } + } + + return String.format("http://%s:8080/", InitUtil.hostname()); + } + @Override public void postRun() throws Exception { }
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 b844ecc..59ceb46 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
@@ -18,6 +18,7 @@ import com.googlesource.gerrit.plugins.github.oauth.GitHubLogin; import com.googlesource.gerrit.plugins.github.oauth.GitHubOAuthConfig; +import com.googlesource.gerrit.plugins.github.oauth.OAuthProtocol; import com.googlesource.gerrit.plugins.github.oauth.OAuthProtocol.Scope; import com.googlesource.gerrit.plugins.github.oauth.ScopedProvider; @@ -33,6 +34,7 @@ import javax.servlet.ServletRequest; import javax.servlet.ServletResponse; import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; @Singleton public class GitHubOAuthFilter implements Filter { @@ -40,12 +42,14 @@ private final ScopedProvider<GitHubLogin> loginProvider; private final Scope[] authScopes; + private final OAuthProtocol oauth; @Inject - public GitHubOAuthFilter(final ScopedProvider<GitHubLogin> loginProvider, - final GitHubOAuthConfig githubOAuthConfig) { + public GitHubOAuthFilter(ScopedProvider<GitHubLogin> loginProvider, + GitHubOAuthConfig githubOAuthConfig, OAuthProtocol oauth) { this.loginProvider = loginProvider; this.authScopes = githubOAuthConfig.getDefaultScopes(); + this.oauth = oauth; } @Override @@ -58,7 +62,8 @@ GitHubLogin hubLogin = loginProvider.get((HttpServletRequest) request); LOG.debug("GitHub login: " + hubLogin); if (!hubLogin.isLoggedIn()) { - hubLogin.login(request, response, authScopes); + hubLogin.login((HttpServletRequest) request, + (HttpServletResponse) response, oauth, authScopes); return; } else { chain.doFilter(request, response);
diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/GitHubRepository.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/GitHubRepository.java index 8e4f57e..c5cf07a 100644 --- a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/GitHubRepository.java +++ b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/GitHubRepository.java
@@ -25,6 +25,7 @@ import org.eclipse.jgit.transport.CredentialsProvider; import org.eclipse.jgit.transport.URIish; import org.kohsuke.github.GHRepository; +import org.kohsuke.github.GitHub; import java.io.IOException; @@ -39,14 +40,15 @@ private final String organisation; private final String repository; - private final GitHubLogin ghLogin; private final String cloneUrl; + private final String username; + private final String password; @Delegate private GHRepository ghRepository; public String getCloneUrl() { - return cloneUrl.replace("://", "://" + ghLogin.getMyself().getLogin() + "@"); + return cloneUrl.replace("://", "://" + username + "@"); } public String getOrganisation() { @@ -65,9 +67,12 @@ this.cloneUrl = gitHubUrl + "/" + organisation + "/" + repository + ".git"; this.organisation = organisation; this.repository = repository; - this.ghLogin = ghLoginProvider.get(); + GitHubLogin ghLogin = ghLoginProvider.get(); + GitHub gh = ghLogin.getHub(); + this.username = ghLogin.getMyself().getLogin(); + this.password = ghLogin.getToken().accessToken; this.ghRepository = - ghLogin.getHub().getRepository(organisation + "/" + repository); + gh.getRepository(organisation + "/" + repository); } public CredentialsProvider getCredentialsProvider() { @@ -97,13 +102,13 @@ throws UnsupportedCredentialItem { String username = uri.getUser(); if (username == null) { - username = ghLogin.getMyself().getLogin(); + username = GitHubRepository.this.username; } if (username == null) { return false; } - String password = ghLogin.getToken().accessToken; + String password = GitHubRepository.this.password; if (password == null) { return false; }
diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/ReplicateProjectStep.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/ReplicateProjectStep.java index 44e5a23..5410c78 100644 --- a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/ReplicateProjectStep.java +++ b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/ReplicateProjectStep.java
@@ -13,6 +13,8 @@ // limitations under the License. package com.googlesource.gerrit.plugins.github.git; +import java.io.IOException; + import com.google.inject.Inject; import com.google.inject.assistedinject.Assisted; @@ -43,7 +45,7 @@ final ScopedProvider<GitHubLogin> ghLoginProvider, @GitHubURL String gitHubUrl, @Assisted("organisation") String organisation, - @Assisted("name") String repository) { + @Assisted("name") String repository) throws IOException { super(gitHubUrl, organisation, repository, gitHubRepoFactory); LOG.debug("Gerrit ReplicateProject " + organisation + "/" + repository); this.replicationConfig = replicationConfig;
diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/velocity/VelocityViewServlet.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/velocity/VelocityViewServlet.java index b9f5d34..cd1a202 100644 --- a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/velocity/VelocityViewServlet.java +++ b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/velocity/VelocityViewServlet.java
@@ -82,10 +82,7 @@ + nextPage.uri; } - String pathInfo = MoreObjects.firstNonNull(destUrl, servletPath); - if (!pathInfo.startsWith(STATIC_PREFIX)) { - resp.sendError(HttpStatus.SC_NOT_FOUND); - } + String pathInfo = STATIC_PREFIX + MoreObjects.firstNonNull(destUrl, servletPath); try { Template template = velocityRuntime.getTemplate(pathInfo, "UTF-8"); @@ -104,7 +101,7 @@ } } - private PluginVelocityModel initVelocityModel(HttpServletRequest request) { + private PluginVelocityModel initVelocityModel(HttpServletRequest request) throws IOException { PluginVelocityModel model = modelProvider.get(); GitHubLogin gitHubLogin = loginProvider.get(request); model.put("myself", gitHubLogin.getMyself());
diff --git a/github-plugin/src/main/resources/Documentation/about.md b/github-plugin/src/main/resources/Documentation/about.md new file mode 100644 index 0000000..5ba04a6 --- /dev/null +++ b/github-plugin/src/main/resources/Documentation/about.md
@@ -0,0 +1,26 @@ + +This plugins allows to integrate Gerrit with external set of users configured +on GitHub. +It relies on the installation of the github-oauth Java library under the `$GERRIT_SITE/lib` +in order filter all the HTTP requests through the GitHub OAuth 2.0 secure authentication. + +GitHub application registration +------------------------------- + +GitHub uses OAuth2 as protocol to allow external apps request authorization to private +details in a user's GitHub account without getting their password. This is +preferred over Basic Authentication because tokens can be limited to specific +types of data, and can be revoked by users at any time. + +Site owners have to register their application before getting started. For +more information see [GitHub application registration page](https://github.com/settings/applications/new). +A registered OAuth application is assigned a unique `Client ID` and `Client +Secret`. The `Client Secret` should never be shared. + +The Gerrit OAuth callback `<gerrit canonical URL>/oauth` +needs to be specified in the GitHub application registration to establish mutual +trust between the two domains and exchange the authorization codes. The use of HTTPS +for Gerrit is strongly recommended for keeping the secrets exchange confidential. + +`auth.httpHeader` is set to `GITHUB_USER` with this authentication method and `auth.type` +must be set to HTTP. \ No newline at end of file
diff --git a/github-plugin/src/main/resources/Documentation/config.md b/github-plugin/src/main/resources/Documentation/config.md index 2d18944..9d7c87b 100644 --- a/github-plugin/src/main/resources/Documentation/config.md +++ b/github-plugin/src/main/resources/Documentation/config.md
@@ -1,13 +1,6 @@ -Plugin @PLUGIN@ -=============== -This plugins allows to integrate Gerrit with external set of users configured -on GitHub. -It relies on the installation of the github-oauth Java library under the $GERRIT_SITE/lib -in order filter all the HTTP requests through the GitHub OAuth 2.0 secure authentication. - -GitHub init step ----------------- +GitHub configuration during Gerrit init +--------------------------------------- This plugin provides a customized Gerrit init step for the self-configuration of the main GitHub and Gerrit authentication settings for allowing the github-oauth @@ -18,19 +11,58 @@ See below a sample session of relevant init steps for a default configuration pointing to the Web GitHub instance: - *** User Authentication - *** +``` + *** GitHub Integration + *** - Authentication method []: HTTP - Get username from custom HTTP header [Y/n]? Y - Username HTTP header []: GITHUB_USER - SSO logout URL : /oauth/reset + GitHub URL [https://github.com]: + GitHub API URL [https://api.github.com]: + NOTE: You might need to configure a proxy using http.proxy if you run Gerrit behind a firewall. - *** GitHub Integration - *** + *** GitHub OAuth registration and credentials + *** - GitHub URL [https://github.com]: - Use GitHub for Gerrit login ? [Y/n]? Y - ClientId []: 384cbe2e8d98192f9799 - ClientSecret []: f82c3f9b3802666f2adcc4c8cacfb164295b0a99 + Register Gerrit as GitHub application on: + https://github.com/settings/applications/new + + Settings (assumed Gerrit URL: http://localhost:8080/) + * Application name: Gerrit Code Review + * Homepage URL: http://localhost:8080/ + * Authorization callback URL: http://localhost:8080/oauth + + After registration is complete, enter the generated OAuth credentials: + GitHub Client ID [1ebea047915210179cf5]: + ClientSecret []: f82c3f9b3802666f2adcc4c8cacfb164295b0a99 + confirm password : + HTTP Authentication Header [GITHUB_USER]: +``` + +Configuration +------------- + +GitHub plugin read his configuration from gerrit.config under the `[github]` section. + +github.url +: GitHub homepage URL. Default is `https://github.com`. Can be customized to a different + value for GitHub:Enterprise integration + +**NOTE** You might need to configure a proxy using the configuration `http.proxy` if you run +Gerrit behind a firewall. + +github.apiUrl +: GitHub API URL. Default is `https://api.github.com`. Can be customized to a different + value for GitHub:Enterprise integration + +github.clientId +: The `Client ID`, that was received from GitHub when the application was registered. Required. + +github.clientSecret +: The `Client Secret`, that was received from GitHub when the application was registered. Required. + +github.scopes +: The GitHub scopes for allowing access to the user's public or private profile, organisations and + repositories. See [GitHub Scopes definition](https://developer.github.com/v3/oauth/#scopes) + for a detailed description of available scopes and their associated permissions. + Default is empty read-only access to public + information (includes public user profile info, public repository info, and gists).