Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Rwork the token cleanup mechanism #719

Merged
merged 1 commit into from
Sep 10, 2024
Merged

Rwork the token cleanup mechanism #719

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 40 additions & 30 deletions ...g/eclipse/che/api/factory/server/scm/kubernetes/KubernetesPersonalAccessTokenManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,19 +188,7 @@ private List<PersonalAccessToken> doGetPersonalAccessTokens(
cheUser.getUserId(),
oAuthProviderName);
for (KubernetesNamespaceMeta namespaceMeta : namespaceFactory.list()) {
List<Secret> secrets =
namespaceFactory
.access(null, namespaceMeta.getName())
.secrets()
.get(KUBERNETES_PERSONAL_ACCESS_TOKEN_LABEL_SELECTOR);

// sort secrets to get the newest one first
// Assign to new list to avoid UnsupportedOperationException (ImmutableList)
secrets =
secrets.stream()
.sorted(Comparator.comparing(secret -> secret.getMetadata().getCreationTimestamp()))
.collect(Collectors.toList());
Collections.reverse(secrets);
List<Secret> secrets = doGetPersonalAccessTokenSecrets(namespaceMeta);

for (Secret secret : secrets) {
LOG.debug("Checking secret {}", secret.getMetadata().getName());
Expand Down Expand Up @@ -257,6 +245,29 @@ private List<PersonalAccessToken> doGetPersonalAccessTokens(
return result;
}

private List<Secret> doGetPersonalAccessTokenSecrets(KubernetesNamespaceMeta namespaceMeta)
throws ScmConfigurationPersistenceException {
try {
List<Secret> secrets =
namespaceFactory
.access(null, namespaceMeta.getName())
.secrets()
.get(KUBERNETES_PERSONAL_ACCESS_TOKEN_LABEL_SELECTOR);

// sort secrets to get the newest one first
// Assign to new list to avoid UnsupportedOperationException (ImmutableList)
secrets =
secrets.stream()
.sorted(Comparator.comparing(secret -> secret.getMetadata().getCreationTimestamp()))
.collect(Collectors.toList());
Collections.reverse(secrets);
return secrets;
} catch (InfrastructureException e) {
LOG.debug("Failed to get personal access token secret", e);
throw new ScmConfigurationPersistenceException(e.getMessage(), e);
}
}

/**
* Returns the name of the SCM provider. If the name is not set, the name of the token is used.
* This is used to support back compatibility with the old token secrets, which do not have the
Expand Down Expand Up @@ -354,28 +365,27 @@ public void forceRefreshPersonalAccessToken(String scmServerUrl)
scmPersonalAccessTokenFetcher.refreshPersonalAccessToken(subject, scmServerUrl);
gitCredentialManager.createOrReplace(personalAccessToken);
store(personalAccessToken);
removePreviousTokensIfPresent(subject, scmServerUrl);
removePreviousTokenSecretsIfPresent(scmServerUrl);
}

private void removePreviousTokensIfPresent(Subject subject, String scmServerUrl)
private void removePreviousTokenSecretsIfPresent(String scmServerUrl)
throws ScmConfigurationPersistenceException, UnsatisfiedScmPreconditionException {
List<PersonalAccessToken> personalAccessTokens =
doGetPersonalAccessTokens(subject, null, scmServerUrl);
for (int i = 1; i < personalAccessTokens.size(); i++) {
PersonalAccessToken token = personalAccessTokens.get(i);
if (token.getScmProviderUrl().equals(scmServerUrl)) {
try {
String namespace = getFirstNamespace();
cheServerKubernetesClientFactory
.create()
.secrets()
.inNamespace(namespace)
.withName(token.getScmTokenId())
.delete();
} catch (InfrastructureException e) {
throw new ScmConfigurationPersistenceException(e.getMessage(), e);
try {
for (KubernetesNamespaceMeta namespaceMeta : namespaceFactory.list()) {
List<Secret> secrets = doGetPersonalAccessTokenSecrets(namespaceMeta);
for (int i = 1; i < secrets.size(); i++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we remove all secrets or keep the last one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to keep the last one

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems we keep the very first one, should it be the last one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I mixed up, according to https://github.com/eclipse-che/che-server/pull/719/files#diff-c0fa2851d5bdc5b65a29d8ac5d7d71d4a961b03f526f43c50027e82b7948f775R257 we need to keep the first one - the newest.

Secret secret = secrets.get(i);
if (secret.getMetadata().getAnnotations().get(ANNOTATION_SCM_URL).equals(scmServerUrl)) {
cheServerKubernetesClientFactory
.create()
.secrets()
.inNamespace(getFirstNamespace())
.delete(secret);
}
}
}
} catch (InfrastructureException e) {
throw new ScmConfigurationPersistenceException(e.getMessage(), e);
}
}

Expand Down
66 changes: 65 additions & 1 deletion ...lipse/che/api/factory/server/scm/kubernetes/KubernetesPersonalAccessTokenManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -464,7 +464,7 @@ public void shouldDeleteInvalidTokensOnGet() throws Exception {
}

@Test(dependsOnMethods = "shouldDeleteInvalidTokensOnGet")
public void shouldReturnFirstValidTokenAndDeleteTheOlderOne() throws Exception {
public void shouldReturnFirstValidTokenAndDeleteTheInvalidOne() throws Exception {
// given
KubernetesNamespaceMeta meta = new KubernetesNamespaceMetaImpl("test");
when(namespaceFactory.list()).thenReturn(singletonList(meta));
Expand Down Expand Up @@ -529,4 +529,68 @@ public void shouldReturnFirstValidTokenAndDeleteTheOlderOne() throws Exception {
assertEquals(token.get().getScmTokenId(), "id2");
verify(nonNamespaceOperation, times(1)).delete(eq(secret1));
}

@Test
public void shouldReturnFirstValidTokenAndDeleteTheOlderOne() throws Exception {
// given
KubernetesNamespaceMeta meta = new KubernetesNamespaceMetaImpl("test");
when(namespaceFactory.list()).thenReturn(singletonList(meta));
KubernetesNamespace kubernetesnamespace = Mockito.mock(KubernetesNamespace.class);
KubernetesSecrets secrets = Mockito.mock(KubernetesSecrets.class);
when(kubernetesnamespace.secrets()).thenReturn(secrets);
when(cheServerKubernetesClientFactory.create()).thenReturn(kubeClient);
when(kubeClient.secrets()).thenReturn(secretsMixedOperation);
Map<String, String> data1 =
Map.of("token", Base64.getEncoder().encodeToString("token1".getBytes(UTF_8)));
Map<String, String> data2 =
Map.of("token", Base64.getEncoder().encodeToString("token2".getBytes(UTF_8)));
ObjectMeta meta1 =
new ObjectMetaBuilder()
.withCreationTimestamp("2021-07-01T12:00:00Z")
.withAnnotations(
Map.of(
ANNOTATION_SCM_PERSONAL_ACCESS_TOKEN_NAME,
"github",
ANNOTATION_CHE_USERID,
"user1",
ANNOTATION_SCM_URL,
"http://host1",
ANNOTATION_SCM_PERSONAL_ACCESS_TOKEN_ID,
"id1"))
.build();
ObjectMeta meta2 =
new ObjectMetaBuilder()
.withCreationTimestamp("2021-07-02T12:00:00Z")
.withAnnotations(
Map.of(
ANNOTATION_SCM_PERSONAL_ACCESS_TOKEN_NAME,
"github",
ANNOTATION_CHE_USERID,
"user1",
ANNOTATION_SCM_URL,
"http://host1",
ANNOTATION_SCM_PERSONAL_ACCESS_TOKEN_ID,
"id2"))
.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(namespaceFactory.access(eq(null), eq(meta.getName()))).thenReturn(kubernetesnamespace);
when(cheServerKubernetesClientFactory.create()).thenReturn(kubeClient);
when(kubeClient.secrets()).thenReturn(secretsMixedOperation);
when(secretsMixedOperation.inNamespace(eq(meta.getName()))).thenReturn(nonNamespaceOperation);
when(kubernetesnamespace.secrets()).thenReturn(secrets);
PersonalAccessToken token =
new PersonalAccessToken(
"http://host1", "provider", "cheUser", "username", "token-name", "tid-24", "token123");
when(scmPersonalAccessTokenFetcher.refreshPersonalAccessToken(
any(org.eclipse.che.commons.subject.Subject.class), eq("http://host1")))
.thenReturn(token);

// when
personalAccessTokenManager.forceRefreshPersonalAccessToken("http://host1");

// then
verify(nonNamespaceOperation, times(1)).delete(eq(secret1));
}
}
Loading