diff --git a/gradle/changelog/wrong_permissions.yaml b/gradle/changelog/wrong_permissions.yaml new file mode 100644 index 0000000000..95ee4be6f6 --- /dev/null +++ b/gradle/changelog/wrong_permissions.yaml @@ -0,0 +1,2 @@ +- type: fixed + description: Wrong repositories in post-receive hooks under some circumstances diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/protocolcommand/git/BaseReceivePackFactory.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/protocolcommand/git/BaseReceivePackFactory.java index 9f32812817..99a304e3cd 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/protocolcommand/git/BaseReceivePackFactory.java +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/protocolcommand/git/BaseReceivePackFactory.java @@ -39,17 +39,19 @@ import sonia.scm.web.GitReceiveHook; public abstract class BaseReceivePackFactory implements ReceivePackFactory { + private final GitChangesetConverterFactory converterFactory; private final GitRepositoryHandler handler; - private final GitReceiveHook hook; + private final HookEventFacade hookEventFacade; private final GitRepositoryConfigStoreProvider storeProvider; protected BaseReceivePackFactory(GitChangesetConverterFactory converterFactory, GitRepositoryHandler handler, HookEventFacade hookEventFacade, GitRepositoryConfigStoreProvider storeProvider) { + this.converterFactory = converterFactory; this.handler = handler; + this.hookEventFacade = hookEventFacade; this.storeProvider = storeProvider; - this.hook = new GitReceiveHook(converterFactory, hookEventFacade, handler); } @Override @@ -57,6 +59,7 @@ public abstract class BaseReceivePackFactory implements ReceivePackFactory ReceivePack receivePack = createBasicReceivePack(connection, repository); receivePack.setAllowNonFastForwards(isNonFastForwardAllowed(repository)); + GitReceiveHook hook = new GitReceiveHook(converterFactory, hookEventFacade, handler); receivePack.setPreReceiveHook(hook); receivePack.setPostReceiveHook(hook); // apply collecting listener, to be able to check which commits are new diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/web/CollectingPackParserListener.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/web/CollectingPackParserListener.java index a4988f0daa..fc6b8e67ee 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/web/CollectingPackParserListener.java +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/web/CollectingPackParserListener.java @@ -55,7 +55,7 @@ public class CollectingPackParserListener implements PackParserListener private Set newObjectIds; - public CollectingPackParserListener(GitReceiveHook hook) { + private CollectingPackParserListener(GitReceiveHook hook) { this.hook = hook; } diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/web/GitReceiveHook.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/web/GitReceiveHook.java index c34ac01ec9..208717bfc2 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/web/GitReceiveHook.java +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/web/GitReceiveHook.java @@ -43,7 +43,9 @@ import java.io.IOException; import java.util.Collection; import java.util.List; - +/** + * This class is not thread safe! + */ public class GitReceiveHook implements PreReceiveHook, PostReceiveHook { private static final Logger LOG = LoggerFactory.getLogger(GitReceiveHook.class); @@ -96,6 +98,9 @@ public class GitReceiveHook implements PreReceiveHook, PostReceiveHook { GitHookContextProvider context = new GitHookContextProvider(converterFactory, rpack, receiveCommands, repository, repositoryId); if (type == RepositoryHookType.POST_RECEIVE) { + if (postReceiveContext != null) { + throw new IllegalStateException("Looks like this hook is re-used. This must not happen."); + } postReceiveContext = context; } else { hookEventFacade.handle(repositoryId).fireHookEvent(type, context); diff --git a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/protocolcommand/git/BaseReceivePackFactoryTest.java b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/protocolcommand/git/BaseReceivePackFactoryTest.java index e59c5b5896..202abd0cb0 100644 --- a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/protocolcommand/git/BaseReceivePackFactoryTest.java +++ b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/protocolcommand/git/BaseReceivePackFactoryTest.java @@ -50,6 +50,8 @@ import java.io.File; import java.io.IOException; import static org.hamcrest.Matchers.instanceOf; +import static org.hamcrest.Matchers.not; +import static org.hamcrest.Matchers.sameInstance; import static org.junit.Assert.*; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -88,8 +90,7 @@ public class BaseReceivePackFactoryTest { when(handler.getConfig()).thenReturn(gitConfig); when(handler.getRepositoryId(repository.getConfig())).thenReturn("heart-of-gold"); - ReceivePack receivePack = new ReceivePack(repository); - when(wrappedReceivePackFactory.create(request, repository)).thenReturn(receivePack); + when(wrappedReceivePackFactory.create(request, repository)).thenAnswer(invocation -> new ReceivePack(repository)); when(gitRepositoryConfigStoreProvider.getGitRepositoryConfig("heart-of-gold")).thenReturn(gitRepositoryConfig); @@ -130,4 +131,11 @@ public class BaseReceivePackFactoryTest { assertFalse(receivePack.isAllowNonFastForwards()); } + @Test + public void shouldNotReUseGitReceiveHook() throws Exception { + ReceivePack receivePack1 = factory.create(request, repository); + ReceivePack receivePack2 = factory.create(request, repository); + + assertThat(receivePack1.getPostReceiveHook(), not(sameInstance(receivePack2.getPostReceiveHook()))); + } }