Allows user changing scope within a Gerrit session Even after the user has been already authenticated with GitHub with a default OAuth scope, it is possible now to visualize the current scope and changing it on-the-fly. This is particularly useful during repositories import where a user realises that the initial scope wasn’t enough to display private organizations or repositories. Change-Id: I5af497571db7482bd74ddf25ffccc26fea3bcaa3
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 8a358e5..8cc52ae 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
@@ -93,11 +93,8 @@ return token != null; } - public boolean login(HttpServletRequest request, + public void login(HttpServletRequest request, HttpServletResponse response, OAuthProtocol oauth, Scope... scopes) throws IOException { - if (isLoggedIn()) { - return true; - } log.debug("Login " + this); if (OAuthProtocol.isOAuthFinal(request)) { @@ -108,7 +105,6 @@ if (isLoggedIn()) { log.debug("Login-SUCCESS " + this); response.sendRedirect(OAuthProtocol.getTargetUrl(request)); - return true; } } else { Set<String> configuredScopesProfiles = config.scopes.keySet(); @@ -124,7 +120,6 @@ state = oauth.loginPhase1(request, response, loginScopes); } } - return false; } public void logout() { @@ -144,7 +139,11 @@ } public GitHub getHub() throws IOException { - return GitHub.connectUsingOAuth(config.gitHubApiUrl, token.accessToken); + if (token == null) { + return null; + } else { + return GitHub.connectUsingOAuth(config.gitHubApiUrl, token.accessToken); + } } private String getScopesKey(HttpServletRequest request, @@ -167,7 +166,7 @@ return scopeRequested; } - private String getScopesKeyFromCookie(HttpServletRequest request) { + public String getScopesKeyFromCookie(HttpServletRequest request) { Cookie[] cookies = request.getCookies(); if (cookies == null) { return null;
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 d8f620f..e2ea0f8 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
@@ -67,6 +67,8 @@ public final int fileUpdateMaxRetryCount; public final int fileUpdateMaxRetryIntervalMsec; public final String oauthHttpHeader; + + @Getter public final String scopeSelectionUrl; @Inject
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 978fd34..2f98970 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
@@ -13,6 +13,7 @@ // limitations under the License. package com.googlesource.gerrit.plugins.github.oauth; +import com.google.common.base.Strings; import com.google.common.collect.Sets; import com.google.gerrit.httpd.GitOverHttpServlet; import com.google.gerrit.httpd.XGerritAuth; @@ -25,6 +26,7 @@ import java.io.IOException; import java.util.Arrays; +import java.util.List; import java.util.Set; import java.util.regex.Pattern; @@ -43,8 +45,12 @@ private static Pattern GIT_HTTP_REQUEST_PATTERN = Pattern .compile(GitOverHttpServlet.URL_REGEX); private static final Set<String> GERRIT_STATIC_RESOURCES_EXTS = Sets - .newHashSet(Arrays.asList("css", "png", "jpg", "gif", "woff", "otf", - "ttf", "map", "js", "swf", "txt")); + .newHashSet("css", "png", "jpg", "gif", "woff", "otf", + "ttf", "map", "js", "swf", "txt"); + private static final Set<String> GERRIT_WHITELISTED_PATHS = Sets + .newHashSet("Documentation"); + private static final Set<String> GERRIT_WHITELISTED_PAGES = Sets + .newHashSet("scope.html"); private final GitHubOAuthConfig config; private final OAuthGitFilter gitFilter; @@ -70,36 +76,54 @@ FilterChain chain) throws IOException, ServletException { HttpServletRequest httpRequest = (HttpServletRequest) request; - if (!config.enabled || isStaticResource(httpRequest) - || isRpcCall(httpRequest) || isAuthenticatedRestCall(httpRequest)) { - chain.doFilter(request, response); - return; - } - String requestUrl = httpRequest.getRequestURI(); - if (GIT_HTTP_REQUEST_PATTERN.matcher(requestUrl).matches()) { - gitFilter.doFilter(request, response, chain); + + if (!config.enabled || skipOAuth(httpRequest)) { + chain.doFilter(request, response); } else { - webFilter.doFilter(request, response, chain); + + if (GIT_HTTP_REQUEST_PATTERN.matcher(requestUrl).matches()) { + gitFilter.doFilter(request, response, chain); + } else { + System.out.println("Authorising " + requestUrl); + webFilter.doFilter(request, response, chain); + } } } - private boolean isAuthenticatedRestCall(HttpServletRequest httpRequest) { + public static boolean skipOAuth(HttpServletRequest httpRequest) { + return isStaticResource(httpRequest) + || isRpcCall(httpRequest) || isAuthenticatedRestCall(httpRequest) + || isWhitelisted(httpRequest); + } + + private static boolean isAuthenticatedRestCall(HttpServletRequest httpRequest) { return !StringUtils.isEmpty(httpRequest .getHeader(XGerritAuth.X_GERRIT_AUTH)); } - private boolean isStaticResource(HttpServletRequest httpRequest) { - String pathExt = - StringUtils.substringAfterLast(httpRequest.getRequestURI(), "."); + private static boolean isStaticResource(HttpServletRequest httpRequest) { + String requestURI = httpRequest.getRequestURI(); + String pathExt = StringUtils.substringAfterLast(requestURI, "."); if (StringUtils.isEmpty(pathExt)) { return false; } - return GERRIT_STATIC_RESOURCES_EXTS.contains(pathExt.toLowerCase()); + boolean staticResource = + GERRIT_STATIC_RESOURCES_EXTS.contains(pathExt.toLowerCase()); + System.out.println("requestUri: " + requestURI + " pathExt: " + pathExt + + " static: " + staticResource); + return staticResource; } - private boolean isRpcCall(HttpServletRequest httpRequest) { + private static boolean isWhitelisted(HttpServletRequest httpRequest) { + String[] requestPathParts = httpRequest.getRequestURI().split("/"); + return (requestPathParts.length > 1 && (GERRIT_WHITELISTED_PATHS + .contains(requestPathParts[1]) || GERRIT_WHITELISTED_PAGES + .contains(requestPathParts[requestPathParts.length - 1]))); + } + + private static boolean isRpcCall(HttpServletRequest httpRequest) { return httpRequest.getRequestURI().indexOf("/rpc/") >= 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 5b5a0a8..a349c42 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
@@ -192,6 +192,7 @@ private static final String ME_SEPARATOR = ","; private static final Logger log = LoggerFactory .getLogger(OAuthProtocol.class); + private static final String FINAL_URL_PARAM = "final"; private static SecureRandom randomState = newRandomGenerator(); private final GitHubOAuthConfig config; @@ -315,16 +316,18 @@ } public static boolean isOAuthFinal(HttpServletRequest request) { - return Strings.emptyToNull(request.getParameter("code")) != null; + return Strings.emptyToNull(request.getParameter("code")) != null + && request.getParameter(FINAL_URL_PARAM) == null; } public static boolean isOAuthFinalForOthers(HttpServletRequest request) { String targetUrl = getTargetUrl(request); - if(targetUrl.equals(request.getRequestURI())) { + if (targetUrl.equals(request.getRequestURI())) { return false; } - - return Strings.emptyToNull(request.getParameter("code")) != null; + + return Strings.emptyToNull(request.getParameter("code")) != null + && request.getParameter(FINAL_URL_PARAM) == null; } public String newRandomState(String redirectUrl) { @@ -334,7 +337,9 @@ } public static boolean isOAuthLogin(HttpServletRequest request) { - return request.getRequestURI().indexOf(GitHubOAuthConfig.GERRIT_LOGIN) >= 0; + String requestUri = request.getRequestURI(); + return requestUri.indexOf(GitHubOAuthConfig.GERRIT_LOGIN) >= 0 + && request.getParameter(FINAL_URL_PARAM) == null; } public static boolean isOAuthLogout(HttpServletRequest request) { @@ -349,7 +354,8 @@ HttpServletResponse response, String state) throws IOException { String requestState = request.getParameter("state"); if (!Objects.equals(state, requestState)) { - throw new IOException("Invalid authentication state"); + throw new IOException("Invalid authentication state: expected '" + state + + "' but was '" + requestState + "'"); } return getAccessToken(new OAuthVerifier(request.getParameter("code"))); @@ -398,10 +404,11 @@ public static String getTargetUrl(ServletRequest request) { int meEnd = state(request).indexOf(ME_SEPARATOR); + String finalUrlSuffix = "?" + FINAL_URL_PARAM + "=true"; if (meEnd > 0) { - return state(request).substring(meEnd + 1); + return state(request).substring(meEnd + 1) + finalUrlSuffix; } else { - return ""; + return finalUrlSuffix; } }
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 6291481..d11745e 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
@@ -85,26 +85,21 @@ try { GitHubLogin ghLogin = loginProvider.get(httpRequest); - if (OAuthProtocol.isOAuthLogout(httpRequest)) { - logout(request, response, chain, httpRequest); - } else if (OAuthProtocol.isOAuthRequest(httpRequest) - && !ghLogin.isLoggedIn()) { + if (OAuthProtocol.isOAuthRequest(httpRequest)) { login(request, httpRequest, httpResponse, ghLogin); } else { + if (OAuthProtocol.isOAuthLogout(httpRequest)) { + response = logout(request, response, chain, httpRequest); + } + if (ghLogin != null && ghLogin.isLoggedIn()) { httpRequest = new AuthenticatedHttpRequest(httpRequest, config.httpHeader, - ghLogin.getMyself().getLogin(), - config.oauthHttpHeader, + ghLogin.getMyself().getLogin(), config.oauthHttpHeader, GITHUB_EXT_ID + ghLogin.getToken().accessToken); } - if (OAuthProtocol.isOAuthFinalForOthers(httpRequest)) { - httpResponse.sendRedirect(OAuthProtocol - .getTargetOAuthFinal(httpRequest)); - } else { - chain.doFilter(httpRequest, response); - } + chain.doFilter(httpRequest, response); } } finally { HttpSession httpSession = httpRequest.getSession(); @@ -124,7 +119,8 @@ private void login(ServletRequest request, HttpServletRequest httpRequest, HttpServletResponse httpResponse, GitHubLogin ghLogin) throws IOException { - if (ghLogin.login(httpRequest, httpResponse, oauth)) { + ghLogin.login(httpRequest, httpResponse, oauth); + if (ghLogin.isLoggedIn()) { GHMyself myself = ghLogin.getMyself(); String user = myself.getLogin(); @@ -133,14 +129,12 @@ } } - private void logout(ServletRequest request, ServletResponse response, - FilterChain chain, HttpServletRequest httpRequest) throws IOException, - ServletException { + private ServletResponse logout(ServletRequest request, + ServletResponse response, FilterChain chain, + HttpServletRequest httpRequest) throws IOException, ServletException { getGitHubLogin(request).logout(); - GitHubLogoutServletResponse bufferedResponse = - new GitHubLogoutServletResponse((HttpServletResponse) response, - config.logoutRedirectUrl); - chain.doFilter(httpRequest, bufferedResponse); + return new GitHubLogoutServletResponse((HttpServletResponse) response, + config.logoutRedirectUrl); } private GitHubLogin getGitHubLogin(ServletRequest request) {
diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/GitHubTopMenu.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/GitHubTopMenu.java index 20696ee..61431e0 100644 --- a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/GitHubTopMenu.java +++ b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/GitHubTopMenu.java
@@ -38,10 +38,12 @@ @Inject public GitHubTopMenu(@PluginName String pluginName, Provider<CurrentUser> userProvider, - AuthConfig authConfig) { + AuthConfig authConfig, + GitHubConfig ghConfig) { String baseUrl = "/plugins/" + pluginName; this.menuEntries = Arrays.asList(new MenuEntry("GitHub", Arrays.asList( + getItem("Scope", ghConfig.scopeSelectionUrl), getItem("Profile", baseUrl + "/static/account.html"), getItem("Repositories", baseUrl + "/static/repositories.html"), getItem("Pull Requests", baseUrl + "/static/pullrequests.html"))));
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 2bac968..933533f 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
@@ -13,11 +13,22 @@ // limitations under the License. package com.googlesource.gerrit.plugins.github.filters; +import com.google.common.base.Predicate; +import com.google.common.collect.Collections2; +import com.google.gerrit.reviewdb.client.AccountExternalId; +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.AccountState; 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.GitHubOAuthConfig; +import com.googlesource.gerrit.plugins.github.oauth.OAuthFilter; import com.googlesource.gerrit.plugins.github.oauth.OAuthProtocol; +import com.googlesource.gerrit.plugins.github.oauth.OAuthProtocol.AccessToken; +import com.googlesource.gerrit.plugins.github.oauth.OAuthWebFilter; import com.googlesource.gerrit.plugins.github.oauth.OAuthProtocol.Scope; import com.googlesource.gerrit.plugins.github.oauth.ScopedProvider; @@ -26,6 +37,7 @@ import java.io.IOException; import java.util.Arrays; +import java.util.Collection; import java.util.List; import javax.servlet.Filter; @@ -41,21 +53,25 @@ public class GitHubOAuthFilter implements Filter { private Logger LOG = LoggerFactory.getLogger(GitHubOAuthFilter.class); - private static final List<String> whiteList = Arrays.asList(".png", ".jpg", - ".js", ".css"); - private final ScopedProvider<GitHubLogin> loginProvider; private final Scope[] authScopes; private final OAuthProtocol oauth; private final GitHubOAuthConfig config; + private final Provider<CurrentUser> userProvider; + private final AccountCache accountCache; @Inject public GitHubOAuthFilter(ScopedProvider<GitHubLogin> loginProvider, - GitHubOAuthConfig githubOAuthConfig, OAuthProtocol oauth) { + GitHubOAuthConfig githubOAuthConfig, + OAuthProtocol oauth, + Provider<CurrentUser> userProvider, + AccountCache accountCache) { this.loginProvider = loginProvider; this.authScopes = githubOAuthConfig.getDefaultScopes(); this.oauth = oauth; this.config = githubOAuthConfig; + this.userProvider = userProvider; + this.accountCache = accountCache; } @Override @@ -67,23 +83,41 @@ FilterChain chain) throws IOException, ServletException { GitHubLogin hubLogin = loginProvider.get((HttpServletRequest) request); LOG.debug("GitHub login: " + hubLogin); - if (!hubLogin.isLoggedIn() && !isWhiteListed(request)) { - hubLogin.login((HttpServletRequest) request, - (HttpServletResponse) response, oauth, authScopes); - return; - } else { - chain.doFilter(request, response); + CurrentUser user = userProvider.get(); + if (!hubLogin.isLoggedIn() + && !OAuthFilter.skipOAuth((HttpServletRequest) request) + && user.isIdentifiedUser()) { + AccountExternalId gitHubExtId = getGitHubExternalId(user); + + String oauthToken = + gitHubExtId.getSchemeRest() + .substring(OAuthWebFilter.GITHUB_EXT_ID.length()); + hubLogin.login(new AccessToken(oauthToken)); } + + chain.doFilter(request, response); } - private boolean isWhiteListed(ServletRequest request) { - String requestUri = ((HttpServletRequest) request).getRequestURI(); - for (String suffix : whiteList) { - if (requestUri.endsWith(suffix)) { - return true; - } + private AccountExternalId getGitHubExternalId(CurrentUser user) { + Collection<AccountExternalId> accountExtIds = + accountCache.get(((IdentifiedUser) user).getAccountId()) + .getExternalIds(); + Collection<AccountExternalId> gitHubExtId = + Collections2.filter(accountExtIds, + new Predicate<AccountExternalId>() { + @Override + public boolean apply(AccountExternalId externalId) { + return externalId.isScheme(AccountExternalId.SCHEME_EXTERNAL) + && externalId.getSchemeRest().startsWith( + OAuthWebFilter.GITHUB_EXT_ID); + } + }); + + if (gitHubExtId.isEmpty()) { + throw new IllegalStateException("Current Gerrit user " + + user.getUserName() + " has no GitHub OAuth external ID"); } - return config.scopeSelectionUrl.endsWith(requestUri); + return gitHubExtId.iterator().next(); } @Override
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 9b876d4..c91b853 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
@@ -110,6 +110,7 @@ if (user.isIdentifiedUser()) { model.put("user", user); model.put("hub", gitHubLogin.getHub()); + model.put("scopeCookie", gitHubLogin.getScopesKeyFromCookie(request)); } for (Entry<String, String[]> reqPar : request.getParameterMap().entrySet()) {
diff --git a/github-plugin/src/main/resources/static/repositories.html b/github-plugin/src/main/resources/static/repositories.html index e4cd855..fe8deb1 100644 --- a/github-plugin/src/main/resources/static/repositories.html +++ b/github-plugin/src/main/resources/static/repositories.html
@@ -53,6 +53,7 @@ </select> <input type="text" id="filter" class="filter" name="filter" placeholder="Filter by name" /> </li> + <li class="info"><p>Not seeing your organizations or repositories? <a href="$config.scopeSelectionUrl">Login with a different GitHub Scope</a> and try again.</p></li> </ul> <div class="loading"> <p>Loading list of GitHub repositories ...</p>
diff --git a/github-plugin/src/main/resources/static/scope.html b/github-plugin/src/main/resources/static/scope.html index 2b131fc..29795f0 100644 --- a/github-plugin/src/main/resources/static/scope.html +++ b/github-plugin/src/main/resources/static/scope.html
@@ -37,23 +37,28 @@ <h5>Which level of GitHub access do you need?</h5> <ul class="scopes"> #foreach ( $scope in $config.scopes.keySet() ) - <li> - #set ( $scopeName = $scope.substring(6) ) - #if ( $scopeName == "") - <input type="radio" name="scope" value="scopes" checked><p class="scope">DEFAULT</p> - #else - <input type="radio" name="scope" value="scopes$scopeName"><p class="scope">$scopeName</p> - #end - <p> - #set ( $scopeItems = $config.scopes.get($scope) ) - #foreach ( $scopeItem in $scopeItems ) - $scopeItem.description - #if ( $velocityCount < $scopeItems.size()) - , - #end - #end - </p> - </li> + <li> + #set ( $scopeName = $scope.substring(6) ) + #set ( $checked = "" ) + #if ( ( $scopeCookie && $scopeCookie == $scope ) || $scopeName == "" ) + #set ( $checked = "checked" ) + #end + + #if ( $scopeName == "") + <input type="radio" name="scope" value="scopes" $checked ><p class="scope">DEFAULT</p> + #else + <input type="radio" name="scope" value="scopes$scopeName" $checked ><p class="scope">$scopeName</p> + #end + <p> + #set ( $scopeItems = $config.scopes.get($scope) ) + #foreach ( $scopeItem in $scopeItems ) + $scopeItem.description + #if ( $velocityCount < $scopeItems.size()) + , + #end + #end + </p> + </li> #end </li> </ul>