Prevent group deletion for groups referenced in AutoGroups configurat…

…ion (#34) * prevent group deletion if group to delete is configured in AutoGroups * add unit & integration tests for group deletion prevention * update documentation
stjosh · May 5, 2020 · 0e52fd9 · 0e52fd9
1 parent 7294297
commit 0e52fd9
Show file tree

Hide file tree

Showing 5 changed files with 109 additions and 19 deletions.
diff --git a/README.md b/README.md
@@ -15,7 +15,7 @@ Unit and Integration Tests are executed with PHP v7.3 and v7.4.
 * Install and enable the App
 * Go to "Admin -> Additional Settings" to configure the Auto Groups, Override Groups and further behavior.
 
-Note that this app requires Nextcloud 18 or later.
+Note that this app prevents group deletions for groups referenced as Auto Groups or Override Groups.
 
 ## Manual Testing
 

diff --git a/appinfo/info.xml b/appinfo/info.xml
@@ -15,6 +15,8 @@ Optionally, the group assignment can be triggered on every successful login, whi
 - If required, configure the Override Groups (e.g., for Service Accounts)
 - Optionally, enable the Login Hook, which will enforce correct group membership on every successful login
 
+Note that this app prevents group deletions for groups referenced as Auto Groups or Override Groups.
+
 ## Comparison to similar Apps
 
 * [Everyone Group](https://apps.nextcloud.com/apps/group_everyone): The "Everyone Group" app adds a virtual Group Backend, always returning all users. In contrast, "Auto Groups" operates on "real" groups in your normal Group Backend. Additionally, it is possible to specify Override Groups which will prevent users from being added to the Auto Group(s).

diff --git a/lib/AutoGroupsManager.php b/lib/AutoGroupsManager.php
@@ -29,48 +29,58 @@
 use OCP\User\Events\PostLoginEvent;
 use OCP\Group\Events\UserAddedEvent;
 use OCP\Group\Events\UserRemovedEvent;
+use OCP\Group\Events\BeforeGroupDeletedEvent;
 
 use OCP\IGroupManager;
 use OCP\EventDispatcher\IEventDispatcher;
 use OCP\IConfig;
 use OCP\ILogger;
+use OCP\IL10N;
+
+use OCP\AppFramework\OCS\OCSBadRequestException;
 
 class AutoGroupsManager
 {
     private $groupManager;
     private $logger;
     private $config;
+    private $l;
 
     /**
      * Listener manager constructor.
      */
-    public function __construct(IGroupManager $groupManager, IEventDispatcher $eventDispatcher, IConfig $config, ILogger $logger)
+    public function __construct(IGroupManager $groupManager, IEventDispatcher $eventDispatcher, IConfig $config, ILogger $logger, IL10N $l)
     {
         $this->groupManager = $groupManager;
         $this->logger = $logger;
         $this->config = $config;
+        $this->l = $l;
 
         // The callback as a PHP callable
-        $callback = [ $this, 'addAndRemoveAutoGroups' ]; 
+        $groupAssignmentCallback = [$this, 'addAndRemoveAutoGroups'];
 
         // Get the loginHook config
         $loginHook = $this->config->getAppValue("AutoGroups", "login_hook", 'false');
-        
+
         // Always add user to / remove user from auto groups on creation, group addition or group deletion
-        $eventDispatcher->addListener(UserCreatedEvent::class, $callback);
-        $eventDispatcher->addListener(UserAddedEvent::class, $callback);
-        $eventDispatcher->addListener(UserRemovedEvent::class, $callback);
-        
+        $eventDispatcher->addListener(UserCreatedEvent::class, $groupAssignmentCallback);
+        $eventDispatcher->addListener(UserAddedEvent::class, $groupAssignmentCallback);
+        $eventDispatcher->addListener(UserRemovedEvent::class, $groupAssignmentCallback);
+
         // If login hook is enabled, add user to / remove user from auto groups on every successful login
         if (filter_var($loginHook, FILTER_VALIDATE_BOOLEAN)) {
-            $eventDispatcher->addListener(PostLoginEvent::class, $callback);
-        } 
+            $eventDispatcher->addListener(PostLoginEvent::class, $groupAssignmentCallback);
+        }
+
+        // Handle group deletion events
+        $eventDispatcher->addListener(BeforeGroupDeletedEvent::class, [$this, 'handleGroupDeletion']);
     }
 
     /**
-     * The actual event handler
+     * The event handler to check group assignmnet for a user
      */
-     public function addAndRemoveAutoGroups($event) {
+    public function addAndRemoveAutoGroups($event)
+    {
         // Get configuration
         $groupNames = json_decode($this->config->getAppValue("AutoGroups", "auto_groups", '[]'));
         $overrideGroupNames = json_decode($this->config->getAppValue("AutoGroups", "override_groups", '[]'));
@@ -99,4 +109,27 @@ public function addAndRemoveAutoGroups($event) {
             }
         }
     }
+
+    /**
+     * The event handler to handle group deletions
+     * 
+     * @throws OCSBadRequestException
+     * 
+     */
+    public function handleGroupDeletion($event)
+    {
+        // Get all group names
+        $groupNames = json_decode($this->config->getAppValue("AutoGroups", "auto_groups", '[]'));
+        $overrideGroupNames = json_decode($this->config->getAppValue("AutoGroups", "override_groups", '[]'));
+
+        $allGroupNames = array_merge($groupNames, $overrideGroupNames);
+
+        // Get group name of group do delete
+        $groupNameToDelete = $event->getGroup()->getGID();
+
+        // Prevent deletion if group to delete is configured in AutoGroups
+        if (in_array($groupNameToDelete, $allGroupNames)) {
+            throw new OCSBadRequestException($this->l->t('Group "%1$s" is used in the AutoGroups App and cannot be deleted.', [$groupNameToDelete]));
+        }
+    }
 }
diff --git a/tests/Integration/EventsTest.php b/tests/Integration/EventsTest.php
@@ -30,6 +30,8 @@
 use OCP\IConfig;
 use OCP\IUserSession;
 
+use OCP\AppFramework\OCS\OCSBadRequestException;
+
 use Test\TestCase;
 use OCA\AutoGroups\AppInfo\Application;
 
@@ -112,4 +114,14 @@ public function testLoginHook()
         $this->assertNotContains('autogroup1', $groups);
         $this->assertNotContains('autogroup2', $groups);
     }
+
+    public function testBeforeGroupDeletionHook()
+    {
+        $this->config->setAppValue("AutoGroups", "auto_groups", '["autogroup1", "autogroup2"]');
+
+        $autogroup = $this->groupManager->search('autogroup1')[0];
+
+        $this->expectException(OCSBadRequestException::class);
+        $autogroup->delete();
+    }
 }
diff --git a/tests/Unit/AutoGroupsManagerTest.php b/tests/Unit/AutoGroupsManagerTest.php
@@ -28,11 +28,15 @@
 use OCP\User\Events\PostLoginEvent;
 use OCP\Group\Events\UserAddedEvent;
 use OCP\Group\Events\UserRemovedEvent;
+use OCP\Group\Events\BeforeGroupDeletedEvent;
 
 use OCP\IGroupManager;
 use OCP\EventDispatcher\IEventDispatcher;
 use OCP\IConfig;
 use OCP\ILogger;
+use OCP\IL10N;
+
+use OCP\AppFramework\OCS\OCSBadRequestException;
 
 use OCP\IUser;
 use OCP\IGroup;
@@ -48,6 +52,7 @@ class AutoGroupsManagerTest extends TestCase
     private $eventDispatcher;
     private $config;
     private $logger;
+    private $il10n;
 
     protected function setUp(): void
     {
@@ -57,6 +62,7 @@ protected function setUp(): void
         $this->eventDispatcher = $this->createMock(IEventDispatcher::class);
         $this->config = $this->createMock(IConfig::class);
         $this->logger = $this->createMock(ILogger::class);
+        $this->il10n = $this->createMock(IL10N::class);
 
         $this->testUser = $this->createMock(IUser::class);
         $this->testUser->expects($this->any())
@@ -75,17 +81,18 @@ private function createAutoGroupsManager($auto_groups = [], $override_groups = [
             )
             ->willReturnOnConsecutiveCalls($login_hook, json_encode($auto_groups), json_encode($override_groups));
 
-        return new AutoGroupsManager($this->groupManager, $this->eventDispatcher, $this->config, $this->logger);
+        return new AutoGroupsManager($this->groupManager, $this->eventDispatcher, $this->config, $this->logger, $this->il10n);
     }
 
     private function initEventHandlerTests($auto_groups = [], $override_groups = [])
     {
-        $this->eventDispatcher->expects($this->exactly(3))
+        $this->eventDispatcher->expects($this->exactly(4))
             ->method('addListener')
             ->withConsecutive(
                 [UserCreatedEvent::class, $this->callback('is_callable')],
                 [UserAddedEvent::class, $this->callback('is_callable')],
-                [UserRemovedEvent::class, $this->callback('is_callable')]
+                [UserRemovedEvent::class, $this->callback('is_callable')],
+                [BeforeGroupDeletedEvent::class, $this->callback('is_callable')]
             );
 
         $agm = $this->createAutoGroupsManager($auto_groups, $override_groups);
@@ -94,26 +101,28 @@ private function initEventHandlerTests($auto_groups = [], $override_groups = [])
 
     public function testCreatedAddedRemovedHooksWithDefaultSettings()
     {
-        $this->eventDispatcher->expects($this->exactly(3))
+        $this->eventDispatcher->expects($this->exactly(4))
             ->method('addListener')
             ->withConsecutive(
                 [UserCreatedEvent::class, $this->callback('is_callable')],
                 [UserAddedEvent::class, $this->callback('is_callable')],
-                [UserRemovedEvent::class, $this->callback('is_callable')]
+                [UserRemovedEvent::class, $this->callback('is_callable')],
+                [BeforeGroupDeletedEvent::class, $this->callback('is_callable')]
             );
 
         $agm = $this->createAutoGroupsManager([], [], false, 1);
     }
 
     public function testAlsoLoginHookIfEnabled()
     {
-        $this->eventDispatcher->expects($this->exactly(4))
+        $this->eventDispatcher->expects($this->exactly(5))
             ->method('addListener')
             ->withConsecutive(
                 [UserCreatedEvent::class, $this->callback('is_callable')],
                 [UserAddedEvent::class, $this->callback('is_callable')],
                 [UserRemovedEvent::class, $this->callback('is_callable')],
-                [PostLoginEvent::class, $this->callback('is_callable')]
+                [PostLoginEvent::class, $this->callback('is_callable')],
+                [BeforeGroupDeletedEvent::class, $this->callback('is_callable')]
             );
 
         $agm = $this->createAutoGroupsManager([], [], true, 1);
@@ -222,4 +231,38 @@ public function testRemoveNotRequired()
         $agm = $this->initEventHandlerTests(['autogroup1', 'autogroup2'], ['overridegroup1', 'overridegroup2']);
         $agm->addAndRemoveAutoGroups($event);
     }
+
+    public function testGroupDeletionPrevented()
+    {
+        $groupMock = $this->createMock(IGroup::class);
+        $groupMock->expects($this->any())
+            ->method('getGID')
+            ->willReturn('autogroup2');
+
+        $event = $this->createMock(BeforeGroupDeletedEvent::class);
+        $event->expects($this->once())
+            ->method('getGroup')
+            ->willReturn($groupMock);
+
+        $this->expectException(OCSBadRequestException::class);
+
+        $agm = $this->initEventHandlerTests(['autogroup1', 'autogroup2'], ['overridegroup1', 'overridegroup2']);
+        $agm->handleGroupDeletion($event);
+    }
+
+    public function testGroupDeletionPreventionNotNeeded()
+    {
+        $groupMock = $this->createMock(IGroup::class);
+        $groupMock->expects($this->any())
+            ->method('getGID')
+            ->willReturn('some other group');
+
+        $event = $this->createMock(BeforeGroupDeletedEvent::class);
+        $event->expects($this->once())
+            ->method('getGroup')
+            ->willReturn($groupMock);
+
+        $agm = $this->initEventHandlerTests(['autogroup1', 'autogroup2'], ['overridegroup1', 'overridegroup2']);
+        $agm->handleGroupDeletion($event);
+    }
 }