use data-confirm handler to confirm studygroup deletion, prevent security handler to interfere and cleanup delete action, fixes #5254

Closes #5254

Merge request studip/studip!3939

2 files changed, 97 insertions, 94 deletions

diff --git a/app/controllers/course/studygroup.php b/app/controllers/course/studygroup.php
index 4edbd76..617e394 100644
--- a/app/controllers/course/studygroup.php
+++ b/app/controllers/course/studygroup.php
@@ -326,8 +326,12 @@ class Course_StudygroupController extends AuthenticatedController
         $actions->addLink(
             _('Diese Studiengruppe löschen'),
             $this->deleteURL(),
-            Icon::create('trash')
-        );
+            Icon::create('trash'),
+            [
+                'data-confirm' => ('Sind Sie sicher, dass Sie diese Studiengruppe löschen möchten?'),
+                'data-secure-disable' => '',
+            ]
+        )->asButton();
 
         Sidebar::get()->addWidget($actions);
 
@@ -745,42 +749,29 @@ class Course_StudygroupController extends AuthenticatedController
 
     /**
      * deletes a studygroup
-     *
-     * @param string id of a studypgroup
-     * @param boolean approveDelete
-     * @param string studipticket
-     *
-     * @return void
-     *
      */
-    public function delete_action($approveDelete = false)
+    public function delete_action(): void
     {
-        global $perm;
+        CSRFProtection::verifyUnsafeRequest();
 
-        $id = Context::getId();
+        if (!$GLOBALS['perm']->have_studip_perm('dozent', Context::getId())) {
+            throw new AccessDeniedException();
+        }
 
-        if ($perm->have_studip_perm('dozent', $id)) {
+        $course = Context::get();
 
-            if ($approveDelete && check_ticket(Request::get('studip_ticket'))) {
-                $course = Course::find($id);
-                if (!$course->delete()) {
-                    PageLayout::postError(_('Die Studiengruppe konnte nicht gelöscht werden.'));
-                }
-                $this->redirect(URLHelper::getURL('dispatch.php/studygroup/browse', [], true));
-                return;
-            } elseif (!$approveDelete) {
-                PageLayout::postQuestion(
-                    _('Sind Sie sicher, dass Sie diese Studiengruppe löschen möchten?'),
-                    $this->deleteURL('true')
-                )->includeTicket();
+        if (!($course instanceof Course) || !$course->isStudygroup()) {
+            throw new InvalidArgumentException(_('Studiengruppe nicht vorhanden oder Veranstaltung ist keine Studiengruppe'));
+        }
 
-                $this->redirect('course/studygroup/edit');
-                return;
-            }
+        if (!Context::get()->delete()) {
+            PageLayout::postError(_('Die Studiengruppe konnte nicht gelöscht werden.'));
+        } else {
+            PageLayout::postSuccess(_('Die Studiengruppe wurde gelöscht.'));
         }
-        throw new Trails\Exception(401);
-    }
 
+        $this->redirect(URLHelper::getURL('dispatch.php/studygroup/browse', [], true));
+    }
 
     /**
      * Displays admin settings concerning the studygroups
diff --git a/resources/assets/javascripts/bootstrap/data_secure.js b/resources/assets/javascripts/bootstrap/data_secure.js
index 1b3b7a1..98d36ed 100644
--- a/resources/assets/javascripts/bootstrap/data_secure.js
+++ b/resources/assets/javascripts/bootstrap/data_secure.js
@@ -51,79 +51,88 @@ import { $gettext } from '../lib/gettext';
  * @since   Stud.IP 3.4
  */
 
-/**
- * Normalize arbitrary input to config option object
- *
- * @param mixed input Arbitrary input
- * @return Object config
- */
-function normalizeConfig(input) {
-    var config = {
-        always: null,
-        exists: false
-    };
-    if ($.isPlainObject(input)) {
-        config = $.extend(config, input);
-    } else if (input === false || input === true) {
-        config.always = input;
-    } else {
-        config.exists = input || false;
-    }
-    return config;
-}
+const DataSecurity = {
+    // Set this to true to disable all security checks
+    disabled: false,
 
-/**
- * Detect any changes on elements with the data-secure attribute
- * in a given context.
- *
- * @param mixed context Optional context in which the elements should be
- *                      located
- * @return bool indicating whether any changes have occured
- */
-function detectChanges(context) {
-    var changed = false;
-
-    $('[data-secure]', context || document).each(function() {
-        if (
-            $(this)
-                .closest('form')
-                .data().secureSkip
-        ) {
-            return;
+    /**
+     * Normalize arbitrary input to config option object
+     *
+     * @param {any} input Arbitrary input
+     * @return Object config
+     */
+    normalizeConfig(input) {
+        let config = {
+            always: null,
+            exists: false
+        };
+        if ($.isPlainObject(input)) {
+            config = $.extend(config, input);
+        } else if (input === false || input === true) {
+            config.always = input;
+        } else {
+            config.exists = input || false;
         }
+        return config;
+    },
 
-        var data = $(this).data().secure;
-        var config = normalizeConfig(data);
-        var items = $(this).is('form') ? $(this).find(':input:not([data-secure])') : $(this);
-
-        if (config.always === true) {
-            changed = true;
-        } else if (config.always !== false && config.exists === false) {
-            items
-                .filter('[name]')
-                .filter(':not(:checkbox,:radio)')
-                .each(function() {
-                    changed = changed || (this.defaultValue !== undefined && this.value !== this.defaultValue);
-                });
-            items
-                .filter('[name]')
-                .filter(':checkbox,:radio')
-                .each(function() {
-                    changed = changed || (this.defaultChecked !== undefined && this.checked !== this.defaultChecked);
-                });
+    /**
+     * Detect any changes on elements with the data-secure attribute
+     * in a given context.
+     *
+     * @param {any} context Optional context in which the elements should be
+     *                      located
+     * @return bool indicating whether any changes have occured
+     */
+    detectChanges(context = null) {
+        if (DataSecurity.disabled) {
+            return false;
         }
 
-        if (!changed && config.exists !== false) {
-            changed = $(config.exists, this).length > 0;
-        }
-    });
+        let changed = false;
+
+        $('[data-secure]', context ?? document).each(function() {
+            if (
+                $(this)
+                    .closest('form')
+                    .data().secureSkip
+            ) {
+                return;
+            }
+
+            const data = $(this).data().secure;
+            const config = DataSecurity.normalizeConfig(data);
+            const items = $(this).is('form') ? $(this).find(':input:not([data-secure])') : $(this);
 
-    return changed;
+            if (config.always === true) {
+                changed = true;
+            } else if (config.always !== false && config.exists === false) {
+                items
+                    .filter('[name]')
+                    .filter(':not(:checkbox,:radio)')
+                    .each(function() {
+                        changed = changed || (this.defaultValue !== undefined && this.value !== this.defaultValue);
+                    });
+                items
+                    .filter('[name]')
+                    .filter(':checkbox,:radio')
+                    .each(function() {
+                        changed = changed || (this.defaultChecked !== undefined && this.checked !== this.defaultChecked);
+                    });
+            }
+
+            if (!changed && config.exists !== false) {
+                changed = $(config.exists, this).length > 0;
+            }
+        });
+
+        return changed;
+    }
 }
 
 // Secure browser window on refresh via the beforeunload event
 $(window).on('beforeunload', function(event) {
-    if (detectChanges() === false) {
+    if (DataSecurity.detectChanges() === false) {
         return;
     }
 
@@ -134,7 +143,7 @@ $(window).on('beforeunload', function(event) {
 
 // Secure dialogs on close via the dialogbeforeclose event
 $(document).on('dialogbeforeclose', function(event) {
-    if (detectChanges(event.target) === false) {
+    if (DataSecurity.detectChanges(event.target) === false) {
         return true;
     }
 
@@ -158,4 +167,7 @@ $(document)
         $(this)
             .closest('form')
             .data('secure-skip', false);
+    })
+    .on('submit', 'form[data-secure-disable],form:has(button[data-secure-disable])', () => {
+        DataSecurity.disabled = true;
     });