Omit scm-username annotation from the PAT secret

Signed-off-by: Igor Vinokur <ivinokur@redhat.com>
eclipse-che · Jul 19, 2023 · 1657ebb · 1657ebb
1 parent c5963e0
commit 1657ebb
Show file tree

Hide file tree

Showing 33 changed files with 715 additions and 399 deletions.
diff --git a/...ava/org/eclipse/che/api/factory/server/scm/kubernetes/KubernetesGitCredentialManager.java b/...ava/org/eclipse/che/api/factory/server/scm/kubernetes/KubernetesGitCredentialManager.java
@@ -126,19 +126,14 @@ public void createOrReplace(PersonalAccessToken personalAccessToken)
                                       '/'))
                           && personalAccessToken
                               .getCheUserId()
-                              .equals(s.getMetadata().getAnnotations().get(ANNOTATION_CHE_USERID))
-                          && personalAccessToken
-                              .getScmUserName()
-                              .equals(
-                                  s.getMetadata().getAnnotations().get(ANNOTATION_SCM_USERNAME)))
+                              .equals(s.getMetadata().getAnnotations().get(ANNOTATION_CHE_USERID)))
               .findFirst();
 
       Secret secret =
           existing.orElseGet(
               () -> {
                 Map<String, String> annotations = new HashMap<>(DEFAULT_SECRET_ANNOTATIONS);
                 annotations.put(ANNOTATION_SCM_URL, personalAccessToken.getScmProviderUrl());
-                annotations.put(ANNOTATION_SCM_USERNAME, personalAccessToken.getScmUserName());
                 annotations.put(ANNOTATION_CHE_USERID, personalAccessToken.getCheUserId());
                 ObjectMeta meta =
                     new ObjectMetaBuilder()

diff --git a/...g/eclipse/che/api/factory/server/scm/kubernetes/KubernetesPersonalAccessTokenManager.java b/...g/eclipse/che/api/factory/server/scm/kubernetes/KubernetesPersonalAccessTokenManager.java
@@ -11,6 +11,8 @@
  */
 package org.eclipse.che.api.factory.server.scm.kubernetes;
 
+import static com.google.common.base.Strings.isNullOrEmpty;
+
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableMap;
 import io.fabric8.kubernetes.api.model.LabelSelector;
@@ -30,6 +32,7 @@
 import org.eclipse.che.api.factory.server.scm.GitCredentialManager;
 import org.eclipse.che.api.factory.server.scm.PersonalAccessToken;
 import org.eclipse.che.api.factory.server.scm.PersonalAccessTokenManager;
+import org.eclipse.che.api.factory.server.scm.PersonalAccessTokenParams;
 import org.eclipse.che.api.factory.server.scm.ScmPersonalAccessTokenFetcher;
 import org.eclipse.che.api.factory.server.scm.exception.ScmCommunicationException;
 import org.eclipse.che.api.factory.server.scm.exception.ScmConfigurationPersistenceException;
@@ -59,7 +62,6 @@ public class KubernetesPersonalAccessTokenManager implements PersonalAccessToken
   public static final String NAME_PATTERN = "personal-access-token-";
 
   public static final String ANNOTATION_CHE_USERID = "che.eclipse.org/che-userid";
-  public static final String ANNOTATION_SCM_USERNAME = "che.eclipse.org/scm-username";
   public static final String ANNOTATION_SCM_ORGANIZATION = "che.eclipse.org/scm-organization";
   public static final String ANNOTATION_SCM_PERSONAL_ACCESS_TOKEN_ID =
       "che.eclipse.org/scm-personal-access-token-id";
@@ -96,7 +98,6 @@ void save(PersonalAccessToken personalAccessToken)
               .withAnnotations(
                   new ImmutableMap.Builder<String, String>()
                       .put(ANNOTATION_CHE_USERID, personalAccessToken.getCheUserId())
-                      .put(ANNOTATION_SCM_USERNAME, personalAccessToken.getScmUserName())
                       .put(ANNOTATION_SCM_URL, personalAccessToken.getScmProviderUrl())
                       .put(
                           ANNOTATION_SCM_PERSONAL_ACCESS_TOKEN_ID,
@@ -191,28 +192,34 @@ private Optional<PersonalAccessToken> doGetPersonalAccessToken(
                   || trimmedUrl.equals(StringUtils.trimEnd(scmServerUrl, '/')))) {
             String token =
                 new String(Base64.getDecoder().decode(secret.getData().get("token"))).trim();
-            PersonalAccessToken personalAccessToken =
-                new PersonalAccessToken(
-                    trimmedUrl,
-                    annotations.get(ANNOTATION_CHE_USERID),
-                    annotations.get(ANNOTATION_SCM_ORGANIZATION),
-                    annotations.get(ANNOTATION_SCM_USERNAME),
-                    annotations.get(ANNOTATION_SCM_PERSONAL_ACCESS_TOKEN_NAME),
-                    annotations.get(ANNOTATION_SCM_PERSONAL_ACCESS_TOKEN_ID),
-                    token);
-            if (scmPersonalAccessTokenFetcher.isValid(personalAccessToken)) {
+            String providerName = annotations.get(ANNOTATION_SCM_PERSONAL_ACCESS_TOKEN_NAME);
+            String tokenId = annotations.get(ANNOTATION_SCM_PERSONAL_ACCESS_TOKEN_ID);
+            String organization = annotations.get(ANNOTATION_SCM_ORGANIZATION);
+            String scmUsername =
+                scmPersonalAccessTokenFetcher.isValid(
+                    new PersonalAccessTokenParams(
+                        trimmedUrl, providerName, tokenId, token, organization));
+            if (!isNullOrEmpty(scmUsername)) {
+              PersonalAccessToken personalAccessToken =
+                  new PersonalAccessToken(
+                      trimmedUrl,
+                      annotations.get(ANNOTATION_CHE_USERID),
+                      organization,
+                      scmUsername,
+                      providerName,
+                      tokenId,
+                      token);
               return Optional.of(personalAccessToken);
-            } else {
-              // Removing token that is no longer valid. If several tokens exist the next one could
-              // be valid. If no valid token can be found, the caller should react in the same way
-              // as it reacts if no token exists. Usually, that means that process of new token
-              // retrieval would be initiated.
-              cheServerKubernetesClientFactory
-                  .create()
-                  .secrets()
-                  .inNamespace(namespaceMeta.getName())
-                  .delete(secret);
             }
+            // Removing token that is no longer valid. If several tokens exist the next one could
+            // be valid. If no valid token can be found, the caller should react in the same way
+            // as it reacts if no token exists. Usually, that means that process of new token
+            // retrieval would be initiated.
+            cheServerKubernetesClientFactory
+                .create()
+                .secrets()
+                .inNamespace(namespaceMeta.getName())
+                .delete(secret);
           }
         }
       }

diff --git a/...org/eclipse/che/api/factory/server/scm/kubernetes/KubernetesGitCredentialManagerTest.java b/...org/eclipse/che/api/factory/server/scm/kubernetes/KubernetesGitCredentialManagerTest.java
@@ -15,7 +15,6 @@
 import static java.util.Collections.singletonList;
 import static org.eclipse.che.api.factory.server.scm.kubernetes.KubernetesGitCredentialManager.ANNOTATION_CHE_USERID;
 import static org.eclipse.che.api.factory.server.scm.kubernetes.KubernetesGitCredentialManager.ANNOTATION_SCM_URL;
-import static org.eclipse.che.api.factory.server.scm.kubernetes.KubernetesGitCredentialManager.ANNOTATION_SCM_USERNAME;
 import static org.eclipse.che.api.factory.server.scm.kubernetes.KubernetesGitCredentialManager.DEFAULT_SECRET_ANNOTATIONS;
 import static org.eclipse.che.api.factory.server.scm.kubernetes.KubernetesGitCredentialManager.NAME_PATTERN;
 import static org.mockito.ArgumentMatchers.anyMap;
@@ -125,7 +124,6 @@ public void shouldUseHardcodedUsernameIfScmOrganizationIsDefined() throws Except
     Map<String, String> annotations = new HashMap<>(DEFAULT_SECRET_ANNOTATIONS);
 
     annotations.put(ANNOTATION_SCM_URL, token.getScmProviderUrl() + "/");
-    annotations.put(ANNOTATION_SCM_USERNAME, token.getScmUserName());
     annotations.put(ANNOTATION_CHE_USERID, token.getCheUserId());
     ObjectMeta objectMeta =
         new ObjectMetaBuilder()
@@ -210,7 +208,6 @@ public void testUpdateTokenInExistingCredential() throws Exception {
     Map<String, String> annotations = new HashMap<>(DEFAULT_SECRET_ANNOTATIONS);
 
     annotations.put(ANNOTATION_SCM_URL, token.getScmProviderUrl() + "/");
-    annotations.put(ANNOTATION_SCM_USERNAME, token.getScmUserName());
     annotations.put(ANNOTATION_CHE_USERID, token.getCheUserId());
     ObjectMeta objectMeta =
         new ObjectMetaBuilder()

diff --git a/...lipse/che/api/factory/server/scm/kubernetes/KubernetesPersonalAccessTokenManagerTest.java b/...lipse/che/api/factory/server/scm/kubernetes/KubernetesPersonalAccessTokenManagerTest.java
@@ -43,6 +43,7 @@
 import java.util.Optional;
 import org.eclipse.che.api.factory.server.scm.GitCredentialManager;
 import org.eclipse.che.api.factory.server.scm.PersonalAccessToken;
+import org.eclipse.che.api.factory.server.scm.PersonalAccessTokenParams;
 import org.eclipse.che.api.factory.server.scm.ScmPersonalAccessTokenFetcher;
 import org.eclipse.che.commons.subject.SubjectImpl;
 import org.eclipse.che.workspace.infrastructure.kubernetes.CheServerKubernetesClientFactory;
@@ -95,7 +96,8 @@ public void shouldTrimBlankCharsInToken() throws Exception {
     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);
+    when(scmPersonalAccessTokenFetcher.isValid(any(PersonalAccessTokenParams.class)))
+        .thenReturn("user");
 
     Map<String, String> data =
         Map.of("token", Base64.getEncoder().encodeToString(" token_value \n".getBytes(UTF_8)));
@@ -161,7 +163,8 @@ public void testGetTokenFromNamespace() throws Exception {
     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);
+    when(scmPersonalAccessTokenFetcher.isValid(any(PersonalAccessTokenParams.class)))
+        .thenReturn("user");
 
     Map<String, String> data1 =
         Map.of("token", Base64.getEncoder().encodeToString("token1".getBytes(UTF_8)));
@@ -214,7 +217,8 @@ public void testGetTokenFromNamespaceWithTrailingSlashMismatch() throws Exceptio
     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);
+    when(scmPersonalAccessTokenFetcher.isValid(any(PersonalAccessTokenParams.class)))
+        .thenReturn("user");
 
     Map<String, String> data1 =
         Map.of("token", Base64.getEncoder().encodeToString("token1".getBytes(UTF_8)));
@@ -261,7 +265,8 @@ public void shouldDeleteInvalidTokensOnGet() throws Exception {
     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(false);
+    when(scmPersonalAccessTokenFetcher.isValid(any(PersonalAccessTokenParams.class)))
+        .thenReturn(null);
     when(cheServerKubernetesClientFactory.create()).thenReturn(kubeClient);
     when(kubeClient.secrets()).thenReturn(secretsMixedOperation);
     when(secretsMixedOperation.inNamespace(eq(meta.getName()))).thenReturn(nonNamespaceOperation);
@@ -292,12 +297,12 @@ public void shouldReturnFirstValidToken() throws Exception {
     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)))
+    when(scmPersonalAccessTokenFetcher.isValid(any(PersonalAccessTokenParams.class)))
         .thenAnswer(
-            (Answer<Boolean>)
+            (Answer<String>)
                 invocation -> {
-                  PersonalAccessToken token = invocation.getArgument(0);
-                  return "id2".equals(token.getScmTokenId());
+                  PersonalAccessTokenParams params = invocation.getArgument(0);
+                  return "id2".equals(params.getScmTokenId()) ? "user" : null;
                 });
     when(cheServerKubernetesClientFactory.create()).thenReturn(kubeClient);
     when(kubeClient.secrets()).thenReturn(secretsMixedOperation);

diff --git a/...rg/eclipse/che/api/factory/server/azure/devops/AzureDevOpsPersonalAccessTokenFetcher.java b/...rg/eclipse/che/api/factory/server/azure/devops/AzureDevOpsPersonalAccessTokenFetcher.java
@@ -27,12 +27,14 @@
 import org.eclipse.che.api.core.UnauthorizedException;
 import org.eclipse.che.api.factory.server.scm.PersonalAccessToken;
 import org.eclipse.che.api.factory.server.scm.PersonalAccessTokenFetcher;
+import org.eclipse.che.api.factory.server.scm.PersonalAccessTokenParams;
 import org.eclipse.che.api.factory.server.scm.exception.ScmBadRequestException;
 import org.eclipse.che.api.factory.server.scm.exception.ScmCommunicationException;
 import org.eclipse.che.api.factory.server.scm.exception.ScmItemNotFoundException;
 import org.eclipse.che.api.factory.server.scm.exception.ScmUnauthorizedException;
 import org.eclipse.che.api.factory.server.scm.exception.UnknownScmProviderException;
 import org.eclipse.che.commons.lang.NameGenerator;
+import org.eclipse.che.commons.lang.Pair;
 import org.eclipse.che.commons.subject.Subject;
 import org.eclipse.che.security.oauth.OAuthAPI;
 import org.slf4j.Logger;
@@ -80,30 +82,31 @@ public PersonalAccessToken fetchPersonalAccessToken(Subject cheSubject, String s
 
     try {
       oAuthToken = oAuthAPI.getToken(AzureDevOps.PROVIDER_NAME);
-      // Find the user associated to the OAuth token by querying the Azure DevOps API.
-      AzureDevOpsUser user = azureDevOpsApiClient.getUserWithOAuthToken(oAuthToken.getToken());
-      PersonalAccessToken token =
-          new PersonalAccessToken(
-              scmServerUrl,
-              cheSubject.getUserId(),
-              user.getEmailAddress(),
-              NameGenerator.generate(OAUTH_2_PREFIX, 5),
-              NameGenerator.generate("id-", 5),
-              oAuthToken.getToken());
-      Optional<Boolean> valid = isValid(token);
+      String tokenName = NameGenerator.generate(OAUTH_2_PREFIX, 5);
+      String tokenId = NameGenerator.generate("id-", 5);
+      Optional<Pair<Boolean, String>> valid =
+          isValid(
+              new PersonalAccessTokenParams(
+                  scmServerUrl, tokenName, tokenId, oAuthToken.getToken(), null));
       if (valid.isEmpty()) {
         throw new ScmCommunicationException(
             "Unable to verify if current token is a valid Azure DevOps token.  Token's scm-url needs to be '"
                 + azureDevOpsScmApiEndpoint
                 + "' and was '"
-                + token.getScmProviderUrl()
+                + scmServerUrl
                 + "'");
-      } else if (!valid.get()) {
+      } else if (!valid.get().first) {
         throw new ScmCommunicationException(
             "Current token doesn't have the necessary privileges. Please make sure Che app scopes are correct and containing at least: "
                 + Arrays.toString(scopes));
       }
-      return token;
+      return new PersonalAccessToken(
+          scmServerUrl,
+          cheSubject.getUserId(),
+          valid.get().second,
+          tokenName,
+          tokenId,
+          oAuthToken.getToken());
     } catch (UnauthorizedException e) {
       throw new ScmUnauthorizedException(
           cheSubject.getUserName()
@@ -115,12 +118,7 @@ public PersonalAccessToken fetchPersonalAccessToken(Subject cheSubject, String s
           getLocalAuthenticateUrl());
     } catch (NotFoundException nfe) {
       throw new UnknownScmProviderException(nfe.getMessage(), scmServerUrl);
-    } catch (ServerException
-        | ForbiddenException
-        | BadRequestException
-        | ScmItemNotFoundException
-        | ScmBadRequestException
-        | ConflictException e) {
+    } catch (ServerException | ForbiddenException | BadRequestException | ConflictException e) {
       LOG.error(e.getMessage());
       throw new ScmCommunicationException(e.getMessage(), e);
     }
@@ -149,6 +147,26 @@ public Optional<Boolean> isValid(PersonalAccessToken personalAccessToken) {
     }
   }
 
+  @Override
+  public Optional<Pair<Boolean, String>> isValid(PersonalAccessTokenParams params) {
+    if (!isValidScmServerUrl(params.getScmProviderUrl())) {
+      LOG.debug("not a valid url {} for current fetcher ", params.getScmProviderUrl());
+      return Optional.empty();
+    }
+
+    try {
+      AzureDevOpsUser user;
+      if (params.getScmTokenName() != null && params.getScmTokenName().startsWith(OAUTH_2_PREFIX)) {
+        user = azureDevOpsApiClient.getUserWithOAuthToken(params.getToken());
+      } else {
+        user = azureDevOpsApiClient.getUserWithPAT(params.getToken(), params.getOrganization());
+      }
+      return Optional.of(Pair.of(Boolean.TRUE, user.getEmailAddress()));
+    } catch (ScmItemNotFoundException | ScmCommunicationException | ScmBadRequestException e) {
+      return Optional.empty();
+    }
+  }
+
   private String getLocalAuthenticateUrl() {
     return cheApiEndpoint + getAuthenticateUrlPath(scopes);
   }