From f2ab7bd680c2fef5e7f68d6a850e96a967c8b09c Mon Sep 17 00:00:00 2001 From: Max Shaposhnik Date: Thu, 13 May 2021 12:20:39 +0300 Subject: [PATCH] Fixup SCM URL matching sometimes fails --- .../KubernetesGitCredentialManager.java | 6 ++- .../KubernetesPersonalAccessTokenManager.java | 6 ++- .../KubernetesGitCredentialManagerTest.java | 2 +- ...ernetesPersonalAccessTokenManagerTest.java | 49 ++++++++++++++++++- .../oauth1/BitbucketServerApiProvider.java | 16 +++++- .../BitbucketServerApiClientProviderTest.java | 16 ++++++ .../api/factory/server/github/GithubUrl.java | 4 +- .../gitlab/GitlabOAuthTokenFetcher.java | 32 +++++++++--- .../gitlab/GitlabOAuthTokenFetcherTest.java | 24 +++++++++ 9 files changed, 138 insertions(+), 17 deletions(-) diff --git a/infrastructures/infrastructure-factory/src/main/java/org/eclipse/che/api/factory/server/scm/kubernetes/KubernetesGitCredentialManager.java b/infrastructures/infrastructure-factory/src/main/java/org/eclipse/che/api/factory/server/scm/kubernetes/KubernetesGitCredentialManager.java index 191b179b22..0863990ef4 100644 --- a/infrastructures/infrastructure-factory/src/main/java/org/eclipse/che/api/factory/server/scm/kubernetes/KubernetesGitCredentialManager.java +++ b/infrastructures/infrastructure-factory/src/main/java/org/eclipse/che/api/factory/server/scm/kubernetes/KubernetesGitCredentialManager.java @@ -39,6 +39,7 @@ import org.eclipse.che.api.factory.server.scm.exception.ScmConfigurationPersiste import org.eclipse.che.api.factory.server.scm.exception.UnsatisfiedScmPreconditionException; import org.eclipse.che.api.workspace.server.spi.InfrastructureException; import org.eclipse.che.commons.lang.NameGenerator; +import org.eclipse.che.commons.lang.StringUtils; import org.eclipse.che.workspace.infrastructure.kubernetes.KubernetesClientFactory; import org.eclipse.che.workspace.infrastructure.kubernetes.api.shared.KubernetesNamespaceMeta; import org.eclipse.che.workspace.infrastructure.kubernetes.namespace.KubernetesNamespaceFactory; @@ -103,7 +104,10 @@ public class KubernetesGitCredentialManager implements GitCredentialManager { s.getMetadata().getAnnotations().get(ANNOTATION_GIT_CREDENTIALS)) && personalAccessToken .getScmProviderUrl() - .equals(s.getMetadata().getAnnotations().get(ANNOTATION_SCM_URL)) + .equals( + StringUtils.trimEnd( + s.getMetadata().getAnnotations().get(ANNOTATION_SCM_URL), + '/')) && personalAccessToken .getCheUserId() .equals(s.getMetadata().getAnnotations().get(ANNOTATION_CHE_USERID)) diff --git a/infrastructures/infrastructure-factory/src/main/java/org/eclipse/che/api/factory/server/scm/kubernetes/KubernetesPersonalAccessTokenManager.java b/infrastructures/infrastructure-factory/src/main/java/org/eclipse/che/api/factory/server/scm/kubernetes/KubernetesPersonalAccessTokenManager.java index f56f5ac14d..646370428d 100644 --- a/infrastructures/infrastructure-factory/src/main/java/org/eclipse/che/api/factory/server/scm/kubernetes/KubernetesPersonalAccessTokenManager.java +++ b/infrastructures/infrastructure-factory/src/main/java/org/eclipse/che/api/factory/server/scm/kubernetes/KubernetesPersonalAccessTokenManager.java @@ -37,6 +37,7 @@ import org.eclipse.che.api.factory.server.scm.exception.UnknownScmProviderExcept import org.eclipse.che.api.factory.server.scm.exception.UnsatisfiedScmPreconditionException; import org.eclipse.che.api.workspace.server.spi.InfrastructureException; import org.eclipse.che.commons.lang.NameGenerator; +import org.eclipse.che.commons.lang.StringUtils; import org.eclipse.che.commons.subject.Subject; import org.eclipse.che.workspace.infrastructure.kubernetes.KubernetesClientFactory; import org.eclipse.che.workspace.infrastructure.kubernetes.api.shared.KubernetesNamespaceMeta; @@ -143,11 +144,12 @@ public class KubernetesPersonalAccessTokenManager implements PersonalAccessToken .get(KUBERNETES_PERSONAL_ACCESS_TOKEN_LABEL_SELECTOR); for (Secret secret : secrets) { Map annotations = secret.getMetadata().getAnnotations(); + String trimmedUrl = StringUtils.trimEnd(annotations.get(ANNOTATION_SCM_URL), '/'); if (annotations.get(ANNOTATION_CHE_USERID).equals(cheUser.getUserId()) - && annotations.get(ANNOTATION_SCM_URL).equals(scmServerUrl)) { + && trimmedUrl.equals(StringUtils.trimEnd(scmServerUrl, '/'))) { PersonalAccessToken token = new PersonalAccessToken( - annotations.get(ANNOTATION_SCM_URL), + trimmedUrl, annotations.get(ANNOTATION_CHE_USERID), annotations.get(ANNOTATION_SCM_USERNAME), annotations.get(ANNOTATION_SCM_USERID), diff --git a/infrastructures/infrastructure-factory/src/test/java/org/eclipse/che/api/factory/server/scm/kubernetes/KubernetesGitCredentialManagerTest.java b/infrastructures/infrastructure-factory/src/test/java/org/eclipse/che/api/factory/server/scm/kubernetes/KubernetesGitCredentialManagerTest.java index df7c00e683..40dfc6a10e 100644 --- a/infrastructures/infrastructure-factory/src/test/java/org/eclipse/che/api/factory/server/scm/kubernetes/KubernetesGitCredentialManagerTest.java +++ b/infrastructures/infrastructure-factory/src/test/java/org/eclipse/che/api/factory/server/scm/kubernetes/KubernetesGitCredentialManagerTest.java @@ -129,7 +129,7 @@ public class KubernetesGitCredentialManagerTest { Map annotations = new HashMap<>(DEFAULT_SECRET_ANNOTATIONS); - annotations.put(ANNOTATION_SCM_URL, token.getScmProviderUrl()); + annotations.put(ANNOTATION_SCM_URL, token.getScmProviderUrl() + "/"); annotations.put(ANNOTATION_SCM_USERNAME, token.getScmUserName()); annotations.put(ANNOTATION_CHE_USERID, token.getCheUserId()); ObjectMeta objectMeta = diff --git a/infrastructures/infrastructure-factory/src/test/java/org/eclipse/che/api/factory/server/scm/kubernetes/KubernetesPersonalAccessTokenManagerTest.java b/infrastructures/infrastructure-factory/src/test/java/org/eclipse/che/api/factory/server/scm/kubernetes/KubernetesPersonalAccessTokenManagerTest.java index 3a2e3d54fc..9400a474a1 100644 --- a/infrastructures/infrastructure-factory/src/test/java/org/eclipse/che/api/factory/server/scm/kubernetes/KubernetesPersonalAccessTokenManagerTest.java +++ b/infrastructures/infrastructure-factory/src/test/java/org/eclipse/che/api/factory/server/scm/kubernetes/KubernetesPersonalAccessTokenManagerTest.java @@ -150,7 +150,7 @@ public class KubernetesPersonalAccessTokenManagerTest { ObjectMeta meta3 = new ObjectMetaBuilder() .withAnnotations( - Map.of(ANNOTATION_CHE_USERID, "user2", ANNOTATION_SCM_URL, "http://host2")) + Map.of(ANNOTATION_CHE_USERID, "user2", ANNOTATION_SCM_URL, "http://host3")) .build(); Secret secret1 = new SecretBuilder().withMetadata(meta1).withData(data1).build(); @@ -172,6 +172,53 @@ public class KubernetesPersonalAccessTokenManagerTest { assertEquals(token.getToken(), "token1"); } + @Test + public void testGetTokenFromNamespaceWithTrailingSlashMismatch() throws Exception { + + KubernetesNamespaceMeta meta = new KubernetesNamespaceMetaImpl("test"); + when(namespaceFactory.list()).thenReturn(Collections.singletonList(meta)); + KubernetesNamespace kubernetesnamespace = Mockito.mock(KubernetesNamespace.class); + KubernetesSecrets secrets = Mockito.mock(KubernetesSecrets.class); + when(namespaceFactory.access(eq(null), eq(meta.getName()))).thenReturn(kubernetesnamespace); + when(kubernetesnamespace.secrets()).thenReturn(secrets); + when(scmPersonalAccessTokenFetcher.isValid(any(PersonalAccessToken.class))).thenReturn(true); + + Map data1 = + Map.of("token", Base64.getEncoder().encodeToString("token1".getBytes(UTF_8))); + Map data2 = + Map.of("token", Base64.getEncoder().encodeToString("token2".getBytes(UTF_8))); + + ObjectMeta meta1 = + new ObjectMetaBuilder() + .withAnnotations( + Map.of(ANNOTATION_CHE_USERID, "user1", ANNOTATION_SCM_URL, "http://host1.com/")) + .build(); + ObjectMeta meta2 = + new ObjectMetaBuilder() + .withAnnotations( + Map.of(ANNOTATION_CHE_USERID, "user1", ANNOTATION_SCM_URL, "http://host2.com")) + .build(); + + Secret secret1 = new SecretBuilder().withMetadata(meta1).withData(data1).build(); + Secret secret2 = new SecretBuilder().withMetadata(meta2).withData(data2).build(); + + when(secrets.get(any(LabelSelector.class))).thenReturn(Arrays.asList(secret1, secret2)); + + // when + PersonalAccessToken token1 = + personalAccessTokenManager + .get(new SubjectImpl("user", "user1", "t1", false), "http://host1.com") + .get(); + PersonalAccessToken token2 = + personalAccessTokenManager + .get(new SubjectImpl("user", "user1", "t1", false), "http://host2.com/") + .get(); + + // then + assertNotNull(token1); + assertNotNull(token2); + } + @Test public void shouldDeleteInvalidTokensOnGet() throws Exception { // given diff --git a/wsmaster/che-core-api-factory-bitbucket-server/src/main/java/org/eclipse/che/security/oauth1/BitbucketServerApiProvider.java b/wsmaster/che-core-api-factory-bitbucket-server/src/main/java/org/eclipse/che/security/oauth1/BitbucketServerApiProvider.java index 1e94c7b148..98e0a89865 100644 --- a/wsmaster/che-core-api-factory-bitbucket-server/src/main/java/org/eclipse/che/security/oauth1/BitbucketServerApiProvider.java +++ b/wsmaster/che-core-api-factory-bitbucket-server/src/main/java/org/eclipse/che/security/oauth1/BitbucketServerApiProvider.java @@ -13,8 +13,11 @@ package org.eclipse.che.security.oauth1; import static com.google.common.base.Strings.isNullOrEmpty; +import com.google.common.base.Splitter; +import java.util.List; import java.util.Optional; import java.util.Set; +import java.util.stream.Collectors; import javax.inject.Inject; import javax.inject.Named; import javax.inject.Provider; @@ -23,6 +26,7 @@ import org.eclipse.che.api.factory.server.bitbucket.server.BitbucketServerApiCli import org.eclipse.che.api.factory.server.bitbucket.server.HttpBitbucketServerApiClient; import org.eclipse.che.api.factory.server.bitbucket.server.NoopBitbucketServerApiClient; import org.eclipse.che.commons.annotation.Nullable; +import org.eclipse.che.commons.lang.StringUtils; import org.eclipse.che.inject.ConfigurationException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -49,17 +53,25 @@ public class BitbucketServerApiProvider implements Provider authenticators) { if (isNullOrEmpty(bitbucketOauth1Endpoint)) { return new NoopBitbucketServerApiClient(); } else { - if (isNullOrEmpty(bitbucketEndpoints)) { + if (isNullOrEmpty(rawBitbucketEndpoints)) { throw new ConfigurationException( "`che.integration.bitbucket.server_endpoints` bitbucket configuration is missing." + " It should contain values from 'che.oauth1.bitbucket.endpoint'"); } else { + // sanitise URL-s first + bitbucketOauth1Endpoint = StringUtils.trimEnd(bitbucketOauth1Endpoint, '/'); + List bitbucketEndpoints = + Splitter.on(",") + .splitToList(rawBitbucketEndpoints) + .stream() + .map(s -> StringUtils.trimEnd(s, '/')) + .collect(Collectors.toList()); if (bitbucketEndpoints.contains(bitbucketOauth1Endpoint)) { Optional authenticator = authenticators diff --git a/wsmaster/che-core-api-factory-bitbucket-server/src/test/java/org/eclipse/che/security/oauth1/BitbucketServerApiClientProviderTest.java b/wsmaster/che-core-api-factory-bitbucket-server/src/test/java/org/eclipse/che/security/oauth1/BitbucketServerApiClientProviderTest.java index dee74107b9..e2471cff35 100644 --- a/wsmaster/che-core-api-factory-bitbucket-server/src/test/java/org/eclipse/che/security/oauth1/BitbucketServerApiClientProviderTest.java +++ b/wsmaster/che-core-api-factory-bitbucket-server/src/test/java/org/eclipse/che/security/oauth1/BitbucketServerApiClientProviderTest.java @@ -52,6 +52,22 @@ public class BitbucketServerApiClientProviderTest { assertTrue(HttpBitbucketServerApiClient.class.isAssignableFrom(actual.getClass())); } + @Test + public void shouldNormalizeURLsBeforeCreateBitbucketServerApi() { + // given + BitbucketServerApiProvider bitbucketServerApiProvider = + new BitbucketServerApiProvider( + "https://bitbucket.server.com/, https://bitbucket2.server.com/", + "https://bitbucket.server.com/", + ImmutableSet.of(oAuthAuthenticator)); + // when + BitbucketServerApiClient actual = bitbucketServerApiProvider.get(); + // then + assertNotNull(actual); + // internal representation always w/out slashes + assertTrue(actual.isConnected("https://bitbucket.server.com")); + } + @Test(dataProvider = "noopConfig") public void shouldProvideNoopOAuthAuthenticatorIfSomeConfigurationIsNotSet( @Nullable String bitbucketEndpoints, diff --git a/wsmaster/che-core-api-factory-github/src/main/java/org/eclipse/che/api/factory/server/github/GithubUrl.java b/wsmaster/che-core-api-factory-github/src/main/java/org/eclipse/che/api/factory/server/github/GithubUrl.java index b62d04eb77..95704c5554 100644 --- a/wsmaster/che-core-api-factory-github/src/main/java/org/eclipse/che/api/factory/server/github/GithubUrl.java +++ b/wsmaster/che-core-api-factory-github/src/main/java/org/eclipse/che/api/factory/server/github/GithubUrl.java @@ -31,7 +31,7 @@ import org.eclipse.che.api.factory.server.urlfactory.RemoteFactoryUrl; */ public class GithubUrl implements RemoteFactoryUrl { - private static final String HOSTNAME = "https://github.com/"; + private static final String HOSTNAME = "https://github.com"; /** Username part of github URL */ private String username; @@ -173,6 +173,6 @@ public class GithubUrl implements RemoteFactoryUrl { * @return location of the repository. */ protected String repositoryLocation() { - return HOSTNAME + this.username + "/" + this.repository; + return HOSTNAME + "/" + this.username + "/" + this.repository; } } diff --git a/wsmaster/che-core-api-factory-gitlab/src/main/java/org/eclipse/che/api/factory/server/gitlab/GitlabOAuthTokenFetcher.java b/wsmaster/che-core-api-factory-gitlab/src/main/java/org/eclipse/che/api/factory/server/gitlab/GitlabOAuthTokenFetcher.java index 796569ad3d..8c2245445a 100644 --- a/wsmaster/che-core-api-factory-gitlab/src/main/java/org/eclipse/che/api/factory/server/gitlab/GitlabOAuthTokenFetcher.java +++ b/wsmaster/che-core-api-factory-gitlab/src/main/java/org/eclipse/che/api/factory/server/gitlab/GitlabOAuthTokenFetcher.java @@ -33,6 +33,7 @@ import org.eclipse.che.api.factory.server.scm.exception.ScmItemNotFoundException import org.eclipse.che.api.factory.server.scm.exception.ScmUnauthorizedException; import org.eclipse.che.commons.annotation.Nullable; import org.eclipse.che.commons.lang.NameGenerator; +import org.eclipse.che.commons.lang.StringUtils; import org.eclipse.che.commons.subject.Subject; import org.eclipse.che.security.oauth.OAuthAPI; import org.slf4j.Logger; @@ -45,6 +46,7 @@ public class GitlabOAuthTokenFetcher implements PersonalAccessTokenFetcher { private static final String OAUTH_PROVIDER_NAME = "gitlab"; public static final Set DEFAULT_TOKEN_SCOPES = ImmutableSet.of("api", "write_repository", "openid"); + public static final String OAUTH_2_PREFIX = "oauth2-"; private final OAuthAPI oAuthAPI; private final String apiEndpoint; @@ -58,8 +60,8 @@ public class GitlabOAuthTokenFetcher implements PersonalAccessTokenFetcher { this.apiEndpoint = apiEndpoint; this.oAuthAPI = oAuthAPI; if (gitlabEndpoints != null) { - this.gitlabApiClient = - new GitlabApiClient(Splitter.on(",").splitToList(gitlabEndpoints).get(0)); + final String oAuthEndpoint = Splitter.on(",").splitToList(gitlabEndpoints).get(0); + this.gitlabApiClient = new GitlabApiClient(StringUtils.trimEnd(oAuthEndpoint, '/')); } else { this.gitlabApiClient = null; } @@ -68,6 +70,7 @@ public class GitlabOAuthTokenFetcher implements PersonalAccessTokenFetcher { @Override public PersonalAccessToken fetchPersonalAccessToken(Subject cheSubject, String scmServerUrl) throws ScmUnauthorizedException, ScmCommunicationException { + scmServerUrl = StringUtils.trimEnd(scmServerUrl, '/'); if (gitlabApiClient == null || !gitlabApiClient.isConnected(scmServerUrl)) { LOG.debug("not a valid url {} for current fetcher ", scmServerUrl); return null; @@ -82,7 +85,7 @@ public class GitlabOAuthTokenFetcher implements PersonalAccessTokenFetcher { cheSubject.getUserId(), user.getUsername(), Long.toString(user.getId()), - NameGenerator.generate("oauth2-", 5), + NameGenerator.generate(OAUTH_2_PREFIX, 5), NameGenerator.generate("id-", 5), oAuthToken.getToken()); Optional valid = isValid(token); @@ -122,11 +125,24 @@ public class GitlabOAuthTokenFetcher implements PersonalAccessTokenFetcher { "not a valid url {} for current fetcher ", personalAccessToken.getScmProviderUrl()); return Optional.empty(); } - try { - GitlabOauthTokenInfo info = gitlabApiClient.getTokenInfo(personalAccessToken.getToken()); - return Optional.of(Sets.newHashSet(info.getScope()).containsAll(DEFAULT_TOKEN_SCOPES)); - } catch (ScmItemNotFoundException | ScmCommunicationException e) { - return Optional.of(Boolean.FALSE); + if (personalAccessToken.getScmTokenName() != null + && personalAccessToken.getScmTokenName().startsWith(OAUTH_2_PREFIX)) { + // validation OAuth token by special API call + try { + GitlabOauthTokenInfo info = gitlabApiClient.getTokenInfo(personalAccessToken.getToken()); + return Optional.of(Sets.newHashSet(info.getScope()).containsAll(DEFAULT_TOKEN_SCOPES)); + } catch (ScmItemNotFoundException | ScmCommunicationException e) { + return Optional.of(Boolean.FALSE); + } + } else { + // validating personal access token from secret. Since PAT API is accessible only in + // latest GitLab version, we just perform check by accessing something from API. + try { + gitlabApiClient.getUser(personalAccessToken.getToken()); + return Optional.of(Boolean.TRUE); + } catch (ScmItemNotFoundException | ScmCommunicationException | ScmBadRequestException e) { + return Optional.of(Boolean.FALSE); + } } } diff --git a/wsmaster/che-core-api-factory-gitlab/src/test/java/org/eclipse/che/api/factory/server/gitlab/GitlabOAuthTokenFetcherTest.java b/wsmaster/che-core-api-factory-gitlab/src/test/java/org/eclipse/che/api/factory/server/gitlab/GitlabOAuthTokenFetcherTest.java index 354d0a3467..9552eec8c6 100644 --- a/wsmaster/che-core-api-factory-gitlab/src/test/java/org/eclipse/che/api/factory/server/gitlab/GitlabOAuthTokenFetcherTest.java +++ b/wsmaster/che-core-api-factory-gitlab/src/test/java/org/eclipse/che/api/factory/server/gitlab/GitlabOAuthTokenFetcherTest.java @@ -21,6 +21,7 @@ import static org.eclipse.che.dto.server.DtoFactory.newDto; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.when; import static org.testng.Assert.assertNotNull; +import static org.testng.Assert.assertTrue; import com.github.tomakehurst.wiremock.WireMockServer; import com.github.tomakehurst.wiremock.client.WireMock; @@ -131,4 +132,27 @@ public class GitlabOAuthTokenFetcherTest { oAuthTokenFetcher.fetchPersonalAccessToken(subject, wireMockServer.url("/")); assertNotNull(token); } + + @Test + public void shouldValidatePersonalToken() throws Exception { + stubFor( + get(urlEqualTo("/api/v4/user")) + .withHeader(HttpHeaders.AUTHORIZATION, equalTo("Bearer token123")) + .willReturn( + aResponse() + .withHeader("Content-Type", "application/json; charset=utf-8") + .withBodyFile("gitlab/rest/api/v4/user/response.json"))); + + PersonalAccessToken token = + new PersonalAccessToken( + wireMockServer.baseUrl(), + "cheUser", + "username", + "userId", + "token-name", + "tid-23434", + "token123"); + + assertTrue(oAuthTokenFetcher.isValid(token).get()); + } }