From 9898cd372123008f6502b061c70c2e0b6bad9abf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Pfeuffer?= Date: Wed, 23 Jan 2019 15:00:48 +0100 Subject: [PATCH] Fix authorization events --- .../scm/repository/RepositoryPermission.java | 11 +- .../repository/RepositoryPermissionTest.java | 49 ++++++++ .../AuthorizationChangedEventProducer.java | 4 +- ...AuthorizationChangedEventProducerTest.java | 108 ++++++++++-------- 4 files changed, 121 insertions(+), 51 deletions(-) create mode 100644 scm-core/src/test/java/sonia/scm/repository/RepositoryPermissionTest.java diff --git a/scm-core/src/main/java/sonia/scm/repository/RepositoryPermission.java b/scm-core/src/main/java/sonia/scm/repository/RepositoryPermission.java index 97872f29eb..e83eaca66c 100644 --- a/scm-core/src/main/java/sonia/scm/repository/RepositoryPermission.java +++ b/scm-core/src/main/java/sonia/scm/repository/RepositoryPermission.java @@ -37,6 +37,7 @@ package sonia.scm.repository; import com.google.common.base.MoreObjects; import com.google.common.base.Objects; +import org.apache.commons.collections.CollectionUtils; import sonia.scm.security.PermissionObject; import javax.xml.bind.annotation.XmlAccessType; @@ -45,8 +46,10 @@ import javax.xml.bind.annotation.XmlElement; import javax.xml.bind.annotation.XmlRootElement; import java.io.Serializable; import java.util.Collection; +import java.util.HashSet; import static java.util.Collections.emptyList; +import static java.util.Collections.unmodifiableCollection; //~--- JDK imports ------------------------------------------------------------ @@ -76,7 +79,7 @@ public class RepositoryPermission implements PermissionObject, Serializable public RepositoryPermission(String name, Collection verbs, boolean groupPermission) { this.name = name; - this.verbs = verbs; + this.verbs = unmodifiableCollection(new HashSet<>(verbs)); this.groupPermission = groupPermission; } @@ -106,7 +109,7 @@ public class RepositoryPermission implements PermissionObject, Serializable final RepositoryPermission other = (RepositoryPermission) obj; return Objects.equal(name, other.name) - && Objects.equal(verbs, other.verbs) + && CollectionUtils.isEqualCollection(verbs, other.verbs) && Objects.equal(groupPermission, other.groupPermission); } @@ -119,7 +122,9 @@ public class RepositoryPermission implements PermissionObject, Serializable @Override public int hashCode() { - return Objects.hashCode(name, verbs, groupPermission); + // Normally we do not have a log of repository permissions having the same size of verbs, but different content. + // Therefore we do not use the verbs themselves for the hash code but only the number of verbs. + return Objects.hashCode(name, verbs.size(), groupPermission); } diff --git a/scm-core/src/test/java/sonia/scm/repository/RepositoryPermissionTest.java b/scm-core/src/test/java/sonia/scm/repository/RepositoryPermissionTest.java new file mode 100644 index 0000000000..2e9383b2e2 --- /dev/null +++ b/scm-core/src/test/java/sonia/scm/repository/RepositoryPermissionTest.java @@ -0,0 +1,49 @@ +package sonia.scm.repository; + +import org.junit.jupiter.api.Test; + +import static java.util.Arrays.asList; +import static org.assertj.core.api.Assertions.assertThat; + +class RepositoryPermissionTest { + + @Test + void shouldBeEqualWithSameVerbs() { + RepositoryPermission permission1 = new RepositoryPermission("name", asList("one", "two"), false); + RepositoryPermission permission2 = new RepositoryPermission("name", asList("two", "one"), false); + + assertThat(permission1).isEqualTo(permission2); + } + + @Test + void shouldHaveSameHashCodeWithSameVerbs() { + long hash1 = new RepositoryPermission("name", asList("one", "two"), false).hashCode(); + long hash2 = new RepositoryPermission("name", asList("two", "one"), false).hashCode(); + + assertThat(hash1).isEqualTo(hash2); + } + + @Test + void shouldNotBeEqualWithSameVerbs() { + RepositoryPermission permission1 = new RepositoryPermission("name", asList("one", "two"), false); + RepositoryPermission permission2 = new RepositoryPermission("name", asList("three", "one"), false); + + assertThat(permission1).isNotEqualTo(permission2); + } + + @Test + void shouldNotBeEqualWithDifferentType() { + RepositoryPermission permission1 = new RepositoryPermission("name", asList("one"), false); + RepositoryPermission permission2 = new RepositoryPermission("name", asList("one"), true); + + assertThat(permission1).isNotEqualTo(permission2); + } + + @Test + void shouldNotBeEqualWithDifferentName() { + RepositoryPermission permission1 = new RepositoryPermission("name1", asList("one"), false); + RepositoryPermission permission2 = new RepositoryPermission("name2", asList("one"), false); + + assertThat(permission1).isNotEqualTo(permission2); + } +} diff --git a/scm-webapp/src/main/java/sonia/scm/security/AuthorizationChangedEventProducer.java b/scm-webapp/src/main/java/sonia/scm/security/AuthorizationChangedEventProducer.java index c3e7dc4f0c..0586db2bb3 100644 --- a/scm-webapp/src/main/java/sonia/scm/security/AuthorizationChangedEventProducer.java +++ b/scm-webapp/src/main/java/sonia/scm/security/AuthorizationChangedEventProducer.java @@ -167,9 +167,7 @@ public class AuthorizationChangedEventProducer { private boolean isAuthorizationDataModified(Repository repository, Repository beforeModification) { return repository.isArchived() != beforeModification.isArchived() || repository.isPublicReadable() != beforeModification.isPublicReadable() - // TODO RP -// || !(repository.getPermissions().containsAll(beforeModification.getPermissions()) && beforeModification.getPermissions().containsAll(repository.getPermissions())) - ; + || !(repository.getPermissions().containsAll(beforeModification.getPermissions()) && beforeModification.getPermissions().containsAll(repository.getPermissions())); } private void fireEventForEveryUser() { diff --git a/scm-webapp/src/test/java/sonia/scm/security/AuthorizationChangedEventProducerTest.java b/scm-webapp/src/test/java/sonia/scm/security/AuthorizationChangedEventProducerTest.java index de31aa1298..59b6951025 100644 --- a/scm-webapp/src/test/java/sonia/scm/security/AuthorizationChangedEventProducerTest.java +++ b/scm-webapp/src/test/java/sonia/scm/security/AuthorizationChangedEventProducerTest.java @@ -31,21 +31,31 @@ package sonia.scm.security; -import org.junit.Test; -import static org.junit.Assert.*; +import com.google.common.collect.Lists; import org.junit.Before; +import org.junit.Test; import sonia.scm.HandlerEventType; import sonia.scm.group.Group; import sonia.scm.group.GroupEvent; import sonia.scm.group.GroupModificationEvent; import sonia.scm.repository.Repository; import sonia.scm.repository.RepositoryEvent; +import sonia.scm.repository.RepositoryModificationEvent; +import sonia.scm.repository.RepositoryPermission; import sonia.scm.repository.RepositoryTestData; import sonia.scm.user.User; import sonia.scm.user.UserEvent; import sonia.scm.user.UserModificationEvent; import sonia.scm.user.UserTestData; +import static java.util.Arrays.asList; +import static java.util.Collections.singletonList; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; + /** * Unit tests for {@link AuthorizationChangedEventProducer}. * @@ -83,7 +93,12 @@ public class AuthorizationChangedEventProducerTest { assertTrue(producer.event.isEveryUserAffected()); assertEquals(username, producer.event.getNameOfAffectedUser()); } - + + private void assertGlobalEventIsFired(){ + assertNotNull(producer.event); + assertFalse(producer.event.isEveryUserAffected()); + } + /** * Tests {@link AuthorizationChangedEventProducer#onEvent(sonia.scm.user.UserEvent)} with modified user. */ @@ -123,11 +138,6 @@ public class AuthorizationChangedEventProducerTest { assertGlobalEventIsFired(); } - private void assertGlobalEventIsFired(){ - assertNotNull(producer.event); - assertFalse(producer.event.isEveryUserAffected()); - } - /** * Tests {@link AuthorizationChangedEventProducer#onEvent(sonia.scm.group.GroupEvent)} with modified groups. */ @@ -168,43 +178,51 @@ public class AuthorizationChangedEventProducerTest { @Test public void testOnRepositoryModificationEvent() { - // TODO RP -// Repository repositoryModified = RepositoryTestData.createHeartOfGold(); -// repositoryModified.setName("test123"); -// repositoryModified.setPermissions(Lists.newArrayList(new RepositoryPermission("test"))); -// -// Repository repository = RepositoryTestData.createHeartOfGold(); -// repository.setPermissions(Lists.newArrayList(new RepositoryPermission("test"))); -// -// producer.onEvent(new RepositoryModificationEvent(HandlerEventType.BEFORE_CREATE, repositoryModified, repository)); -// assertEventIsNotFired(); -// -// producer.onEvent(new RepositoryModificationEvent(HandlerEventType.CREATE, repositoryModified, repository)); -// assertEventIsNotFired(); -// -// repositoryModified.setPermissions(Lists.newArrayList(new RepositoryPermission("test"))); -// producer.onEvent(new RepositoryModificationEvent(HandlerEventType.CREATE, repositoryModified, repository)); -// assertEventIsNotFired(); -// -// repositoryModified.setPermissions(Lists.newArrayList(new RepositoryPermission("test123"))); -// producer.onEvent(new RepositoryModificationEvent(HandlerEventType.CREATE, repositoryModified, repository)); -// assertGlobalEventIsFired(); -// -// resetStoredEvent(); -// -// repositoryModified.setPermissions( -// Lists.newArrayList(new RepositoryPermission("test", PermissionType.READ, true)) -// ); -// producer.onEvent(new RepositoryModificationEvent(HandlerEventType.CREATE, repositoryModified, repository)); -// assertGlobalEventIsFired(); -// -// resetStoredEvent(); -// -// repositoryModified.setPermissions( -// Lists.newArrayList(new RepositoryPermission("test", PermissionType.WRITE)) -// ); -// producer.onEvent(new RepositoryModificationEvent(HandlerEventType.CREATE, repositoryModified, repository)); -// assertGlobalEventIsFired(); + Repository repositoryModified = RepositoryTestData.createHeartOfGold(); + repositoryModified.setName("test123"); + repositoryModified.setPermissions(Lists.newArrayList(new RepositoryPermission("test", singletonList("read"), false))); + + Repository repository = RepositoryTestData.createHeartOfGold(); + repository.setPermissions(Lists.newArrayList(new RepositoryPermission("test", singletonList("read"), false))); + + producer.onEvent(new RepositoryModificationEvent(HandlerEventType.BEFORE_CREATE, repositoryModified, repository)); + assertEventIsNotFired(); + + producer.onEvent(new RepositoryModificationEvent(HandlerEventType.CREATE, repositoryModified, repository)); + assertEventIsNotFired(); + + repositoryModified.setPermissions(Lists.newArrayList(new RepositoryPermission("test", singletonList("read"), false))); + producer.onEvent(new RepositoryModificationEvent(HandlerEventType.CREATE, repositoryModified, repository)); + assertEventIsNotFired(); + + repositoryModified.setPermissions(Lists.newArrayList(new RepositoryPermission("test123", singletonList("read"), false))); + producer.onEvent(new RepositoryModificationEvent(HandlerEventType.CREATE, repositoryModified, repository)); + assertGlobalEventIsFired(); + + resetStoredEvent(); + + repositoryModified.setPermissions( + Lists.newArrayList(new RepositoryPermission("test", singletonList("read"), true)) + ); + producer.onEvent(new RepositoryModificationEvent(HandlerEventType.CREATE, repositoryModified, repository)); + assertGlobalEventIsFired(); + + resetStoredEvent(); + + repositoryModified.setPermissions( + Lists.newArrayList(new RepositoryPermission("test", asList("read", "write"), false)) + ); + producer.onEvent(new RepositoryModificationEvent(HandlerEventType.CREATE, repositoryModified, repository)); + assertGlobalEventIsFired(); + + resetStoredEvent(); + repository.setPermissions(Lists.newArrayList(new RepositoryPermission("test", asList("read", "write"), false))); + + repositoryModified.setPermissions( + Lists.newArrayList(new RepositoryPermission("test", asList("write", "read"), false)) + ); + producer.onEvent(new RepositoryModificationEvent(HandlerEventType.CREATE, repositoryModified, repository)); + assertEventIsNotFired(); } private void resetStoredEvent(){