Loading list of GitHub repositories ...

diff --git a/github-plugin/src/main/resources/static/scope.html b/github-plugin/src/main/resources/static/scope.html index af861394..ebc2580f 100644 --- a/github-plugin/src/main/resources/static/scope.html +++ b/github-plugin/src/main/resources/static/scope.html @@ -44,7 +44,7 @@

Login Scope Selection

Which level of GitHub access do you need?

#set ( $scopeName = $scope.name.substring(6) ) #set ( $scopeDescription = $scope.description ) @@ -60,7 +60,7 @@
Which level of GitHub access do you need?
#end
$scopeDescription

Allow to: - #set ( $scopeItems = $config.scopes.get($scope) ) + #set ( $scopeItems = $scopes.get($scope) ) #foreach ( $scopeItem in $scopeItems ) $scopeItem.description #if ( $foreach.count < $scopeItems.size()) diff --git a/github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/GitHubConfigTest.java b/github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/GitHubConfigTest.java index 56f7b90c..b7dfc60d 100644 --- a/github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/GitHubConfigTest.java +++ b/github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/GitHubConfigTest.java @@ -117,6 +117,6 @@ private GitHubConfig newGitHubConfig(String configText) throws Exception { + "clientId = myclientid\n" + "clientSecret = mysecret\n" + configText); - return new GitHubConfig(gerritConfig, site, ALL_PROJECTS_NAME_PROVIDER, null); + return new GitHubConfig(gerritConfig, site, ALL_PROJECTS_NAME_PROVIDER); } } From c438c579480502708b77d6a9e38833f52232aec9 Mon Sep 17 00:00:00 2001 From: Dariusz Luksza Date: Mon, 30 Oct 2023 12:33:26 +0000 Subject: [PATCH 12/44] Fix typos in CanonicalWebUrls Fixes typos in the `CanonicalWebUrls` class and variable names. Plus use `assertTrue` in the `GitHubOAuthConfigTest`. This is a follow-up change after post submit review comments[1] [1] https://gerrit-review.googlesource.com/c/plugins/github/+/391296/comment/4f6cb71e_eedd6d1b/ Change-Id: I232663ad940a2bc378721a9841e7415764abc6f9 --- .../{CannonicalWebUrls.java => CanonicalWebUrls.java} | 10 +++++----- .../gerrit/plugins/github/oauth/GitHubLogin.java | 8 ++++---- .../gerrit/plugins/github/oauth/GitHubOAuthConfig.java | 2 +- .../gerrit/plugins/github/oauth/OAuthProtocol.java | 8 ++++---- .../plugins/github/oauth/GitHubOAuthConfigTest.java | 3 ++- .../plugins/github/velocity/VelocityViewServlet.java | 10 +++++----- 6 files changed, 21 insertions(+), 20 deletions(-) rename github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/{CannonicalWebUrls.java => CanonicalWebUrls.java} (87%) diff --git a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/CannonicalWebUrls.java b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/CanonicalWebUrls.java similarity index 87% rename from github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/CannonicalWebUrls.java rename to github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/CanonicalWebUrls.java index 321d0ea4..faca0f98 100644 --- a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/CannonicalWebUrls.java +++ b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/CanonicalWebUrls.java @@ -24,19 +24,19 @@ import com.google.inject.Singleton; @Singleton -public class CannonicalWebUrls { +public class CanonicalWebUrls { private final GitHubOAuthConfig oauthConf; - private final HttpCanonicalWebUrlProvider canonnicalWebUrlProvider; + private final HttpCanonicalWebUrlProvider canonicalWebUrlProvider; static String trimTrailingSlash(String url) { return CharMatcher.is('/').trimTrailingFrom(url); } @Inject - CannonicalWebUrls( + CanonicalWebUrls( GitHubOAuthConfig oauthConf, HttpCanonicalWebUrlProvider canonicalWebUrlProvider) { this.oauthConf = oauthConf; - this.canonnicalWebUrlProvider = canonicalWebUrlProvider; + this.canonicalWebUrlProvider = canonicalWebUrlProvider; } public String getScopeSelectionUrl() { @@ -49,6 +49,6 @@ String getOAuthFinalRedirectUrl() { } private String getCannonicalWebUrl() { - return trimTrailingSlash(canonnicalWebUrlProvider.get()); + return trimTrailingSlash(canonicalWebUrlProvider.get()); } } 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 dafca3f1..adfe5e3d 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 @@ -64,7 +64,7 @@ public GitHubLogin get(HttpServletRequest request) { private SortedSet loginScopes; private final GitHubOAuthConfig config; - private final CannonicalWebUrls cannonicalWebUrls; + private final CanonicalWebUrls canonicalWebUrls; private final VirtualDomainConfig virtualDomainConfig; private final GitHubConnector gitHubConnector; @@ -85,11 +85,11 @@ public Set getMyOrganisationsLogins() throws IOException { @Inject public GitHubLogin( GitHubOAuthConfig config, - CannonicalWebUrls cannonicalWebUrls, + CanonicalWebUrls canonicalWebUrls, VirtualDomainConfig virutalDomainConfig, GitHubHttpConnector httpConnector) { this.config = config; - this.cannonicalWebUrls = cannonicalWebUrls; + this.canonicalWebUrls = canonicalWebUrls; this.virtualDomainConfig = virutalDomainConfig; this.gitHubConnector = GitHubConnectorHttpConnectorAdapter.adapt(httpConnector); } @@ -119,7 +119,7 @@ public void login( Set configuredScopesProfiles = virtualDomainConfig.getScopes(request).keySet(); String scopeRequested = getScopesKey(request, response); if (Strings.isNullOrEmpty(scopeRequested) && configuredScopesProfiles.size() > 1) { - response.sendRedirect(cannonicalWebUrls.getScopeSelectionUrl()); + response.sendRedirect(canonicalWebUrls.getScopeSelectionUrl()); } else { this.loginScopes = getScopes(request, MoreObjects.firstNonNull(scopeRequested, "scopes"), scopes); 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 f396a805..16fcb288 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 @@ -13,7 +13,7 @@ // limitations under the License. package com.googlesource.gerrit.plugins.github.oauth; -import static com.googlesource.gerrit.plugins.github.oauth.CannonicalWebUrls.trimTrailingSlash; +import static com.googlesource.gerrit.plugins.github.oauth.CanonicalWebUrls.trimTrailingSlash; import static com.googlesource.gerrit.plugins.github.oauth.GitHubOAuthConfig.KeyConfig.PASSWORD_DEVICE_CONFIG_LABEL; import com.google.common.base.MoreObjects; 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 3b8d7082..035f1cea 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 @@ -155,7 +155,7 @@ private Scope(final String value, final String description) { private static SecureRandom randomState = newRandomGenerator(); private final GitHubOAuthConfig config; - private final CannonicalWebUrls cannonicalWebUrls; + private final CanonicalWebUrls canonicalWebUrls; private final Gson gson; private final Provider httpProvider; @@ -232,7 +232,7 @@ public OAuthToken toOAuthToken() { @Inject public OAuthProtocol( GitHubOAuthConfig config, - CannonicalWebUrls cannonicalWebUrls, + CanonicalWebUrls canonicalWebUrls, PooledHttpClientProvider httpClientProvider, /* * We need to explicitly tell Guice which Provider<> we need as this class @@ -241,7 +241,7 @@ public OAuthProtocol( */ GsonProvider gsonProvider) { this.config = config; - this.cannonicalWebUrls = cannonicalWebUrls; + this.canonicalWebUrls = canonicalWebUrls; this.httpProvider = httpClientProvider; this.gson = gsonProvider.get(); } @@ -259,7 +259,7 @@ public String getAuthorizationUrl(String scopesString, String state, HttpServlet + "?client_id=" + config.gitHubClientId + getURLEncodedParameter("&scope=", scopesString) - + getURLEncodedParameter("&redirect_uri=", cannonicalWebUrls.getOAuthFinalRedirectUrl()) + + getURLEncodedParameter("&redirect_uri=", canonicalWebUrls.getOAuthFinalRedirectUrl()) + getURLEncodedParameter("&state=", state); } diff --git a/github-oauth/src/test/java/com/googlesource/gerrit/plugins/github/oauth/GitHubOAuthConfigTest.java b/github-oauth/src/test/java/com/googlesource/gerrit/plugins/github/oauth/GitHubOAuthConfigTest.java index 6833e718..7808ff4a 100644 --- a/github-oauth/src/test/java/com/googlesource/gerrit/plugins/github/oauth/GitHubOAuthConfigTest.java +++ b/github-oauth/src/test/java/com/googlesource/gerrit/plugins/github/oauth/GitHubOAuthConfigTest.java @@ -22,6 +22,7 @@ import static com.googlesource.gerrit.plugins.github.oauth.GitHubOAuthConfig.KeyConfig.SECRET_KEY_CONFIG_LABEL; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; import com.google.gerrit.extensions.client.AuthType; import java.util.ArrayList; @@ -185,7 +186,7 @@ public void shouldReturnOverridesForSpecificHostName() { Map>> virtualScopes = githubOAuthConfig().getVirtualScopes(); - assertEquals(virtualScopes.containsKey(vhost), true); + assertTrue(virtualScopes.containsKey(vhost)); SortedMap> vhostConfig = virtualScopes.get(vhost); List>> entries = 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 aab1ef00..b7dc08f1 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 @@ -20,7 +20,7 @@ import com.google.inject.Singleton; import com.google.inject.name.Named; import com.googlesource.gerrit.plugins.github.GitHubConfig; -import com.googlesource.gerrit.plugins.github.oauth.CannonicalWebUrls; +import com.googlesource.gerrit.plugins.github.oauth.CanonicalWebUrls; import com.googlesource.gerrit.plugins.github.oauth.GitHubLogin; import com.googlesource.gerrit.plugins.github.oauth.ScopedProvider; import com.googlesource.gerrit.plugins.github.oauth.VirtualDomainConfig; @@ -52,7 +52,7 @@ public class VelocityViewServlet extends HttpServlet { private final Provider userProvider; private final GitHubConfig config; private final VirtualDomainConfig virtualDomainConfig; - private final CannonicalWebUrls cannonicalWebUrls; + private final CanonicalWebUrls canonicalWebUrls; @Inject public VelocityViewServlet( @@ -62,7 +62,7 @@ public VelocityViewServlet( Provider userProvider, GitHubConfig config, VirtualDomainConfig virutalDomainConfig, - CannonicalWebUrls cannonicalWebUrls) { + CanonicalWebUrls canonicalWebUrls) { this.velocityRuntime = velocityRuntime; this.modelProvider = modelProvider; @@ -70,7 +70,7 @@ public VelocityViewServlet( this.userProvider = userProvider; this.config = config; this.virtualDomainConfig = virutalDomainConfig; - this.cannonicalWebUrls = cannonicalWebUrls; + this.canonicalWebUrls = canonicalWebUrls; } @Override @@ -104,7 +104,7 @@ private PluginVelocityModel initVelocityModel(HttpServletRequest request) throws GitHubLogin gitHubLogin = loginProvider.get(request); model.put("myself", gitHubLogin.getMyself()); model.put("config", config); - model.put("scopeSelectionUrl", cannonicalWebUrls.getScopeSelectionUrl()); + model.put("scopeSelectionUrl", canonicalWebUrls.getScopeSelectionUrl()); model.put("scopes", virtualDomainConfig.getScopes(request)); CurrentUser user = userProvider.get(); From 812ab6255e73e9053241fe85a116b6d5a73cd864 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Thu, 9 Nov 2023 00:58:44 +0000 Subject: [PATCH 13/44] Refresh GitHub groups upon Gerrit successful login GitHub groups are cached and persisted on disk, for preventing GitHub outages to impact Gerrit Code Review. The persistent cache may however cause security and inconsistency issues with the original copy of the groups and memebership on GitHub. Make sure that groups are refreshed every time a user is logging on Gerrit Code Review UI successfully. NOTE: Git/HTTP is excluded because it could lead to bursts of GitHub APIs that could exhaust the user's allowance. Because of the lack of login notification from Gerrit, the solution needs to be based on the presence of the Set-Cookie header in the HTTP response, hence the group refresh needs to use a ServletFilter that is invoked for every request heading to Gerrit. Bug: Issue 308979847 Change-Id: Ie5d012fb7c34e3edbb5576b53de32fd5b140ec17 --- .../plugins/github/GuiceHttpModule.java | 7 + .../GitHubGroupCacheRefreshFilter.java | 78 +++ .../github/group/CurrentUsernameProvider.java | 41 ++ .../github/group/GitHubGroupsCache.java | 24 +- .../github/FakeHttpServletRequest.java | 465 ++++++++++++++++++ .../github/FakeHttpServletResponse.java | 249 ++++++++++ .../plugins/github/FakeHttpSession.java | 112 +++++ .../GitHubGroupCacheRefreshFilterTest.java | 120 +++++ 8 files changed, 1089 insertions(+), 7 deletions(-) create mode 100644 github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/filters/GitHubGroupCacheRefreshFilter.java create mode 100644 github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/group/CurrentUsernameProvider.java create mode 100644 github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/FakeHttpServletRequest.java create mode 100644 github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/FakeHttpServletResponse.java create mode 100644 github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/FakeHttpSession.java create mode 100644 github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/GitHubGroupCacheRefreshFilterTest.java 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 661b0bf9..ef241915 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 @@ -18,10 +18,13 @@ import com.google.gerrit.extensions.registration.DynamicSet; import com.google.gerrit.extensions.webui.JavaScriptPlugin; import com.google.gerrit.extensions.webui.WebUiPlugin; +import com.google.gerrit.httpd.AllRequestFilter; +import com.google.inject.Scopes; import com.google.inject.TypeLiteral; 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.GitHubGroupCacheRefreshFilter; import com.googlesource.gerrit.plugins.github.filters.GitHubOAuthFilter; import com.googlesource.gerrit.plugins.github.git.CreateProjectStep; import com.googlesource.gerrit.plugins.github.git.GitCloneStep; @@ -103,5 +106,9 @@ protected void configureServlets() { serve("/static/*").with(VelocityViewServlet.class); filterRegex("(?!/webhook).*").through(GitHubOAuthFilter.class); + + DynamicSet.bind(binder(), AllRequestFilter.class) + .to(GitHubGroupCacheRefreshFilter.class) + .in(Scopes.SINGLETON); } } diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/filters/GitHubGroupCacheRefreshFilter.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/filters/GitHubGroupCacheRefreshFilter.java new file mode 100644 index 00000000..eb34962b --- /dev/null +++ b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/filters/GitHubGroupCacheRefreshFilter.java @@ -0,0 +1,78 @@ +// Copyright (C) 2023 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.filters; + +import com.google.common.annotations.VisibleForTesting; +import com.google.common.flogger.FluentLogger; +import com.google.gerrit.httpd.AllRequestFilter; +import com.googlesource.gerrit.plugins.github.group.GitHubGroupsCache; +import java.io.IOException; +import java.util.Optional; +import javax.inject.Inject; +import javax.servlet.FilterChain; +import javax.servlet.FilterConfig; +import javax.servlet.ServletException; +import javax.servlet.ServletRequest; +import javax.servlet.ServletResponse; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +public class GitHubGroupCacheRefreshFilter extends AllRequestFilter { + private static final FluentLogger logger = FluentLogger.forEnclosingClass(); + private static final String LOGIN_URL = "/login"; + private static final String LOGIN_QUERY_FINAL = "final=true"; + private static final String ACCOUNT_COOKIE = "GerritAccount"; + private static final String INVALIDATE_CACHED_GROUPS = "RefreshGroups"; + + private final GitHubGroupsCache ghGroupsCache; + + @Inject + @VisibleForTesting + public GitHubGroupCacheRefreshFilter(GitHubGroupsCache ghGroupsCache) { + this.ghGroupsCache = ghGroupsCache; + } + + @Override + public void init(FilterConfig filterConfig) throws ServletException {} + + @Override + public void doFilter( + ServletRequest servletRequest, ServletResponse servletResponse, FilterChain filterChain) + throws IOException, ServletException { + filterChain.doFilter(servletRequest, servletResponse); + + HttpServletRequest req = (HttpServletRequest) servletRequest; + if (req.getRequestURI().endsWith(LOGIN_URL) && req.getQueryString().equals(LOGIN_QUERY_FINAL)) { + HttpServletResponse resp = (HttpServletResponse) servletResponse; + String cookieResponse = resp.getHeader("Set-Cookie"); + if (cookieResponse != null && cookieResponse.contains(ACCOUNT_COOKIE)) { + req.getSession().setAttribute(INVALIDATE_CACHED_GROUPS, Boolean.TRUE); + } + } else if (hasSessionFlagForInvalidatingCachedUserGroups(req)) { + ghGroupsCache.invalidateCurrentUserGroups(); + req.getSession().removeAttribute(INVALIDATE_CACHED_GROUPS); + } + } + + private static boolean hasSessionFlagForInvalidatingCachedUserGroups(HttpServletRequest req) { + return Optional.ofNullable(req.getSession(false)) + .flatMap(session -> Optional.ofNullable(session.getAttribute(INVALIDATE_CACHED_GROUPS))) + .filter(refresh -> (Boolean) refresh) + .isPresent(); + } + + @Override + public void destroy() {} +} diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/group/CurrentUsernameProvider.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/group/CurrentUsernameProvider.java new file mode 100644 index 00000000..028aa6b0 --- /dev/null +++ b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/group/CurrentUsernameProvider.java @@ -0,0 +1,41 @@ +// Copyright (C) 2023 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.group; + +import com.google.gerrit.server.CurrentUser; +import com.google.gerrit.server.IdentifiedUser; +import com.google.inject.Inject; +import com.google.inject.Provider; +import java.util.Optional; + +public class CurrentUsernameProvider implements Provider { + public static final String CURRENT_USERNAME = "CurrentUsername"; + + private final Provider userProvider; + + @Inject + CurrentUsernameProvider(Provider userProvider) { + this.userProvider = userProvider; + } + + @Override + public String get() { + return Optional.ofNullable(userProvider.get()) + .filter(CurrentUser::isIdentifiedUser) + .map(CurrentUser::asIdentifiedUser) + .flatMap(IdentifiedUser::getUserName) + .orElse(null); + } +} diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/group/GitHubGroupsCache.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/group/GitHubGroupsCache.java index 3504f2c4..a5301e9f 100644 --- a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/group/GitHubGroupsCache.java +++ b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/group/GitHubGroupsCache.java @@ -14,19 +14,21 @@ package com.googlesource.gerrit.plugins.github.group; +import static com.googlesource.gerrit.plugins.github.group.CurrentUsernameProvider.CURRENT_USERNAME; import static java.time.temporal.ChronoUnit.MINUTES; +import com.google.common.annotations.VisibleForTesting; import com.google.common.cache.CacheLoader; import com.google.common.cache.LoadingCache; import com.google.common.collect.ImmutableSet; import com.google.gerrit.entities.AccountGroup.UUID; -import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.cache.CacheModule; import com.google.inject.Inject; import com.google.inject.Module; import com.google.inject.Provider; import com.google.inject.Singleton; import com.google.inject.name.Named; +import com.google.inject.name.Names; import com.googlesource.gerrit.plugins.github.groups.OrganizationStructure; import com.googlesource.gerrit.plugins.github.oauth.GitHubLogin; import com.googlesource.gerrit.plugins.github.oauth.UserScopedProvider; @@ -106,6 +108,9 @@ public static Module module() { return new CacheModule() { @Override protected void configure() { + bind(String.class) + .annotatedWith(Names.named(CurrentUsernameProvider.CURRENT_USERNAME)) + .toProvider(CurrentUsernameProvider.class); persist(ORGS_CACHE_NAME, String.class, OrganizationStructure.class) .expireAfterWrite(Duration.of(GROUPS_CACHE_TTL_MINS, MINUTES)) .loader(OrganisationLoader.class); @@ -115,14 +120,15 @@ protected void configure() { } private final LoadingCache orgTeamsByUsername; - private final Provider userProvider; + private final Provider usernameProvider; @Inject - GitHubGroupsCache( + @VisibleForTesting + public GitHubGroupsCache( @Named(ORGS_CACHE_NAME) LoadingCache byUsername, - Provider userProvider) { + @Named(CURRENT_USERNAME) Provider usernameProvider) { this.orgTeamsByUsername = byUsername; - this.userProvider = userProvider; + this.usernameProvider = usernameProvider; } Set getOrganizationsForUser(String username) { @@ -135,7 +141,7 @@ Set getOrganizationsForUser(String username) { } Set getOrganizationsForCurrentUser() throws ExecutionException { - return orgTeamsByUsername.get(userProvider.get().getUserName().get()).keySet(); + return orgTeamsByUsername.get(usernameProvider.get()).keySet(); } Set getTeamsForUser(String organizationName, String username) { @@ -156,7 +162,7 @@ Set getTeamsForUser(String organizationName, String username) { } Set getTeamsForCurrentUser(String organizationName) { - return getTeamsForUser(organizationName, userProvider.get().getUserName().get()); + return getTeamsForUser(organizationName, usernameProvider.get()); } public Set getGroupsForUser(String username) { @@ -170,4 +176,8 @@ public Set getGroupsForUser(String username) { } return groupsBuilder.build(); } + + public void invalidateCurrentUserGroups() { + orgTeamsByUsername.invalidate(usernameProvider.get()); + } } diff --git a/github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/FakeHttpServletRequest.java b/github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/FakeHttpServletRequest.java new file mode 100644 index 00000000..cdb229dd --- /dev/null +++ b/github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/FakeHttpServletRequest.java @@ -0,0 +1,465 @@ +// Copyright (C) 2023 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; + +import static com.google.common.base.Strings.nullToEmpty; +import static java.nio.charset.StandardCharsets.UTF_8; +import static java.util.Objects.requireNonNull; +import static java.util.stream.Collectors.toList; + +import com.google.common.base.Splitter; +import com.google.common.collect.Iterables; +import com.google.common.collect.LinkedListMultimap; +import com.google.common.collect.ListMultimap; +import com.google.common.collect.Maps; +import com.google.gerrit.common.Nullable; +import java.io.BufferedReader; +import java.io.UnsupportedEncodingException; +import java.net.URLDecoder; +import java.security.Principal; +import java.time.format.DateTimeFormatter; +import java.util.Collection; +import java.util.Collections; +import java.util.Enumeration; +import java.util.List; +import java.util.Locale; +import java.util.Map; +import javax.servlet.AsyncContext; +import javax.servlet.DispatcherType; +import javax.servlet.RequestDispatcher; +import javax.servlet.ServletContext; +import javax.servlet.ServletInputStream; +import javax.servlet.ServletRequest; +import javax.servlet.ServletResponse; +import javax.servlet.http.Cookie; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import javax.servlet.http.HttpSession; +import javax.servlet.http.HttpUpgradeHandler; +import javax.servlet.http.Part; + +/** Simple fake implementation of {@link HttpServletRequest}. */ +public class FakeHttpServletRequest implements HttpServletRequest { + public static final String SERVLET_PATH = "/b"; + public static final DateTimeFormatter rfcDateformatter = + DateTimeFormatter.ofPattern("EEE, dd MMM yyyy HH:mm:ss ZZZ"); + + private final Map attributes; + private final ListMultimap headers; + + private ListMultimap parameters; + private String queryString; + private String servletPath; + private String path; + private String method; + private HttpSession session; + + public FakeHttpServletRequest() { + this(SERVLET_PATH, null); + } + + public FakeHttpServletRequest(String servletPath, HttpSession existingSession) { + this.servletPath = requireNonNull(servletPath, "servletPath"); + attributes = Maps.newConcurrentMap(); + parameters = LinkedListMultimap.create(); + headers = LinkedListMultimap.create(); + session = existingSession; + } + + @Override + public Object getAttribute(String name) { + return attributes.get(name); + } + + @Override + public Enumeration getAttributeNames() { + return Collections.enumeration(attributes.keySet()); + } + + @Override + public String getCharacterEncoding() { + return UTF_8.name(); + } + + @Override + public int getContentLength() { + return 0; + } + + @Nullable + @Override + public String getContentType() { + return null; + } + + @Override + public ServletInputStream getInputStream() { + throw new UnsupportedOperationException(); + } + + @Override + public String getLocalAddr() { + return "1.2.3.4"; + } + + @Override + public String getLocalName() { + return "localhost"; + } + + @Override + public int getLocalPort() { + return 80; + } + + @Override + public Locale getLocale() { + return Locale.US; + } + + @Override + public Enumeration getLocales() { + return Collections.enumeration(Collections.singleton(Locale.US)); + } + + @Override + public String getParameter(String name) { + return Iterables.getFirst(parameters.get(name), null); + } + + @Override + public Map getParameterMap() { + return Collections.unmodifiableMap( + Maps.transformValues(parameters.asMap(), vs -> vs.toArray(new String[0]))); + } + + @Override + public Enumeration getParameterNames() { + return Collections.enumeration(parameters.keySet()); + } + + @Override + public String[] getParameterValues(String name) { + return parameters.get(name).toArray(new String[0]); + } + + public void setQueryString(String qs) { + this.queryString = qs; + ListMultimap params = LinkedListMultimap.create(); + for (String entry : Splitter.on('&').split(qs)) { + List kv = Splitter.on('=').limit(2).splitToList(entry); + try { + params.put( + URLDecoder.decode(kv.get(0), UTF_8.name()), + kv.size() == 2 ? URLDecoder.decode(kv.get(1), UTF_8.name()) : ""); + } catch (UnsupportedEncodingException e) { + throw new IllegalArgumentException(e); + } + } + parameters = params; + } + + @Override + public String getProtocol() { + return "HTTP/1.1"; + } + + @Override + public BufferedReader getReader() { + throw new UnsupportedOperationException(); + } + + @Override + @Deprecated + public String getRealPath(String path) { + throw new UnsupportedOperationException(); + } + + @Override + public String getRemoteAddr() { + return "5.6.7.8"; + } + + @Override + public String getRemoteHost() { + return "remotehost"; + } + + @Override + public int getRemotePort() { + return 1234; + } + + @Override + public RequestDispatcher getRequestDispatcher(String path) { + throw new UnsupportedOperationException(); + } + + @Override + public String getScheme() { + return "http"; + } + + @Override + public String getServerName() { + return "localhost"; + } + + @Override + public int getServerPort() { + return 80; + } + + @Override + public boolean isSecure() { + return false; + } + + @Override + public void removeAttribute(String name) { + attributes.remove(name); + } + + @Override + public void setAttribute(String name, Object value) { + attributes.put(name, value); + } + + @Override + public void setCharacterEncoding(String env) throws UnsupportedOperationException { + throw new UnsupportedOperationException(); + } + + @Override + public String getAuthType() { + return null; + } + + @Override + public String getContextPath() { + return ""; + } + + @Override + public Cookie[] getCookies() { + return Splitter.on(";").splitToList(nullToEmpty(getHeader("Cookie"))).stream() + .filter(s -> !s.isEmpty()) + .map( + (String cookieValue) -> { + List kv = Splitter.on("=").splitToList(cookieValue); + return new Cookie(kv.get(0), kv.get(1)); + }) + .collect(toList()) + .toArray(new Cookie[0]); + } + + @Override + public long getDateHeader(String name) { + return 0L; + } + + @Override + public String getHeader(String name) { + return Iterables.getFirst(headers.get(name), null); + } + + @Override + public Enumeration getHeaderNames() { + return Collections.enumeration(headers.keySet()); + } + + @Override + public Enumeration getHeaders(String name) { + return Collections.enumeration(headers.get(name)); + } + + @Override + public int getIntHeader(String name) { + return Integer.parseInt(getHeader(name)); + } + + @Override + public String getMethod() { + return method; + } + + public void setMethod(String method) { + this.method = method; + } + + @Override + public String getPathInfo() { + return path; + } + + public FakeHttpServletRequest setPathInfo(String path) { + this.path = path; + return this; + } + + @Override + public String getPathTranslated() { + return path; + } + + @Override + public String getQueryString() { + return queryString; + } + + @Override + public String getRemoteUser() { + return null; + } + + @Override + public String getRequestURI() { + return nullToEmpty(servletPath) + nullToEmpty(path); + } + + @Override + public StringBuffer getRequestURL() { + return new StringBuffer("http://localhost" + getRequestURI()); + } + + @Override + public String getRequestedSessionId() { + return null; + } + + @Override + public String getServletPath() { + return servletPath; + } + + @Override + public HttpSession getSession() { + return getSession(true); + } + + @Override + public HttpSession getSession(boolean create) { + if (session == null && create) { + session = new FakeHttpSession(); + } + return session; + } + + @Override + public Principal getUserPrincipal() { + throw new UnsupportedOperationException(); + } + + @Override + public boolean isRequestedSessionIdFromCookie() { + throw new UnsupportedOperationException(); + } + + @Override + public boolean isRequestedSessionIdFromURL() { + throw new UnsupportedOperationException(); + } + + @Override + @Deprecated + public boolean isRequestedSessionIdFromUrl() { + throw new UnsupportedOperationException(); + } + + @Override + public boolean isRequestedSessionIdValid() { + throw new UnsupportedOperationException(); + } + + @Override + public boolean isUserInRole(String role) { + throw new UnsupportedOperationException(); + } + + @Override + public AsyncContext getAsyncContext() { + throw new UnsupportedOperationException(); + } + + @Override + public DispatcherType getDispatcherType() { + throw new UnsupportedOperationException(); + } + + @Override + public ServletContext getServletContext() { + throw new UnsupportedOperationException(); + } + + @Override + public boolean isAsyncStarted() { + return false; + } + + @Override + public boolean isAsyncSupported() { + return false; + } + + @Override + public AsyncContext startAsync() { + throw new UnsupportedOperationException(); + } + + @Override + public AsyncContext startAsync(ServletRequest req, ServletResponse res) { + throw new UnsupportedOperationException(); + } + + @Override + public boolean authenticate(HttpServletResponse res) { + throw new UnsupportedOperationException(); + } + + @Override + public Part getPart(String name) { + throw new UnsupportedOperationException(); + } + + @Override + public Collection getParts() { + throw new UnsupportedOperationException(); + } + + @Override + public void login(String username, String password) { + throw new UnsupportedOperationException(); + } + + @Override + public void logout() { + throw new UnsupportedOperationException(); + } + + @Override + public long getContentLengthLong() { + return getContentLength(); + } + + @Override + public String changeSessionId() { + throw new UnsupportedOperationException(); + } + + @Override + public T upgrade(Class httpUpgradeHandlerClass) { + throw new UnsupportedOperationException(); + } +} diff --git a/github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/FakeHttpServletResponse.java b/github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/FakeHttpServletResponse.java new file mode 100644 index 00000000..6824a155 --- /dev/null +++ b/github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/FakeHttpServletResponse.java @@ -0,0 +1,249 @@ +// Copyright (C) 2015 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; + +import static com.google.common.base.Preconditions.checkArgument; +import static java.nio.charset.StandardCharsets.UTF_8; +import static java.util.Objects.requireNonNull; + +import com.google.common.collect.Iterables; +import com.google.common.collect.LinkedListMultimap; +import com.google.common.collect.ListMultimap; +import com.google.common.net.HttpHeaders; +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.io.PrintWriter; +import java.nio.charset.Charset; +import java.util.Collection; +import java.util.Locale; +import javax.servlet.ServletOutputStream; +import javax.servlet.http.Cookie; +import javax.servlet.http.HttpServletResponse; + +/** Simple fake implementation of {@link HttpServletResponse}. */ +public class FakeHttpServletResponse implements HttpServletResponse { + private final ByteArrayOutputStream actualBody = new ByteArrayOutputStream(); + private final ListMultimap headers = LinkedListMultimap.create(); + + private int status = SC_OK; + private boolean committed; + private ServletOutputStream outputStream; + private PrintWriter writer; + + public FakeHttpServletResponse() {} + + @Override + public synchronized void flushBuffer() throws IOException { + if (outputStream != null) { + outputStream.flush(); + } + if (writer != null) { + writer.flush(); + } + } + + @Override + public int getBufferSize() { + throw new UnsupportedOperationException(); + } + + @Override + public String getCharacterEncoding() { + return UTF_8.name(); + } + + @Override + public String getContentType() { + return null; + } + + @Override + public Locale getLocale() { + return Locale.US; + } + + @Override + public synchronized ServletOutputStream getOutputStream() { + throw new UnsupportedOperationException(); + } + + @Override + public synchronized PrintWriter getWriter() { + throw new UnsupportedOperationException(); + } + + @Override + public synchronized boolean isCommitted() { + return committed; + } + + @Override + public void reset() { + throw new UnsupportedOperationException(); + } + + @Override + public void resetBuffer() { + throw new UnsupportedOperationException(); + } + + @Override + public void setBufferSize(int sz) { + throw new UnsupportedOperationException(); + } + + @Override + public void setCharacterEncoding(String name) { + checkArgument(UTF_8.equals(Charset.forName(name)), "unsupported charset: %s", name); + } + + @Override + public void setContentLength(int length) { + setContentLengthLong(length); + } + + @Override + public void setContentLengthLong(long length) { + headers.removeAll(HttpHeaders.CONTENT_LENGTH); + addHeader(HttpHeaders.CONTENT_LENGTH, Long.toString(length)); + } + + @Override + public void setContentType(String type) { + headers.removeAll(HttpHeaders.CONTENT_TYPE); + addHeader(HttpHeaders.CONTENT_TYPE, type); + } + + @Override + public void setLocale(Locale locale) { + throw new UnsupportedOperationException(); + } + + @Override + public void addCookie(Cookie cookie) { + addHeader("Set-Cookie", cookie.getName() + "=" + cookie.getValue()); + } + + @Override + public void addDateHeader(String name, long value) { + throw new UnsupportedOperationException(); + } + + @Override + public void addHeader(String name, String value) { + headers.put(name.toLowerCase(Locale.US), value); + } + + @Override + public void addIntHeader(String name, int value) { + addHeader(name, Integer.toString(value)); + } + + @Override + public boolean containsHeader(String name) { + return headers.containsKey(name.toLowerCase(Locale.US)); + } + + @Override + public String encodeRedirectURL(String url) { + throw new UnsupportedOperationException(); + } + + @Override + @Deprecated + public String encodeRedirectUrl(String url) { + throw new UnsupportedOperationException(); + } + + @Override + public String encodeURL(String url) { + throw new UnsupportedOperationException(); + } + + @Override + @Deprecated + public String encodeUrl(String url) { + throw new UnsupportedOperationException(); + } + + @Override + public synchronized void sendError(int sc) { + status = sc; + committed = true; + } + + @Override + public synchronized void sendError(int sc, String msg) { + status = sc; + committed = true; + } + + @Override + public synchronized void sendRedirect(String loc) { + status = SC_FOUND; + setHeader(HttpHeaders.LOCATION, loc); + committed = true; + } + + @Override + public void setDateHeader(String name, long value) { + setHeader(name, Long.toString(value)); + } + + @Override + public void setHeader(String name, String value) { + headers.removeAll(name.toLowerCase(Locale.US)); + addHeader(name, value); + } + + @Override + public void setIntHeader(String name, int value) { + headers.removeAll(name.toLowerCase(Locale.US)); + addIntHeader(name, value); + } + + @Override + public synchronized void setStatus(int sc) { + status = sc; + committed = true; + } + + @Override + @Deprecated + public synchronized void setStatus(int sc, String msg) { + status = sc; + committed = true; + } + + @Override + public synchronized int getStatus() { + return status; + } + + @Override + public String getHeader(String name) { + return Iterables.getFirst(headers.get(requireNonNull(name.toLowerCase(Locale.US))), null); + } + + @Override + public Collection getHeaderNames() { + return headers.keySet(); + } + + @Override + public Collection getHeaders(String name) { + return headers.get(requireNonNull(name.toLowerCase(Locale.US))); + } +} diff --git a/github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/FakeHttpSession.java b/github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/FakeHttpSession.java new file mode 100644 index 00000000..6a820e76 --- /dev/null +++ b/github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/FakeHttpSession.java @@ -0,0 +1,112 @@ +// Copyright (C) 2023 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; + +import java.util.Enumeration; +import java.util.HashMap; +import javax.servlet.ServletContext; +import javax.servlet.http.HttpSession; +import javax.servlet.http.HttpSessionContext; + +public class FakeHttpSession implements HttpSession { + private final HashMap attributes; + + public FakeHttpSession() { + this.attributes = new HashMap<>(); + } + + @Override + public long getCreationTime() { + return 0; + } + + @Override + public String getId() { + return null; + } + + @Override + public long getLastAccessedTime() { + return 0; + } + + @Override + public ServletContext getServletContext() { + return null; + } + + @Override + public void setMaxInactiveInterval(int i) {} + + @Override + public int getMaxInactiveInterval() { + return 0; + } + + @Override + public HttpSessionContext getSessionContext() { + return null; + } + + @Override + public Object getAttribute(String s) { + return attributes.get(s); + } + + @Override + public Object getValue(String s) { + return getAttribute(s); + } + + @Override + public Enumeration getAttributeNames() { + return java.util.Collections.enumeration(attributes.keySet()); + } + + @Override + public String[] getValueNames() { + return attributes.keySet().toArray(new String[0]); + } + + @Override + public void setAttribute(String s, Object o) { + attributes.put(s, o); + } + + @Override + public void putValue(String s, Object o) { + setAttribute(s, o); + } + + @Override + public void removeAttribute(String s) { + attributes.remove(s); + } + + @Override + public void removeValue(String s) { + removeAttribute(s); + } + + @Override + public void invalidate() { + attributes.clear(); + } + + @Override + public boolean isNew() { + return false; + } +} diff --git a/github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/GitHubGroupCacheRefreshFilterTest.java b/github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/GitHubGroupCacheRefreshFilterTest.java new file mode 100644 index 00000000..59a8e3ac --- /dev/null +++ b/github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/GitHubGroupCacheRefreshFilterTest.java @@ -0,0 +1,120 @@ +// Copyright (C) 2023 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; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.common.cache.CacheBuilder; +import com.google.common.cache.CacheLoader; +import com.google.common.cache.LoadingCache; +import com.googlesource.gerrit.plugins.github.filters.GitHubGroupCacheRefreshFilter; +import com.googlesource.gerrit.plugins.github.group.GitHubGroupsCache; +import com.googlesource.gerrit.plugins.github.groups.OrganizationStructure; +import javax.servlet.FilterChain; +import javax.servlet.ServletRequest; +import javax.servlet.http.HttpServletResponse; +import javax.servlet.http.HttpSession; +import org.junit.Before; +import org.junit.Test; + +public class GitHubGroupCacheRefreshFilterTest { + private static final FilterChain NOOP_FILTER_CHAIN_TEST = (req, res) -> {}; + private static final String GITHUB_USERNAME_TEST = "somegithubuser"; + private static final OrganizationStructure GITHUB_USER_ORGANIZATION = new OrganizationStructure(); + + private LoadingCache groupsByUsernameCache; + private GitHubGroupCacheRefreshFilter filter; + private FakeGroupCacheLoader groupsCacheLoader; + private int initialLoadCount; + + private static class FakeGroupCacheLoader extends CacheLoader { + private final String username; + private final OrganizationStructure organizationStructure; + private int loadCount; + + FakeGroupCacheLoader(String username, OrganizationStructure organizationStructure) { + this.username = username; + this.organizationStructure = organizationStructure; + } + + @Override + public OrganizationStructure load(String u) throws Exception { + if (u.equals(username)) { + loadCount++; + return organizationStructure; + } else { + return null; + } + } + + public int getLoadCount() { + return loadCount; + } + } + + @Before + public void setUp() throws Exception { + groupsCacheLoader = new FakeGroupCacheLoader(GITHUB_USERNAME_TEST, GITHUB_USER_ORGANIZATION); + groupsByUsernameCache = CacheBuilder.newBuilder().build(groupsCacheLoader); + filter = + new GitHubGroupCacheRefreshFilter( + new GitHubGroupsCache(groupsByUsernameCache, () -> GITHUB_USERNAME_TEST)); + // Trigger the initial load of the groups cache + assertThat(groupsByUsernameCache.get(GITHUB_USERNAME_TEST)).isEqualTo(GITHUB_USER_ORGANIZATION); + initialLoadCount = groupsCacheLoader.getLoadCount(); + } + + @Test + public void shouldReloadGroupsUponSuccessfulLogin() throws Exception { + FakeHttpServletRequest finalLoginRequest = newFinalLoginRequest(); + filter.doFilter(finalLoginRequest, newFinalLoginRedirectWithCookie(), NOOP_FILTER_CHAIN_TEST); + filter.doFilter( + newHomepageRequest(finalLoginRequest.getSession()), + new FakeHttpServletResponse(), + NOOP_FILTER_CHAIN_TEST); + + assertThat(groupsByUsernameCache.get(GITHUB_USERNAME_TEST)).isEqualTo(GITHUB_USER_ORGANIZATION); + assertThat(groupsCacheLoader.getLoadCount()).isEqualTo(initialLoadCount + 1); + } + + @Test + public void shouldNotReloadGroupsOnRegularRequests() throws Exception { + FakeHttpServletRequest regularRequest = new FakeHttpServletRequest(); + filter.doFilter(regularRequest, new FakeHttpServletResponse(), NOOP_FILTER_CHAIN_TEST); + filter.doFilter( + newHomepageRequest(regularRequest.getSession()), + new FakeHttpServletResponse(), + NOOP_FILTER_CHAIN_TEST); + + assertThat(groupsByUsernameCache.get(GITHUB_USERNAME_TEST)).isEqualTo(GITHUB_USER_ORGANIZATION); + assertThat(groupsCacheLoader.getLoadCount()).isEqualTo(initialLoadCount); + } + + private ServletRequest newHomepageRequest(HttpSession session) { + return new FakeHttpServletRequest("/", session); + } + + private static HttpServletResponse newFinalLoginRedirectWithCookie() { + HttpServletResponse res = new FakeHttpServletResponse(); + res.setHeader("Set-Cookie", "GerritAccount=foo"); + return res; + } + + private static FakeHttpServletRequest newFinalLoginRequest() { + FakeHttpServletRequest req = new FakeHttpServletRequest("/login", null); + req.setQueryString("final=true"); + return req; + } +} From 6239163808007d38ffb3dc0a8c9100964eeee9f2 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Thu, 9 Nov 2023 21:28:38 +0000 Subject: [PATCH 14/44] Bump to v3.9.0-rc4 and reuse FakeHttpServletRequest/Response Bump to v3.9.0-rc4 and reuse Gerrit's FakeHttpServletRequest/Response from its acceptance framework. Remove the duplicated copy in the plugin and the ad-hoc implementation tailored to the tests. Change-Id: Id86945fcede1ba5beb1ccd08b4e944c2bc621be9 --- github-oauth/pom.xml | 2 +- github-plugin/pom.xml | 8 +- .../github/FakeHttpServletRequest.java | 465 ------------------ .../github/FakeHttpServletResponse.java | 249 ---------- .../GitHubGroupCacheRefreshFilterTest.java | 15 +- pom.xml | 2 +- 6 files changed, 19 insertions(+), 722 deletions(-) delete mode 100644 github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/FakeHttpServletRequest.java delete mode 100644 github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/FakeHttpServletResponse.java diff --git a/github-oauth/pom.xml b/github-oauth/pom.xml index 256abc84..454e0fe1 100644 --- a/github-oauth/pom.xml +++ b/github-oauth/pom.xml @@ -21,7 +21,7 @@ limitations under the License. com.googlesource.gerrit.plugins.github github-parent - 3.9.0-rc2 + 3.9.0-rc4 github-oauth Gerrit Code Review - GitHub OAuth login diff --git a/github-plugin/pom.xml b/github-plugin/pom.xml index 96d32901..79e76a00 100644 --- a/github-plugin/pom.xml +++ b/github-plugin/pom.xml @@ -20,7 +20,7 @@ limitations under the License. github-parent com.googlesource.gerrit.plugins.github - 3.9.0-rc2 + 3.9.0-rc4 github-plugin @@ -151,6 +151,12 @@ limitations under the License. ${Gerrit-ApiVersion} provided + + com.google.gerrit + gerrit-acceptance-framework + ${Gerrit-ApiVersion} + test + ${project.groupId} github-oauth diff --git a/github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/FakeHttpServletRequest.java b/github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/FakeHttpServletRequest.java deleted file mode 100644 index cdb229dd..00000000 --- a/github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/FakeHttpServletRequest.java +++ /dev/null @@ -1,465 +0,0 @@ -// Copyright (C) 2023 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; - -import static com.google.common.base.Strings.nullToEmpty; -import static java.nio.charset.StandardCharsets.UTF_8; -import static java.util.Objects.requireNonNull; -import static java.util.stream.Collectors.toList; - -import com.google.common.base.Splitter; -import com.google.common.collect.Iterables; -import com.google.common.collect.LinkedListMultimap; -import com.google.common.collect.ListMultimap; -import com.google.common.collect.Maps; -import com.google.gerrit.common.Nullable; -import java.io.BufferedReader; -import java.io.UnsupportedEncodingException; -import java.net.URLDecoder; -import java.security.Principal; -import java.time.format.DateTimeFormatter; -import java.util.Collection; -import java.util.Collections; -import java.util.Enumeration; -import java.util.List; -import java.util.Locale; -import java.util.Map; -import javax.servlet.AsyncContext; -import javax.servlet.DispatcherType; -import javax.servlet.RequestDispatcher; -import javax.servlet.ServletContext; -import javax.servlet.ServletInputStream; -import javax.servlet.ServletRequest; -import javax.servlet.ServletResponse; -import javax.servlet.http.Cookie; -import javax.servlet.http.HttpServletRequest; -import javax.servlet.http.HttpServletResponse; -import javax.servlet.http.HttpSession; -import javax.servlet.http.HttpUpgradeHandler; -import javax.servlet.http.Part; - -/** Simple fake implementation of {@link HttpServletRequest}. */ -public class FakeHttpServletRequest implements HttpServletRequest { - public static final String SERVLET_PATH = "/b"; - public static final DateTimeFormatter rfcDateformatter = - DateTimeFormatter.ofPattern("EEE, dd MMM yyyy HH:mm:ss ZZZ"); - - private final Map attributes; - private final ListMultimap headers; - - private ListMultimap parameters; - private String queryString; - private String servletPath; - private String path; - private String method; - private HttpSession session; - - public FakeHttpServletRequest() { - this(SERVLET_PATH, null); - } - - public FakeHttpServletRequest(String servletPath, HttpSession existingSession) { - this.servletPath = requireNonNull(servletPath, "servletPath"); - attributes = Maps.newConcurrentMap(); - parameters = LinkedListMultimap.create(); - headers = LinkedListMultimap.create(); - session = existingSession; - } - - @Override - public Object getAttribute(String name) { - return attributes.get(name); - } - - @Override - public Enumeration getAttributeNames() { - return Collections.enumeration(attributes.keySet()); - } - - @Override - public String getCharacterEncoding() { - return UTF_8.name(); - } - - @Override - public int getContentLength() { - return 0; - } - - @Nullable - @Override - public String getContentType() { - return null; - } - - @Override - public ServletInputStream getInputStream() { - throw new UnsupportedOperationException(); - } - - @Override - public String getLocalAddr() { - return "1.2.3.4"; - } - - @Override - public String getLocalName() { - return "localhost"; - } - - @Override - public int getLocalPort() { - return 80; - } - - @Override - public Locale getLocale() { - return Locale.US; - } - - @Override - public Enumeration getLocales() { - return Collections.enumeration(Collections.singleton(Locale.US)); - } - - @Override - public String getParameter(String name) { - return Iterables.getFirst(parameters.get(name), null); - } - - @Override - public Map getParameterMap() { - return Collections.unmodifiableMap( - Maps.transformValues(parameters.asMap(), vs -> vs.toArray(new String[0]))); - } - - @Override - public Enumeration getParameterNames() { - return Collections.enumeration(parameters.keySet()); - } - - @Override - public String[] getParameterValues(String name) { - return parameters.get(name).toArray(new String[0]); - } - - public void setQueryString(String qs) { - this.queryString = qs; - ListMultimap params = LinkedListMultimap.create(); - for (String entry : Splitter.on('&').split(qs)) { - List kv = Splitter.on('=').limit(2).splitToList(entry); - try { - params.put( - URLDecoder.decode(kv.get(0), UTF_8.name()), - kv.size() == 2 ? URLDecoder.decode(kv.get(1), UTF_8.name()) : ""); - } catch (UnsupportedEncodingException e) { - throw new IllegalArgumentException(e); - } - } - parameters = params; - } - - @Override - public String getProtocol() { - return "HTTP/1.1"; - } - - @Override - public BufferedReader getReader() { - throw new UnsupportedOperationException(); - } - - @Override - @Deprecated - public String getRealPath(String path) { - throw new UnsupportedOperationException(); - } - - @Override - public String getRemoteAddr() { - return "5.6.7.8"; - } - - @Override - public String getRemoteHost() { - return "remotehost"; - } - - @Override - public int getRemotePort() { - return 1234; - } - - @Override - public RequestDispatcher getRequestDispatcher(String path) { - throw new UnsupportedOperationException(); - } - - @Override - public String getScheme() { - return "http"; - } - - @Override - public String getServerName() { - return "localhost"; - } - - @Override - public int getServerPort() { - return 80; - } - - @Override - public boolean isSecure() { - return false; - } - - @Override - public void removeAttribute(String name) { - attributes.remove(name); - } - - @Override - public void setAttribute(String name, Object value) { - attributes.put(name, value); - } - - @Override - public void setCharacterEncoding(String env) throws UnsupportedOperationException { - throw new UnsupportedOperationException(); - } - - @Override - public String getAuthType() { - return null; - } - - @Override - public String getContextPath() { - return ""; - } - - @Override - public Cookie[] getCookies() { - return Splitter.on(";").splitToList(nullToEmpty(getHeader("Cookie"))).stream() - .filter(s -> !s.isEmpty()) - .map( - (String cookieValue) -> { - List kv = Splitter.on("=").splitToList(cookieValue); - return new Cookie(kv.get(0), kv.get(1)); - }) - .collect(toList()) - .toArray(new Cookie[0]); - } - - @Override - public long getDateHeader(String name) { - return 0L; - } - - @Override - public String getHeader(String name) { - return Iterables.getFirst(headers.get(name), null); - } - - @Override - public Enumeration getHeaderNames() { - return Collections.enumeration(headers.keySet()); - } - - @Override - public Enumeration getHeaders(String name) { - return Collections.enumeration(headers.get(name)); - } - - @Override - public int getIntHeader(String name) { - return Integer.parseInt(getHeader(name)); - } - - @Override - public String getMethod() { - return method; - } - - public void setMethod(String method) { - this.method = method; - } - - @Override - public String getPathInfo() { - return path; - } - - public FakeHttpServletRequest setPathInfo(String path) { - this.path = path; - return this; - } - - @Override - public String getPathTranslated() { - return path; - } - - @Override - public String getQueryString() { - return queryString; - } - - @Override - public String getRemoteUser() { - return null; - } - - @Override - public String getRequestURI() { - return nullToEmpty(servletPath) + nullToEmpty(path); - } - - @Override - public StringBuffer getRequestURL() { - return new StringBuffer("http://localhost" + getRequestURI()); - } - - @Override - public String getRequestedSessionId() { - return null; - } - - @Override - public String getServletPath() { - return servletPath; - } - - @Override - public HttpSession getSession() { - return getSession(true); - } - - @Override - public HttpSession getSession(boolean create) { - if (session == null && create) { - session = new FakeHttpSession(); - } - return session; - } - - @Override - public Principal getUserPrincipal() { - throw new UnsupportedOperationException(); - } - - @Override - public boolean isRequestedSessionIdFromCookie() { - throw new UnsupportedOperationException(); - } - - @Override - public boolean isRequestedSessionIdFromURL() { - throw new UnsupportedOperationException(); - } - - @Override - @Deprecated - public boolean isRequestedSessionIdFromUrl() { - throw new UnsupportedOperationException(); - } - - @Override - public boolean isRequestedSessionIdValid() { - throw new UnsupportedOperationException(); - } - - @Override - public boolean isUserInRole(String role) { - throw new UnsupportedOperationException(); - } - - @Override - public AsyncContext getAsyncContext() { - throw new UnsupportedOperationException(); - } - - @Override - public DispatcherType getDispatcherType() { - throw new UnsupportedOperationException(); - } - - @Override - public ServletContext getServletContext() { - throw new UnsupportedOperationException(); - } - - @Override - public boolean isAsyncStarted() { - return false; - } - - @Override - public boolean isAsyncSupported() { - return false; - } - - @Override - public AsyncContext startAsync() { - throw new UnsupportedOperationException(); - } - - @Override - public AsyncContext startAsync(ServletRequest req, ServletResponse res) { - throw new UnsupportedOperationException(); - } - - @Override - public boolean authenticate(HttpServletResponse res) { - throw new UnsupportedOperationException(); - } - - @Override - public Part getPart(String name) { - throw new UnsupportedOperationException(); - } - - @Override - public Collection getParts() { - throw new UnsupportedOperationException(); - } - - @Override - public void login(String username, String password) { - throw new UnsupportedOperationException(); - } - - @Override - public void logout() { - throw new UnsupportedOperationException(); - } - - @Override - public long getContentLengthLong() { - return getContentLength(); - } - - @Override - public String changeSessionId() { - throw new UnsupportedOperationException(); - } - - @Override - public T upgrade(Class httpUpgradeHandlerClass) { - throw new UnsupportedOperationException(); - } -} diff --git a/github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/FakeHttpServletResponse.java b/github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/FakeHttpServletResponse.java deleted file mode 100644 index 6824a155..00000000 --- a/github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/FakeHttpServletResponse.java +++ /dev/null @@ -1,249 +0,0 @@ -// Copyright (C) 2015 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; - -import static com.google.common.base.Preconditions.checkArgument; -import static java.nio.charset.StandardCharsets.UTF_8; -import static java.util.Objects.requireNonNull; - -import com.google.common.collect.Iterables; -import com.google.common.collect.LinkedListMultimap; -import com.google.common.collect.ListMultimap; -import com.google.common.net.HttpHeaders; -import java.io.ByteArrayOutputStream; -import java.io.IOException; -import java.io.PrintWriter; -import java.nio.charset.Charset; -import java.util.Collection; -import java.util.Locale; -import javax.servlet.ServletOutputStream; -import javax.servlet.http.Cookie; -import javax.servlet.http.HttpServletResponse; - -/** Simple fake implementation of {@link HttpServletResponse}. */ -public class FakeHttpServletResponse implements HttpServletResponse { - private final ByteArrayOutputStream actualBody = new ByteArrayOutputStream(); - private final ListMultimap headers = LinkedListMultimap.create(); - - private int status = SC_OK; - private boolean committed; - private ServletOutputStream outputStream; - private PrintWriter writer; - - public FakeHttpServletResponse() {} - - @Override - public synchronized void flushBuffer() throws IOException { - if (outputStream != null) { - outputStream.flush(); - } - if (writer != null) { - writer.flush(); - } - } - - @Override - public int getBufferSize() { - throw new UnsupportedOperationException(); - } - - @Override - public String getCharacterEncoding() { - return UTF_8.name(); - } - - @Override - public String getContentType() { - return null; - } - - @Override - public Locale getLocale() { - return Locale.US; - } - - @Override - public synchronized ServletOutputStream getOutputStream() { - throw new UnsupportedOperationException(); - } - - @Override - public synchronized PrintWriter getWriter() { - throw new UnsupportedOperationException(); - } - - @Override - public synchronized boolean isCommitted() { - return committed; - } - - @Override - public void reset() { - throw new UnsupportedOperationException(); - } - - @Override - public void resetBuffer() { - throw new UnsupportedOperationException(); - } - - @Override - public void setBufferSize(int sz) { - throw new UnsupportedOperationException(); - } - - @Override - public void setCharacterEncoding(String name) { - checkArgument(UTF_8.equals(Charset.forName(name)), "unsupported charset: %s", name); - } - - @Override - public void setContentLength(int length) { - setContentLengthLong(length); - } - - @Override - public void setContentLengthLong(long length) { - headers.removeAll(HttpHeaders.CONTENT_LENGTH); - addHeader(HttpHeaders.CONTENT_LENGTH, Long.toString(length)); - } - - @Override - public void setContentType(String type) { - headers.removeAll(HttpHeaders.CONTENT_TYPE); - addHeader(HttpHeaders.CONTENT_TYPE, type); - } - - @Override - public void setLocale(Locale locale) { - throw new UnsupportedOperationException(); - } - - @Override - public void addCookie(Cookie cookie) { - addHeader("Set-Cookie", cookie.getName() + "=" + cookie.getValue()); - } - - @Override - public void addDateHeader(String name, long value) { - throw new UnsupportedOperationException(); - } - - @Override - public void addHeader(String name, String value) { - headers.put(name.toLowerCase(Locale.US), value); - } - - @Override - public void addIntHeader(String name, int value) { - addHeader(name, Integer.toString(value)); - } - - @Override - public boolean containsHeader(String name) { - return headers.containsKey(name.toLowerCase(Locale.US)); - } - - @Override - public String encodeRedirectURL(String url) { - throw new UnsupportedOperationException(); - } - - @Override - @Deprecated - public String encodeRedirectUrl(String url) { - throw new UnsupportedOperationException(); - } - - @Override - public String encodeURL(String url) { - throw new UnsupportedOperationException(); - } - - @Override - @Deprecated - public String encodeUrl(String url) { - throw new UnsupportedOperationException(); - } - - @Override - public synchronized void sendError(int sc) { - status = sc; - committed = true; - } - - @Override - public synchronized void sendError(int sc, String msg) { - status = sc; - committed = true; - } - - @Override - public synchronized void sendRedirect(String loc) { - status = SC_FOUND; - setHeader(HttpHeaders.LOCATION, loc); - committed = true; - } - - @Override - public void setDateHeader(String name, long value) { - setHeader(name, Long.toString(value)); - } - - @Override - public void setHeader(String name, String value) { - headers.removeAll(name.toLowerCase(Locale.US)); - addHeader(name, value); - } - - @Override - public void setIntHeader(String name, int value) { - headers.removeAll(name.toLowerCase(Locale.US)); - addIntHeader(name, value); - } - - @Override - public synchronized void setStatus(int sc) { - status = sc; - committed = true; - } - - @Override - @Deprecated - public synchronized void setStatus(int sc, String msg) { - status = sc; - committed = true; - } - - @Override - public synchronized int getStatus() { - return status; - } - - @Override - public String getHeader(String name) { - return Iterables.getFirst(headers.get(requireNonNull(name.toLowerCase(Locale.US))), null); - } - - @Override - public Collection getHeaderNames() { - return headers.keySet(); - } - - @Override - public Collection getHeaders(String name) { - return headers.get(requireNonNull(name.toLowerCase(Locale.US))); - } -} diff --git a/github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/GitHubGroupCacheRefreshFilterTest.java b/github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/GitHubGroupCacheRefreshFilterTest.java index 59a8e3ac..5c1707f9 100644 --- a/github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/GitHubGroupCacheRefreshFilterTest.java +++ b/github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/GitHubGroupCacheRefreshFilterTest.java @@ -19,6 +19,8 @@ import com.google.common.cache.CacheBuilder; import com.google.common.cache.CacheLoader; import com.google.common.cache.LoadingCache; +import com.google.gerrit.util.http.testutil.FakeHttpServletRequest; +import com.google.gerrit.util.http.testutil.FakeHttpServletResponse; import com.googlesource.gerrit.plugins.github.filters.GitHubGroupCacheRefreshFilter; import com.googlesource.gerrit.plugins.github.group.GitHubGroupsCache; import com.googlesource.gerrit.plugins.github.groups.OrganizationStructure; @@ -33,6 +35,8 @@ public class GitHubGroupCacheRefreshFilterTest { private static final FilterChain NOOP_FILTER_CHAIN_TEST = (req, res) -> {}; private static final String GITHUB_USERNAME_TEST = "somegithubuser"; private static final OrganizationStructure GITHUB_USER_ORGANIZATION = new OrganizationStructure(); + private static final String TEST_SERVER = "test-server"; + private static final int TEST_PORT = 80; private LoadingCache groupsByUsernameCache; private GitHubGroupCacheRefreshFilter filter; @@ -94,16 +98,14 @@ public void shouldNotReloadGroupsOnRegularRequests() throws Exception { FakeHttpServletRequest regularRequest = new FakeHttpServletRequest(); filter.doFilter(regularRequest, new FakeHttpServletResponse(), NOOP_FILTER_CHAIN_TEST); filter.doFilter( - newHomepageRequest(regularRequest.getSession()), - new FakeHttpServletResponse(), - NOOP_FILTER_CHAIN_TEST); + newHomepageRequest(null), new FakeHttpServletResponse(), NOOP_FILTER_CHAIN_TEST); assertThat(groupsByUsernameCache.get(GITHUB_USERNAME_TEST)).isEqualTo(GITHUB_USER_ORGANIZATION); assertThat(groupsCacheLoader.getLoadCount()).isEqualTo(initialLoadCount); } private ServletRequest newHomepageRequest(HttpSession session) { - return new FakeHttpServletRequest("/", session); + return new FakeHttpServletRequest(TEST_SERVER, TEST_PORT, "", "/", null, session); } private static HttpServletResponse newFinalLoginRedirectWithCookie() { @@ -113,8 +115,11 @@ private static HttpServletResponse newFinalLoginRedirectWithCookie() { } private static FakeHttpServletRequest newFinalLoginRequest() { - FakeHttpServletRequest req = new FakeHttpServletRequest("/login", null); + FakeHttpServletRequest req = + new FakeHttpServletRequest( + TEST_SERVER, TEST_PORT, "", "", () -> new FakeHttpSession(), null); req.setQueryString("final=true"); + req.setPathInfo("/login"); return req; } } diff --git a/pom.xml b/pom.xml index a0c4c213..e4e4b959 100644 --- a/pom.xml +++ b/pom.xml @@ -18,7 +18,7 @@ limitations under the License. 4.0.0 com.googlesource.gerrit.plugins.github github-parent - 3.9.0-rc2 + 3.9.0-rc4 Gerrit Code Review - GitHub integration http://www.gerritforge.com pom From 922696c233cca6f3f368975bf9394efe50896f34 Mon Sep 17 00:00:00 2001 From: Jacek Centkowski Date: Fri, 17 Nov 2023 09:05:38 +0100 Subject: [PATCH 15/44] Do not import changes refs from GitHub Importing changes refs may result in unexpected behaviour (especially when they come from different Gerrit server) therefore they should be excluded from the clone. Bug: Issue 40014491 Change-Id: Iaec0505b67b19a0c04fcbda7a1785af50fc7c5fb --- .../googlesource/gerrit/plugins/github/git/GitCloneStep.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/GitCloneStep.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/GitCloneStep.java index 25d953b3..69d1eac6 100644 --- a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/GitCloneStep.java +++ b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/GitCloneStep.java @@ -109,7 +109,8 @@ public void doImport(ProgressMonitor progress) throws GitException { createNewProject(); String sourceUri = getSourceUri(); try (Git git = Git.open(destinationDirectory)) { - FetchCommand fetch = git.fetch().setRefSpecs("refs/*:refs/*").setRemote(sourceUri); + FetchCommand fetch = + git.fetch().setRefSpecs("^refs/changes/*", "refs/*:refs/*").setRemote(sourceUri); fetch.setCredentialsProvider(getRepository().getCredentialsProvider()); if (progress != null) { fetch.setProgressMonitor(progress); From 97ec6ed8e1714fc6f8fee84ee9e8829e70b64e1b Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Fri, 17 Nov 2023 19:54:58 +0000 Subject: [PATCH 16/44] Bump plugin and Gerrit to v3.9.0-rc5 Change-Id: If5874006f2c17cedb1e7537daf36f4a3b0e3c721 --- github-oauth/pom.xml | 2 +- github-plugin/pom.xml | 2 +- pom.xml | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/github-oauth/pom.xml b/github-oauth/pom.xml index 454e0fe1..d027b61e 100644 --- a/github-oauth/pom.xml +++ b/github-oauth/pom.xml @@ -21,7 +21,7 @@ limitations under the License. com.googlesource.gerrit.plugins.github github-parent - 3.9.0-rc4 + 3.9.0-rc5 github-oauth Gerrit Code Review - GitHub OAuth login diff --git a/github-plugin/pom.xml b/github-plugin/pom.xml index 79e76a00..1eec4f3d 100644 --- a/github-plugin/pom.xml +++ b/github-plugin/pom.xml @@ -20,7 +20,7 @@ limitations under the License. github-parent com.googlesource.gerrit.plugins.github - 3.9.0-rc4 + 3.9.0-rc5 github-plugin diff --git a/pom.xml b/pom.xml index e4e4b959..d0f54d8b 100644 --- a/pom.xml +++ b/pom.xml @@ -18,7 +18,7 @@ limitations under the License. 4.0.0 com.googlesource.gerrit.plugins.github github-parent - 3.9.0-rc4 + 3.9.0-rc5 Gerrit Code Review - GitHub integration http://www.gerritforge.com pom From 488e479f9778af8666a5d0b57067ae0aa07256dc Mon Sep 17 00:00:00 2001 From: Jacek Centkowski Date: Sat, 18 Nov 2023 09:21:18 +0100 Subject: [PATCH 17/44] Detect the default branch during the repo import from GitHub When GitHub repo is being imported the default branch is detected and in case it is not set it falls back to `master`. Bug: Issue 40014935 Change-Id: If6c0cfbc6e0bbd5f26d97407305e03a24a190dd0 --- .../gerrit/plugins/github/git/GitCloneStep.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/GitCloneStep.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/GitCloneStep.java index 69d1eac6..503712ca 100644 --- a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/GitCloneStep.java +++ b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/GitCloneStep.java @@ -13,6 +13,8 @@ // limitations under the License. package com.googlesource.gerrit.plugins.github.git; +import static java.util.stream.Collectors.toList; + import com.google.gerrit.entities.Project; import com.google.gerrit.extensions.api.GerritApi; import com.google.gerrit.extensions.api.changes.NotifyHandling; @@ -30,6 +32,7 @@ import com.googlesource.gerrit.plugins.github.GitHubConfig; import java.io.File; import java.io.IOException; +import java.util.stream.Stream; import org.apache.commons.io.FileUtils; import org.eclipse.jgit.api.FetchCommand; import org.eclipse.jgit.api.Git; @@ -95,7 +98,9 @@ private void createNewProject() throws GitException { try (ManualRequestContext requestContext = context.openAs(config.importAccountId)) { ProjectInput pi = new ProjectInput(); pi.name = projectName; - pi.parent = config.getBaseProject(getRepository().isPrivate()); + GitHubRepository ghRepository = getRepository(); + pi.parent = config.getBaseProject(ghRepository.isPrivate()); + pi.branches = Stream.ofNullable(ghRepository.getDefaultBranch()).collect(toList()); gerritApi.projects().create(pi).get(); } catch (ResourceConflictException e) { throw new GitDestinationAlreadyExistsException(projectName); From 43bb3b702e5a85b1b2ba8b7ed3aa2ab0523a9c5e Mon Sep 17 00:00:00 2001 From: Dariusz Luksza Date: Tue, 21 Nov 2023 10:58:55 +0000 Subject: [PATCH 18/44] Redirect to login on anonymous access to profile pages When an unauthorized session tries to access any of the profile pages, server will return status code 500 and a generic "Server Error" page will be rendered. Instead, user should be redirected to the login page. To fix this we simply check if user is authorized in `VelocityViewServlet` and if not, redirect to the `auth.loginUrl`. Bug: Issue 311627062 Change-Id: I3264f50500733e277ef10ca1a2ff36287f89fc99 --- .../github/velocity/VelocityViewServlet.java | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) 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 b7dc08f1..5e4498f9 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 @@ -13,8 +13,11 @@ // limitations under the License. package com.googlesource.gerrit.plugins.github.velocity; +import static com.googlesource.gerrit.plugins.github.oauth.GitHubOAuthConfig.GITHUB_PLUGIN_OAUTH_SCOPE; + import com.google.common.base.MoreObjects; import com.google.gerrit.server.CurrentUser; +import com.google.gerrit.server.config.AuthConfig; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; @@ -53,6 +56,7 @@ public class VelocityViewServlet extends HttpServlet { private final GitHubConfig config; private final VirtualDomainConfig virtualDomainConfig; private final CanonicalWebUrls canonicalWebUrls; + private final AuthConfig authConfig; @Inject public VelocityViewServlet( @@ -62,7 +66,8 @@ public VelocityViewServlet( Provider userProvider, GitHubConfig config, VirtualDomainConfig virutalDomainConfig, - CanonicalWebUrls canonicalWebUrls) { + CanonicalWebUrls canonicalWebUrls, + AuthConfig authConfig) { this.velocityRuntime = velocityRuntime; this.modelProvider = modelProvider; @@ -71,6 +76,7 @@ public VelocityViewServlet( this.config = config; this.virtualDomainConfig = virutalDomainConfig; this.canonicalWebUrls = canonicalWebUrls; + this.authConfig = authConfig; } @Override @@ -79,6 +85,12 @@ public void service(ServletRequest request, ServletResponse response) HttpServletRequest req = (HttpServletRequest) request; HttpServletResponse resp = (HttpServletResponse) response; + if (!(req.getRequestURI().equals(GITHUB_PLUGIN_OAUTH_SCOPE) + || userProvider.get().isIdentifiedUser())) { + resp.sendRedirect(authConfig.getLoginUrl()); + return; + } + String pathInfo = STATIC_PREFIX + MoreObjects.firstNonNull((String) req.getAttribute("destUrl"), req.getPathInfo()); From 5ead64132961bf1a1650138397cfc0cc6f6259cd Mon Sep 17 00:00:00 2001 From: Dariusz Luksza Date: Wed, 13 Dec 2023 13:07:19 +0000 Subject: [PATCH 19/44] Fix velocity deprecated configuration keys All of the Velocity configuration keys used in `PluginVelocityRuntimeProvider` are now marked as deprecated and were producing warnings when plugin was starting. This is replacing the deprecated keys with their new versions. Change-Id: If62af6098a7db367e2ced41f92974f9a3d734050 --- .../velocity/PluginVelocityRuntimeProvider.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/velocity/PluginVelocityRuntimeProvider.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/velocity/PluginVelocityRuntimeProvider.java index e1b4d9bc..1d81f62e 100644 --- a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/velocity/PluginVelocityRuntimeProvider.java +++ b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/velocity/PluginVelocityRuntimeProvider.java @@ -29,12 +29,12 @@ @Singleton public class PluginVelocityRuntimeProvider implements Provider { - private static final String VELOCITY_FILE_RESOURCE_LOADER_PATH = "file.resource.loader.path"; - private static final String VELOCITY_FILE_RESOURCE_LOADER_CLASS = "file.resource.loader.class"; - private static final String VELOCITY_CLASS_RESOURCE_LOADER_CLASS = "class.resource.loader.class"; - private static final String VELOCITY_JAR_RESOURCE_LOADER_CLASS = "jar.resource.loader.class"; - private static final String VELOCITY_JAR_RESOURCE_LOADER_PATH = "jar.resource.loader.path"; - private static final String VELOCITY_RESOURCE_LOADER = "resource.loader"; + private static final String VELOCITY_FILE_RESOURCE_LOADER_PATH = "resource.loader.file.path"; + private static final String VELOCITY_FILE_RESOURCE_LOADER_CLASS = "resource.loader.jar.path"; + private static final String VELOCITY_CLASS_RESOURCE_LOADER_CLASS = "resource.loader.class.class"; + private static final String VELOCITY_JAR_RESOURCE_LOADER_CLASS = "resource.loader.jar.class"; + private static final String VELOCITY_JAR_RESOURCE_LOADER_PATH = "resource.loader.jar.path"; + private static final String VELOCITY_RESOURCE_LOADER = "resource.loaders"; private final SitePaths site; private String pluginName; From d77146c5e0a6a08baa631a92b283b0bb5905f4ea Mon Sep 17 00:00:00 2001 From: Dariusz Luksza Date: Wed, 13 Dec 2023 14:50:13 +0000 Subject: [PATCH 20/44] Fallback to public organization information GitHub uses various access scopes for its REST API and users can choose how much information they allow to access for the integrations. The narrowest scope is `USER_EMAILl`, which grants only access to the user's email address. It's important to note that the _Reviewer_ scope on eclipse.gerrithub.io and review.gerrithub.io is only asking for access to the email address. To give users proper permissions in Gerrit we load information about user teams and organizations. To read those GitHub requires one of two scopes `org:read` or `user`. None of those are included in the _Reviewer_ scope. Which led to an exception being thrown. As we access organizations and teams API's from a cache loader, getting an exception meant that the value for the given user was never stored. This then means that each call to user group membership would result in yet another attempt to load a cache entry and yet another exception. In the end group membership cache for a user with scope _Reviewer_ will never be computed. Which will result in poor page load time. We already have a mitigation mechanism in place, that would fall to "public organizations" only, when teams cannot be accessed. Unfortunately, the GitHub REST API endpoint for "public organizations", was still requiring "org:read" or "user" scopes. This means that for the _Reviewer_ scope this fallback never worked. This change adds another fallback, this time to the real "public organizations" endpoint that doesn't require additional scopes. It also ensures that we never use `getMyOrganizations()` call directly but always go through the fallback route. Bug: Issue 40014763 Change-Id: I9647b9a1b6e30547f7780c785de37aa6ceb8001d --- .../plugins/github/oauth/GitHubLogin.java | 17 +++++++++++++++-- .../plugins/github/oauth/OAuthWebFilter.java | 2 +- .../plugins/github/group/GitHubGroupsCache.java | 2 +- 3 files changed, 17 insertions(+), 4 deletions(-) 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 adfe5e3d..02889a1c 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 @@ -38,6 +38,7 @@ import org.kohsuke.github.GHMyself; import org.kohsuke.github.GitHub; import org.kohsuke.github.GitHubBuilder; +import org.kohsuke.github.HttpException; import org.kohsuke.github.connector.GitHubConnector; import org.kohsuke.github.internal.GitHubConnectorHttpConnectorAdapter; import org.slf4j.Logger; @@ -75,9 +76,21 @@ public GHMyself getMyself() throws IOException { return null; } - public Set getMyOrganisationsLogins() throws IOException { + public Set getMyOrganisationsLogins(String username) throws IOException { if (isLoggedIn()) { - return getHub().getMyOrganizations().keySet(); + try { + return getHub().getMyOrganizations().keySet(); + } catch (HttpException httpException) { + if (!httpException.getMessage().contains("You need at least")) { + throw httpException; + } + log.info( + "Cannot access organizations for user '{}': falling back to list of public" + + " organisations", + username); + + return getHub().getUserPublicOrganizations(username).keySet(); + } } return Collections.emptySet(); } 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 0bca5702..20b80bd3 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 @@ -131,7 +131,7 @@ private void login( String user = myself.getLogin(); updateSecureConfigWithRetry( - ghLogin.getHub().getMyOrganizations().keySet(), user, ghLogin.getToken().accessToken); + ghLogin.getMyOrganisationsLogins(user), user, ghLogin.getToken().accessToken); } } diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/group/GitHubGroupsCache.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/group/GitHubGroupsCache.java index a5301e9f..8f9c7760 100644 --- a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/group/GitHubGroupsCache.java +++ b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/group/GitHubGroupsCache.java @@ -97,7 +97,7 @@ private void loadOrganisationsAndTeams( private void loadOrganisations( String username, OrganizationStructure orgsTeams, GitHubLogin ghLogin) throws IOException { logger.debug("Getting list of public organisations for user '{}'", username); - Set organisations = ghLogin.getMyOrganisationsLogins(); + Set organisations = ghLogin.getMyOrganisationsLogins(username); for (String org : organisations) { orgsTeams.put(org, EVERYONE_TEAM_NAME); } From 22e3dcbf193b3cc3f72785749f3775b95fc24c40 Mon Sep 17 00:00:00 2001 From: Dariusz Luksza Date: Mon, 26 Feb 2024 10:44:00 +0000 Subject: [PATCH 21/44] Redirect user to previously visited page after login When a user logs in from any page, it expectes to be back on the same page after the successful login. Gerrit's `/login` endpoint already supports redirecting to the previously visited page, but that information was not forwarded from the frontend. To update the `href` in the login URL, we need to subscribe for `nav-report` events. But as the event is emitted, before `window.location` is updated, we also need to delay reading of the current browser URL, until all other events are processed. This way, by using `setTimeout`, we can reliably update the login link `href`. But: Issue 325838103 Change-Id: Icc6498b48fac011a2bc2e92700ff216d781eb993 --- .../src/main/ts/gr-github-oauth-progress.ts | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/github-plugin/src/main/ts/gr-github-oauth-progress.ts b/github-plugin/src/main/ts/gr-github-oauth-progress.ts index 3b53d47b..8ed9e079 100644 --- a/github-plugin/src/main/ts/gr-github-oauth-progress.ts +++ b/github-plugin/src/main/ts/gr-github-oauth-progress.ts @@ -14,6 +14,8 @@ export class GrGitHubOAuthProgress extends LitElement { @state() loggedIn?: boolean + @state() currentNavigationPath?: string; + override connectedCallback() { super.connectedCallback(); const restApi = this.plugin.restApi(); @@ -21,6 +23,13 @@ export class GrGitHubOAuthProgress extends LitElement { restApi.getConfig().then(config => this.authInfo = config?.auth); } restApi.getLoggedIn().then(loggedIn => this.loggedIn = loggedIn); + document.addEventListener( + 'nav-report', + // the `nav-report` event is emitted before `window.location` is updated, in order + // to get the current location, we need to delay `this.updateLocationPath` execution + // by putting it on the end of processing queue + () => setTimeout(() => this.updateLocationPath(), 0)); + this.updateLocationPath(); } static override get styles() { @@ -52,9 +61,13 @@ export class GrGitHubOAuthProgress extends LitElement { if (!this.authInfo || this.loggedIn !== false) { return } + const loginWithRedirect = new URL(this.authInfo.login_url ?? "/", window.location.origin); + if (this.currentNavigationPath) { + loginWithRedirect.pathname = loginWithRedirect.pathname + this.currentNavigationPath; + } return html` - + ${this.authInfo.login_text}
@@ -66,6 +79,10 @@ export class GrGitHubOAuthProgress extends LitElement { ` } + private updateLocationPath() { + this.currentNavigationPath = window.location.pathname; + } + private showModal() { setTimeout(() => this.gitHubOAuthProgress?.showModal(), 550); } From 46ac50766da17a9b94f7e9f3ed9fb2c4ef9f6cf4 Mon Sep 17 00:00:00 2001 From: Dariusz Luksza
Date: Mon, 26 Feb 2024 11:07:20 +0000 Subject: [PATCH 22/44] Add missing semicolons in TypeScript Semicolons in TypeScript are note required, but for consistency with the majority of lines in `gr-github-oauth-progress.ts` three semicolons were added. Change-Id: Ic33bba8456606bd150e1dbd649df8b7e158b36b3 --- github-plugin/src/main/ts/gr-github-oauth-progress.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/github-plugin/src/main/ts/gr-github-oauth-progress.ts b/github-plugin/src/main/ts/gr-github-oauth-progress.ts index 8ed9e079..292e294f 100644 --- a/github-plugin/src/main/ts/gr-github-oauth-progress.ts +++ b/github-plugin/src/main/ts/gr-github-oauth-progress.ts @@ -10,9 +10,9 @@ export class GrGitHubOAuthProgress extends LitElement { @property() plugin!: PluginApi; - @state() authInfo?: AuthInfo + @state() authInfo?: AuthInfo; - @state() loggedIn?: boolean + @state() loggedIn?: boolean; @state() currentNavigationPath?: string; @@ -59,7 +59,7 @@ export class GrGitHubOAuthProgress extends LitElement { override render() { if (!this.authInfo || this.loggedIn !== false) { - return + return; } const loginWithRedirect = new URL(this.authInfo.login_url ?? "/", window.location.origin); if (this.currentNavigationPath) { From 12c9e4e54233262a771a3b4dd44874c8235dbab9 Mon Sep 17 00:00:00 2001 From: David Ostrovsky
Date: Wed, 3 Apr 2024 06:36:14 +0200 Subject: [PATCH 23/44] ScopeKey: Replace lombok annotation with record type Record classes: [1] were introduced in Java 14 as a preview feature and were finalized in Java 17. Given that the compiler level was changed to Java 17 in the latest Gerrit releases per default use Java 17 language feature as a replacement for lombok annotation processor. Note to the tool support: This plugin and gerrit core itself is using google-java-format to automatically format Java code. Unfortunately the currently used gjf verion 1.7 is outdated and doesn't support record classes. Therefore, a new version should be used to re-format reocrd classes, e.g. v1.22.0: [2]. [1] https://docs.oracle.com/javase/specs/jls/se17/html/jls-8.html#jls-8.10 [2] https://github.com/google/google-java-format/releases/tag/v1.22.0 Change-Id: I979fbfe27ee68a1a7315103e90593c5278e4341d --- .../gerrit/plugins/github/oauth/GitHubLogin.java | 2 +- .../plugins/github/oauth/GitHubOAuthConfig.java | 2 +- .../gerrit/plugins/github/oauth/ScopeKey.java | 14 +------------- .../github/oauth/GitHubOAuthConfigTest.java | 12 ++++++------ 4 files changed, 9 insertions(+), 21 deletions(-) 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 02889a1c..b7744191 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 @@ -210,7 +210,7 @@ private SortedSet
getScopes(HttpServletRequest req, String baseScopeKey, private List scopesForKey(HttpServletRequest req, String baseScopeKey) { return virtualDomainConfig.getScopes(req).entrySet().stream() - .filter(entry -> entry.getKey().name.equals(baseScopeKey)) + .filter(entry -> entry.getKey().name().equals(baseScopeKey)) .map(entry -> entry.getValue()) .findFirst() .orElse(DEFAULT_SCOPES); 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 300a945f..7645af5f 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 @@ -165,7 +165,7 @@ private SortedMap> getScopesInSection(Config config, Strin .filter(k -> !k.endsWith("Sequence")) .collect( ImmutableSortedMap.toImmutableSortedMap( - Comparator.comparing(ScopeKey::getSequence), + Comparator.comparing(ScopeKey::sequence), k -> new ScopeKey( k, diff --git a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/ScopeKey.java b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/ScopeKey.java index a4751c7e..6b7ecfb7 100644 --- a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/ScopeKey.java +++ b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/ScopeKey.java @@ -14,16 +14,4 @@ package com.googlesource.gerrit.plugins.github.oauth; -import lombok.Getter; - -public class ScopeKey { - @Getter public final String name; - @Getter public final String description; - @Getter public final int sequence; - - public ScopeKey(String name, String description, int sequence) { - this.name = name; - this.description = description; - this.sequence = sequence; - } -} +public record ScopeKey(String name, String description, int sequence) {} diff --git a/github-oauth/src/test/java/com/googlesource/gerrit/plugins/github/oauth/GitHubOAuthConfigTest.java b/github-oauth/src/test/java/com/googlesource/gerrit/plugins/github/oauth/GitHubOAuthConfigTest.java index 7a11427a..602fb76e 100644 --- a/github-oauth/src/test/java/com/googlesource/gerrit/plugins/github/oauth/GitHubOAuthConfigTest.java +++ b/github-oauth/src/test/java/com/googlesource/gerrit/plugins/github/oauth/GitHubOAuthConfigTest.java @@ -196,13 +196,13 @@ public void shouldReturnOverridesForSpecificHostName() { Map.Entry> firstEntry = entries.get(0); Map.Entry> secondEntry = entries.get(1); - assertEquals(firstEntry.getKey().name, scope1Name); - assertEquals(firstEntry.getKey().description, scope1Description); - assertEquals(firstEntry.getKey().sequence, 0); + assertEquals(firstEntry.getKey().name(), scope1Name); + assertEquals(firstEntry.getKey().description(), scope1Description); + assertEquals(firstEntry.getKey().sequence(), 0); assertEquals(List.of(OAuthProtocol.Scope.REPO), firstEntry.getValue()); - assertEquals(secondEntry.getKey().name, scope2Name); - assertEquals(secondEntry.getKey().description, scope2Description); - assertEquals(secondEntry.getKey().sequence, 1); + assertEquals(secondEntry.getKey().name(), scope2Name); + assertEquals(secondEntry.getKey().description(), scope2Description); + assertEquals(secondEntry.getKey().sequence(), 1); assertEquals(List.of(OAuthProtocol.Scope.USER_EMAIL), secondEntry.getValue()); } From a5354e53b296d34260c881d95475822dc0a12ce3 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Thu, 13 Jun 2024 21:35:03 +0100 Subject: [PATCH 24/44] Simplify usage of AccessToken getter Remove the getter of the AccessToken object using Lombok and replace it with a simpler getter of the accessToken string value as it was effectively needed throughout the code. The OAuth cache keeps the AccessToken object for backward compatibility. Change-Id: I8190618d9f205a8945463dbdfc7e0d777d5ecdd9 --- .../plugins/github/oauth/GitHubLogin.java | 26 ++++++++++--------- .../IdentifiedUserGitHubLoginProvider.java | 2 +- .../plugins/github/oauth/OAuthCache.java | 2 +- .../plugins/github/oauth/OAuthWebFilter.java | 4 +-- .../github/filters/GitHubOAuthFilter.java | 3 +-- .../plugins/github/git/GitHubRepository.java | 2 +- .../github/git/ReplicateProjectStep.java | 2 +- .../github/notification/WebhookServlet.java | 2 +- 8 files changed, 22 insertions(+), 21 deletions(-) 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 b7744191..3aca14a9 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 @@ -20,7 +20,6 @@ import com.google.common.base.Strings; 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 java.io.IOException; import java.io.Serializable; @@ -34,7 +33,6 @@ import javax.servlet.http.Cookie; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; -import lombok.Getter; import org.kohsuke.github.GHMyself; import org.kohsuke.github.GitHub; import org.kohsuke.github.GitHubBuilder; @@ -59,7 +57,7 @@ public GitHubLogin get(HttpServletRequest request) { } } - @Getter private AccessToken token; + private String accessToken; private String state; @@ -69,6 +67,10 @@ public GitHubLogin get(HttpServletRequest request) { private final VirtualDomainConfig virtualDomainConfig; private final GitHubConnector gitHubConnector; + public String getAccessToken() { + return accessToken; + } + public GHMyself getMyself() throws IOException { if (isLoggedIn()) { return new GitHubMyselfWrapper(getHub().getMyself()); @@ -108,7 +110,7 @@ public GitHubLogin( } public boolean isLoggedIn() { - return token != null; + return accessToken != null; } public void login( @@ -121,7 +123,7 @@ public void login( log.debug("Login " + this); if (OAuthProtocol.isOAuthFinal(request)) { log.debug("Login-FINAL " + this); - login(oauth.loginPhase2(request, response, state)); + login(oauth.loginPhase2(request, response, state).accessToken); this.state = ""; // Make sure state is used only once if (isLoggedIn()) { @@ -143,27 +145,27 @@ public void login( } public void logout() { - token = null; + accessToken = null; } - public GitHub login(AccessToken authToken) throws IOException { - log.debug("Logging in using access token {}", authToken.accessToken); - this.token = authToken; + public GitHub login(String authAccessToken) throws IOException { + log.debug("Logging in using access token {}", authAccessToken); + this.accessToken = authAccessToken; return getHub(); } @Override public String toString() { - return "GitHubLogin [token=" + token + ", scopes=" + loginScopes + "]"; + return "GitHubLogin [token=" + accessToken + ", scopes=" + loginScopes + "]"; } public GitHub getHub() throws IOException { - if (token == null) { + if (accessToken == null) { return null; } return new GitHubBuilder() .withEndpoint(config.gitHubApiUrl) - .withOAuthToken(token.accessToken) + .withOAuthToken(accessToken) .withConnector(gitHubConnector) .build(); } 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 63414b20..91505dfe 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 @@ -65,7 +65,7 @@ public GitHubLogin get(String username) { AccessToken accessToken = newAccessTokenFromUser(username); if (accessToken != null) { GitHubLogin login = gitHubLoginProvider.get(); - login.login(accessToken); + login.login(accessToken.accessToken); return login; } return null; diff --git a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthCache.java b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthCache.java index 955c86db..ce1fd5c5 100644 --- a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthCache.java +++ b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthCache.java @@ -37,7 +37,7 @@ public Loader(GitHubLogin ghLogin) { @Override public String load(AccessToken accessToken) throws Exception { - ghLogin.login(accessToken); + ghLogin.login(accessToken.accessToken); return ghLogin.getMyself().getLogin(); } } 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 20b80bd3..60cef6a7 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 @@ -91,7 +91,7 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha } if (ghLogin != null && ghLogin.isLoggedIn()) { - String hashedToken = oAuthTokenCipher.encrypt(ghLogin.getToken().accessToken); + String hashedToken = oAuthTokenCipher.encrypt(ghLogin.getAccessToken()); httpRequest = new AuthenticatedHttpRequest( httpRequest, @@ -131,7 +131,7 @@ private void login( String user = myself.getLogin(); updateSecureConfigWithRetry( - ghLogin.getMyOrganisationsLogins(user), user, ghLogin.getToken().accessToken); + ghLogin.getMyOrganisationsLogins(user), user, ghLogin.getAccessToken()); } } 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 b025737e..17496fa7 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 @@ -25,7 +25,6 @@ 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.OAuthTokenCipher; import com.googlesource.gerrit.plugins.github.oauth.OAuthWebFilter; import com.googlesource.gerrit.plugins.github.oauth.ScopedProvider; @@ -83,7 +82,7 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha .substring( ExternalId.SCHEME_EXTERNAL.length() + OAuthWebFilter.GITHUB_EXT_ID.length() + 1); String decryptedToken = oAuthTokenCipher.decrypt(oauthToken); - hubLogin.login(new AccessToken(decryptedToken)); + hubLogin.login(decryptedToken); } 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 09e3fb73..f4321bd4 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 @@ -66,7 +66,7 @@ public GitHubRepository( GitHubLogin ghLogin = ghLoginProvider.get(); GitHub gh = ghLogin.getHub(); this.username = ghLogin.getMyself().getLogin(); - this.password = ghLogin.getToken().accessToken; + this.password = ghLogin.getAccessToken(); this.ghRepository = gh.getRepository(organisation + "/" + repository); } 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 b321127f..5109cd2b 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 @@ -49,7 +49,7 @@ public ReplicateProjectStep( this.replicationConfig = replicationConfig; GitHubLogin ghLogin = ghLoginProvider.get(); this.authUsername = ghLogin.getMyself().getLogin(); - this.authToken = ghLogin.getToken().accessToken; + this.authToken = ghLogin.getAccessToken(); this.gitHubUrl = gitHubUrl; } diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/notification/WebhookServlet.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/notification/WebhookServlet.java index 1bdce3c0..23bad3b2 100644 --- a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/notification/WebhookServlet.java +++ b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/notification/WebhookServlet.java @@ -167,7 +167,7 @@ protected void doPost(HttpServletRequest req, HttpServletResponse resp) resp.setStatus(SC_INTERNAL_SERVER_ERROR); return; } - requestScopedLoginProvider.get(req).login(login.getToken()); + requestScopedLoginProvider.get(req).login(login.getAccessToken()); if (callHander(handler, body)) { resp.setStatus(SC_NO_CONTENT); From 210329e9f2e4255ad9c649fc4f16e104c5312bd4 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Thu, 13 Jun 2024 21:41:18 +0100 Subject: [PATCH 25/44] Replace Getter with public final Scope fields The Scope enum can safely expose its details as public final fields without having to use Lombok to generate getters that are totally redundant in this Enum type. Change-Id: If3480330f67c879344aeae342065149da9199de6 --- .../gerrit/plugins/github/oauth/OAuthProtocol.java | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) 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 035f1cea..0ab686c7 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 @@ -26,7 +26,6 @@ import javax.servlet.ServletRequest; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; -import lombok.Getter; import org.apache.http.HttpResponse; import org.apache.http.NameValuePair; import org.apache.http.client.HttpClient; @@ -139,11 +138,11 @@ public static enum Scope { /** Grants the ability to add and update GitHub Actions workflow files. */ WORKFLOW("workflow", "Manage actions workflow files."); - @Getter private final String value; + public final String value; - @Getter private final String description; + public final String description; - private Scope(final String value, final String description) { + Scope(final String value, final String description) { this.value = value; this.description = description; } @@ -292,7 +291,7 @@ public String getScope(Set scopes) { if (out.length() > 0) { out.append(","); } - out.append(scope.getValue()); + out.append(scope.value); } return out.toString(); } From 440c5655801541f26a7b248090552aba1b53656c Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Thu, 13 Jun 2024 21:51:41 +0100 Subject: [PATCH 26/44] Expose GitHubUser fields as public final The GitHubUser fields are final and can be safely exposed as public final fields, without having to use Lombok Getter. Change-Id: Ief0a4151669c1f86025051141990eb05569a3c82 --- .../googlesource/gerrit/plugins/github/git/GitHubUser.java | 7 +++---- .../gerrit/plugins/github/git/PullRequestImportJob.java | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/GitHubUser.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/GitHubUser.java index 2109ea93..606cd96a 100644 --- a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/GitHubUser.java +++ b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/GitHubUser.java @@ -17,14 +17,13 @@ import com.google.common.base.Optional; import com.google.common.base.Strings; import java.io.IOException; -import lombok.Getter; import org.kohsuke.github.GHUser; import org.kohsuke.github.GitUser; public class GitHubUser { - @Getter private final String login; - @Getter private final String name; - @Getter private final String email; + public final String login; + public final String name; + public final String email; private GitHubUser(GHUser gitHubUser, GitUser author) throws IOException { this.login = initLogin(gitHubUser).or(generateLogin(author.getName())); diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/PullRequestImportJob.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/PullRequestImportJob.java index da16dc2a..6734e985 100644 --- a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/PullRequestImportJob.java +++ b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/PullRequestImportJob.java @@ -205,7 +205,7 @@ private List addPullRequestToChange(GHPullRequest pr, Repository gitRepo) th private com.google.gerrit.entities.Account.Id getOrRegisterAccount(GitHubUser author) throws BadRequestException, ResourceConflictException, UnprocessableEntityException, IOException, ConfigInvalidException { - return getOrRegisterAccount(author.getLogin(), author.getName(), author.getEmail()); + return getOrRegisterAccount(author.login, author.name, author.email); } private com.google.gerrit.entities.Account.Id getOrRegisterAccount( From d26c807afe65247e861958b1c8b0cc9818d6535a Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Thu, 13 Jun 2024 22:02:04 +0100 Subject: [PATCH 27/44] Expose GitHubOAuthConfig.getVirtualScopes as static method Avoid the use of Lombok's getter for calculating the virtual scopes but expose the method for extracting them from config as a package-protected static method. Change-Id: I68b53e000009564785c87027c634215d4c264d0c --- .../plugins/github/oauth/GitHubOAuthConfig.java | 12 ++++++------ .../plugins/github/oauth/GitHubOAuthConfigTest.java | 7 +++++-- 2 files changed, 11 insertions(+), 8 deletions(-) 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 7645af5f..8993d3fd 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 @@ -39,7 +39,6 @@ import java.util.concurrent.TimeUnit; import java.util.function.Function; import java.util.stream.Collectors; -import lombok.Getter; import org.eclipse.jgit.lib.Config; @Singleton @@ -69,8 +68,8 @@ public class GitHubOAuthConfig { public final String scopeSelectionUrl; public final boolean enabled; - @Getter public final SortedMap> scopes; - @Getter public final Map>> virtualScopes; + public final SortedMap> scopes; + public final Map>> virtualScopes; public final int fileUpdateMaxRetryCount; public final int fileUpdateMaxRetryIntervalMsec; @@ -153,12 +152,13 @@ private SortedMap> getScopes(Config config) { return getScopesInSection(config, null); } - private Map>> getVirtualScopes(Config config) { + static Map>> getVirtualScopes(Config config) { return config.getSubsections(CONF_SECTION).stream() .collect(Collectors.toMap(k -> k, v -> getScopesInSection(config, v))); } - private SortedMap> getScopesInSection(Config config, String subsection) { + private static SortedMap> getScopesInSection( + Config config, String subsection) { return config.getNames(CONF_SECTION, subsection, true).stream() .filter(k -> k.startsWith("scopes")) .filter(k -> !k.endsWith("Description")) @@ -174,7 +174,7 @@ private SortedMap> getScopesInSection(Config config, Strin v -> parseScopesString(config.getString(CONF_SECTION, subsection, v)))); } - private List parseScopesString(String scopesString) { + private static List parseScopesString(String scopesString) { ArrayList result = new ArrayList<>(); if (Strings.emptyToNull(scopesString) != null) { String[] scopesStrings = scopesString.split(","); diff --git a/github-oauth/src/test/java/com/googlesource/gerrit/plugins/github/oauth/GitHubOAuthConfigTest.java b/github-oauth/src/test/java/com/googlesource/gerrit/plugins/github/oauth/GitHubOAuthConfigTest.java index 602fb76e..d79b3b7a 100644 --- a/github-oauth/src/test/java/com/googlesource/gerrit/plugins/github/oauth/GitHubOAuthConfigTest.java +++ b/github-oauth/src/test/java/com/googlesource/gerrit/plugins/github/oauth/GitHubOAuthConfigTest.java @@ -185,8 +185,7 @@ public void shouldReturnOverridesForSpecificHostName() { config.setInt(CONF_SECTION, vhost, scope1Name + "Sequence", 0); config.setString(CONF_SECTION, vhost, scope1Name + "Description", scope1Description); - Map>> virtualScopes = - githubOAuthConfig().getVirtualScopes(); + Map>> virtualScopes = getVirtualScopes(); assertTrue(virtualScopes.containsKey(vhost)); @@ -206,6 +205,10 @@ public void shouldReturnOverridesForSpecificHostName() { assertEquals(List.of(OAuthProtocol.Scope.USER_EMAIL), secondEntry.getValue()); } + private Map>> getVirtualScopes() { + return GitHubOAuthConfig.getVirtualScopes(config); + } + private GitHubOAuthConfig githubOAuthConfig() { return new GitHubOAuthConfig(config); } From bf3124976fbf3d72e662f1e19bc00c37db65e45c Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Thu, 13 Jun 2024 22:13:03 +0100 Subject: [PATCH 28/44] Replace Lombok Getter with getX() methods on GitHubGroup The GitHubGroup is a base class of all groups implemented in the GitHub plugin and the extra boilerplate generated by removing Lombok usage is limited and justified. Change-Id: I9c6f5f42df88741d23f7c8a551952ebb343cf055 --- .../gerrit/plugins/github/group/GitHubGroup.java | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/group/GitHubGroup.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/group/GitHubGroup.java index ba9581c2..68b1a7a6 100644 --- a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/group/GitHubGroup.java +++ b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/group/GitHubGroup.java @@ -14,17 +14,17 @@ package com.googlesource.gerrit.plugins.github.group; +import com.google.gerrit.entities.AccountGroup; import com.google.gerrit.entities.AccountGroup.UUID; import com.google.gerrit.entities.GroupDescription.Basic; -import lombok.Getter; public abstract class GitHubGroup implements Basic { public static final String UUID_PREFIX = "github:"; public static final String NAME_PREFIX = "github/"; - @Getter protected final UUID groupUUID; + protected final UUID groupUUID; - @Getter protected final String url; + protected final String url; GitHubGroup(UUID groupUUID, String url) { this.groupUUID = groupUUID; @@ -35,4 +35,14 @@ public abstract class GitHubGroup implements Basic { public String getEmailAddress() { return ""; } + + @Override + public AccountGroup.UUID getGroupUUID() { + return groupUUID; + } + + @Override + public String getUrl() { + return url; + } } From b9c96df64154840fe72910cf213f2a18e7420b91 Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Mon, 1 Apr 2024 15:08:30 +0200 Subject: [PATCH 29/44] Add Bazel build This change adds Bazel build. Frontend build is not addressed in this change. One complication is the usage of lombok library, that has a known issue with bazel Turbine processor. Add a workaround to disable running Turbine processor for related dependencies. The plugin is split in two different artifacts: github-oauth library and github-plugin. TEST PLAN: Clone the plugin into gerrit's plugins directory. Copy plugin's own external_plugin_deps.bzl into gerrit's plugins directory. To build the github-oauth library run: $> bazel build \ plugins/github/github-oauth:github-oauth_deploy.jar To build the github-plugin run: $> bazel build plugins/github/github-plugin To run the tests: $> bazel test plugins/github/... Feature: Issue 10271 Change-Id: I21e90d50f0a4db36a74a0c34123b3da6c0ec28da --- BUILD | 0 external_plugin_deps.bzl | 115 ++++++++++++++++++ github-oauth/BUILD | 36 ++++++ github-plugin/BUILD | 95 +++++++++++++++ .../plugins/github/FakeHttpSession.java | 2 + github-plugin/web/.eslintrc.js | 9 ++ github-plugin/web/BUILD | 42 +++++++ .../ts => web}/gr-github-oauth-progress.ts | 0 github-plugin/{src/main/ts => web}/main.ts | 0 github-plugin/web/tsconfig.json | 11 ++ java_library_without_header_compilation.bzl | 30 +++++ 11 files changed, 340 insertions(+) create mode 100644 BUILD create mode 100644 external_plugin_deps.bzl create mode 100644 github-oauth/BUILD create mode 100644 github-plugin/BUILD create mode 100644 github-plugin/web/.eslintrc.js create mode 100644 github-plugin/web/BUILD rename github-plugin/{src/main/ts => web}/gr-github-oauth-progress.ts (100%) rename github-plugin/{src/main/ts => web}/main.ts (100%) create mode 100644 github-plugin/web/tsconfig.json create mode 100644 java_library_without_header_compilation.bzl diff --git a/BUILD b/BUILD new file mode 100644 index 00000000..e69de29b diff --git a/external_plugin_deps.bzl b/external_plugin_deps.bzl new file mode 100644 index 00000000..5282ce4c --- /dev/null +++ b/external_plugin_deps.bzl @@ -0,0 +1,115 @@ +load("@bazel_tools//tools/build_defs/repo:java.bzl", "java_import_external") +load("//tools/bzl:maven_jar.bzl", "maven_jar") + +JENKINS = "JENKINS:" +ECLIPSE_EGIT = "ECLIPSE_EGIT:" + +def external_plugin_deps(): + maven_jar( + name = "github-api", + artifact = "org.kohsuke:github-api:1.316", + sha1 = "90ea530f3aeceb46be27b924ae25b4b7b2388c9d", + ) + + maven_jar( + name = "axis", + artifact = "org.apache.axis:axis:1.4", + sha1 = "94a9ce681a42d0352b3ad22659f67835e560d107", + attach_source = False, + ) + + maven_jar( + name = "axis-jaxrpc", + artifact = "org.apache.axis:axis-jaxrpc:1.4", + sha1 = "b393f1f0c0d95b68c86d0b1ab2e687bb71f3c075", + attach_source = False, + ) + + maven_jar( + name = "eclipse-mylyn-github", + artifact = "org.eclipse.mylyn.github:org.eclipse.egit.github.core:6.1.0.202203080745-r", + repository = ECLIPSE_EGIT, + sha1 = "a0bc7ce9f17e2d41bbfbf08e4bc63c3ae0ec15b7", + attach_source = False, + ) + + maven_jar( + name = "commons-discovery", + artifact = "commons-discovery:commons-discovery:0.5", + sha1 = "3a8ac816bbe02d2f88523ef22cbf2c4abd71d6a8", + attach_source = False, + ) + + maven_jar( + name = "velocity", + artifact = "org.apache.velocity:velocity-engine-core:2.3", + sha1 = "e2133b723d0e42be74880d34de6bf6538ea7f915", + ) + + maven_jar( + name = "lombok", + artifact = "org.projectlombok:lombok:1.18.32", + sha1 = "17d46b3e205515e1e8efd3ee4d57ce8018914163", + ) + + maven_jar( + name = "com-sun-mail", + artifact = "com.sun.mail:javax.mail:1.6.2", + sha1 = "935151eb71beff17a2ffac15dd80184a99a0514f", + ) + + maven_jar( + name = "javax-activation", + artifact = "javax.activation:activation:1.1", + sha1 = "e6cb541461c2834bdea3eb920f1884d1eb508b50", + ) + + maven_jar( + name = "jackson-core", + artifact = "com.fasterxml.jackson.core:jackson-core:2.15.2", + sha1 = "a6fe1836469a69b3ff66037c324d75fc66ef137c", + ) + + maven_jar( + name = "jackson-databind", + artifact = "com.fasterxml.jackson.core:jackson-databind:2.15.2", + sha1 = "9353b021f10c307c00328f52090de2bdb4b6ff9c", + ) + + maven_jar( + name = "jackson-annotations", + artifact = "com.fasterxml.jackson.core:jackson-annotations:2.15.2", + sha1 = "4724a65ac8e8d156a24898d50fd5dbd3642870b8", + ) + + maven_jar( + name = "org-ow2-asm", + artifact = "org.ow2.asm:asm:9.6", + sha1 = "aa205cf0a06dbd8e04ece91c0b37c3f5d567546a", + ) + + maven_jar( + name = "org-ow2-asm-tree", + artifact = "org.ow2.asm:asm-tree:9.6", + sha1 = "c0cdda9d211e965d2a4448aa3fd86110f2f8c2de", + ) + + maven_jar( + name = "org-ow2-asm-commons", + artifact = "org.ow2.asm:asm-commons:9.6", + sha1 = "f1a9e5508eff490744144565c47326c8648be309", + ) + + maven_jar( + name = "bridge-method-injector", + artifact = "com.infradna.tool:bridge-method-injector:1.29", + repository = JENKINS, + sha1 = "5b6c616c7a6e04beb4178327d616af4f1bbe88da", + ) + + maven_jar( + name = "bridge-method-annotation", + artifact = "com.infradna.tool:bridge-method-annotation:1.29", + repository = JENKINS, + sha1 = "55dd67d0578d107697803a95cb9c235a9bd83ec1", + ) diff --git a/github-oauth/BUILD b/github-oauth/BUILD new file mode 100644 index 00000000..05e694b2 --- /dev/null +++ b/github-oauth/BUILD @@ -0,0 +1,36 @@ +load("//tools/bzl:junit.bzl", "junit_tests") +load("//tools/bzl:plugin.bzl", "PLUGIN_DEPS", "PLUGIN_DEPS_NEVERLINK") + +java_binary( + name = "github-oauth", + main_class = "Dummy", + runtime_deps = [":github-oauth-lib"], +) + +java_library( + name = "github-oauth-lib", + srcs = glob(["src/main/java/**/*.java"]), + visibility = ["//visibility:public"], + deps = PLUGIN_DEPS_NEVERLINK + [ + "//lib:servlet-api", + "@bridge-method-annotation//jar", + "@bridge-method-injector//jar", + "@commons-io//jar", + "@github-api//jar", + "@jackson-annotations//jar", + "@jackson-core//jar", + "@jackson-databind//jar", + "@org-ow2-asm-commons//jar", + "@org-ow2-asm-tree//jar", + "@org-ow2-asm//jar", + ], +) + +junit_tests( + name = "github-oauth_tests", + srcs = glob(["src/test/java/**/*.java"]), + tags = ["github"], + deps = PLUGIN_DEPS + [ + ":github-oauth-lib", + ], +) diff --git a/github-plugin/BUILD b/github-plugin/BUILD new file mode 100644 index 00000000..589f7c62 --- /dev/null +++ b/github-plugin/BUILD @@ -0,0 +1,95 @@ +load( + "//plugins/github:java_library_without_header_compilation.bzl", + "java_library_without_header_compilation", +) +load("//tools/bzl:junit.bzl", "junit_tests") +load("//tools/bzl:plugin.bzl", "PLUGIN_DEPS", "PLUGIN_DEPS_NEVERLINK", "PLUGIN_TEST_DEPS", "gerrit_plugin") + +SOURCES_WITH_LOMBOK_USAGE = [ + "src/main/java/com/googlesource/gerrit/plugins/github/git/GitHubRepository.java", + "src/main/java/com/googlesource/gerrit/plugins/github/GitHubURL.java", +] + +gerrit_plugin( + name = "github-plugin", + srcs = glob( + ["src/main/java/**/*.java"], + exclude = SOURCES_WITH_LOMBOK_USAGE, + ), + dir_name = "github", + manifest_entries = [ + "Gerrit-PluginName: github-plugin", + "Gerrit-Module: com.googlesource.gerrit.plugins.github.GuiceModule", + "Gerrit-HttpModule: com.googlesource.gerrit.plugins.github.GuiceHttpModule", + "Gerrit-InitStep: com.googlesource.gerrit.plugins.github.InitGitHub", + "Implementation-Title: GitHub plugin", + "Implementation-Vendor: GerritForge", + "Implementation-URL: http://www.gerritforge.com", + ], + resource_jars = ["//plugins/github/github-plugin/web:github-plugin"], + resources = glob(["src/main/resources/**/*"]), + deps = [ + ":github-plugin-lib", + "//plugins/github/github-oauth:github-oauth-lib", + "@axis-jaxrpc//jar", + "@axis//jar", + "@com-sun-mail//jar", + "@commons-codec//jar", + "@commons-discovery//jar", + "@commons-io//jar", + "@eclipse-mylyn-github//jar", + "@github-api//jar", + "@javax-activation//jar", + "@velocity//jar", + ], +) + +java_library_without_header_compilation( + name = "github-plugin-lib", + dep = ":github-plugin-lib_impl", + visibility = ["//visibility:public"], +) + +java_library( + name = "github-plugin-lib_impl", + srcs = SOURCES_WITH_LOMBOK_USAGE, + deps = PLUGIN_DEPS_NEVERLINK + [ + ":lombok", + "//plugins/github/github-oauth:github-oauth-lib", + "@axis-jaxrpc//jar", + "@axis//jar", + "@commons-codec//jar", + "@commons-discovery//jar", + "@commons-io//jar", + "@eclipse-mylyn-github//jar", + "@github-api//jar", + "@velocity//jar", + ], +) + +junit_tests( + name = "github-plugin_tests", + srcs = glob(["src/test/java/**/*.java"]), + tags = ["github"], + deps = PLUGIN_DEPS + PLUGIN_TEST_DEPS + [ + ":github-plugin-lib", + ":github-plugin__plugin", + "//javatests/com/google/gerrit/util/http/testutil", + "//plugins/github/github-oauth:github-oauth-lib", + "@commons-io//jar", + ], +) + +java_plugin( + name = "lombok_plugin", + generates_api = True, + processor_class = "lombok.launch.AnnotationProcessorHider$AnnotationProcessor", + deps = ["@lombok//jar"], +) + +java_library( + name = "lombok", + exported_plugins = [":lombok_plugin"], + neverlink = True, + exports = ["@lombok//jar"], +) diff --git a/github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/FakeHttpSession.java b/github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/FakeHttpSession.java index 6a820e76..b24efba9 100644 --- a/github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/FakeHttpSession.java +++ b/github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/FakeHttpSession.java @@ -19,7 +19,9 @@ import javax.servlet.ServletContext; import javax.servlet.http.HttpSession; import javax.servlet.http.HttpSessionContext; +import org.junit.Ignore; +@Ignore public class FakeHttpSession implements HttpSession { private final HashMap attributes; diff --git a/github-plugin/web/.eslintrc.js b/github-plugin/web/.eslintrc.js new file mode 100644 index 00000000..db4adcd5 --- /dev/null +++ b/github-plugin/web/.eslintrc.js @@ -0,0 +1,9 @@ +/** + * @license + * Copyright 2022 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ +__plugindir = "github/github-plugin/web"; +module.exports = { + extends: "../../../.eslintrc.js", +}; \ No newline at end of file diff --git a/github-plugin/web/BUILD b/github-plugin/web/BUILD new file mode 100644 index 00000000..035d9999 --- /dev/null +++ b/github-plugin/web/BUILD @@ -0,0 +1,42 @@ +load("//tools/js:eslint.bzl", "plugin_eslint") +load("//tools/bzl:js.bzl", "gerrit_js_bundle") +load("@npm//@bazel/typescript:index.bzl", "ts_config", "ts_project") + +package_group( + name = "visibility", + packages = ["//plugins/github/github-plugin/..."], +) + +package(default_visibility = [":visibility"]) + +ts_config( + name = "tsconfig", + src = "tsconfig.json", + deps = [ + "//plugins:tsconfig-plugins-base.json", + ], +) + +ts_project( + name = "github-plugin-ts", + srcs = glob( + ["**/*.ts"], + ), + incremental = True, + out_dir = "_bazel_ts_out", + tsc = "//tools/node_tools:tsc-bin", + tsconfig = ":tsconfig", + deps = [ + "@plugins_npm//@gerritcodereview/typescript-api", + "@plugins_npm//lit", + "@plugins_npm//rxjs", + ], +) + +gerrit_js_bundle( + name = "github-plugin", + srcs = [":github-plugin-ts"], + entry_point = "_bazel_ts_out/main.js", +) + +plugin_eslint() diff --git a/github-plugin/src/main/ts/gr-github-oauth-progress.ts b/github-plugin/web/gr-github-oauth-progress.ts similarity index 100% rename from github-plugin/src/main/ts/gr-github-oauth-progress.ts rename to github-plugin/web/gr-github-oauth-progress.ts diff --git a/github-plugin/src/main/ts/main.ts b/github-plugin/web/main.ts similarity index 100% rename from github-plugin/src/main/ts/main.ts rename to github-plugin/web/main.ts diff --git a/github-plugin/web/tsconfig.json b/github-plugin/web/tsconfig.json new file mode 100644 index 00000000..c2dd829f --- /dev/null +++ b/github-plugin/web/tsconfig.json @@ -0,0 +1,11 @@ +{ + "extends": "../../../tsconfig-plugins-base.json", + "compilerOptions": { + "experimentalDecorators": true, + /* outDir for IDE (overridden by Bazel rule arg) */ + "outDir": "../../../../.ts-out/plugins/github-plugin/ui", + }, + "include": [ + "**/*", + ], +} \ No newline at end of file diff --git a/java_library_without_header_compilation.bzl b/java_library_without_header_compilation.bzl new file mode 100644 index 00000000..a1250d08 --- /dev/null +++ b/java_library_without_header_compilation.bzl @@ -0,0 +1,30 @@ +# See https://github.com/bazelbuild/bazel/issues/12837 +# for workaround suggestion for incompatibility between +# Bazel Turbine processor and Lombok library. + +def _java_header_compilation_transition(settings, attr): + _ignore = (settings, attr) + return {"//command_line_option:java_header_compilation": "False"} + +java_header_compilation_transition = transition( + implementation = _java_header_compilation_transition, + inputs = [], + outputs = ["//command_line_option:java_header_compilation"], +) + +def _java_library_without_header_compilation(ctx): + return [java_common.merge([d[JavaInfo] for d in ctx.attr.dep])] + +java_library_without_header_compilation = rule( + implementation = _java_library_without_header_compilation, + attrs = { + "dep": attr.label( + providers = [JavaInfo], + mandatory = True, + cfg = java_header_compilation_transition, + ), + "_allowlist_function_transition": attr.label( + default = "@bazel_tools//tools/allowlists/function_transition_allowlist", + ), + }, +) From 6c2adb0e71c8abbac4b3b33835e995f9dd345b9e Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Fri, 14 Jun 2024 15:24:34 +0100 Subject: [PATCH 30/44] Remove Maven build The github plugin is now fully converted to Bazel and the Maven build isn't required anymore. Change-Id: I876278c6f45fe11353e2cd21b9b9440b7baa06cb --- github-oauth/pom.xml | 111 --------------- github-plugin/pom.xml | 226 ----------------------------- pom.xml | 321 ------------------------------------------ 3 files changed, 658 deletions(-) delete mode 100644 github-oauth/pom.xml delete mode 100644 github-plugin/pom.xml delete mode 100644 pom.xml diff --git a/github-oauth/pom.xml b/github-oauth/pom.xml deleted file mode 100644 index d027b61e..00000000 --- a/github-oauth/pom.xml +++ /dev/null @@ -1,111 +0,0 @@ - - - - 4.0.0 - - com.googlesource.gerrit.plugins.github - github-parent - 3.9.0-rc5 - - github-oauth - Gerrit Code Review - GitHub OAuth login - http://maven.apache.org - - UTF-8 - - - - - org.apache.maven.plugins - maven-shade-plugin - 1.6 - - true - - - com.google.*:* - javax.inject:*:* - aopalliance:aopalliance:* - org.slf4j:* - log4j:log4j:* - commons-lang:*:* - commons-codec:*:* - com.google.guava:* - - - - - - package - - shade - - - - - - - - - com.google.gerrit - gerrit-plugin-api - ${project.version} - provided - - - javax.servlet - javax.servlet-api - 3.1.0 - provided - - - com.google.inject - guice - 6.0.0 - provided - - - com.google.guava - guava - 32.1.2-jre - provided - - - org.kohsuke - github-api - 1.316 - - - com.infradna.tool - bridge-method-injector - 1.29 - - - org.apache.httpcomponents - httpclient - 4.5.2 - provided - - - junit - junit - test - - - diff --git a/github-plugin/pom.xml b/github-plugin/pom.xml deleted file mode 100644 index 1eec4f3d..00000000 --- a/github-plugin/pom.xml +++ /dev/null @@ -1,226 +0,0 @@ - - - - 4.0.0 - - github-parent - com.googlesource.gerrit.plugins.github - 3.9.0-rc5 - - - github-plugin - jar - Gerrit Code Review - GitHub plugin - - - plugin - ${project.version} - /Users/lucamilanesio/Documents/workspace-gerrit/gerrithub/gerrit - - - - - - org.apache.maven.plugins - maven-shade-plugin - 1.6 - - true - - - com.google.*:* - javax.inject:*:* - aopalliance:aopalliance:* - org.slf4j:* - log4j:log4j:* - commons-lang:*:* - commons-codec:*:* - commons-io:*:* - com.google.guava:* - com.fasterxml.jackson.core:* - org.kohsuke:github-api:* - org.jenkins-ci:annotation-indexer:* - com.infradna.tool:* - - - - - - com.googlesource.gerrit.plugins.github.GuiceModule - com.googlesource.gerrit.plugins.github.GuiceHttpModule - com.googlesource.gerrit.plugins.github.InitGitHub - - GerritForge - http://www.gerritforge.com - - ${Gerrit-ApiType} - ${project.artifactId} - ${project.version} - - ${Gerrit-ApiType} - ${Gerrit-ApiVersion} - github-plugin - - - - - - - - package - - shade - - - - - - org.eclipse.mylyn.github:org.eclipse.egit.github.core - - META-INF/*.SF - META-INF/*.DSA - META-INF/*.RSA - - - - - - - - - com.github.eirslett - frontend-maven-plugin - 1.14.0 - - target - - - - install node and yarn - - install-node-and-yarn - - - v17.9.1 - v1.22.19 - - - - yarn install - - yarn - - - install - - - - yarn build - - yarn - - - build - - - - - - - - - - com.google.gerrit - gerrit-plugin-api - ${Gerrit-ApiVersion} - provided - - - com.google.gerrit - gerrit-acceptance-framework - ${Gerrit-ApiVersion} - test - - - ${project.groupId} - github-oauth - ${project.version} - - - javax.servlet - javax.servlet-api - 3.1.0 - provided - - - org.apache.axis - axis - 1.4 - - - org.apache.axis - axis-jaxrpc - 1.4 - - - org.eclipse.mylyn.github - org.eclipse.egit.github.core - 6.1.0.202203080745-r - - - com.google.code.gson - gson - 2.10.1 - - - org.apache.httpcomponents - httpclient - 4.5.2 - provided - - - com.sun.mail - javax.mail - 1.6.2 - - - commons-io - commons-io - 2.14.0 - - - commons-discovery - commons-discovery - 0.5 - - - org.apache.velocity - velocity-engine-core - 2.3 - - - junit - junit - - - com.google.truth - truth - - - diff --git a/pom.xml b/pom.xml deleted file mode 100644 index d0f54d8b..00000000 --- a/pom.xml +++ /dev/null @@ -1,321 +0,0 @@ - - - - 4.0.0 - com.googlesource.gerrit.plugins.github - github-parent - 3.9.0-rc5 - Gerrit Code Review - GitHub integration - http://www.gerritforge.com - pom - - 17 - - - - Apache License, 2.0 - - Apache License - Version 2.0, January 2004 - http://www.apache.org/licenses/ - - TERMS AND CONDITIONS FOR USE, REPRODUCTION, AND DISTRIBUTION - - 1. Definitions. - - "License" shall mean the terms and conditions for use, reproduction, - and distribution as defined by Sections 1 through 9 of this document. - - "Licensor" shall mean the copyright owner or entity authorized by - the copyright owner that is granting the License. - - "Legal Entity" shall mean the union of the acting entity and all - other entities that control, are controlled by, or are under common - control with that entity. For the purposes of this definition, - "control" means (i) the power, direct or indirect, to cause the - direction or management of such entity, whether by contract or - otherwise, or (ii) ownership of fifty percent (50%) or more of the - outstanding shares, or (iii) beneficial ownership of such entity. - - "You" (or "Your") shall mean an individual or Legal Entity - exercising permissions granted by this License. - - "Source" form shall mean the preferred form for making modifications, - including but not limited to software source code, documentation - source, and configuration files. - - "Object" form shall mean any form resulting from mechanical - transformation or translation of a Source form, including but - not limited to compiled object code, generated documentation, - and conversions to other media types. - - "Work" shall mean the work of authorship, whether in Source or - Object form, made available under the License, as indicated by a - copyright notice that is included in or attached to the work - (an example is provided in the Appendix below). - - "Derivative Works" shall mean any work, whether in Source or Object - form, that is based on (or derived from) the Work and for which the - editorial revisions, annotations, elaborations, or other modifications - represent, as a whole, an original work of authorship. For the purposes - of this License, Derivative Works shall not include works that remain - separable from, or merely link (or bind by name) to the interfaces of, - the Work and Derivative Works thereof. - - "Contribution" shall mean any work of authorship, including - the original version of the Work and any modifications or additions - to that Work or Derivative Works thereof, that is intentionally - submitted to Licensor for inclusion in the Work by the copyright owner - or by an individual or Legal Entity authorized to submit on behalf of - the copyright owner. For the purposes of this definition, "submitted" - means any form of electronic, verbal, or written communication sent - to the Licensor or its representatives, including but not limited to - communication on electronic mailing lists, source code control systems, - and issue tracking systems that are managed by, or on behalf of, the - Licensor for the purpose of discussing and improving the Work, but - excluding communication that is conspicuously marked or otherwise - designated in writing by the copyright owner as "Not a Contribution." - - "Contributor" shall mean Licensor and any individual or Legal Entity - on behalf of whom a Contribution has been received by Licensor and - subsequently incorporated within the Work. - - 2. Grant of Copyright License. Subject to the terms and conditions of - this License, each Contributor hereby grants to You a perpetual, - worldwide, non-exclusive, no-charge, royalty-free, irrevocable - copyright license to reproduce, prepare Derivative Works of, - publicly display, publicly perform, sublicense, and distribute the - Work and such Derivative Works in Source or Object form. - - 3. Grant of Patent License. Subject to the terms and conditions of - this License, each Contributor hereby grants to You a perpetual, - worldwide, non-exclusive, no-charge, royalty-free, irrevocable - (except as stated in this section) patent license to make, have made, - use, offer to sell, sell, import, and otherwise transfer the Work, - where such license applies only to those patent claims licensable - by such Contributor that are necessarily infringed by their - Contribution(s) alone or by combination of their Contribution(s) - with the Work to which such Contribution(s) was submitted. If You - institute patent litigation against any entity (including a - cross-claim or counterclaim in a lawsuit) alleging that the Work - or a Contribution incorporated within the Work constitutes direct - or contributory patent infringement, then any patent licenses - granted to You under this License for that Work shall terminate - as of the date such litigation is filed. - - 4. Redistribution. You may reproduce and distribute copies of the - Work or Derivative Works thereof in any medium, with or without - modifications, and in Source or Object form, provided that You - meet the following conditions: - - (a) You must give any other recipients of the Work or - Derivative Works a copy of this License; and - - (b) You must cause any modified files to carry prominent notices - stating that You changed the files; and - - (c) You must retain, in the Source form of any Derivative Works - that You distribute, all copyright, patent, trademark, and - attribution notices from the Source form of the Work, - excluding those notices that do not pertain to any part of - the Derivative Works; and - - (d) If the Work includes a "NOTICE" text file as part of its - distribution, then any Derivative Works that You distribute must - include a readable copy of the attribution notices contained - within such NOTICE file, excluding those notices that do not - pertain to any part of the Derivative Works, in at least one - of the following places: within a NOTICE text file distributed - as part of the Derivative Works; within the Source form or - documentation, if provided along with the Derivative Works; or, - within a display generated by the Derivative Works, if and - wherever such third-party notices normally appear. The contents - of the NOTICE file are for informational purposes only and - do not modify the License. You may add Your own attribution - notices within Derivative Works that You distribute, alongside - or as an addendum to the NOTICE text from the Work, provided - that such additional attribution notices cannot be construed - as modifying the License. - - You may add Your own copyright statement to Your modifications and - may provide additional or different license terms and conditions - for use, reproduction, or distribution of Your modifications, or - for any such Derivative Works as a whole, provided Your use, - reproduction, and distribution of the Work otherwise complies with - the conditions stated in this License. - - 5. Submission of Contributions. Unless You explicitly state otherwise, - any Contribution intentionally submitted for inclusion in the Work - by You to the Licensor shall be under the terms and conditions of - this License, without any additional terms or conditions. - Notwithstanding the above, nothing herein shall supersede or modify - the terms of any separate license agreement you may have executed - with Licensor regarding such Contributions. - - 6. Trademarks. This License does not grant permission to use the trade - names, trademarks, service marks, or product names of the Licensor, - except as required for reasonable and customary use in describing the - origin of the Work and reproducing the content of the NOTICE file. - - 7. Disclaimer of Warranty. Unless required by applicable law or - agreed to in writing, Licensor provides the Work (and each - Contributor provides its Contributions) on an "AS IS" BASIS, - WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or - implied, including, without limitation, any warranties or conditions - of TITLE, NON-INFRINGEMENT, MERCHANTABILITY, or FITNESS FOR A - PARTICULAR PURPOSE. You are solely responsible for determining the - appropriateness of using or redistributing the Work and assume any - risks associated with Your exercise of permissions under this License. - - 8. Limitation of Liability. In no event and under no legal theory, - whether in tort (including negligence), contract, or otherwise, - unless required by applicable law (such as deliberate and grossly - negligent acts) or agreed to in writing, shall any Contributor be - liable to You for damages, including any direct, indirect, special, - incidental, or consequential damages of any character arising as a - result of this License or out of the use or inability to use the - Work (including but not limited to damages for loss of goodwill, - work stoppage, computer failure or malfunction, or any and all - other commercial damages or losses), even if such Contributor - has been advised of the possibility of such damages. - - 9. Accepting Warranty or Additional Liability. While redistributing - the Work or Derivative Works thereof, You may choose to offer, - and charge a fee for, acceptance of support, warranty, indemnity, - or other liability obligations and/or rights consistent with this - License. However, in accepting such obligations, You may act only - on Your own behalf and on Your sole responsibility, not on behalf - of any other Contributor, and only if You agree to indemnify, - defend, and hold each Contributor harmless for any liability - incurred by, or claims asserted against, such Contributor by reason - of your accepting any such warranty or additional liability. - - END OF TERMS AND CONDITIONS - - APPENDIX: How to apply the Apache License to your work. - - To apply the Apache License to your work, attach the following - boilerplate notice, with the fields enclosed by brackets "[]" - replaced with your own identifying information. (Don't include - the brackets!) The text should be enclosed in the appropriate - comment syntax for the file format. We also recommend that a - file or class name and description of purpose be included on the - same "printed page" as the copyright notice for easier - identification within third-party archives. - - Copyright [yyyy] [name of copyright owner] - - 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. - - - - - github-oauth - github-plugin - - - - - org.apache.maven.plugins - maven-compiler-plugin - 3.1 - - ${javaVersion} - ${javaVersion} - - - - - - - - org.codehaus.mojo - findbugs-maven-plugin - 3.0.5 - - - - - - - true - - - false - - repo.jenkins-ci.org - https://repo.jenkins-ci.org/artifactory/public/ - - - - true - - sonatype - https://oss.sonatype.org/content/repositories/snapshots/ - - - - true - - - false - - gerrit-api-repository - https://gerrit-api.commondatastorage.googleapis.com/release/ - - - - - com.ryanharter.auto.value - auto-value-gson - 1.3.1 - provided - - - org.projectlombok - lombok - 1.18.30 - provided - - - - - - junit - junit - 4.13.2 - test - - - com.google.truth - truth - 1.1.5 - test - - - - From 2c49fead8571cbdf4b22d5e7f77db3f70ac45e1e Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Fri, 14 Jun 2024 16:43:22 +0100 Subject: [PATCH 31/44] Reformat with GJF 1.22.0 Change-Id: I9dd62131dcf6f90b8dd03c2c8c093311efbf85d5 --- .../plugins/github/oauth/OAuthTokenCipher.java | 3 ++- .../plugins/github/oauth/PasswordGenerator.java | 3 ++- .../gerrit/plugins/github/InitGitHub.java | 3 ++- .../github/git/PullRequestCreateChange.java | 5 ++++- .../plugins/github/git/PullRequestImportJob.java | 14 ++++++++++---- 5 files changed, 20 insertions(+), 8 deletions(-) diff --git a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthTokenCipher.java b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthTokenCipher.java index a8133dca..799617c4 100644 --- a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthTokenCipher.java +++ b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthTokenCipher.java @@ -148,7 +148,8 @@ static List splitKeyIdFromMaterial(String base64EncryptedString) { if (nOfTokens != 2) { throw new IllegalStateException( String.format( - "The encrypted key is expected to contain 2 tokens (keyId:key), whereas it contains %d tokens", + "The encrypted key is expected to contain 2 tokens (keyId:key), whereas it contains" + + " %d tokens", nOfTokens)); } return tokens; diff --git a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/PasswordGenerator.java b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/PasswordGenerator.java index 2f36425b..78360868 100644 --- a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/PasswordGenerator.java +++ b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/PasswordGenerator.java @@ -57,7 +57,8 @@ public boolean generate(Path passwordFilePath) { long length = passwordFile.length(); if (length != PASSWORD_LENGTH_DEFAULT) { throw logErrorAndCreateRuntimeException( - "'%s' password file exists but has an invalid length of %d bytes. The expected length is %d bytes.", + "'%s' password file exists but has an invalid length of %d bytes. The expected length" + + " is %d bytes.", passwordFilePath, length, PASSWORD_LENGTH_DEFAULT); } return false; 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 894e1a5b..00ec0bd2 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 @@ -128,7 +128,8 @@ private void setupGitHubOAuthTokenCipher() { if (maybeCurrentKeyId.isPresent()) { if (!ui.yesno( false, - "Current GitHub OAuth token cipher is configured under the %s key id. Do you want to configure a new one?", + "Current GitHub OAuth token cipher is configured under the %s key id. Do you want to" + + " configure a new one?", maybeCurrentKeyId.get())) { return; } diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/PullRequestCreateChange.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/PullRequestCreateChange.java index 810acd84..d1554c3f 100644 --- a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/PullRequestCreateChange.java +++ b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/PullRequestCreateChange.java @@ -96,7 +96,10 @@ public Change.Id addCommitToChange( final RevCommit pullRequestCommit, final String pullRequestMessage, final String topic) - throws NoSuchChangeException, IOException, InvalidChangeOperationException, UpdateException, + throws NoSuchChangeException, + IOException, + InvalidChangeOperationException, + UpdateException, RestApiException { try (BatchUpdate bu = updateFactory.create( diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/PullRequestImportJob.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/PullRequestImportJob.java index 6734e985..be8b7c08 100644 --- a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/PullRequestImportJob.java +++ b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/PullRequestImportJob.java @@ -203,15 +203,21 @@ private List addPullRequestToChange(GHPullRequest pr, Repository gitRepo) th } private com.google.gerrit.entities.Account.Id getOrRegisterAccount(GitHubUser author) - throws BadRequestException, ResourceConflictException, UnprocessableEntityException, - IOException, ConfigInvalidException { + throws BadRequestException, + ResourceConflictException, + UnprocessableEntityException, + IOException, + ConfigInvalidException { return getOrRegisterAccount(author.login, author.name, author.email); } private com.google.gerrit.entities.Account.Id getOrRegisterAccount( String login, String name, String email) - throws BadRequestException, ResourceConflictException, UnprocessableEntityException, - IOException, ConfigInvalidException { + throws BadRequestException, + ResourceConflictException, + UnprocessableEntityException, + IOException, + ConfigInvalidException { Optional gerritId = externalIdByScheme(ExternalId.SCHEME_GERRIT, login); if (gerritId.isPresent()) { return gerritId.get().accountId(); From cc49e2c8cf1393788413721c385d952456084417 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Fri, 14 Jun 2024 15:56:23 +0100 Subject: [PATCH 32/44] Add Jenkinsfile for incoming changes validation Change-Id: Ibdc33c23daf27c4bf4e0b6b3641cba30d12676ca --- Jenkinsfile | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 Jenkinsfile diff --git a/Jenkinsfile b/Jenkinsfile new file mode 100644 index 00000000..7410d3c5 --- /dev/null +++ b/Jenkinsfile @@ -0,0 +1,3 @@ +pluginPipeline(formatCheckId: 'gerritforge:github-format-3852e64366bb37d13b8baf8af9b15cfd38eb9227', + buildCheckId: 'gerritforge:github-3852e64366bb37d13b8baf8af9b15cfd38eb9227', + gjfVersion: '1.22.0') From 06bcc598095c83414def92911dc1f27f6a32dcbf Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Fri, 14 Jun 2024 22:29:57 +0100 Subject: [PATCH 33/44] Set explicit top-level targets for Bazel build Define at the top-level BUILD the two targets that the build process should generate and move them from their inner project top the top-level directory so that becomes easier to archive them from the CI with the correct expected names: - github-oauth.jar - github-plugin.jar Change-Id: Ie1fca005e9c13912f42b6a989798f48054a292c6 --- BUILD | 13 +++++++++++++ github-oauth/BUILD | 1 + 2 files changed, 14 insertions(+) diff --git a/BUILD b/BUILD index e69de29b..501159f2 100644 --- a/BUILD +++ b/BUILD @@ -0,0 +1,13 @@ +genrule( + name = "github-oauth", + srcs = ["//plugins/github/github-oauth:github-oauth_deploy.jar"], + outs = ["github-oauth.jar"], + cmd = "cp $< $@", +) + +genrule( + name = "github-plugin", + srcs = ["//plugins/github/github-plugin"], + outs = ["github-plugin.jar"], + cmd = "cp $< $@", +) diff --git a/github-oauth/BUILD b/github-oauth/BUILD index 05e694b2..4f80450f 100644 --- a/github-oauth/BUILD +++ b/github-oauth/BUILD @@ -4,6 +4,7 @@ load("//tools/bzl:plugin.bzl", "PLUGIN_DEPS", "PLUGIN_DEPS_NEVERLINK") java_binary( name = "github-oauth", main_class = "Dummy", + visibility = ["//visibility:public"], runtime_deps = [":github-oauth-lib"], ) From 87a8a38e62c43dddeca150a8a2e5b400250032d7 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Tue, 18 Jun 2024 00:15:48 +0100 Subject: [PATCH 34/44] Introduce default github build target All plugins are expected to have a main build target having the same name as the plugin's directory. Introduce the 'github' plugin which is the main node of the dependency of all other targets, including the one responsible for extracting the plugin's version into a version file. Change-Id: I9fb3694e5fb2c4cbf0455d54281ee2677429d16f --- BUILD | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/BUILD b/BUILD index 501159f2..41bb112c 100644 --- a/BUILD +++ b/BUILD @@ -1,3 +1,16 @@ +load("//tools/bzl:genrule2.bzl", "genrule2") + +genrule( + name = "github", + srcs = [ + ":github-plugin", + ":github-plugin-version", + ":github-oauth", + ], + outs = ["github.zip"], + cmd = "zip -o $@ $(SRCS)", +) + genrule( name = "github-oauth", srcs = ["//plugins/github/github-oauth:github-oauth_deploy.jar"], @@ -5,6 +18,13 @@ genrule( cmd = "cp $< $@", ) +genrule2( + name = "github-plugin-version", + srcs = [":github-plugin"], + outs = ["github-plugin.jar-version"], + cmd = "jar xvf $< META-INF/MANIFEST.MF; cat META-INF/MANIFEST.MF | grep Implementation-Version | cut -d ':' -f 2 | xargs > $@", +) + genrule( name = "github-plugin", srcs = ["//plugins/github/github-plugin"], From 40ef0cb1cef5771b880b0c72e51771812137436a Mon Sep 17 00:00:00 2001 From: Dariusz Luksza Date: Wed, 13 Mar 2024 14:16:50 +0000 Subject: [PATCH 35/44] Use replication plugin API to update and access its config Instead of directly modifying the replication plugin configuration files in the `$site_path/etc` directory use new API. This guarantee that the configuration will be stored and read in a correct format and location. Bug: Issue 325000746 Change-Id: I9bde7e1148d932fb6861f51edb3521449ec20bef --- github-plugin/BUILD | 9 ++ .../gerrit/plugins/github/GuiceModule.java | 19 ---- .../github/git/FanoutReplicationConfig.java | 72 -------------- .../git/FileBasedReplicationConfig.java | 60 ----------- .../github/git/ReplicateProjectStep.java | 32 +++--- .../plugins/github/git/ReplicationConfig.java | 27 ----- .../git/ReplicationRemoteConfigBuilder.java | 72 ++++++++++++++ .../replication/GitHubDestinations.java | 41 ++------ .../github/FanoutReplicationConfigTest.java | 99 ------------------- .../ReplicationRemoteConfigBuilderTest.java | 86 ++++++++++++++++ 10 files changed, 192 insertions(+), 325 deletions(-) delete mode 100644 github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/FanoutReplicationConfig.java delete mode 100644 github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/FileBasedReplicationConfig.java delete mode 100644 github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/ReplicationConfig.java create mode 100644 github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/ReplicationRemoteConfigBuilder.java delete mode 100644 github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/FanoutReplicationConfigTest.java create mode 100644 github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/git/ReplicationRemoteConfigBuilderTest.java diff --git a/github-plugin/BUILD b/github-plugin/BUILD index 589f7c62..3ff3d067 100644 --- a/github-plugin/BUILD +++ b/github-plugin/BUILD @@ -30,6 +30,7 @@ gerrit_plugin( resources = glob(["src/main/resources/**/*"]), deps = [ ":github-plugin-lib", + ":replication-api", "//plugins/github/github-oauth:github-oauth-lib", "@axis-jaxrpc//jar", "@axis//jar", @@ -76,7 +77,9 @@ junit_tests( ":github-plugin__plugin", "//javatests/com/google/gerrit/util/http/testutil", "//plugins/github/github-oauth:github-oauth-lib", + "//plugins/replication:replication-api", "@commons-io//jar", + "@github-api//jar", ], ) @@ -93,3 +96,9 @@ java_library( neverlink = True, exports = ["@lombok//jar"], ) + +java_library( + name = "replication-api", + neverlink = True, + exports = ["//plugins/replication:replication-api"], +) diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/GuiceModule.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/GuiceModule.java index 9bff585d..991ba5ae 100644 --- a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/GuiceModule.java +++ b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/GuiceModule.java @@ -18,18 +18,13 @@ import com.google.gerrit.extensions.restapi.RestApiModule; import com.google.gerrit.extensions.webui.TopMenu; import com.google.gerrit.server.account.GroupBackend; -import com.google.gerrit.server.config.SitePaths; import com.google.gerrit.server.events.EventListener; import com.google.gerrit.server.project.ProjectResource; import com.google.gson.Gson; import com.google.inject.AbstractModule; -import com.google.inject.Inject; import com.google.inject.Scopes; import com.google.inject.TypeLiteral; import com.google.inject.assistedinject.FactoryModuleBuilder; -import com.googlesource.gerrit.plugins.github.git.FanoutReplicationConfig; -import com.googlesource.gerrit.plugins.github.git.FileBasedReplicationConfig; -import com.googlesource.gerrit.plugins.github.git.ReplicationConfig; import com.googlesource.gerrit.plugins.github.group.GitHubGroupBackend; import com.googlesource.gerrit.plugins.github.group.GitHubGroupMembership; import com.googlesource.gerrit.plugins.github.group.GitHubGroupsCache; @@ -42,17 +37,9 @@ import com.googlesource.gerrit.plugins.github.replication.ReplicationStatusFlatFile; import com.googlesource.gerrit.plugins.github.replication.ReplicationStatusListener; import com.googlesource.gerrit.plugins.github.replication.ReplicationStatusStore; -import java.nio.file.Files; public class GuiceModule extends AbstractModule { - private final SitePaths site; - - @Inject - public GuiceModule(SitePaths site) { - this.site = site; - } - @Override protected void configure() { bind(new TypeLiteral>() {}) @@ -75,12 +62,6 @@ protected void configure() { } }); - if (Files.exists(site.etc_dir.resolve("replication"))) { - bind(ReplicationConfig.class).to(FanoutReplicationConfig.class).in(Scopes.SINGLETON); - } else { - bind(ReplicationConfig.class).to(FileBasedReplicationConfig.class).in(Scopes.SINGLETON); - } - bind(ReplicationStatusStore.class).to(ReplicationStatusFlatFile.class).in(Scopes.SINGLETON); bind(Gson.class).toProvider(GerritGsonProvider.class); } diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/FanoutReplicationConfig.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/FanoutReplicationConfig.java deleted file mode 100644 index 4a3bc4b4..00000000 --- a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/FanoutReplicationConfig.java +++ /dev/null @@ -1,72 +0,0 @@ -// Copyright (C) 2020 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.git; - -import com.google.gerrit.server.config.SitePaths; -import com.google.inject.Inject; -import java.io.File; -import java.io.IOException; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.List; -import org.eclipse.jgit.errors.ConfigInvalidException; -import org.eclipse.jgit.storage.file.FileBasedConfig; -import org.eclipse.jgit.util.FS; - -public class FanoutReplicationConfig implements ReplicationConfig { - private final SitePaths site; - private final FileBasedConfig secureConf; - - @Inject - public FanoutReplicationConfig(final SitePaths site) { - this.site = site; - this.secureConf = new FileBasedConfig(site.secure_config.toFile(), FS.DETECTED); - } - - @Override - public void addSecureCredentials(String authUsername, String authToken) - throws IOException, ConfigInvalidException { - secureConf.load(); - secureConf.setString("remote", authUsername, "username", authUsername); - secureConf.setString("remote", authUsername, "password", authToken); - secureConf.save(); - } - - @Override - public void addReplicationRemote(String githubUsername, String url, String projectName) - throws IOException, ConfigInvalidException { - - FileBasedConfig replicationConf = - new FileBasedConfig( - new File(site.etc_dir.toFile(), String.format("replication/%s.config", githubUsername)), - FS.DETECTED); - - replicationConf.load(); - String currentUrl = replicationConf.getString("remote", null, "url"); - if (currentUrl == null) { - replicationConf.setString("remote", null, "url", url); - } - List projects = - new ArrayList<>(Arrays.asList(replicationConf.getStringList("remote", null, "projects"))); - projects.add(projectName); - replicationConf.setStringList("remote", null, "projects", projects); - - String currentPushRefs = replicationConf.getString("remote", null, "push"); - if (currentPushRefs == null) { - replicationConf.setString("remote", null, "push", "refs/*:refs/*"); - } - replicationConf.save(); - } -} diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/FileBasedReplicationConfig.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/FileBasedReplicationConfig.java deleted file mode 100644 index 842bde39..00000000 --- a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/FileBasedReplicationConfig.java +++ /dev/null @@ -1,60 +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.git; - -import com.google.gerrit.server.config.SitePaths; -import com.google.inject.Inject; -import java.io.File; -import java.io.IOException; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.List; -import org.eclipse.jgit.errors.ConfigInvalidException; -import org.eclipse.jgit.storage.file.FileBasedConfig; -import org.eclipse.jgit.util.FS; - -public class FileBasedReplicationConfig implements ReplicationConfig { - private final FileBasedConfig secureConf; - private final FileBasedConfig replicationConf; - - @Inject - public FileBasedReplicationConfig(final SitePaths site) { - replicationConf = - new FileBasedConfig(new File(site.etc_dir.toFile(), "replication.config"), FS.DETECTED); - secureConf = new FileBasedConfig(site.secure_config.toFile(), FS.DETECTED); - } - - @Override - public synchronized void addSecureCredentials(String authUsername, String authToken) - throws IOException, ConfigInvalidException { - secureConf.load(); - secureConf.setString("remote", authUsername, "username", authUsername); - secureConf.setString("remote", authUsername, "password", authToken); - secureConf.save(); - } - - @Override - public synchronized void addReplicationRemote(String username, String url, String projectName) - throws IOException, ConfigInvalidException { - replicationConf.load(); - replicationConf.setString("remote", username, "url", url); - List projects = - new ArrayList<>( - Arrays.asList(replicationConf.getStringList("remote", username, "projects"))); - projects.add(projectName); - replicationConf.setStringList("remote", username, "projects", projects); - replicationConf.setString("remote", username, "push", "refs/*:refs/*"); - replicationConf.save(); - } -} 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 5109cd2b..4e75d31e 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,22 +13,21 @@ // limitations under the License. package com.googlesource.gerrit.plugins.github.git; +import com.google.gerrit.extensions.registration.DynamicItem; import com.google.inject.Inject; import com.google.inject.assistedinject.Assisted; import com.googlesource.gerrit.plugins.github.GitHubURL; -import com.googlesource.gerrit.plugins.github.oauth.GitHubLogin; -import com.googlesource.gerrit.plugins.github.oauth.ScopedProvider; +import com.googlesource.gerrit.plugins.replication.api.ReplicationRemotesApi; import java.io.IOException; +import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.ProgressMonitor; import org.slf4j.Logger; import org.slf4j.LoggerFactory; public class ReplicateProjectStep extends ImportStep { private static final Logger LOG = LoggerFactory.getLogger(ReplicateProjectStep.class); - private final ReplicationConfig replicationConfig; - private final String authUsername; - private final String authToken; - private final String gitHubUrl; + private final DynamicItem replicationRemotesUpdaterItem; + private final ReplicationRemoteConfigBuilder remoteConfigBuilder; public interface Factory { ReplicateProjectStep create( @@ -37,20 +36,17 @@ ReplicateProjectStep create( @Inject public ReplicateProjectStep( - final ReplicationConfig replicationConfig, + final DynamicItem replicationRemotesUpdaterItem, final GitHubRepository.Factory gitHubRepoFactory, - final ScopedProvider ghLoginProvider, + ReplicationRemoteConfigBuilder remoteConfigBuilder, @GitHubURL String gitHubUrl, @Assisted("organisation") String organisation, @Assisted("name") String repository) throws IOException { super(gitHubUrl, organisation, repository, gitHubRepoFactory); + this.remoteConfigBuilder = remoteConfigBuilder; LOG.debug("Gerrit ReplicateProject " + organisation + "/" + repository); - this.replicationConfig = replicationConfig; - GitHubLogin ghLogin = ghLoginProvider.get(); - this.authUsername = ghLogin.getMyself().getLogin(); - this.authToken = ghLogin.getAccessToken(); - this.gitHubUrl = gitHubUrl; + this.replicationRemotesUpdaterItem = replicationRemotesUpdaterItem; } @Override @@ -58,11 +54,15 @@ public void doImport(ProgressMonitor progress) throws Exception { progress.beginTask("Setting up Gerrit replication", 2); String repositoryName = getOrganisation() + "/" + getRepositoryName(); + Config remoteConfig = remoteConfigBuilder.build(repositoryName); progress.update(1); - replicationConfig.addSecureCredentials(authUsername, authToken); + + ReplicationRemotesApi updater = replicationRemotesUpdaterItem.get(); + if (updater != null) { + updater.update(remoteConfig); + } progress.update(1); - replicationConfig.addReplicationRemote( - authUsername, gitHubUrl + "/${name}.git", repositoryName); + progress.endTask(); } diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/ReplicationConfig.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/ReplicationConfig.java deleted file mode 100644 index 8b7712a3..00000000 --- a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/ReplicationConfig.java +++ /dev/null @@ -1,27 +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.git; - -import java.io.IOException; -import org.eclipse.jgit.errors.ConfigInvalidException; - -public interface ReplicationConfig { - - void addSecureCredentials(String authUsername, String authToken) - throws IOException, ConfigInvalidException; - - void addReplicationRemote(String username, String url, String projectName) - throws IOException, ConfigInvalidException; -} diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/ReplicationRemoteConfigBuilder.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/ReplicationRemoteConfigBuilder.java new file mode 100644 index 00000000..bef109c3 --- /dev/null +++ b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/ReplicationRemoteConfigBuilder.java @@ -0,0 +1,72 @@ +// Copyright (C) 2024 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.git; + +import com.google.gerrit.extensions.registration.DynamicItem; +import com.google.inject.Inject; +import com.googlesource.gerrit.plugins.github.GitHubURL; +import com.googlesource.gerrit.plugins.github.oauth.GitHubLogin; +import com.googlesource.gerrit.plugins.github.oauth.ScopedProvider; +import com.googlesource.gerrit.plugins.replication.api.ReplicationRemotesApi; +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; +import org.eclipse.jgit.lib.Config; + +class ReplicationRemoteConfigBuilder { + private final String gitHubUrl; + private final String username; + private final String authToken; + private final DynamicItem replicationConfigItem; + + @Inject + ReplicationRemoteConfigBuilder( + DynamicItem replicationRemotesItem, + ScopedProvider ghLoginProvider, + @GitHubURL String gitHubUrl) + throws IOException { + this.gitHubUrl = gitHubUrl; + this.replicationConfigItem = replicationRemotesItem; + GitHubLogin ghLogin = ghLoginProvider.get(); + this.username = ghLogin.getMyself().getLogin(); + this.authToken = ghLogin.getAccessToken(); + } + + Config build(String repositoryName) { + Config remoteConfig = new Config(); + + remoteConfig.setString("remote", username, "username", username); + remoteConfig.setString("remote", username, "password", authToken); + + remoteConfig.setString("remote", username, "url", gitHubUrl + "/${name}.git"); + + String[] existingProjects = getProjects(); + List projects = new ArrayList<>(List.of(existingProjects)); + projects.add(repositoryName); + + remoteConfig.setStringList("remote", username, "projects", projects); + remoteConfig.setString("remote", username, "push", "refs/*:refs/*"); + + return remoteConfig; + } + + private String[] getProjects() { + ReplicationRemotesApi config = replicationConfigItem.get(); + if (config != null) { + return config.get(username).getStringList("remote", username, "projects"); + } + + return new String[0]; + } +} diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/replication/GitHubDestinations.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/replication/GitHubDestinations.java index 810632a6..6d24b3e7 100644 --- a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/replication/GitHubDestinations.java +++ b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/replication/GitHubDestinations.java @@ -18,22 +18,17 @@ import com.google.common.collect.Lists; import com.google.gerrit.server.PluginUser; import com.google.gerrit.server.account.GroupBackend; -import com.google.gerrit.server.config.SitePaths; import com.google.inject.Inject; -import java.io.IOException; +import com.googlesource.gerrit.plugins.replication.api.ReplicationConfig; import java.net.URISyntaxException; -import java.nio.file.Files; -import java.nio.file.Path; import java.util.ArrayList; -import java.util.Collections; import java.util.List; import java.util.Set; import org.eclipse.jgit.errors.ConfigInvalidException; -import org.eclipse.jgit.storage.file.FileBasedConfig; +import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.transport.RefSpec; import org.eclipse.jgit.transport.RemoteConfig; import org.eclipse.jgit.transport.URIish; -import org.eclipse.jgit.util.FS; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -60,15 +55,15 @@ static String replaceName(String in, String name) { @Inject GitHubDestinations( - final SitePaths site, + final ReplicationConfig replicationConfig, final RemoteSiteUser.Factory ruf, final GroupBackend gb, final PluginUser pu) - throws ConfigInvalidException, IOException { + throws ConfigInvalidException { pluginUser = pu; replicationUserFactory = ruf; groupBackend = gb; - configs = getDestinations(site.etc_dir.resolve("replication.config")); + configs = getDestinations(replicationConfig.getConfig()); organisations = getOrganisations(configs); } @@ -83,22 +78,7 @@ private List getOrganisations(List destinations) { return result; } - private List getDestinations(Path cfgPath) - throws ConfigInvalidException, IOException { - if (!Files.exists(cfgPath) || Files.size(cfgPath) == 0) { - return Collections.emptyList(); - } - - FileBasedConfig cfg = new FileBasedConfig(cfgPath.toFile(), FS.DETECTED); - try { - cfg.load(); - } catch (ConfigInvalidException e) { - throw new ConfigInvalidException( - String.format("Config file %s is invalid: %s", cfg.getFile(), e.getMessage()), e); - } catch (IOException e) { - throw new IOException(String.format("Cannot read %s: %s", cfg.getFile(), e.getMessage()), e); - } - + private List getDestinations(Config cfg) throws ConfigInvalidException { ImmutableList.Builder dest = ImmutableList.builder(); for (RemoteConfig c : allRemotes(cfg)) { if (c.getURIs().isEmpty()) { @@ -108,9 +88,7 @@ private List getDestinations(Path cfgPath) for (URIish u : c.getURIs()) { if (u.getPath() == null || !u.getPath().contains("${name}")) { throw new ConfigInvalidException( - String.format( - "remote.%s.url \"%s\" lacks ${name} placeholder in %s", - c.getName(), u, cfg.getFile())); + String.format("remote.%s.url \"%s\" lacks ${name} placeholder", c.getName(), u)); } } @@ -131,7 +109,7 @@ private List getDestinations(Path cfgPath) return dest.build(); } - private static List allRemotes(FileBasedConfig cfg) throws ConfigInvalidException { + private static List allRemotes(Config cfg) throws ConfigInvalidException { Set names = cfg.getSubsections("remote"); List result = Lists.newArrayListWithCapacity(names.size()); for (String name : names) { @@ -140,8 +118,7 @@ private static List allRemotes(FileBasedConfig cfg) throws ConfigI result.add(new RemoteConfig(cfg, name)); } } catch (URISyntaxException e) { - throw new ConfigInvalidException( - String.format("remote %s has invalid URL in %s", name, cfg.getFile())); + throw new ConfigInvalidException(String.format("remote %s has invalid URL", name)); } } return result; diff --git a/github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/FanoutReplicationConfigTest.java b/github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/FanoutReplicationConfigTest.java deleted file mode 100644 index 3613c410..00000000 --- a/github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/FanoutReplicationConfigTest.java +++ /dev/null @@ -1,99 +0,0 @@ -// Copyright (C) 2023 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; - -import static com.google.common.truth.Truth.assertThat; - -import com.google.gerrit.server.config.SitePaths; -import com.googlesource.gerrit.plugins.github.git.FanoutReplicationConfig; -import java.nio.file.Files; -import java.nio.file.Path; -import org.apache.commons.io.FileUtils; -import org.eclipse.jgit.storage.file.FileBasedConfig; -import org.eclipse.jgit.util.FS; -import org.junit.After; -import org.junit.Before; -import org.junit.Test; - -public class FanoutReplicationConfigTest { - - private static final String CUSTOM_KEY = "mykey"; - private static final String CUSTOM_VALUE = "myvalue"; - private static final String REMOTE_ENDPOINT = "my-remote-endpoint"; - private static final String TEST_REMOTE_URL = "http://github.com/myurl"; - private static final String TEST_PROJECT_NAME = "myprojectname"; - private Path tempDir; - private SitePaths sitePaths; - - @Before - public void setup() throws Exception { - tempDir = Files.createTempDirectory(getClass().getSimpleName()); - sitePaths = new SitePaths(tempDir); - Files.createDirectories(sitePaths.etc_dir); - } - - @After - public void teardown() throws Exception { - FileUtils.deleteDirectory(tempDir.toFile()); - } - - @Test - public void shoudKeepAdHocSettingsInFanoutReplicationConfig() throws Exception { - FileBasedConfig currConfig = getReplicationConfig(); - currConfig.setString("remote", null, CUSTOM_KEY, CUSTOM_VALUE); - currConfig.save(); - - String url = "http://github.com/myurl"; - FanoutReplicationConfig fanoutReplicationConfig = new FanoutReplicationConfig(sitePaths); - fanoutReplicationConfig.addReplicationRemote(REMOTE_ENDPOINT, url, "myproject"); - - currConfig.load(); - assertThat(currConfig.getString("remote", null, CUSTOM_KEY)).isEqualTo(CUSTOM_VALUE); - } - - @Test - public void shoudKeepCustomUrlInFanoutReplicationConfig() throws Exception { - FileBasedConfig currConfig = getReplicationConfig(); - String customUrl = "http://my-custom-url"; - currConfig.setString("remote", null, "url", customUrl); - currConfig.save(); - - new FanoutReplicationConfig(sitePaths) - .addReplicationRemote(REMOTE_ENDPOINT, TEST_REMOTE_URL, TEST_PROJECT_NAME); - - currConfig.load(); - assertThat(currConfig.getString("remote", null, "url")).isEqualTo(customUrl); - } - - @Test - public void shoudKeepCustomPushRefSpecInFanoutReplicationConfig() throws Exception { - FileBasedConfig currConfig = getReplicationConfig(); - String customPushRefSpec = "+refs/heads/myheads/*:refs/heads/myheads/*"; - currConfig.setString("remote", null, "push", customPushRefSpec); - currConfig.save(); - - new FanoutReplicationConfig(sitePaths) - .addReplicationRemote(REMOTE_ENDPOINT, TEST_REMOTE_URL, TEST_PROJECT_NAME); - - currConfig.load(); - assertThat(currConfig.getString("remote", null, "push")).isEqualTo(customPushRefSpec); - } - - private FileBasedConfig getReplicationConfig() { - return new FileBasedConfig( - sitePaths.etc_dir.resolve("replication").resolve(REMOTE_ENDPOINT + ".config").toFile(), - FS.DETECTED); - } -} diff --git a/github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/git/ReplicationRemoteConfigBuilderTest.java b/github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/git/ReplicationRemoteConfigBuilderTest.java new file mode 100644 index 00000000..c6023e01 --- /dev/null +++ b/github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/git/ReplicationRemoteConfigBuilderTest.java @@ -0,0 +1,86 @@ +// Copyright (C) 2024 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.git; + +import static com.google.common.truth.Truth.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import com.google.gerrit.extensions.registration.DynamicItem; +import com.googlesource.gerrit.plugins.github.oauth.GitHubLogin; +import com.googlesource.gerrit.plugins.github.oauth.ScopedProvider; +import com.googlesource.gerrit.plugins.replication.api.ReplicationRemotesApi; +import org.eclipse.jgit.lib.Config; +import org.junit.Test; +import org.kohsuke.github.GHMyself; + +public class ReplicationRemoteConfigBuilderTest { + private final String repoName = "test-repo"; + private final String username = "test-user"; + private final String password = "myHighlySecretPassword"; + private final String gitHubUrl = "htpps://github.com"; + + @Test + public void shouldBuildConfig() throws Exception { + ReplicationRemoteConfigBuilder builder = newReplicationRemoteConfigBuilder(); + Config actual = builder.build(repoName); + + assertThat(actual.getString("remote", username, "username")).isEqualTo(username); + assertThat(actual.getString("remote", username, "password")).isEqualTo(password); + assertThat(actual.getString("remote", username, "url")).isEqualTo(gitHubUrl + "/${name}.git"); + assertThat(actual.getStringList("remote", username, "projects")) + .isEqualTo(new String[] {repoName}); + assertThat(actual.getString("remote", username, "push")).isEqualTo("refs/*:refs/*"); + } + + @Test + public void shouldAppendProjectToConfig() throws Exception { + String prevProject = "imported-project"; + Config currentConfig = new Config(); + currentConfig.setString("remote", username, "projects", prevProject); + + ReplicationRemoteConfigBuilder builder = newReplicationRemoteConfigBuilder(currentConfig); + Config actual = builder.build(repoName); + + assertThat(actual.getString("remote", username, "username")).isEqualTo(username); + assertThat(actual.getString("remote", username, "password")).isEqualTo(password); + assertThat(actual.getString("remote", username, "url")).isEqualTo(gitHubUrl + "/${name}.git"); + assertThat(actual.getStringList("remote", username, "projects")) + .isEqualTo(new String[] {prevProject, repoName}); + assertThat(actual.getString("remote", username, "push")).isEqualTo("refs/*:refs/*"); + } + + private ReplicationRemoteConfigBuilder newReplicationRemoteConfigBuilder() throws Exception { + return newReplicationRemoteConfigBuilder(new Config()); + } + + private ReplicationRemoteConfigBuilder newReplicationRemoteConfigBuilder(Config currentConfig) + throws Exception { + GitHubLogin gitHubLoginMock = mock(GitHubLogin.class); + GHMyself ghMyselfMock = mock(GHMyself.class); + ScopedProvider scopedProviderMock = mock(ScopedProvider.class); + ReplicationRemotesApi replicationRemotesApi = mock(ReplicationRemotesApi.class); + DynamicItem replicationRemotesItem = mock(DynamicItem.class); + + when(ghMyselfMock.getLogin()).thenReturn(username); + when(gitHubLoginMock.getMyself()).thenReturn(ghMyselfMock); + when(gitHubLoginMock.getAccessToken()).thenReturn(password); + when(scopedProviderMock.get()).thenReturn(gitHubLoginMock); + when(replicationRemotesApi.get(username)).thenReturn(currentConfig); + when(replicationRemotesItem.get()).thenReturn(replicationRemotesApi); + + return new ReplicationRemoteConfigBuilder( + replicationRemotesItem, scopedProviderMock, gitHubUrl); + } +} From 374ddf9612eb8898a67c0c721bb37c5144222381 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Wed, 19 Jun 2024 18:23:03 +0100 Subject: [PATCH 36/44] Fix accidental overwrite of existing url and push specs When importing additional repositories on an existing organisation, the GitHub plugin preserves the legacy 'url' and 'push' specs for allowing custom ad-hoc settings, such as the use of stable SSH keys for replication or more restrictive push specs. The abstraction of the replication configuration update implemented with I9bde7e1148 has accidental broken the current behaviour causing a regression in how the remotes are configured. Reintroduce the correct behaviour by reading the existing config when updating a remote and avoid overwriting the url and push replication settings. Change-Id: Idd7113f6c0549d752b1dd31bf52d960aee1f77b1 --- .../git/ReplicationRemoteConfigBuilder.java | 14 +++++++++++--- .../git/ReplicationRemoteConfigBuilderTest.java | 15 +++++++++++++++ 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/ReplicationRemoteConfigBuilder.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/ReplicationRemoteConfigBuilder.java index bef109c3..fc808d83 100644 --- a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/ReplicationRemoteConfigBuilder.java +++ b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/ReplicationRemoteConfigBuilder.java @@ -13,6 +13,7 @@ // limitations under the License. package com.googlesource.gerrit.plugins.github.git; +import com.google.common.base.Strings; import com.google.gerrit.extensions.registration.DynamicItem; import com.google.inject.Inject; import com.googlesource.gerrit.plugins.github.GitHubURL; @@ -44,23 +45,30 @@ class ReplicationRemoteConfigBuilder { } Config build(String repositoryName) { - Config remoteConfig = new Config(); + Config remoteConfig = replicationConfigItem.get().get(username); remoteConfig.setString("remote", username, "username", username); remoteConfig.setString("remote", username, "password", authToken); - remoteConfig.setString("remote", username, "url", gitHubUrl + "/${name}.git"); + setRemoteConfigIfNotSet(remoteConfig, "url", gitHubUrl + "/${name}.git"); String[] existingProjects = getProjects(); List projects = new ArrayList<>(List.of(existingProjects)); projects.add(repositoryName); remoteConfig.setStringList("remote", username, "projects", projects); - remoteConfig.setString("remote", username, "push", "refs/*:refs/*"); + setRemoteConfigIfNotSet(remoteConfig, "push", "refs/*:refs/*"); return remoteConfig; } + private void setRemoteConfigIfNotSet(Config remoteConfig, String key, String value) { + String existingValue = remoteConfig.getString("remote", username, key); + if (Strings.isNullOrEmpty(existingValue)) { + remoteConfig.setString("remote", username, key, value); + } + } + private String[] getProjects() { ReplicationRemotesApi config = replicationConfigItem.get(); if (config != null) { diff --git a/github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/git/ReplicationRemoteConfigBuilderTest.java b/github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/git/ReplicationRemoteConfigBuilderTest.java index c6023e01..015b52e7 100644 --- a/github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/git/ReplicationRemoteConfigBuilderTest.java +++ b/github-plugin/src/test/java/com/googlesource/gerrit/plugins/github/git/ReplicationRemoteConfigBuilderTest.java @@ -61,6 +61,21 @@ public void shouldAppendProjectToConfig() throws Exception { assertThat(actual.getString("remote", username, "push")).isEqualTo("refs/*:refs/*"); } + @Test + public void shoudKeepCustomUrlAndPushRefSpecInRemoteConfig() throws Exception { + Config currentConfig = new Config(); + String customPushRefSpec = "+refs/heads/myheads/*:refs/heads/myheads/*"; + String customUrl = "ssh://github@github.com/myuser/${name}.git"; + currentConfig.setString("remote", username, "push", customPushRefSpec); + currentConfig.setString("remote", username, "url", customUrl); + + ReplicationRemoteConfigBuilder builder = newReplicationRemoteConfigBuilder(currentConfig); + Config actual = builder.build(repoName); + + assertThat(actual.getString("remote", username, "push")).isEqualTo(customPushRefSpec); + assertThat(actual.getString("remote", username, "url")).isEqualTo(customUrl); + } + private ReplicationRemoteConfigBuilder newReplicationRemoteConfigBuilder() throws Exception { return newReplicationRemoteConfigBuilder(new Config()); } From 5129b6b338c96df79e3eb7eed529f2d0792dc927 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Fri, 21 Jun 2024 22:13:41 +0100 Subject: [PATCH 37/44] Remove generation of the version file The github-pugin.jar-version is generated automatically from the Gerrit-CI's jobs and should not be created by the plugin's Bazel build. Change-Id: I262146669b089efe755e48f2a0cbb39d790d5dca --- BUILD | 8 -------- 1 file changed, 8 deletions(-) diff --git a/BUILD b/BUILD index 41bb112c..919c4531 100644 --- a/BUILD +++ b/BUILD @@ -4,7 +4,6 @@ genrule( name = "github", srcs = [ ":github-plugin", - ":github-plugin-version", ":github-oauth", ], outs = ["github.zip"], @@ -18,13 +17,6 @@ genrule( cmd = "cp $< $@", ) -genrule2( - name = "github-plugin-version", - srcs = [":github-plugin"], - outs = ["github-plugin.jar-version"], - cmd = "jar xvf $< META-INF/MANIFEST.MF; cat META-INF/MANIFEST.MF | grep Implementation-Version | cut -d ':' -f 2 | xargs > $@", -) - genrule( name = "github-plugin", srcs = ["//plugins/github/github-plugin"], From 0aa142942bf733652b9fc419386cb00105296449 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Sun, 30 Jun 2024 12:28:08 +0100 Subject: [PATCH 38/44] Use Java record-style methods for accessing Scope The introduction of the Java 17 record type for Scope with I979fbfe27 has caused incompatiblity with the current Velocity templates because the fields are now accessible as methods. Adapt the Velocity template calls in scope.html to resume the current scopes selection functionality. Also create the missing getter for the Scope enum in OAuthProtocol for allowing Velocity to access it. Change-Id: I8f6e404c4a3f401deafa25061c1749028319e5b4 --- .../gerrit/plugins/github/oauth/OAuthProtocol.java | 4 ++++ github-plugin/src/main/resources/static/scope.html | 6 +++--- 2 files changed, 7 insertions(+), 3 deletions(-) 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 0ab686c7..d3678c19 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 @@ -146,6 +146,10 @@ public static enum Scope { this.value = value; this.description = description; } + + public String getDescription() { + return description; + } } private static final String ME_SEPARATOR = ","; diff --git a/github-plugin/src/main/resources/static/scope.html b/github-plugin/src/main/resources/static/scope.html index ebc2580f..f8d05466 100644 --- a/github-plugin/src/main/resources/static/scope.html +++ b/github-plugin/src/main/resources/static/scope.html @@ -46,10 +46,10 @@
Which level of GitHub access do you need?

#foreach ( $scope in $scopes.keySet() )
- #set ( $scopeName = $scope.name.substring(6) ) - #set ( $scopeDescription = $scope.description ) + #set ( $scopeName = $scope.name().substring(6) ) + #set ( $scopeDescription = $scope.description() ) #set ( $checked = "" ) - #if ( ( $scopeCookie && $scopeCookie == $scope.name ) || $scopeName == "" ) + #if ( ( $scopeCookie && $scopeCookie == $scope.name() ) || $scopeName == "" ) #set ( $checked = "checked" ) #end From dd52f958f13afdee670e34864949a9c23173966d Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Tue, 11 Feb 2025 22:51:23 +0000 Subject: [PATCH 39/44] Use patchSetInserter.disableValidation() when importing pull-requests The master branch removed the patchSetInserter.setValidate(false) in favour of batter named patchSetInserter.disableValidation(). Change-Id: I1c808eeee503f53a20640470f0b0351c054b09fa --- .../gerrit/plugins/github/git/PullRequestCreateChange.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/PullRequestCreateChange.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/PullRequestCreateChange.java index d1554c3f..e69b1183 100644 --- a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/PullRequestCreateChange.java +++ b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/PullRequestCreateChange.java @@ -233,7 +233,7 @@ private void insertPatchSet( PatchSetInserter patchSetInserter = patchSetInserterFactory.create(changeNotes, psId, cherryPickCommit); patchSetInserter.setMessage(pullRequestMessage); - patchSetInserter.setValidate(false); + patchSetInserter.disableValidation(); bu.addOp(change.getId(), patchSetInserter); bu.execute(); From 5494f929be6d9b0b54ac40a44d8c956bf4757306 Mon Sep 17 00:00:00 2001 From: Paladox none Date: Sun, 20 Jul 2025 06:44:37 -0700 Subject: [PATCH 40/44] Migrate .eslintrc.js to eslint.config.js Change-Id: I49980b6d14a45846630cdc99e208086ac84626ae --- github-plugin/web/.eslintrc.js | 9 --------- github-plugin/web/eslint.config.js | 18 ++++++++++++++++++ 2 files changed, 18 insertions(+), 9 deletions(-) delete mode 100644 github-plugin/web/.eslintrc.js create mode 100644 github-plugin/web/eslint.config.js diff --git a/github-plugin/web/.eslintrc.js b/github-plugin/web/.eslintrc.js deleted file mode 100644 index db4adcd5..00000000 --- a/github-plugin/web/.eslintrc.js +++ /dev/null @@ -1,9 +0,0 @@ -/** - * @license - * Copyright 2022 Google LLC - * SPDX-License-Identifier: Apache-2.0 - */ -__plugindir = "github/github-plugin/web"; -module.exports = { - extends: "../../../.eslintrc.js", -}; \ No newline at end of file diff --git a/github-plugin/web/eslint.config.js b/github-plugin/web/eslint.config.js new file mode 100644 index 00000000..86e21afd --- /dev/null +++ b/github-plugin/web/eslint.config.js @@ -0,0 +1,18 @@ +/** + * @license + * Copyright 2022 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +const {defineConfig} = require('eslint/config'); + +// eslint-disable-next-line no-undef +__plugindir = 'github/github-plugin/web'; + +const gerritEslint = require('../../eslint.config.js'); + +module.exports = defineConfig([ + { + extends: [gerritEslint], + }, +]); \ No newline at end of file From 048a68f3b2a3cbbacc215a888ef9dde5f2f63360 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Tue, 2 Sep 2025 07:59:01 +0100 Subject: [PATCH 41/44] Amend the addition of external-ids to the latest master version Change-Id: I44a141dc18c2f5097b3ca833b2e51334cbbaa1e7 --- .../gerrit/plugins/github/wizard/AccountController.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 0bb7e3e0..1b7327f3 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 @@ -156,7 +156,7 @@ private void setAccountIdentity(IdentifiedUser user, HttpServletRequest req) .update( "Set Username from GitHub", accountId, - u -> u.addExternalId(externalIdFactory.create(key, accountId, null, null))); + u -> u.addExternalId(externalIdFactory.create(key, accountId))); } catch (Exception e) { throw new IllegalArgumentException( "Internal error while trying to set username='" + username + "'"); From 8c09fce1d02a746f1ef39f5b60dba40dbd212eb6 Mon Sep 17 00:00:00 2001 From: Antonio Barone Date: Thu, 18 Sep 2025 16:43:20 +0200 Subject: [PATCH 42/44] Honor repo size quota enforcer during project import Imports from GitHub did not consult repository-size quota enforcers, allowing projects that exceeded configured limits to proceed, leading to quota violations. Introduce a preflight quota check to enforce limits early: * Add `QuotaCheckStep`, which fetches the GitHub repository size and performs a `QuotaBackend` dry-run against `REPOSITORY_SIZE_GROUP`, failing on error. * Wire the step via Guice and `GitImporter`, placing it before clone, project creation so we fail fast with a clear error. * Add `QuotaEnforcedException` for consistent, user-friendly error reporting when quota blocks an import. When quotas allow, the import flow is unchanged. This ensures project imports respect site policies rather than exceeding quota size limits. Bug: Issue 445826646 Change-Id: I2ad8d35fac57581fc5e8fbc25134e0fdb90f52ab --- README.md | 18 +++++ .../plugins/github/GuiceHttpModule.java | 5 ++ .../plugins/github/git/GitImporter.java | 5 ++ .../plugins/github/git/QuotaCheckStep.java | 80 +++++++++++++++++++ .../github/git/QuotaEnforcedException.java | 28 +++++++ 5 files changed, 136 insertions(+) create mode 100644 github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/QuotaCheckStep.java create mode 100644 github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/QuotaEnforcedException.java diff --git a/README.md b/README.md index c497a893..f5c3d022 100644 --- a/README.md +++ b/README.md @@ -197,3 +197,21 @@ Found 2 ref(s): Please remove or rename the following refs and try again: ``` More information on Gerrit magic refs can be found [here](https://gerrit-review.googlesource.com/Documentation/intro-user.html#upload-change) + +#### Enforcing Size Quota + +When a repository is imported from GitHub, the plugin verifies that its size does not exceed any +configured quota constraints. + +A common case is a quota defined by the +[quota plugin](https://gerrit.googlesource.com/plugins/quota/+/refs/heads/master/src/main/resources/Documentation/config.md#quota), +which can restrict how much storage space repositories may consume or how many repositories can be +created within a given namespace. + +If an imported repository exceeds the allowed quota, the operation fails with an error message. +For example: + +```text +Unable to create repository foo/bar: project cannot be created because a quota for the namespace +'foo/*' allows at most 3 projects and 3 projects already exist. +``` 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 ef241915..add26570 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 @@ -33,6 +33,7 @@ import com.googlesource.gerrit.plugins.github.git.MagicRefCheckStep; import com.googlesource.gerrit.plugins.github.git.ProtectedBranchesCheckStep; import com.googlesource.gerrit.plugins.github.git.PullRequestImportJob; +import com.googlesource.gerrit.plugins.github.git.QuotaCheckStep; import com.googlesource.gerrit.plugins.github.git.ReplicateProjectStep; import com.googlesource.gerrit.plugins.github.notification.WebhookServlet; import com.googlesource.gerrit.plugins.github.oauth.GitHubLogin; @@ -77,6 +78,10 @@ protected void configureServlets() { new FactoryModuleBuilder() .implement(MagicRefCheckStep.class, MagicRefCheckStep.class) .build(MagicRefCheckStep.Factory.class)); + install( + new FactoryModuleBuilder() + .implement(QuotaCheckStep.class, QuotaCheckStep.class) + .build(QuotaCheckStep.Factory.class)); install( new FactoryModuleBuilder() .implement(PullRequestImportJob.class, PullRequestImportJob.class) diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/GitImporter.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/GitImporter.java index ff814115..d901d8a6 100644 --- a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/GitImporter.java +++ b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/GitImporter.java @@ -28,6 +28,7 @@ public static class Provider extends HttpSessionProvider {} private static final Logger log = LoggerFactory.getLogger(GitImporter.class); private final ProtectedBranchesCheckStep.Factory protectedBranchesCheckFactory; private final MagicRefCheckStep.Factory magicRefCheckFactory; + private final QuotaCheckStep.Factory quotaCheckFactory; private final GitCloneStep.Factory cloneFactory; private final CreateProjectStep.Factory projectFactory; private final ReplicateProjectStep.Factory replicateFactory; @@ -39,6 +40,7 @@ public GitImporter( CreateProjectStep.Factory projectFactory, ReplicateProjectStep.Factory replicateFactory, MagicRefCheckStep.Factory magicRefCheckFactory, + QuotaCheckStep.Factory quotaCheckFactory, JobExecutor executor, IdentifiedUser user) { super(executor, user); @@ -47,6 +49,7 @@ public GitImporter( this.projectFactory = projectFactory; this.replicateFactory = replicateFactory; this.magicRefCheckFactory = magicRefCheckFactory; + this.quotaCheckFactory = quotaCheckFactory; } public void clone(int idx, String organisation, String repository, String description) { @@ -57,12 +60,14 @@ public void clone(int idx, String organisation, String repository, String descri MagicRefCheckStep magicRefCheckStep = magicRefCheckFactory.create(organisation, repository); CreateProjectStep projectStep = projectFactory.create(organisation, repository, description, user.getUserName().get()); + QuotaCheckStep quotaCheckStep = quotaCheckFactory.create(organisation, repository); ReplicateProjectStep replicateStep = replicateFactory.create(organisation, repository); GitImportJob gitCloneJob = new GitImportJob( idx, organisation, repository, + quotaCheckStep, protectedBranchesCheckStep, magicRefCheckStep, cloneStep, diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/QuotaCheckStep.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/QuotaCheckStep.java new file mode 100644 index 00000000..057e565e --- /dev/null +++ b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/QuotaCheckStep.java @@ -0,0 +1,80 @@ +// Copyright (C) 2025 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.git; + +import static com.google.gerrit.server.quota.QuotaGroupDefinitions.REPOSITORY_SIZE_GROUP; + +import com.google.gerrit.entities.Project; +import com.google.gerrit.server.quota.QuotaBackend; +import com.google.gerrit.server.quota.QuotaResponse; +import com.google.inject.Inject; +import com.google.inject.assistedinject.Assisted; +import com.googlesource.gerrit.plugins.github.GitHubConfig; +import org.eclipse.jgit.lib.ProgressMonitor; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class QuotaCheckStep extends ImportStep { + private static final Logger LOG = LoggerFactory.getLogger(QuotaCheckStep.class); + private final QuotaBackend quotaBackend; + + private static final long BYTES_PER_KB = 1024L; + + public interface Factory { + QuotaCheckStep create( + @Assisted("organisation") String organisation, @Assisted("repository") String repository); + } + + @Inject + public QuotaCheckStep( + QuotaBackend quotaBackend, + GitHubConfig config, + GitHubRepository.Factory gitHubRepoFactory, + @Assisted("organisation") String organisation, + @Assisted("repository") String repository) { + super(config.gitHubUrl, organisation, repository, gitHubRepoFactory); + this.quotaBackend = quotaBackend; + } + + @Override + public void doImport(ProgressMonitor progress) throws GitException { + Project.NameKey fullProjectName = + Project.nameKey(getOrganisation() + "/" + getRepositoryName()); + try { + progress.beginTask("Getting repository size", 1); + LOG.debug("{}|Getting repository size", fullProjectName); + long size = getRepository().getSize() * BYTES_PER_KB; + LOG.debug("{}|Repository size: {} Kb", fullProjectName, size); + + LOG.debug("{}|Checking repository size is allowed by quota", fullProjectName); + if (size > 0) { + QuotaResponse.Aggregated aggregated = + quotaBackend.currentUser().project(fullProjectName).dryRun(REPOSITORY_SIZE_GROUP, size); + aggregated.throwOnError(); + } + progress.update(1); + } catch (Exception e) { + LOG.error("{}|Quota does not allow importing repo", fullProjectName, e); + throw new QuotaEnforcedException(e.getMessage(), e); + } finally { + progress.endTask(); + } + LOG.debug("{}|SUCCESS repository size is allowed", fullProjectName); + } + + @Override + public boolean rollback() { + return true; + } +} diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/QuotaEnforcedException.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/QuotaEnforcedException.java new file mode 100644 index 00000000..8ee4aea8 --- /dev/null +++ b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/QuotaEnforcedException.java @@ -0,0 +1,28 @@ +// Copyright (C) 2025 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.git; + +public class QuotaEnforcedException extends GitException { + + private static final long serialVersionUID = 1L; + + public QuotaEnforcedException(String message, Exception e) { + super(message, e); + } + + @Override + public String getErrorDescription() { + return String.format("Quota enforcing error. %s", getMessage()); + } +} From 435b6670a00e317ffcb9a1b3889a7ed289192edb Mon Sep 17 00:00:00 2001 From: Antonio Barone Date: Fri, 19 Sep 2025 12:53:18 +0200 Subject: [PATCH 43/44] Fix misleading error on project import quota violation Previously, when a project import failed during the GitCloneStep, the error handling logic assumed that all `ResourceConflictException`s indicated the repository already existed. This led to incorrect error messages being shown to users, even when the actual cause was unrelated. This patch removes the special handling for `ResourceConflictException` and instead exposes the full underlying message from the `RestApiException`. As a result, users now receive accurate feedback, for example, when import fails due to exceeding the `maxProjects` quota in a namespace. Improves user experience by providing correct diagnostics for import failures, aiding in quicker debugging and resolution. Bug: Issue 445765274 Change-Id: I7ab32c23140288b3c259d4b8e5fcb3135f14ce7c --- .../gerrit/plugins/github/git/GitCloneStep.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/GitCloneStep.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/GitCloneStep.java index 9d09f51e..d2b589f8 100644 --- a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/GitCloneStep.java +++ b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/git/GitCloneStep.java @@ -21,7 +21,6 @@ import com.google.gerrit.extensions.api.projects.ProjectInput; import com.google.gerrit.extensions.events.ProjectDeletedListener; import com.google.gerrit.extensions.registration.DynamicSet; -import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.account.AccountState; @@ -114,10 +113,9 @@ private void createNewProject() throws GitException { pi.parent = config.getBaseProject(ghRepository.isPrivate()); pi.branches = Stream.ofNullable(ghRepository.getDefaultBranch()).collect(toList()); gerritApi.projects().create(pi).get(); - } catch (ResourceConflictException e) { - throw new GitDestinationAlreadyExistsException(projectName); } catch (RestApiException e) { - throw new GitException("Unable to create repository " + projectName, e); + throw new GitException( + "Unable to create repository " + projectName + ":" + e.getMessage(), e); } } From b24e742438a329214c6c5d33308174e49aff564d Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Fri, 3 Oct 2025 09:35:29 -0700 Subject: [PATCH 44/44] Add deprecation notice Change-Id: Ie0209529fc56a2dc694a97b9e0c7dd7a2ae95e8b --- README.md | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index f5c3d022..f4e98c3d 100644 --- a/README.md +++ b/README.md @@ -1,4 +1,11 @@ -GitHub plugin +# DEPRECATION NOTICE + +GerritForge has decided to [change the license to BSL](https://gitenterprise.me/2025/09/30/re-licensing-gerritforge-plugins-welcome-to-gerrit-enterprise/) +therefore the Apache 2.0 version of this plugin is deprecated. +The recommended version of the github plugin is on [GitHub](https://github.com/GerritForge/github) +and the development continues on [GerritHub.io](https://review.gerrithub.io/admin/repos/GerritForge/github,general). + +GitHub plugin (DEPRECATED) ============= This plugin allows existing GitHub repositories to be integrated as Gerrit projects.

Login Scope Selection

Select GitHub repositories to clone and replicate

Login Scope Selection

Which level of GitHub access do you need?

Which level of GitHub access do you need?

Which level of GitHub access do you need?