Fix authorization events

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<String> 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);
   }
 
 

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);
+  }
+}

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() {

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}.
  * 
@@ -84,6 +94,11 @@ public class AuthorizationChangedEventProducerTest {
     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(){