Share GitHub login on Gerrit filter and plugin.
Allows to sign-in to GitHub only once during
Gerrit login and then share the GitHub OAuth token
within the plugin screens.
Previously the two logins were completely isolated
and the user had to connect twice.
With this change is not possible anymore to have
multiple logins with different scopes: not a regression
but a simplification IMHO, as having multiple sessions
with multiple permissions have lead to confusion
on why GitHub permissions are requested twice.
Change-Id: I8aeb131704ee015d67b56670295c231dd6db1478
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 6c6ff4f..8ebc73b 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
@@ -15,10 +15,6 @@
package com.googlesource.gerrit.plugins.github.oauth;
import java.io.IOException;
-import java.util.Arrays;
-import java.util.Set;
-import java.util.SortedSet;
-import java.util.TreeSet;
import javax.servlet.ServletRequest;
import javax.servlet.ServletResponse;
@@ -49,14 +45,13 @@
public AccessToken token;
public GitHub hub;
- private SortedSet<Scope> scopesSet = new TreeSet<OAuthProtocol.Scope>();
private transient OAuthProtocol oauth;
private GHMyself myself;
public GHMyself getMyself() {
- if (isLoggedIn(scopesSet)) {
+ if (isLoggedIn()) {
return myself;
} else {
return null;
@@ -68,18 +63,8 @@
this.oauth = oauth;
}
- public GitHubLogin(GitHub hub, AccessToken token, Scope... scopes) {
- this.hub = hub;
- this.token = token;
- this.scopesSet = new TreeSet<OAuthProtocol.Scope>(Arrays.asList(scopes));
- }
-
- public boolean isLoggedIn(Scope... scopes) {
- return isLoggedIn(new TreeSet<Scope>(Arrays.asList(scopes)));
- }
-
- public boolean isLoggedIn(Set<Scope> scopes) {
- boolean loggedIn = scopesSet.equals(scopes) && token != null && hub != null;
+ public boolean isLoggedIn() {
+ boolean loggedIn = token != null && hub != null;
if (loggedIn && myself == null) {
try {
myself = hub.getMyself();
@@ -100,16 +85,14 @@
public boolean login(HttpServletRequest request,
HttpServletResponse response, Scope... scopes) throws IOException {
- if (isLoggedIn(scopes)) {
+ if (isLoggedIn()) {
return true;
}
- setScopes(scopes);
-
- if (oauth.isOAuthFinal(request)) {
- init(oauth.loginPhase2(request, response));
- if (isLoggedIn(scopes)) {
- response.sendRedirect(oauth.getTargetUrl(request));
+ if (OAuthProtocol.isOAuthFinal(request)) {
+ login(oauth.loginPhase2(request, response));
+ if (isLoggedIn()) {
+ response.sendRedirect(OAuthProtocol.getTargetUrl(request));
return true;
} else {
response.sendError(HttpStatus.SC_UNAUTHORIZED);
@@ -122,17 +105,17 @@
}
public void logout() {
- scopesSet = new TreeSet<OAuthProtocol.Scope>();
hub = null;
token = null;
}
- private void setScopes(Scope... scopes) {
- this.scopesSet = new TreeSet<Scope>(Arrays.asList(scopes));
+ public OAuthProtocol getOAuthProtocol() {
+ return oauth;
}
- private void init(GitHubLogin initValues) {
- this.hub = initValues.hub;
- this.token = initValues.token;
+ public GitHub login(AccessToken authToken) throws IOException {
+ this.token = authToken;
+ this.hub = GitHub.connectUsingOAuth(authToken.access_token);
+ return this.hub;
}
}
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 e5d34bf..69ac33c 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
@@ -43,7 +43,9 @@
import com.google.gerrit.server.config.SitePaths;
import com.google.gson.Gson;
import com.google.inject.Inject;
+import com.google.inject.Provider;
import com.google.inject.Singleton;
+import com.googlesource.gerrit.plugins.github.oauth.OAuthProtocol.AccessToken;
@Singleton
public class OAuthFilter implements Filter {
@@ -53,18 +55,19 @@
private final GitHubOAuthConfig config;
private final OAuthCookieProvider cookieProvider;
- private final OAuthProtocol oauth;
private final Random retryRandom = new Random(System.currentTimeMillis());
private SitePaths sites;
+ private ScopedProvider<GitHubLogin> loginProvider;
@Inject
- public OAuthFilter(GitHubOAuthConfig config, SitePaths sites) {
+ public OAuthFilter(GitHubOAuthConfig config, SitePaths sites,
+ // 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.cookieProvider = new OAuthCookieProvider(new TokenCipher());
- HttpClient httpClient;
- httpClient = new GitHubHttpProvider().get();
- this.oauth = new OAuthProtocol(config, httpClient, new Gson());
this.sites = sites;
+ this.loginProvider = loginProvider;
}
@Override
@@ -74,6 +77,7 @@
@Override
public void doFilter(ServletRequest request, ServletResponse response,
FilterChain chain) throws IOException, ServletException {
+
if (!config.enabled) {
chain.doFilter(request, response);
return;
@@ -82,23 +86,25 @@
HttpServletRequest httpRequest = (HttpServletRequest) request;
HttpServletResponse httpResponse = (HttpServletResponse) response;
log.debug("doFilter(" + httpRequest.getRequestURI() + ") code="
- + request.getParameter("code") + " me=" + oauth.me());
+ + request.getParameter("code"));
Cookie gerritCookie = getGerritCookie(httpRequest);
try {
OAuthCookie authCookie =
getOAuthCookie(httpRequest, (HttpServletResponse) response);
-
- if(oauth.isOAuthLogout((HttpServletRequest) request)) {
+
+ if(OAuthProtocol.isOAuthLogout((HttpServletRequest) request)) {
+ getGitHubLogin(request).logout();
GitHubLogoutServletResponse bufferedResponse = new GitHubLogoutServletResponse((HttpServletResponse) response,
config.logoutRedirectUrl);
chain.doFilter(httpRequest, bufferedResponse);
- } else if (((oauth.isOAuthLogin(httpRequest) || oauth.isOAuthFinal(httpRequest)) && authCookie == null)
+ } else if (((OAuthProtocol.isOAuthLogin(httpRequest) || OAuthProtocol.isOAuthFinal(httpRequest)) && authCookie == null)
|| (authCookie == null && gerritCookie == null)) {
- if (oauth.isOAuthFinal(httpRequest)) {
+ GitHubLogin ghLogin = getGitHubLogin(httpRequest);
+ if (OAuthProtocol.isOAuthFinal(httpRequest)) {
- GitHubLogin hubLogin = oauth.loginPhase2(httpRequest, httpResponse);
- GitHub hub = hubLogin.hub;
+ AccessToken authToken = ghLogin.getOAuthProtocol().loginPhase2(httpRequest, httpResponse);
+ GitHub hub = ghLogin.login(authToken);
GHMyself myself =
hub.getMyself();
String user = myself.getLogin();
@@ -108,28 +114,28 @@
.getName();
updateSecureConfigWithRetry(hub.getMyOrganizations().keySet(), user,
- hubLogin.token.access_token);
+ ghLogin.token.access_token);
if (user != null) {
OAuthCookie userCookie =
cookieProvider.getFromUser(user, email, fullName);
httpResponse.addCookie(userCookie);
- httpResponse.sendRedirect(oauth.getTargetUrl(request));
+ httpResponse.sendRedirect(OAuthProtocol.getTargetUrl(request));
return;
} else {
httpResponse.sendError(HttpURLConnection.HTTP_UNAUTHORIZED,
"Login failed");
}
} else {
- if (oauth.isOAuthLogin(httpRequest)) {
- oauth.loginPhase1(httpRequest, httpResponse);
+ if (OAuthProtocol.isOAuthLogin(httpRequest)) {
+ ghLogin.getOAuthProtocol().loginPhase1(httpRequest, httpResponse);
} else {
chain.doFilter(request, response);
}
}
return;
} else {
- if (gerritCookie != null && !oauth.isOAuthLogin(httpRequest)) {
+ if (gerritCookie != null && !OAuthProtocol.isOAuthLogin(httpRequest)) {
if (authCookie != null) {
authCookie.setMaxAge(0);
authCookie.setValue("");
@@ -142,8 +148,8 @@
authCookie.fullName, config.httpEmailHeader, authCookie.email);
}
- if (oauth.isOAuthFinalForOthers(httpRequest)) {
- httpResponse.sendRedirect(oauth.getTargetOAuthFinal(httpRequest));
+ if (OAuthProtocol.isOAuthFinalForOthers(httpRequest)) {
+ httpResponse.sendRedirect(OAuthProtocol.getTargetOAuthFinal(httpRequest));
return;
} else {
chain.doFilter(httpRequest, response);
@@ -165,6 +171,10 @@
}
}
+ private GitHubLogin getGitHubLogin(ServletRequest request) {
+ return loginProvider.get((HttpServletRequest) request);
+ }
+
private void updateSecureConfigWithRetry(Set<String> organisations,
String user, String access_token) {
int retryCount = 0;
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 1d281c6..f90bf9d 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
@@ -21,7 +21,6 @@
import org.apache.http.client.methods.HttpPost;
import org.apache.http.message.BasicNameValuePair;
import org.apache.http.protocol.HTTP;
-import org.kohsuke.github.GitHub;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -67,20 +66,20 @@
public static class AccessToken {
public String access_token;
public String token_type;
-
+
public AccessToken() {
}
-
- public AccessToken(String token, String type) {
+
+ public AccessToken(String token, String type, Scope... scopes) {
this.access_token = token;
this.token_type = type;
}
}
@Inject
- public OAuthProtocol(GitHubOAuthConfig config, HttpClient httpClient, Gson gson) {
+ public OAuthProtocol(GitHubOAuthConfig config, GitHubHttpProvider httpClientProvider, Gson gson) {
this.config = config;
- this.http = httpClient;
+ this.http = httpClientProvider.get();
this.gson = gson;
}
@@ -112,38 +111,32 @@
"scope=" + out.toString();
}
- public boolean isOAuthFinal(HttpServletRequest request) {
- return Strings.emptyToNull(request.getParameter("code")) != null
- && wasInitiatedByMe(request);
+ public static boolean isOAuthFinal(HttpServletRequest request) {
+ return Strings.emptyToNull(request.getParameter("code")) != null;
}
- public boolean isOAuthFinalForOthers(HttpServletRequest request) {
+ public static boolean isOAuthFinalForOthers(HttpServletRequest request) {
String targetUrl = getTargetUrl(request);
if(targetUrl.equals(request.getRequestURI())) {
return false;
}
- return Strings.emptyToNull(request.getParameter("code")) != null
- && !wasInitiatedByMe(request);
+ return Strings.emptyToNull(request.getParameter("code")) != null;
}
public String me() {
return "" + hashCode() + ME_SEPARATOR;
}
- public boolean wasInitiatedByMe(HttpServletRequest request) {
- return state(request).startsWith(me());
- }
-
- public boolean isOAuthLogin(HttpServletRequest request) {
+ public static boolean isOAuthLogin(HttpServletRequest request) {
return request.getRequestURI().indexOf(GitHubOAuthConfig.OAUTH_LOGIN) >= 0;
}
- public boolean isOAuthLogout(HttpServletRequest request) {
+ public static boolean isOAuthLogout(HttpServletRequest request) {
return request.getRequestURI().indexOf(GitHubOAuthConfig.OAUTH_LOGOUT) >= 0;
}
- public GitHubLogin loginPhase2(HttpServletRequest request,
+ public AccessToken loginPhase2(HttpServletRequest request,
HttpServletResponse response) throws IOException {
HttpPost post = null;
@@ -175,8 +168,7 @@
AccessToken token =
gson.fromJson(new InputStreamReader(postResponse.getEntity()
.getContent(), "UTF-8"), AccessToken.class);
- GitHub github = GitHub.connectUsingOAuth(token.access_token);
- return new GitHubLogin(github, token);
+ return token;
} catch (IOException e) {
log.error("POST " + config.gitHubOAuthAccessTokenUrl
+ " request for access token failed", e);
@@ -186,7 +178,7 @@
}
}
- private String getURLEncoded(String url) {
+ private static String getURLEncoded(String url) {
try {
return URLEncoder.encode(url, "UTF-8");
} catch (UnsupportedEncodingException e) {
@@ -195,7 +187,7 @@
}
}
- public String getTargetUrl(ServletRequest request) {
+ public static String getTargetUrl(ServletRequest request) {
int meEnd = state(request).indexOf(ME_SEPARATOR);
if (meEnd > 0) {
return state(request).substring(meEnd+1);
@@ -204,11 +196,11 @@
}
}
- private String state(ServletRequest request) {
+ private static String state(ServletRequest request) {
return Strings.nullToEmpty(request.getParameter("state"));
}
- public String getTargetOAuthFinal(HttpServletRequest httpRequest) {
+ public static String getTargetOAuthFinal(HttpServletRequest httpRequest) {
String targetUrl = getTargetUrl(httpRequest);
String code = getURLEncoded(httpRequest.getParameter("code"));
String state = getURLEncoded(httpRequest.getParameter("state"));
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 19f0000..bda7484 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
@@ -51,7 +51,7 @@
public void doFilter(ServletRequest request, ServletResponse response,
FilterChain chain) throws IOException, ServletException {
GitHubLogin hubLogin = loginProvider.get((HttpServletRequest) request);
- if (!hubLogin.isLoggedIn(authScopes)) {
+ if (!hubLogin.isLoggedIn()) {
hubLogin.login(request, response, authScopes);
return;
} else {
