-
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 1 commit
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 |
---|---|---|
@@ -0,0 +1,55 @@ | ||
/* | ||
* 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.assertNull; | ||
|
||
import org.mockito.testng.MockitoTestNGListener; | ||
import org.testng.annotations.Listeners; | ||
import org.testng.annotations.Test; | ||
|
||
/** Testing {@link DefaultFactoryUrl} */ | ||
@Listeners(MockitoTestNGListener.class) | ||
public class DefaultFactoryUrlTest { | ||
@Test | ||
public void shouldCredentials() { | ||
// given | ||
DefaultFactoryUrl factoryUrl = | ||
new DefaultFactoryUrl().withUrl("https://username:password@hostname/path"); | ||
vinokurig marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// when | ||
String credentials = factoryUrl.getCredentials(); | ||
// then | ||
assertEquals(credentials, "username:password"); | ||
} | ||
|
||
@Test | ||
public void shouldCredentialsWithoutPassword() { | ||
// given | ||
DefaultFactoryUrl factoryUrl = | ||
new DefaultFactoryUrl().withUrl("https://username@hostname/path"); | ||
// when | ||
String credentials = factoryUrl.getCredentials(); | ||
// then | ||
assertEquals(credentials, "username:"); | ||
} | ||
|
||
@Test | ||
public void shouldGetNullCredentials() { | ||
// given | ||
DefaultFactoryUrl factoryUrl = new DefaultFactoryUrl().withUrl("https://hostname/path"); | ||
// when | ||
String credentials = factoryUrl.getCredentials(); | ||
// then | ||
assertNull(credentials); | ||
} | ||
} |
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.