From 48fd37dd34730fd562900f320f032b553a45bd49 Mon Sep 17 00:00:00 2001 From: Anatolii Bazko Date: Thu, 9 Mar 2023 16:35:56 +0200 Subject: [PATCH] fix: update the way how Azure OAuth2 token is validated Signed-off-by: Anatolii Bazko --- .../azure/devops/AzureDevOpsApiClient.java | 10 -- ...AzureDevOpsPersonalAccessTokenFetcher.java | 13 +- .../devops/AzureDevOpsURLParserTest.java | 118 ------------- ...eDevOpsPersonalAccessTokenFetcherTest.java | 53 +++--- .../devops/AzureDevOpsURLParserTest.java | 158 ++++++++++++++++++ .../test/resources/logback-test.xml | 0 6 files changed, 184 insertions(+), 168 deletions(-) delete mode 100644 wsmaster/che-core-api-factory-azure-devops/src/main/test/java/org/eclipse/che/api/factory/server/azure/devops/AzureDevOpsURLParserTest.java rename wsmaster/che-core-api-factory-azure-devops/src/{main => }/test/java/org/eclipse/che/api/factory/server/azure/devops/AzureDevOpsPersonalAccessTokenFetcherTest.java (59%) create mode 100644 wsmaster/che-core-api-factory-azure-devops/src/test/java/org/eclipse/che/api/factory/server/azure/devops/AzureDevOpsURLParserTest.java rename wsmaster/che-core-api-factory-azure-devops/src/{main => }/test/resources/logback-test.xml (100%) diff --git a/wsmaster/che-core-api-factory-azure-devops/src/main/java/org/eclipse/che/api/factory/server/azure/devops/AzureDevOpsApiClient.java b/wsmaster/che-core-api-factory-azure-devops/src/main/java/org/eclipse/che/api/factory/server/azure/devops/AzureDevOpsApiClient.java index 07db562cfe8..f47e09bd506 100644 --- a/wsmaster/che-core-api-factory-azure-devops/src/main/java/org/eclipse/che/api/factory/server/azure/devops/AzureDevOpsApiClient.java +++ b/wsmaster/che-core-api-factory-azure-devops/src/main/java/org/eclipse/che/api/factory/server/azure/devops/AzureDevOpsApiClient.java @@ -126,16 +126,6 @@ private AzureDevOpsUser getUser(String url, String authorizationHeader) }); } - /** - * Returns the scopes of the OAuth token. Consider using the REST API: - * - *

https://learn.microsoft.com/en-us/rest/api/azure/devops/tokens/pats/get?view=azure-devops-rest-7.0&tabs=HTTP - */ - public String[] getTokenScopes(String authenticationToken) - throws ScmItemNotFoundException, ScmCommunicationException, ScmBadRequestException { - return scopes; - } - private T executeRequest( HttpClient httpClient, HttpRequest request, diff --git a/wsmaster/che-core-api-factory-azure-devops/src/main/java/org/eclipse/che/api/factory/server/azure/devops/AzureDevOpsPersonalAccessTokenFetcher.java b/wsmaster/che-core-api-factory-azure-devops/src/main/java/org/eclipse/che/api/factory/server/azure/devops/AzureDevOpsPersonalAccessTokenFetcher.java index 4199bab25d8..8d19308d959 100644 --- a/wsmaster/che-core-api-factory-azure-devops/src/main/java/org/eclipse/che/api/factory/server/azure/devops/AzureDevOpsPersonalAccessTokenFetcher.java +++ b/wsmaster/che-core-api-factory-azure-devops/src/main/java/org/eclipse/che/api/factory/server/azure/devops/AzureDevOpsPersonalAccessTokenFetcher.java @@ -14,7 +14,6 @@ import static org.eclipse.che.api.factory.server.azure.devops.AzureDevOps.getAuthenticateUrlPath; import static org.eclipse.che.commons.lang.StringUtils.trimEnd; -import com.google.common.collect.Sets; import java.util.Arrays; import java.util.Optional; import javax.inject.Inject; @@ -136,20 +135,16 @@ public Optional isValid(PersonalAccessToken personalAccessToken) { } try { + AzureDevOpsUser user; if (personalAccessToken.getScmTokenName() != null && personalAccessToken.getScmTokenName().startsWith(OAUTH_2_PREFIX)) { - String[] scopes = azureDevOpsApiClient.getTokenScopes(personalAccessToken.getToken()); - return Optional.of(Sets.newHashSet(scopes).containsAll(Sets.newHashSet(this.scopes))); + user = azureDevOpsApiClient.getUserWithOAuthToken(personalAccessToken.getToken()); } else { - AzureDevOpsUser user = + user = azureDevOpsApiClient.getUserWithPAT( personalAccessToken.getToken(), personalAccessToken.getScmOrganization()); - if (personalAccessToken.getScmUserId().equals(user.getId())) { - return Optional.of(Boolean.TRUE); - } else { - return Optional.of(Boolean.FALSE); - } } + return Optional.of(personalAccessToken.getScmUserId().equals(user.getId())); } catch (ScmItemNotFoundException | ScmCommunicationException | ScmBadRequestException e) { return Optional.of(Boolean.FALSE); } diff --git a/wsmaster/che-core-api-factory-azure-devops/src/main/test/java/org/eclipse/che/api/factory/server/azure/devops/AzureDevOpsURLParserTest.java b/wsmaster/che-core-api-factory-azure-devops/src/main/test/java/org/eclipse/che/api/factory/server/azure/devops/AzureDevOpsURLParserTest.java deleted file mode 100644 index 921334073d4..00000000000 --- a/wsmaster/che-core-api-factory-azure-devops/src/main/test/java/org/eclipse/che/api/factory/server/azure/devops/AzureDevOpsURLParserTest.java +++ /dev/null @@ -1,118 +0,0 @@ -/* - * Copyright (c) 2012-2023 Red Hat, Inc. - * This program and the accompanying materials are made - * available under the terms of the Eclipse Public License 2.0 - * which is available at https://www.eclipse.org/legal/epl-2.0/ - * - * SPDX-License-Identifier: EPL-2.0 - * - * Contributors: - * Red Hat, Inc. - initial API and implementation - */ -package org.eclipse.che.api.factory.server.azure.devops; - -import org.eclipse.che.api.factory.server.urlfactory.DevfileFilenamesProvider; -import org.mockito.testng.MockitoTestNGListener; -import org.testng.annotations.BeforeMethod; -import org.testng.annotations.DataProvider; -import org.testng.annotations.Listeners; -import org.testng.annotations.Test; - -import java.util.Optional; - -import static org.mockito.Mockito.mock; -import static org.testng.Assert.assertEquals; - -/** - * @author Anatalii Bazko - */ -@Listeners(MockitoTestNGListener.class) -public class AzureDevOpsURLParserTest { - - private AzureDevOpsURLParser azureDevOpsURLParser; - - @BeforeMethod - protected void start() { - azureDevOpsURLParser = - new AzureDevOpsURLParser(mock(DevfileFilenamesProvider.class), "https://dev.azure.com/"); - } - - @Test(expectedExceptions = IllegalArgumentException.class) - public void testParseInvalidUrl(){ - azureDevOpsURLParser.parse("http://www.eclipse.org"); - } - - - @Test(dataProvider = "parsing") - public void testParse( - String url, String organization, String project, String repository, String branch, String tag) { - AzureDevOpsUrl azureDevOpsUrl = azureDevOpsURLParser.parse(url); - - assertEquals(azureDevOpsUrl.getOrganization(), organization); - assertEquals(azureDevOpsUrl.getProject(), project); - assertEquals(azureDevOpsUrl.getRepository(), repository); - assertEquals(azureDevOpsUrl.getBranch(), branch); - assertEquals(azureDevOpsUrl.getTag(), tag); - } - - @DataProvider(name = "parsing") - public Object[][] expectedParsing() { - return new Object[][]{ - {"https://MyOrg@dev.azure.com/MyOrg/MyProject/_git/MyRepo", "MyOrg", "MyProject", "MyRepo", null, null}, - {"https://MyOrg@dev.azure.com/MyOrg/MyProject/_git/MyRepo.git", "MyOrg", "MyProject", "MyRepo", null, null}, - {"https://MyOrg@dev.azure.com/MyOrg/MyProject/_git/MyRepo.dot.git", "MyOrg", "MyProject", "MyRepo.dot", null, null}, - {"https://dev.azure.com/MyOrg/MyProject/_git/MyRepo", "MyOrg", "MyProject", "MyRepo", null, null}, - {"https://dev.azure.com/MyOrg/MyProject/_git/MyRepo-with-hypen", "MyOrg", "MyProject", "MyRepo-with-hypen", null, null}, - {"https://dev.azure.com/MyOrg/MyProject/_git/-", "MyOrg", "MyProject", "-", null, null}, - {"https://dev.azure.com/MyOrg/MyProject/_git/-j.git", "MyOrg", "MyProject", "-j", null, null}, - { - "https://MyOrg@dev.azure.com/MyOrg/MyProject/_git/MyRepo?path=MyFile&version=GBmain&_a=contents", - "MyOrg", - "MyProject", - "MyRepo", - "main", - null - }, - { - "https://MyOrg@dev.azure.com/MyOrg/MyProject/_git/MyRepo?path=MyFile&version=GBmain", - "MyOrg", - "MyProject", - "MyRepo", - "main", - null - }, - { - "https://MyOrg@dev.azure.com/MyOrg/MyProject/_git/MyRepo?path=MyFile&version=GTMyTag&_a=contents", - "MyOrg", - "MyProject", - "MyRepo", - null, - "MyTag" - }, - { - "https://MyOrg@dev.azure.com/MyOrg/MyProject/_git/MyRepo?path=MyFile&version=GTMyTag", - "MyOrg", - "MyProject", - "MyRepo", - null, - "MyTag" - }, - }; - } - - @Test(dataProvider = "url") - public void testCredentials(String url, String organization, Optional credentials) { - AzureDevOpsUrl azureDevOpsUrl = azureDevOpsURLParser.parse(url); - - assertEquals(azureDevOpsUrl.getOrganization(), organization); - assertEquals(azureDevOpsUrl.getCredentials(), credentials); - } - - @DataProvider(name = "url") - public Object[][] url() { - return new Object[][]{ - {"https://MyOrg@dev.azure.com/MyOrg/MyProject/_git/MyRepo", "MyOrg", Optional.empty()}, - {"https://user:pwd@dev.azure.com/MyOrg/MyProject/_git/MyRepo", "MyOrg", Optional.of("user:pwd")}, - }; - } -} diff --git a/wsmaster/che-core-api-factory-azure-devops/src/main/test/java/org/eclipse/che/api/factory/server/azure/devops/AzureDevOpsPersonalAccessTokenFetcherTest.java b/wsmaster/che-core-api-factory-azure-devops/src/test/java/org/eclipse/che/api/factory/server/azure/devops/AzureDevOpsPersonalAccessTokenFetcherTest.java similarity index 59% rename from wsmaster/che-core-api-factory-azure-devops/src/main/test/java/org/eclipse/che/api/factory/server/azure/devops/AzureDevOpsPersonalAccessTokenFetcherTest.java rename to wsmaster/che-core-api-factory-azure-devops/src/test/java/org/eclipse/che/api/factory/server/azure/devops/AzureDevOpsPersonalAccessTokenFetcherTest.java index eccbbf544db..aac1a6e44a6 100644 --- a/wsmaster/che-core-api-factory-azure-devops/src/main/test/java/org/eclipse/che/api/factory/server/azure/devops/AzureDevOpsPersonalAccessTokenFetcherTest.java +++ b/wsmaster/che-core-api-factory-azure-devops/src/test/java/org/eclipse/che/api/factory/server/azure/devops/AzureDevOpsPersonalAccessTokenFetcherTest.java @@ -11,55 +11,44 @@ */ package org.eclipse.che.api.factory.server.azure.devops; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; +import static org.testng.Assert.*; + import org.eclipse.che.api.auth.shared.dto.OAuthToken; import org.eclipse.che.api.factory.server.scm.PersonalAccessToken; -import org.eclipse.che.api.factory.server.urlfactory.DevfileFilenamesProvider; -import org.eclipse.che.api.factory.server.urlfactory.URLFactoryBuilder; import org.eclipse.che.commons.subject.Subject; import org.eclipse.che.security.oauth.OAuthAPI; import org.mockito.Mock; import org.mockito.testng.MockitoTestNGListener; import org.testng.annotations.BeforeMethod; -import org.testng.annotations.DataProvider; import org.testng.annotations.Listeners; import org.testng.annotations.Test; -import java.util.Optional; - -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; -import static org.testng.Assert.*; - -/** - * @author Anatalii Bazko - */ +/** @author Anatalii Bazko */ @Listeners(MockitoTestNGListener.class) public class AzureDevOpsPersonalAccessTokenFetcherTest { - @Mock - private AzureDevOpsApiClient azureDevOpsApiClient; - @Mock - private OAuthAPI oAuthAPI; - @Mock - private OAuthToken oAuthToken; - @Mock - private AzureDevOpsUser azureDevOpsUser; + @Mock private AzureDevOpsApiClient azureDevOpsApiClient; + @Mock private OAuthAPI oAuthAPI; + @Mock private OAuthToken oAuthToken; + @Mock private AzureDevOpsUser azureDevOpsUser; private AzureDevOpsPersonalAccessTokenFetcher personalAccessTokenFetcher; @BeforeMethod protected void start() { - personalAccessTokenFetcher = new AzureDevOpsPersonalAccessTokenFetcher( - "localhost", - "https://dev.azure.com", - new String[]{}, - azureDevOpsApiClient, - oAuthAPI); + personalAccessTokenFetcher = + new AzureDevOpsPersonalAccessTokenFetcher( + "localhost", "https://dev.azure.com", new String[] {}, azureDevOpsApiClient, oAuthAPI); } @Test - public void fetchPersonalAccessTokenShouldReturnNullIfScmServerUrlIsNotAzureDevOps() throws Exception { - PersonalAccessToken personalAccessToken = personalAccessTokenFetcher.fetchPersonalAccessToken(mock(Subject.class), "https://eclipse.org"); + public void fetchPersonalAccessTokenShouldReturnNullIfScmServerUrlIsNotAzureDevOps() + throws Exception { + PersonalAccessToken personalAccessToken = + personalAccessTokenFetcher.fetchPersonalAccessToken( + mock(Subject.class), "https://eclipse.org"); assertNull(personalAccessToken); } @@ -68,9 +57,11 @@ public void fetchPersonalAccessTokenShouldReturnNullIfScmServerUrlIsNotAzureDevO public void fetchPersonalAccessTokenShouldReturnToken() throws Exception { when(oAuthAPI.getToken(AzureDevOps.PROVIDER_NAME)).thenReturn(oAuthToken); when(azureDevOpsApiClient.getUserWithOAuthToken(any())).thenReturn(azureDevOpsUser); - when(azureDevOpsApiClient.getTokenScopes(any())).thenReturn(new String[]{"vso.code_full"}); + when(azureDevOpsUser.getId()).thenReturn("user-id"); - PersonalAccessToken personalAccessToken = personalAccessTokenFetcher.fetchPersonalAccessToken(mock(Subject.class), "https://dev.azure.com/"); + PersonalAccessToken personalAccessToken = + personalAccessTokenFetcher.fetchPersonalAccessToken( + mock(Subject.class), "https://dev.azure.com/"); assertNotNull(personalAccessToken); } diff --git a/wsmaster/che-core-api-factory-azure-devops/src/test/java/org/eclipse/che/api/factory/server/azure/devops/AzureDevOpsURLParserTest.java b/wsmaster/che-core-api-factory-azure-devops/src/test/java/org/eclipse/che/api/factory/server/azure/devops/AzureDevOpsURLParserTest.java new file mode 100644 index 00000000000..9a631c95655 --- /dev/null +++ b/wsmaster/che-core-api-factory-azure-devops/src/test/java/org/eclipse/che/api/factory/server/azure/devops/AzureDevOpsURLParserTest.java @@ -0,0 +1,158 @@ +/* + * Copyright (c) 2012-2023 Red Hat, Inc. + * This program and the accompanying materials are made + * available under the terms of the Eclipse Public License 2.0 + * which is available at https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + * + * Contributors: + * Red Hat, Inc. - initial API and implementation + */ +package org.eclipse.che.api.factory.server.azure.devops; + +import static org.mockito.Mockito.mock; +import static org.testng.Assert.assertEquals; + +import java.util.Optional; +import org.eclipse.che.api.factory.server.urlfactory.DevfileFilenamesProvider; +import org.mockito.testng.MockitoTestNGListener; +import org.testng.annotations.BeforeMethod; +import org.testng.annotations.DataProvider; +import org.testng.annotations.Listeners; +import org.testng.annotations.Test; + +/** @author Anatalii Bazko */ +@Listeners(MockitoTestNGListener.class) +public class AzureDevOpsURLParserTest { + + private AzureDevOpsURLParser azureDevOpsURLParser; + + @BeforeMethod + protected void start() { + azureDevOpsURLParser = + new AzureDevOpsURLParser(mock(DevfileFilenamesProvider.class), "https://dev.azure.com/"); + } + + @Test(expectedExceptions = IllegalArgumentException.class) + public void testParseInvalidUrl() { + azureDevOpsURLParser.parse("http://www.eclipse.org"); + } + + @Test(dataProvider = "parsing") + public void testParse( + String url, + String organization, + String project, + String repository, + String branch, + String tag) { + AzureDevOpsUrl azureDevOpsUrl = azureDevOpsURLParser.parse(url); + + assertEquals(azureDevOpsUrl.getOrganization(), organization); + assertEquals(azureDevOpsUrl.getProject(), project); + assertEquals(azureDevOpsUrl.getRepository(), repository); + assertEquals(azureDevOpsUrl.getBranch(), branch); + assertEquals(azureDevOpsUrl.getTag(), tag); + } + + @DataProvider(name = "parsing") + public Object[][] expectedParsing() { + return new Object[][] { + { + "https://MyOrg@dev.azure.com/MyOrg/MyProject/_git/MyRepo", + "MyOrg", + "MyProject", + "MyRepo", + null, + null + }, + { + "https://MyOrg@dev.azure.com/MyOrg/MyProject/_git/MyRepo.git", + "MyOrg", + "MyProject", + "MyRepo", + null, + null + }, + { + "https://MyOrg@dev.azure.com/MyOrg/MyProject/_git/MyRepo.dot.git", + "MyOrg", + "MyProject", + "MyRepo.dot", + null, + null + }, + { + "https://dev.azure.com/MyOrg/MyProject/_git/MyRepo", + "MyOrg", + "MyProject", + "MyRepo", + null, + null + }, + { + "https://dev.azure.com/MyOrg/MyProject/_git/MyRepo-with-hypen", + "MyOrg", + "MyProject", + "MyRepo-with-hypen", + null, + null + }, + {"https://dev.azure.com/MyOrg/MyProject/_git/-", "MyOrg", "MyProject", "-", null, null}, + {"https://dev.azure.com/MyOrg/MyProject/_git/-j.git", "MyOrg", "MyProject", "-j", null, null}, + { + "https://MyOrg@dev.azure.com/MyOrg/MyProject/_git/MyRepo?path=MyFile&version=GBmain&_a=contents", + "MyOrg", + "MyProject", + "MyRepo", + "main", + null + }, + { + "https://MyOrg@dev.azure.com/MyOrg/MyProject/_git/MyRepo?path=MyFile&version=GBmain", + "MyOrg", + "MyProject", + "MyRepo", + "main", + null + }, + { + "https://MyOrg@dev.azure.com/MyOrg/MyProject/_git/MyRepo?path=MyFile&version=GTMyTag&_a=contents", + "MyOrg", + "MyProject", + "MyRepo", + null, + "MyTag" + }, + { + "https://MyOrg@dev.azure.com/MyOrg/MyProject/_git/MyRepo?path=MyFile&version=GTMyTag", + "MyOrg", + "MyProject", + "MyRepo", + null, + "MyTag" + }, + }; + } + + @Test(dataProvider = "url") + public void testCredentials(String url, String organization, Optional credentials) { + AzureDevOpsUrl azureDevOpsUrl = azureDevOpsURLParser.parse(url); + + assertEquals(azureDevOpsUrl.getOrganization(), organization); + assertEquals(azureDevOpsUrl.getCredentials(), credentials); + } + + @DataProvider(name = "url") + public Object[][] url() { + return new Object[][] { + {"https://MyOrg@dev.azure.com/MyOrg/MyProject/_git/MyRepo", "MyOrg", Optional.empty()}, + { + "https://user:pwd@dev.azure.com/MyOrg/MyProject/_git/MyRepo", + "MyOrg", + Optional.of("user:pwd") + }, + }; + } +} diff --git a/wsmaster/che-core-api-factory-azure-devops/src/main/test/resources/logback-test.xml b/wsmaster/che-core-api-factory-azure-devops/src/test/resources/logback-test.xml similarity index 100% rename from wsmaster/che-core-api-factory-azure-devops/src/main/test/resources/logback-test.xml rename to wsmaster/che-core-api-factory-azure-devops/src/test/resources/logback-test.xml