Mind review findings

This commit is contained in:
Eduard Heimbuch
2020-12-02 10:42:26 +01:00
parent e7b7bf5b0f
commit 7db33d2e65
18 changed files with 152 additions and 351 deletions

View File

@@ -41,8 +41,8 @@ import java.util.List;
* {@link AdvancedImportHandler}.
*
* @author Sebastian Sdorra
* @since 1.12
* @deprecated
* @since 2.11.0
*/
@Deprecated
public abstract class AbstactImportHandler implements AdvancedImportHandler

View File

@@ -30,8 +30,8 @@ package sonia.scm.repository;
* {@link ImportHandler}.
*
* @author Sebastian Sdorra
* @since 1.43
* @deprecated
* @since 2.11.0
*/
@Deprecated
public interface AdvancedImportHandler extends ImportHandler

View File

@@ -33,7 +33,7 @@ import java.util.List;
* Searches and import existing repositories.
*
* @author Sebastian Sdorra
* @since 2.11.0
* @since 1.12
* @deprecated
*/
@Deprecated

View File

@@ -38,8 +38,8 @@ import static com.google.common.base.Preconditions.checkNotNull;
* Import result of the {@link AdvancedImportHandler}.
*
* @author Sebastian Sdorra
* @since 1.43
* @deprecated
* @since 2.11.0
*/
@EqualsAndHashCode
@ToString

View File

@@ -48,11 +48,12 @@ public interface RepositoryHandler
*
*
* @return {@link ImportHandler} for the repository type of this handler
* @since 1.12
* @deprecated
* @since 2.11.0
*
* @throws FeatureNotSupportedException
*/
@Deprecated
public ImportHandler getImportHandler() throws FeatureNotSupportedException;
/**

View File

@@ -29,6 +29,11 @@ import lombok.Getter;
import sonia.scm.HandlerEventType;
import sonia.scm.event.Event;
/**
* Event which is fired whenever repository import is successful or failed.
*
* @since 2.11.0
*/
@Event
@Getter
@EqualsAndHashCode(callSuper = true)

View File

@@ -99,6 +99,15 @@ public interface RepositoryManager
Collection<String> getAllNamespaces();
/**
* Creates a new repository and afterwards executes the logic from the {@param afterCreation}.
*
* @param repository the repository {@link Repository}
* @param afterCreation consumer which expects a repository {@link Repository}
* @return {@link Repository} the created repository
*
* @since 2.11.0
*/
default Repository create(Repository repository, Consumer<Repository> afterCreation) {
Repository newRepository = create(repository);
afterCreation.accept(newRepository);

View File

@@ -29,6 +29,11 @@ import sonia.scm.ExceptionWithContext;
import java.util.List;
/**
* This exception is thrown if the repository import fails.
*
* @since 2.11.0
*/
public class ImportFailedException extends ExceptionWithContext {
private static final String CODE = "D6SHRfqQw1";

View File

@@ -31,6 +31,7 @@ import org.slf4j.LoggerFactory;
/**
* @author Sebastian Sdorra
* @deprecated
*/
@Deprecated
public class GitImportHandler extends AbstactImportHandler {

View File

@@ -43,7 +43,6 @@ import org.slf4j.LoggerFactory;
import sonia.scm.ContextEntry;
import sonia.scm.repository.GitRepositoryHandler;
import sonia.scm.repository.GitUtil;
import sonia.scm.repository.InternalRepositoryException;
import sonia.scm.repository.Repository;
import sonia.scm.repository.api.ImportFailedException;
import sonia.scm.repository.api.PullResponse;
@@ -217,15 +216,11 @@ public class GitPullCommand extends AbstractGitPushOrPullCommand
response = convert(git, result);
} catch
(GitAPIException ex) {
if (ex.getMessage().contains("not authorized")) {
throw new ImportFailedException(
ContextEntry.ContextBuilder.entity(repository).build(),
"Repository import failed. The credentials are wrong or missing.",
ex
);
}
throw new InternalRepositoryException(repository, "error during pull", ex);
throw new ImportFailedException(
ContextEntry.ContextBuilder.entity(repository).build(),
"Repository import failed. The credentials are wrong or missing.",
ex
);
}
return response;

View File

@@ -42,7 +42,6 @@ import java.io.IOException;
*
* @author Sebastian Sdorra
* @deprecated
* @since 2.11.0
*/
@Deprecated
public class HgImportHandler extends AbstactImportHandler

View File

@@ -28,7 +28,6 @@ package sonia.scm.repository;
*
* @author Sebastian Sdorra
* @deprecated
* @since 2.11.0
*/
@Deprecated
public class SvnImportHandler extends AbstactImportHandler

View File

@@ -138,3 +138,86 @@ describe("test path validation", () => {
});
}
});
describe("test url validation", () => {
const invalid = [
"file:///blah/index.html",
"http://",
"http://.",
"http://..",
"http://../",
"http://?",
"http://??",
"http://??/",
"http://#",
"http://##",
"http://##/",
"http://foo.bar?q=Spaces should be encoded",
"//",
"//a",
"///a",
"///",
"foo.com",
"rdar://1234",
"h://test",
"http:// shouldfail.com",
":// should fail",
"http://foo.bar/foo(bar)baz quux",
"ftps://foo.bar/",
"http://.www.foo.bar/",
"http://.www.foo.bar./"
];
for (const url of invalid) {
it(`should return false for '${url}'`, () => {
expect(validator.isUrlValid(url)).toBe(false);
});
}
const valid = [
"https://foo.com/blah_blah",
"https://foo.com/blah_blah/",
"https://foo.com/blah_blah_(wikipedia)",
"https://foo.com/blah_blah_(wikipedia)_(again)",
"http://www.example.com/wpstyle/?p=364",
"http://foo.com/blah_blah",
"http://foo.com/blah_blah/",
"http://foo.com/blah_blah_(wikipedia)",
"http://foo.com/blah_blah_(wikipedia)_(again)",
"http://www.example.com/wpstyle/?p=364",
"https://www.example.com/foo/?bar=baz&inga=42&quux",
"http://✪df.ws/123",
"http://userid:password@example.com:8080",
"http://userid:password@example.com:8080/",
"http://userid@example.com",
"http://userid@example.com/",
"http://userid@example.com:8080",
"http://userid@example.com:8080/",
"http://userid:password@example.com",
"http://userid:password@example.com/",
"http://142.42.1.1/",
"http://142.42.1.1:8080/",
"http://➡.ws/䨹",
"http://⌘.ws",
"http://⌘.ws/",
"http://foo.com/blah_(wikipedia)#cite-1",
"http://foo.com/blah_(wikipedia)_blah#cite-1",
"http://foo.com/unicode_(✪)_in_parens",
"http://foo.com/(something)?after=parens",
"http://☺.damowmow.com/",
"http://code.google.com/events/#&product=browser",
"http://j.mp",
"http://foo.bar/?q=Test%20URL-encoded%20stuff",
"http://مثال.إختبار",
"http://例子.测试",
"http://उदाहरण.परीक्षा",
"http://-.~_!$&'()*+,;=:%40:80%2f::::::@example.com",
"http://1337.net",
"http://a.b-c.de",
"http://223.255.255.254",
"http://0.0.0.0"
];
for (const url of valid) {
it(`should return true for '${url}'`, () => {
expect(validator.isUrlValid(url)).toBe(true);
});
}
});

View File

@@ -50,7 +50,7 @@ export const isPathValid = (path: string) => {
return pathRegex.test(path);
};
const urlRegex = /[-a-zA-Z0-9@:%._\+~#=]{1,256}\.[a-zA-Z0-9()]{1,6}\b([-a-zA-Z0-9()@:%_\+.~#?&//=]*)/;
const urlRegex = /^(ftp|https?):\/\/[^\s$.?#].[^\s]*$/;
export const isUrlValid = (url: string) => {
return urlRegex.test(url);

View File

@@ -70,8 +70,7 @@ const ImportRepositoryFromUrl: FC<Props> = ({ url, setImportPending }) => {
.post(url, repo, "application/vnd.scmm-repository+json;v=2")
.then(response => {
const location = response.headers.get("Location");
// @ts-ignore Location is always set if the repository import was successful
return apiClient.get(location);
return apiClient.get(location!);
})
.then(response => response.json())
.then(repo => {

View File

@@ -25,11 +25,10 @@
import { validation } from "@scm-manager/ui-components";
const nameRegex = /(?!^\.\.$)(?!^\.$)(?!.*[.]git$)(?!.*[\\\[\]])^[A-Za-z0-9\.][A-Za-z0-9\.\-_]*$/;
const namespaceExceptionsRegexCreate = /^(([0-9]{1,3})|(create))$/;
const namespaceExceptionsRegexImport = /^(([0-9]{1,3})|(import))$/;
const namespaceExceptionsRegex = /^(([0-9]{1,3})|(create)|(import))$/;
export const isNamespaceValid = (name: string) => {
return nameRegex.test(name) && !namespaceExceptionsRegexCreate.test(name) && !namespaceExceptionsRegexImport.test(name);
return nameRegex.test(name) && !namespaceExceptionsRegex.test(name);
};
export const isNameValid = (name: string) => {

View File

@@ -54,8 +54,12 @@ import sonia.scm.repository.api.Command;
import sonia.scm.repository.api.PullCommandBuilder;
import sonia.scm.repository.api.RepositoryService;
import sonia.scm.repository.api.RepositoryServiceFactory;
import sonia.scm.util.ValidationUtil;
import sonia.scm.web.VndMediaType;
import javax.validation.constraints.Email;
import javax.validation.constraints.NotEmpty;
import javax.validation.constraints.Pattern;
import javax.ws.rs.Consumes;
import javax.ws.rs.POST;
import javax.ws.rs.Path;
@@ -66,6 +70,8 @@ import javax.ws.rs.core.Response;
import javax.ws.rs.core.UriInfo;
import java.io.IOException;
import java.net.URI;
import java.time.Instant;
import java.util.List;
import java.util.Set;
import java.util.function.Consumer;
@@ -93,74 +99,6 @@ public class RepositoryImportResource {
this.eventBus = eventBus;
}
// /**
// * Imports a repository type specific bundle. The bundle file is uploaded to
// * the server which is running scm-manager. After the upload has finished, the
// * bundle file is passed to the {@link UnbundleCommandBuilder}. <strong>Note:</strong> This method
// * requires admin privileges.
// *
// * @param uriInfo uri info
// * @param type repository type
// * @param name name of the repository
// * @param inputStream input bundle
// * @param compressed true if the bundle is gzip compressed
// * @return empty response with location header which points to the imported repository
// * @since 1.43
// */
// @POST
// @Path("{type}/bundle")
// @Consumes(MediaType.MULTIPART_FORM_DATA)
// public Response importFromBundle(@Context UriInfo uriInfo,
// @PathParam("type") String type,
// @FormParam("namespace") String namespace,
// @FormParam("name") String name,
// @FormParam("bundle") InputStream inputStream,
// @QueryParam("compressed")
// @DefaultValue("false") boolean compressed) {
// Repository repository = doImportFromBundle(type, namespace, name, inputStream, compressed);
//
// return buildResponse(uriInfo, repository);
// }
//
// /**
// * This method works exactly like
// * {@link #importFromBundle(UriInfo, String, String, String, InputStream, boolean)}, but this
// * method returns an html content-type. The method exists only for a
// * workaround of the javascript ui extjs. <strong>Note:</strong> This method requires admin
// * privileges.
// *
// * @param type repository type
// * @param name name of the repository
// * @param inputStream input bundle
// * @param compressed true if the bundle is gzip compressed
// * @return empty response with location header which points to the imported
// * repository
// * @since 1.43
// */
// @POST
// @Path("{type}/bundle.html")
// @Consumes(MediaType.MULTIPART_FORM_DATA)
// @Produces(MediaType.TEXT_HTML)
// public Response importFromBundleUI(@PathParam("type") String type,
// @FormParam("namespace") String namespace,
// @FormParam("name") String name,
// @FormParam("bundle") InputStream inputStream,
// @QueryParam("compressed")
// @DefaultValue("false") boolean compressed) {
// Response response;
//
// try {
// doImportFromBundle(type, namespace, name, inputStream, compressed);
// response = Response.ok(new RestActionUploadResult(true)).build();
// } catch (WebApplicationException ex) {
// logger.warn("error durring bundle import", ex);
// response = Response.fromResponse(ex.getResponse()).entity(
// new RestActionUploadResult(false)).build();
// }
//
// return response;
// }
/**
* Imports a external repository which is accessible via url. The method can
* only be used, if the repository type supports the {@link Command#PULL}. The
@@ -183,7 +121,7 @@ public class RepositoryImportResource {
description = "Repository import was successful",
content = @Content(
mediaType = VndMediaType.REPOSITORY,
schema = @Schema(implementation = RepositoryDto.class)
schema = @Schema(implementation = ImportRepositoryDto.class)
)
)
@ApiResponse(
@@ -225,13 +163,13 @@ public class RepositoryImportResource {
repository,
pullChangesFromRemoteUrl(request)
);
eventBus.post(new RepositoryImportEvent(HandlerEventType.MODIFY, repository, false));
return Response.created(URI.create(resourceLinks.repository().self(repository.getNamespace(), repository.getName()))).build();
} catch (Exception e) {
eventBus.post(new RepositoryImportEvent(HandlerEventType.MODIFY, repository, true));
throw e;
}
eventBus.post(new RepositoryImportEvent(HandlerEventType.MODIFY, repository, false));
return Response.created(URI.create(resourceLinks.repository().self(repository.getNamespace(), repository.getName()))).build();
}
@VisibleForTesting
@@ -252,136 +190,6 @@ public class RepositoryImportResource {
};
}
// /**
// * Imports repositories of the given type from the configured repository
// * directory. <strong>Note:</strong> This method requires admin privileges.
// *
// * @param type repository type
// * @return imported repositories
// */
// @POST
// @Path("{type}")
// @Produces({MediaType.APPLICATION_JSON, MediaType.APPLICATION_XML})
// public Response importRepositories(@PathParam("type") String type) {
// RepositoryPermissions.create().check();
//
// List<Repository> repositories = new ArrayList<>();
//
// importFromDirectory(repositories, type);
//
// //J-
// return Response.ok(
// new GenericEntity<List<Repository>>(repositories) {
// }
// ).build();
// //J+
// }
//
// /**
// * Imports repositories of all supported types from the configured repository
// * directories. <strong>Note:</strong> This method requires admin privileges.
// *
// * @return imported repositories
// */
// @POST
// @Produces({MediaType.APPLICATION_JSON, MediaType.APPLICATION_XML})
// public Response importRepositories() {
// RepositoryPermissions.create().check();
//
// logger.info("start directory import for all supported repository types");
//
// List<Repository> repositories = new ArrayList<Repository>();
//
// for (Type t : findImportableTypes()) {
// importFromDirectory(repositories, t.getName());
// }
//
// //J-
// return Response.ok(
// new GenericEntity<List<Repository>>(repositories) {
// }
// ).build();
// //J+
// }
//
// /**
// * Imports repositories of the given type from the configured repository
// * directory. Returns a list of successfully imported directories and a list
// * of failed directories. <strong>Note:</strong> This method requires admin privileges.
// *
// * @param type repository type
// * @return imported repositories
// * @since 1.43
// */
// @POST
// @Path("{type}/directory")
// @Produces({MediaType.APPLICATION_JSON, MediaType.APPLICATION_XML})
// public Response importRepositoriesFromDirectory(
// @PathParam("type") String type) {
// RepositoryPermissions.create().check();
//
// Response response;
//
// RepositoryHandler handler = manager.getHandler(type);
//
// if (handler != null) {
// logger.info("start directory import for repository type {}", type);
//
// try {
// ImportResult result;
// ImportHandler importHandler = handler.getImportHandler();
//
// if (importHandler instanceof AdvancedImportHandler) {
// logger.debug("start directory import, using advanced import handler");
// result =
// ((AdvancedImportHandler) importHandler)
// .importRepositoriesFromDirectory(manager);
// } else {
// logger.debug("start directory import, using normal import handler");
// result = new ImportResult(importHandler.importRepositories(manager),
// ImmutableList.of());
// }
//
// response = Response.ok(result).build();
// } catch (FeatureNotSupportedException ex) {
// logger
// .warn(
// "import feature is not supported by repository handler for type "
// .concat(type), ex);
// response = Response.status(Response.Status.BAD_REQUEST).build();
// } catch (IOException ex) {
// logger.warn("exception occured durring directory import", ex);
// response = Response.serverError().build();
// }
// } else {
// logger.warn("could not find reposiotry handler for type {}", type);
// response = Response.status(Response.Status.BAD_REQUEST).build();
// }
//
// return response;
// }
//
// /**
// * Returns a list of repository types, which support the directory import
// * feature. <strong>Note:</strong> This method requires admin privileges.
// *
// * @return list of repository types
// */
// @GET
// @Produces({MediaType.APPLICATION_JSON, MediaType.APPLICATION_XML})
// public Response getImportableTypes() {
// RepositoryPermissions.create().check();
//
// List<Type> types = findImportableTypes();
//
// //J-
// return Response.ok(
// new GenericEntity<List<Type>>(types) {
// }
// ).build();
// //J+
// }
/**
* Check repository type for support for the given command.
*
@@ -406,125 +214,6 @@ public class RepositoryImportResource {
}
}
// /**
// * Start bundle import.
// *
// * @param type repository type
// * @param name name of the repository
// * @param inputStream bundle stream
// * @param compressed true if the bundle is gzip compressed
// * @return imported repository
// */
// private Repository doImportFromBundle(String type, String namespace, String name,
// InputStream inputStream, boolean compressed) {
// RepositoryPermissions.create().check();
//
// checkArgument(!Strings.isNullOrEmpty(name),
// "request does not contain name of the repository");
// checkNotNull(inputStream, "bundle inputStream is required");
//
// Repository repository;
//
// try {
// Type t = type(type);
// checkSupport(t, Command.UNBUNDLE, "bundle");
// repository = create(namespace, name, type);
// importFromBundle(repository, inputStream, compressed);
// } catch (IOException ex) {
// logger.warn("could not create temporary file", ex);
//
// throw new WebApplicationException(ex);
// }
//
// return repository;
// }
//
// private void importFromBundle(Repository repository, InputStream inputStream, boolean compressed) throws IOException {
// File file = File.createTempFile("scm-import-", ".bundle");
//
// try (RepositoryService service = serviceFactory.create(repository)) {
// long length = Files.asByteSink(file).writeFrom(inputStream);
//
// logger.info("copied {} bytes to temp, start bundle import", length);
// service.getUnbundleCommand().setCompressed(compressed).unbundle(file);
// } catch (InternalRepositoryException ex) {
// handleImportFailure(ex, repository);
// } finally {
// IOUtil.delete(file);
// }
// }
//
// private List<Type> findImportableTypes() {
// List<Type> types = new ArrayList<>();
// Collection<Type> handlerTypes = manager.getTypes();
//
// for (Type t : handlerTypes) {
// RepositoryHandler handler = manager.getHandler(t.getName());
//
// if (handler != null) {
// try {
// if (handler.getImportHandler() != null) {
// types.add(t);
// }
// } catch (FeatureNotSupportedException ex) {
// if (logger.isTraceEnabled()) {
// logger.trace("import handler is not supported", ex);
// } else if (logger.isInfoEnabled()) {
// logger.info("{} handler does not support import of repositories",
// t.getName());
// }
// }
// } else if (logger.isWarnEnabled()) {
// logger.warn("could not find handler for type {}", t.getName());
// }
// }
//
// return types;
// }
// /**
// * Import repositories from a specific type.
// *
// * @param repositories repository list
// * @param type type of repository
// */
// private void importFromDirectory(List<Repository> repositories, String type) {
// RepositoryHandler handler = manager.getHandler(type);
//
// if (handler != null) {
// logger.info("start directory import for repository type {}", type);
//
// try {
// List<String> repositoryNames =
// handler.getImportHandler().importRepositories(manager);
//
// if (repositoryNames != null) {
// for (String repositoryName : repositoryNames) {
// // TODO #8783
// /*Repository repository = null; //manager.get(type, repositoryName);
//
// if (repository != null)
// {
// repositories.add(repository);
// }
// else if (logger.isWarnEnabled())
// {
// logger.warn("could not find imported repository {}",
// repositoryName);
// }*/
// }
// }
// } catch (FeatureNotSupportedException ex) {
// throw new WebApplicationException(ex, Response.Status.BAD_REQUEST);
// } catch (IOException ex) {
// throw new WebApplicationException(ex);
// } catch (InternalRepositoryException ex) {
// throw new WebApplicationException(ex);
// }
// } else {
// throw new WebApplicationException(Response.Status.BAD_REQUEST);
// }
// }
private Type type(String type) {
RepositoryHandler handler = manager.getHandler(type);
@@ -541,7 +230,7 @@ public class RepositoryImportResource {
@Setter
@NoArgsConstructor
@SuppressWarnings("java:S2160")
public static class RepositoryImportDto extends RepositoryDto {
public static class RepositoryImportDto extends RepositoryDto implements ImportRepositoryDto {
private String importUrl;
private String username;
private String password;
@@ -550,4 +239,21 @@ public class RepositoryImportResource {
super(links, embedded);
}
}
interface ImportRepositoryDto {
String getNamespace();
@Pattern(regexp = ValidationUtil.REGEX_REPOSITORYNAME)
String getName();
@NotEmpty
String getType();
@Email
String getContact();
String getDescription();
List<HealthCheckFailureDto> getHealthCheckFailures();
Instant getLastModified();
String getImportUrl();
String getUsername();
String getPassword();
}
}

View File

@@ -193,7 +193,8 @@ public class AuthorizationChangedEventProducer {
}
}
private boolean isAuthorizationDataModified(Collection<RepositoryPermission> newPermissions, Collection<RepositoryPermission> permissionsBeforeModification) {
private boolean isAuthorizationDataModified
(Collection<RepositoryPermission> newPermissions, Collection<RepositoryPermission> permissionsBeforeModification) {
return !(newPermissions.containsAll(permissionsBeforeModification) && permissionsBeforeModification.containsAll(newPermissions));
}
@@ -201,7 +202,7 @@ public class AuthorizationChangedEventProducer {
sendEvent(AuthorizationChangedEvent.createForEveryUser());
}
private void handleRepositoryEvent(RepositoryEvent event){
private void handleRepositoryEvent(RepositoryEvent event) {
logger.debug(
"fire authorization changed event, because of received {} event for repository {}",
event.getEventType(), event.getItem().getName()
@@ -237,8 +238,8 @@ public class AuthorizationChangedEventProducer {
private void handleUserPermissionChange(AssignedPermission permission) {
logger.debug(
"fire authorization changed event for user {}, because permission {} has changed",
permission.getName(), permission.getPermission()
"fire authorization changed event for user {}, because permission {} has changed",
permission.getName(), permission.getPermission()
);
fireEventForUser(permission.getName());
}
@@ -281,7 +282,7 @@ public class AuthorizationChangedEventProducer {
return !group.getMembers().equals(beforeModification.getMembers());
}
private void handleGroupEvent(GroupEvent event){
private void handleGroupEvent(GroupEvent event) {
logger.debug(
"fire authorization changed event, because of received group event {} for group {}",
event.getEventType(),
@@ -294,5 +295,4 @@ public class AuthorizationChangedEventProducer {
protected void sendEvent(AuthorizationChangedEvent event) {
ScmEventBus.getInstance().post(event);
}
}