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

[DO NOT MERGE] Delete personal access tokens without provider name annotation #685

Closed
wants to merge 1 commit into from
Closed

[DO NOT MERGE] Delete personal access tokens without provider name annotation #685

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
59 changes: 40 additions & 19 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 @@ -71,8 +71,14 @@ public class KubernetesPersonalAccessTokenManager implements PersonalAccessToken
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";
public static final String ANNOTATION_SCM_PERSONAL_ACCESS_TOKEN_IS_OAUTH =
"che.eclipse.org/scm-personal-access-token-is-oauth";

@Deprecated
// This annotation is deprecated and will be removed in the future.
public static final String ANNOTATION_SCM_PERSONAL_ACCESS_TOKEN_NAME =
"che.eclipse.org/scm-personal-access-token-name";

public static final String ANNOTATION_SCM_URL = "che.eclipse.org/scm-url";
public static final String TOKEN_DATA_FIELD = "token";

Expand Down Expand Up @@ -112,9 +118,6 @@ public void store(PersonalAccessToken personalAccessToken)
.put(
ANNOTATION_SCM_PERSONAL_ACCESS_TOKEN_ID,
personalAccessToken.getScmTokenId())
.put(
ANNOTATION_SCM_PERSONAL_ACCESS_TOKEN_NAME,
personalAccessToken.getScmTokenName())
.build())
.withLabels(SECRET_LABELS)
.build();
Expand Down Expand Up @@ -214,7 +217,9 @@ private List<PersonalAccessToken> doGetPersonalAccessTokens(
PersonalAccessToken personalAccessToken =
new PersonalAccessToken(
personalAccessTokenParams.getScmProviderUrl(),
getScmProviderName(personalAccessTokenParams),
getScmProviderName(
personalAccessTokenParams.getScmProviderName(),
personalAccessTokenParams.getScmTokenName()),
secretAnnotations.get(ANNOTATION_CHE_USERID),
personalAccessTokenParams.getOrganization(),
scmUsername.get(),
Expand Down Expand Up @@ -273,13 +278,12 @@ private List<Secret> doGetPersonalAccessTokenSecrets(KubernetesNamespaceMeta nam
* This is used to support back compatibility with the old token secrets, which do not have the
* 'che.eclipse.org/scm-provider-name' annotation.
*
* @param params the parameters of the personal access token
* @param providerName the name of the SCM provider
* @param tokenName the name of the token
* @return the name of the SCM provider
*/
private String getScmProviderName(PersonalAccessTokenParams params) {
return isNullOrEmpty(params.getScmProviderName())
? params.getScmTokenName()
: params.getScmProviderName();
private String getScmProviderName(@Nullable String providerName, String tokenName) {
return isNullOrEmpty(providerName) ? tokenName : providerName;
}

private boolean deleteSecretIfMisconfigured(Secret secret) throws InfrastructureException {
Expand All @@ -289,8 +293,8 @@ private boolean deleteSecretIfMisconfigured(Secret secret) throws Infrastructure
LOG.debug("SCM server URL: {}", configuredScmServerUrl);
String configuredCheUserId = secretAnnotations.get(ANNOTATION_CHE_USERID);
LOG.debug("Che user ID: {}", configuredCheUserId);
String configuredOAuthProviderName =
secretAnnotations.get(ANNOTATION_SCM_PERSONAL_ACCESS_TOKEN_NAME);
String providerName = secretAnnotations.get(ANNOTATION_SCM_PROVIDER_NAME);
String configuredOAuthProviderName = getScmProviderName(providerName, getTokenName(secret));
LOG.debug("OAuth provider name: {}", configuredOAuthProviderName);

// if any of the required annotations is missing, the secret is not valid
Expand All @@ -309,24 +313,42 @@ private boolean deleteSecretIfMisconfigured(Secret secret) throws Infrastructure
return false;
}

/**
* Returns the token name. If the token name is not set, the name of the secret in format:
* personal-access-token-<token-name> is used. This is used to support back compatibility with the
* old token secrets, which do not have the 'che.eclipse.org/scm-provider-name' annotation, but
* have the deprecated 'che.eclipse.org/scm-personal-access-token-name' annotation.
*
* @param secret the secret
* @return the token name
*/
private String getTokenName(Secret secret) {
String tokenName =
secret.getMetadata().getAnnotations().get(ANNOTATION_SCM_PERSONAL_ACCESS_TOKEN_NAME);
String secretName = secret.getMetadata().getName();
return isNullOrEmpty(tokenName)
? secretName.substring(secretName.lastIndexOf("-") + 1)
: tokenName;
}

private PersonalAccessTokenParams secret2PersonalAccessTokenParams(Secret secret) {
Map<String, String> secretAnnotations = secret.getMetadata().getAnnotations();

String token = new String(Base64.getDecoder().decode(secret.getData().get("token"))).trim();
String configuredOAuthTokenName =
secretAnnotations.get(ANNOTATION_SCM_PERSONAL_ACCESS_TOKEN_NAME);
String configuredTokenId = secretAnnotations.get(ANNOTATION_SCM_PERSONAL_ACCESS_TOKEN_ID);
String configuredScmOrganization = secretAnnotations.get(ANNOTATION_SCM_ORGANIZATION);
String configuredScmServerUrl = secretAnnotations.get(ANNOTATION_SCM_URL);
String configuredScmProviderName = secretAnnotations.get(ANNOTATION_SCM_PROVIDER_NAME);
String configuredIsOauth = secretAnnotations.get(ANNOTATION_SCM_PERSONAL_ACCESS_TOKEN_IS_OAUTH);

return new PersonalAccessTokenParams(
trimEnd(configuredScmServerUrl, '/'),
configuredScmProviderName,
configuredOAuthTokenName,
getTokenName(secret),
configuredTokenId,
token,
configuredScmOrganization);
configuredScmOrganization,
Boolean.parseBoolean(configuredIsOauth));
}

private boolean isSecretMatchesSearchCriteria(
Expand All @@ -337,8 +359,8 @@ private boolean isSecretMatchesSearchCriteria(
Map<String, String> secretAnnotations = secret.getMetadata().getAnnotations();
String configuredScmServerUrl = secretAnnotations.get(ANNOTATION_SCM_URL);
String configuredCheUserId = secretAnnotations.get(ANNOTATION_CHE_USERID);
String configuredOAuthProviderName =
secretAnnotations.get(ANNOTATION_SCM_PERSONAL_ACCESS_TOKEN_NAME);
String providerName = secretAnnotations.get(ANNOTATION_SCM_PROVIDER_NAME);
String configuredOAuthProviderName = getScmProviderName(providerName, getTokenName(secret));

return (configuredCheUserId.equals(cheUser.getUserId()))
&& (oAuthProviderName == null || oAuthProviderName.equals(configuredOAuthProviderName))
Expand Down Expand Up @@ -391,8 +413,7 @@ private void removePreviousTokenSecretsIfPresent(String scmServerUrl)

@Override
public void storeGitCredentials(String scmServerUrl)
throws UnsatisfiedScmPreconditionException, ScmConfigurationPersistenceException,
ScmCommunicationException, ScmUnauthorizedException {
throws UnsatisfiedScmPreconditionException, ScmConfigurationPersistenceException {
Subject subject = EnvironmentContext.getCurrent().getSubject();
Optional<PersonalAccessToken> tokenOptional = get(subject, scmServerUrl);
if (tokenOptional.isPresent()) {
Expand Down
84 changes: 69 additions & 15 deletions ...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 @@ -102,10 +102,11 @@ public void shouldTrimBlankCharsInToken() throws Exception {

ObjectMeta meta1 =
new ObjectMetaBuilder()
.withName(NAME_PATTERN + "token_name")
.withCreationTimestamp("2021-07-01T12:00:00Z")
.withAnnotations(
Map.of(
ANNOTATION_SCM_PERSONAL_ACCESS_TOKEN_NAME,
ANNOTATION_SCM_PROVIDER_NAME,
"github",
ANNOTATION_CHE_USERID,
"user",
Expand Down Expand Up @@ -186,10 +187,11 @@ public void testGetTokenFromNamespace() throws Exception {

ObjectMeta meta1 =
new ObjectMetaBuilder()
.withName(NAME_PATTERN + "token_name_1")
.withCreationTimestamp("2021-07-01T12:00:00Z")
.withAnnotations(
Map.of(
ANNOTATION_SCM_PERSONAL_ACCESS_TOKEN_NAME,
ANNOTATION_SCM_PROVIDER_NAME,
"github",
ANNOTATION_CHE_USERID,
"user1",
Expand All @@ -198,10 +200,11 @@ public void testGetTokenFromNamespace() throws Exception {
.build();
ObjectMeta meta2 =
new ObjectMetaBuilder()
.withName(NAME_PATTERN + "token_name_2")
.withCreationTimestamp("2021-07-02T12:00:00Z")
.withAnnotations(
Map.of(
ANNOTATION_SCM_PERSONAL_ACCESS_TOKEN_NAME,
ANNOTATION_SCM_PROVIDER_NAME,
"github",
ANNOTATION_CHE_USERID,
"user1",
Expand All @@ -210,10 +213,11 @@ public void testGetTokenFromNamespace() throws Exception {
.build();
ObjectMeta meta3 =
new ObjectMetaBuilder()
.withName(NAME_PATTERN + "token_name_3")
.withCreationTimestamp("2021-07-03T12:00:00Z")
.withAnnotations(
Map.of(
ANNOTATION_SCM_PERSONAL_ACCESS_TOKEN_NAME,
ANNOTATION_SCM_PROVIDER_NAME,
"github",
ANNOTATION_CHE_USERID,
"user2",
Expand Down Expand Up @@ -257,9 +261,10 @@ public void shouldGetTokenFromASecretWithSCMUsername() throws Exception {

ObjectMeta metaData =
new ObjectMetaBuilder()
.withName(NAME_PATTERN + "token_name")
.withAnnotations(
Map.of(
ANNOTATION_SCM_PERSONAL_ACCESS_TOKEN_NAME,
ANNOTATION_SCM_PROVIDER_NAME,
"github",
ANNOTATION_CHE_USERID,
"user1",
Expand Down Expand Up @@ -302,6 +307,53 @@ public void shouldGetTokenFromASecretWithoutSCMUsername() throws Exception {

ObjectMeta metaData =
new ObjectMetaBuilder()
.withName(NAME_PATTERN + "token_name")
.withAnnotations(
Map.of(
ANNOTATION_SCM_PROVIDER_NAME,
"github",
ANNOTATION_CHE_USERID,
"user1",
ANNOTATION_SCM_URL,
"http://host1"))
.build();

Secret secret = new SecretBuilder().withMetadata(metaData).withData(data).build();

when(secrets.get(any(LabelSelector.class))).thenReturn(singletonList(secret));

// when
Optional<PersonalAccessToken> tokenOptional =
personalAccessTokenManager.get(
new SubjectImpl("user", "user1", "t1", false), "http://host1");

// then
assertTrue(tokenOptional.isPresent());
assertEquals(tokenOptional.get().getCheUserId(), "user1");
assertEquals(tokenOptional.get().getScmProviderUrl(), "http://host1");
assertEquals(tokenOptional.get().getToken(), "token1");
}

// TODO remove this test after removing the deprecated scm-personal-access-token-name annotation
@Test
public void shouldGetTokenFromASecretWithTokenNameButWithoutProviderAnnotation()
throws Exception {

KubernetesNamespaceMeta meta = new KubernetesNamespaceMetaImpl("test");
when(namespaceFactory.list()).thenReturn(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.getScmUsername(any(PersonalAccessTokenParams.class)))
.thenReturn(Optional.of("user"));

Map<String, String> data =
Map.of("token", Base64.getEncoder().encodeToString("token1".getBytes(UTF_8)));

ObjectMeta metaData =
new ObjectMetaBuilder()
.withName(NAME_PATTERN + "token_name")
.withAnnotations(
Map.of(
ANNOTATION_SCM_PERSONAL_ACCESS_TOKEN_NAME,
Expand Down Expand Up @@ -347,10 +399,11 @@ public void testGetTokenFromNamespaceWithTrailingSlashMismatch() throws Exceptio

ObjectMeta meta1 =
new ObjectMetaBuilder()
.withName(NAME_PATTERN + "token_name_1")
.withCreationTimestamp("2021-07-01T12:00:00Z")
.withAnnotations(
Map.of(
ANNOTATION_SCM_PERSONAL_ACCESS_TOKEN_NAME,
ANNOTATION_SCM_PROVIDER_NAME,
"github",
ANNOTATION_CHE_USERID,
"user1",
Expand All @@ -359,10 +412,11 @@ public void testGetTokenFromNamespaceWithTrailingSlashMismatch() throws Exceptio
.build();
ObjectMeta meta2 =
new ObjectMetaBuilder()
.withName(NAME_PATTERN + "token_name_2")
.withCreationTimestamp("2021-08-01T12:00:00Z")
.withAnnotations(
Map.of(
ANNOTATION_SCM_PERSONAL_ACCESS_TOKEN_NAME,
ANNOTATION_SCM_PROVIDER_NAME,
"github",
ANNOTATION_CHE_USERID,
"user1",
Expand Down Expand Up @@ -406,13 +460,10 @@ public void shouldDeleteMisconfiguredTokensOnGet() throws Exception {
Map.of("token", Base64.getEncoder().encodeToString("token1".getBytes(UTF_8)));
ObjectMeta meta1 =
new ObjectMetaBuilder()
.withName(NAME_PATTERN + "token_name")
.withNamespace("test")
.withAnnotations(
Map.of(
ANNOTATION_SCM_PERSONAL_ACCESS_TOKEN_NAME,
"github",
ANNOTATION_CHE_USERID,
"user1"))
Map.of(ANNOTATION_SCM_PROVIDER_NAME, "github", ANNOTATION_CHE_USERID, "user1"))
.build();
Secret secret1 = new SecretBuilder().withMetadata(meta1).withData(data1).build();
when(secrets.get(any(LabelSelector.class))).thenReturn(Arrays.asList(secret1));
Expand Down Expand Up @@ -443,9 +494,10 @@ public void shouldDeleteInvalidTokensOnGet() throws Exception {
Map.of("token", Base64.getEncoder().encodeToString("token1".getBytes(UTF_8)));
ObjectMeta meta1 =
new ObjectMetaBuilder()
.withName(NAME_PATTERN + "token_name")
.withAnnotations(
Map.of(
ANNOTATION_SCM_PERSONAL_ACCESS_TOKEN_NAME,
ANNOTATION_SCM_PROVIDER_NAME,
"github",
ANNOTATION_CHE_USERID,
"user1",
Expand Down Expand Up @@ -491,10 +543,11 @@ public void shouldReturnFirstValidTokenAndDeleteTheInvalidOne() throws Exception
Map.of("token", Base64.getEncoder().encodeToString("token2".getBytes(UTF_8)));
ObjectMeta meta1 =
new ObjectMetaBuilder()
.withName(NAME_PATTERN + "token_name_1")
.withCreationTimestamp("2021-07-01T12:00:00Z")
.withAnnotations(
Map.of(
ANNOTATION_SCM_PERSONAL_ACCESS_TOKEN_NAME,
ANNOTATION_SCM_PROVIDER_NAME,
"github",
ANNOTATION_CHE_USERID,
"user1",
Expand All @@ -505,10 +558,11 @@ public void shouldReturnFirstValidTokenAndDeleteTheInvalidOne() throws Exception
.build();
ObjectMeta meta2 =
new ObjectMetaBuilder()
.withName(NAME_PATTERN + "token_name_2")
.withCreationTimestamp("2021-07-02T12:00:00Z")
.withAnnotations(
Map.of(
ANNOTATION_SCM_PERSONAL_ACCESS_TOKEN_NAME,
ANNOTATION_SCM_PROVIDER_NAME,
"github",
ANNOTATION_CHE_USERID,
"user1",
Expand Down
3 changes: 2 additions & 1 deletion ...rg/eclipse/che/api/factory/server/azure/devops/AzureDevOpsPersonalAccessTokenFetcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,8 @@ private PersonalAccessToken fetchOrRefreshPersonalAccessToken(
tokenName,
tokenId,
oAuthToken.getToken(),
null));
null,
true));
if (valid.isEmpty()) {
throw buildScmUnauthorizedException(cheSubject);
} else if (!valid.get().first) {
Expand Down
3 changes: 2 additions & 1 deletion ...ava/org/eclipse/che/api/factory/server/bitbucket/BitbucketPersonalAccessTokenFetcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,8 @@ private PersonalAccessToken fetchOrRefreshPersonalAccessToken(
tokenName,
tokenId,
oAuthToken.getToken(),
null));
null,
true));
if (valid.isEmpty()) {
throw buildScmUnauthorizedException(cheSubject);
} else if (!valid.get().first) {
Expand Down
12 changes: 8 additions & 4 deletions ...org/eclipse/che/api/factory/server/bitbucket/BitbucketPersonalAccessTokenFetcherTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,8 @@ public void shouldNotValidateSCMServerWithTrailingSlash() throws Exception {
"scmTokenName",
"scmTokenId",
bitbucketOauthToken,
null);
null,
true);
assertTrue(
bitbucketPersonalAccessTokenFetcher.isValid(personalAccessTokenParams).isEmpty(),
"Should not validate SCM server with trailing /");
Expand Down Expand Up @@ -175,7 +176,8 @@ public void shouldValidatePersonalToken() throws Exception {
"params-name",
"tid-23434",
bitbucketOauthToken,
null);
null,
true);

Optional<Pair<Boolean, String>> valid = bitbucketPersonalAccessTokenFetcher.isValid(params);
assertTrue(valid.isPresent());
Expand All @@ -202,7 +204,8 @@ public void shouldValidateOauthToken() throws Exception {
OAUTH_2_PREFIX + "-params-name",
"tid-23434",
bitbucketOauthToken,
null);
null,
true);

Optional<Pair<Boolean, String>> valid = bitbucketPersonalAccessTokenFetcher.isValid(params);
assertTrue(valid.isPresent());
Expand All @@ -220,7 +223,8 @@ public void shouldNotValidateExpiredOauthToken() throws Exception {
OAUTH_2_PREFIX + "-token-name",
"tid-23434",
bitbucketOauthToken,
null);
null,
true);

assertFalse(bitbucketPersonalAccessTokenFetcher.isValid(params).isPresent());
}
Expand Down
Loading
Loading