-
Notifications
You must be signed in to change notification settings - Fork 70
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.
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
Support Basic authentication for devfile factory URL #451
Changes from all commits
5d86582
419ec92
2745230
81cceea
25474b5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,10 +11,13 @@ | |
*/ | ||
package org.eclipse.che.api.factory.server.scm; | ||
|
||
import static com.google.common.base.Strings.isNullOrEmpty; | ||
|
||
import java.io.FileNotFoundException; | ||
import java.io.IOException; | ||
import java.net.URI; | ||
import java.net.URISyntaxException; | ||
import java.util.Base64; | ||
import javax.net.ssl.SSLException; | ||
import org.eclipse.che.api.factory.server.scm.exception.ScmCommunicationException; | ||
import org.eclipse.che.api.factory.server.scm.exception.ScmConfigurationPersistenceException; | ||
|
@@ -25,6 +28,7 @@ | |
import org.eclipse.che.api.workspace.server.devfile.FileContentProvider; | ||
import org.eclipse.che.api.workspace.server.devfile.URLFetcher; | ||
import org.eclipse.che.api.workspace.server.devfile.exception.DevfileException; | ||
import org.eclipse.che.commons.annotation.Nullable; | ||
|
||
/** | ||
* Common implementation of file content provider which is able to access content of private | ||
|
@@ -48,26 +52,41 @@ public AuthorizingFileContentProvider( | |
|
||
@Override | ||
public String fetchContent(String fileURL) throws IOException, DevfileException { | ||
return fetchContent(fileURL, false); | ||
return fetchContent(fileURL, false, null); | ||
} | ||
|
||
@Override | ||
public String fetchContent(String fileURL, String credentials) | ||
throws IOException, DevfileException { | ||
return fetchContent(fileURL, false, credentials); | ||
} | ||
|
||
@Override | ||
public String fetchContentWithoutAuthentication(String fileURL) | ||
throws IOException, DevfileException { | ||
return fetchContent(fileURL, true); | ||
return fetchContent(fileURL, true, null); | ||
} | ||
|
||
protected String fetchContent(String fileURL, boolean skipAuthentication) | ||
private String fetchContent( | ||
String fileURL, boolean skipAuthentication, @Nullable String credentials) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would be nice to use Optional here also There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think that passing |
||
throws IOException, DevfileException { | ||
final String requestURL = formatUrl(fileURL); | ||
try { | ||
if (skipAuthentication) { | ||
return urlFetcher.fetch(requestURL); | ||
} else { | ||
// try to authenticate for the given URL | ||
PersonalAccessToken token = | ||
personalAccessTokenManager.getAndStore(remoteFactoryUrl.getHostName()); | ||
return urlFetcher.fetch(requestURL, formatAuthorization(token.getToken())); | ||
String authorization; | ||
if (isNullOrEmpty(credentials)) { | ||
authorization = | ||
formatAuthorization( | ||
personalAccessTokenManager | ||
.getAndStore(remoteFactoryUrl.getHostName()) | ||
.getToken()); | ||
} else { | ||
authorization = getCredentialsAuthorization(credentials); | ||
} | ||
return urlFetcher.fetch(requestURL, authorization); | ||
} | ||
} catch (UnknownScmProviderException e) { | ||
return fetchContentWithoutToken(requestURL, e); | ||
|
@@ -143,4 +162,8 @@ protected String formatUrl(String fileURL) throws DevfileException { | |
protected String formatAuthorization(String token) { | ||
return "Bearer " + token; | ||
} | ||
|
||
private String getCredentialsAuthorization(String credentials) { | ||
return "Basic " + new String(Base64.getEncoder().encode(credentials.getBytes())); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
/* | ||
* Copyright (c) 2012-2021 Red Hat, Inc. | ||
* 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/ | ||
|
@@ -11,9 +11,13 @@ | |
*/ | ||
package org.eclipse.che.api.factory.server.urlfactory; | ||
|
||
import static com.google.common.base.Strings.isNullOrEmpty; | ||
import static java.lang.String.format; | ||
import static java.util.Collections.singletonList; | ||
|
||
import java.net.MalformedURLException; | ||
import java.net.URI; | ||
import java.net.URL; | ||
import java.util.List; | ||
import java.util.Optional; | ||
|
||
|
@@ -24,6 +28,7 @@ | |
public class DefaultFactoryUrl implements RemoteFactoryUrl { | ||
|
||
private String devfileFileLocation; | ||
private URL url; | ||
|
||
@Override | ||
public String getProviderName() { | ||
|
@@ -61,6 +66,33 @@ public String getBranch() { | |
return null; | ||
} | ||
|
||
@Override | ||
public Optional<String> getCredentials() { | ||
if (url == null || isNullOrEmpty(url.getUserInfo())) { | ||
return Optional.empty(); | ||
} | ||
String userInfo = url.getUserInfo(); | ||
String[] credentials = userInfo.split(":"); | ||
String username = credentials[0]; | ||
String password = credentials.length == 2 ? credentials[1] : null; | ||
if (!isNullOrEmpty(username) || !isNullOrEmpty(password)) { | ||
return Optional.of( | ||
format( | ||
"%s:%s", | ||
isNullOrEmpty(username) ? "" : username, isNullOrEmpty(password) ? "" : password)); | ||
} | ||
Comment on lines
+78
to
+83
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a bit lost, can we really handle a case from auth perspective if either username or password is null / blank? I thought we need both, no? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, this is probably for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually we don't need both parts. If we have only username, we pass the credentials as
So |
||
return Optional.empty(); | ||
} | ||
|
||
public <U extends DefaultFactoryUrl> U withUrl(String url) { | ||
try { | ||
this.url = new URL(url); | ||
} catch (MalformedURLException e) { | ||
// Do nothing, wrong URL. | ||
} | ||
return (U) this; | ||
} | ||
|
||
public DefaultFactoryUrl withDevfileFileLocation(String devfileFileLocation) { | ||
this.devfileFileLocation = devfileFileLocation; | ||
return this; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
/* | ||
* 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.urlfactory; | ||
|
||
import static org.testng.Assert.assertEquals; | ||
import static org.testng.Assert.assertFalse; | ||
import static org.testng.Assert.assertTrue; | ||
|
||
import java.util.Optional; | ||
import org.mockito.testng.MockitoTestNGListener; | ||
import org.testng.annotations.DataProvider; | ||
import org.testng.annotations.Listeners; | ||
import org.testng.annotations.Test; | ||
|
||
/** Testing {@link DefaultFactoryUrl} */ | ||
@Listeners(MockitoTestNGListener.class) | ||
public class DefaultFactoryUrlTest { | ||
@Test(dataProvider = "urlsProvider") | ||
public void shouldGetCredentials(String url, String credentials) { | ||
// given | ||
DefaultFactoryUrl factoryUrl = new DefaultFactoryUrl().withUrl(url); | ||
// when | ||
Optional<String> credentialsOptional = factoryUrl.getCredentials(); | ||
// then | ||
assertTrue(credentialsOptional.isPresent()); | ||
assertEquals(credentialsOptional.get(), credentials); | ||
} | ||
|
||
@DataProvider(name = "urlsProvider") | ||
private Object[][] urlsProvider() { | ||
return new Object[][] { | ||
{"https://username:password@hostname/path", "username:password"}, | ||
{"https://token@hostname/path/user/repo/", "token:"}, | ||
{"http://token@hostname/path/user/repo/", "token:"}, | ||
{"https://token@dev.azure.com/user/repo/", "token:"}, | ||
vinokurig marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{"https://token@dev.azure.com/user/repo?a=&b=b&c=/.devfile.yaml&api-version=7.0", "token:"}, | ||
{"https://token@gitlub.com/user/repo/", "token:"}, | ||
{"https://token@bitbucket.org/user/repo/", "token:"}, | ||
{ | ||
"https://personal-access-token@raw.githubusercontent.com/user/repo/branch/.devfile.yaml", | ||
"personal-access-token:" | ||
} | ||
}; | ||
} | ||
|
||
@Test | ||
public void shouldGetEmptyCredentials() { | ||
// given | ||
DefaultFactoryUrl factoryUrl = new DefaultFactoryUrl().withUrl("https://hostname/path"); | ||
// when | ||
Optional<String> credentialsOptional = factoryUrl.getCredentials(); | ||
// then | ||
assertFalse(credentialsOptional.isPresent()); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use Optional here also?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no need to use
Optional
orNullable
here. This is an overloaded method which is supposed to receive non null parameters.