Add more display output for security alerts

Show the information in summary to users looking at the SiteSummary report on screen. This includes the ability to see a tag notifying which modules have alerts issued for them, a form level warning alerting users that some modules have alerts (as they may not display on the first page), and the ability to expand an individual module's information to display the alerts that have been issued (in a summary of title, ID number and externa link)
bringyourownideas · May 29, 2018 · 7746d57 · 7746d57
1 parent 5701642
commit 7746d57
Show file tree

Hide file tree

Showing 11 changed files with 129 additions and 35 deletions.
diff --git a/.editorconfig b/.editorconfig
@@ -10,6 +10,13 @@ indent_style = space
 insert_final_newline = true
 trim_trailing_whitespace = true
 
+[*.md]
+trim_trailing_whitespace = false
+
+[*{.yml,.js}]
+indent_size = 2
+indent_style = space
+
 [{*.yml,package.json}]
 indent_size = 2
 

diff --git a/composer.json b/composer.json
@@ -19,15 +19,15 @@
     ],
     "require": {
         "php": ">=5.6.0",
-        "silverstripe/framework": "^3",
+        "silverstripe/framework": "^3.2",
         "sensiolabs/security-checker": "^4",
         "symbiote/silverstripe-queuedjobs": "^2"
     },
     "replace": {
         "spekulatius/silverstripe-composer-security-checker": "self.version"
     },
     "suggest": {
-        "friendsofsilverstripe/silverstripe-maintenance": "Display advisory information via Site Summary Report",
+        "bringyourownideas/silverstripe-maintenance": "Display advisory information via Site Summary Report",
         "roave/security-advisories": "Raises a conflict on attempt to install a vulnerable module"
     },
     "support": {

diff --git a/css/securityalerts.css b/css/securityalerts.css
@@ -0,0 +1,7 @@
+.package-summary__security-alerts {
+  margin-top: 8px;
+}
+
+.security-alerts__list {
+  display: none;
+}
diff --git a/javascript/summaryalerts.js b/javascript/summaryalerts.js
@@ -0,0 +1,29 @@
+(function($) {
+  $.entwine('ss', function($) {
+    $('.package-summary__security-alerts').entwine({
+      IsShown: false,
+      onclick: function(event) {
+        if ($(event.target).is('strong, strong>span')) {
+          this.toggleSecurityNotices();
+        }
+      },
+      toggleSecurityNotices: function() {
+        if (this.getIsShown()) {
+          this.hideSecurityNotices();
+        } else {
+          this.showSecurityNotices();
+        }
+      },
+      showSecurityNotices: function() {
+        this.children('dl').show();
+        this.find('strong>span').text('Hide');
+        this.setIsShown(true);
+      },
+      hideSecurityNotices: function() {
+        this.children('dl').hide();
+        this.find('strong>span').text('Show');
+        this.setIsShown(false);
+      }
+    });
+  });
+})(jQuery)
diff --git a/src/Extensions/PackageSecruityExtension.php b/src/Extensions/PackageSecruityExtension.php
@@ -10,16 +10,48 @@ class PackageSecurityExtension extends DataExtension
         'listCVEs' => 'Security alerts',
     ];
 
+    /**
+     * Simply returns a comma separated list of active CVE numbers for this record.
+     * Used in CSV exports as a type of brief indication (as opposed to full info)
+     */
     public function listCVEs()
     {
         $alerts = $this->owner->SecurityAlerts()->Column('CVE');
         return $alerts ? implode(', ', $alerts) : null;
     }
 
+    /**
+     * Renders the SecurityAlerts relationship
+     * intended for use in the on screen version of the SiteSummary Report.
+     */
     public function listAlerts()
     {
         $templates = ['PackageSecurityAlerts'];
         $this->owner->extend('updateListAlerts', $templates);
         return $this->owner->renderWith($templates);
     }
+
+    /**
+     * updates the badges that render as part of the screen targeted
+     * summary for this Package
+     *
+     * @param array $badges in the format of [title => type]
+     */
+    public function updateBadges(&$badges)
+    {
+        if ($this->owner->SecurityAlerts()->exists()) {
+            $badges['RISK: Security'] = 'warning';
+        }
+    }
+
+    /**
+     * Appends our own summary info to that of the default output
+     * of the Package getSummary method.
+     *
+     * @param HTMLText $summary
+     */
+    public function updateSummary(&$summary)
+    {
+        $summary->setValue($summary . $this->listAlerts());
+    }
 }
diff --git a/src/Extensions/SiteSummaryExtension.php b/src/Extensions/SiteSummaryExtension.php
@@ -2,14 +2,31 @@
 
 class SiteSummaryExtension extends Extension
 {
+    /**
+     * Update screen bound report columns to remove the text (csv) column
+     * listing CVE numbers, and include the view assets to render appropriately
+     *
+     * @param array $columns Report display columns
+     */
     public function updateColumns(&$columns)
     {
+        Requirements::css('silverstripe-composer-security-checker/css/securityalerts.css');
+        Requirements::javascript('silverstripe-composer-security-checker/javascript/summaryalerts.js');
         unset($columns['listCVEs']);
     }
 
-    public function updateCMSFields(&$fields)
+    /**
+     * Update the Package's screen bound summary info with little badges to indicate
+     * security alerts are present for this package
+     *
+     * @param array $alerts a list of alerts to display
+     */
+    public function updateAlerts(&$alerts)
     {
-        $summaryInfo = $this->owner->sourceRecords()->renderWith('PackageSecurityAlerts');
-        $fields->unshift(LiteralField::create('AlertSummary', $summaryInfo));
+        $securityWarnings = $this->owner->sourceRecords()->filter('SecurityAlerts.ID:GreaterThan', 0);
+
+        if ($securityWarnings->exists()) {
+            $alerts['SecurityAlerts'] = $securityWarnings->renderWith('SecurityAlertSummary');
+        }
     }
 }
diff --git a/src/Jobs/CVECheckJob.php b/src/Jobs/CVECheckJob.php
@@ -19,16 +19,16 @@ class CVECheckJob extends AbstractQueuedJob implements QueuedJob
     /**
      * @return CVECheckTask
      */
-    public function getCVECheckTask()
+    public function getCheckTask()
     {
         return $this->checkTask;
     }
 
     /**
-     * @param CVECheckTask
+     * @param CVECheckTask $checkTask
      * @return CVECheckJob
      */
-    public function setCVECheckTask($checkTask)
+    public function setCheckTask(CVECheckTask $checkTask)
     {
         $this->checkTask = $checkTask;
         return $this;
@@ -52,7 +52,7 @@ public function getJobType()
     public function process()
     {
         // run the task
-        $task = $this->getCVECheckTask();
+        $task = $this->getCheckTask();
         $task->run(null);
 
         // mark job as completed

diff --git a/src/Tasks/CVECheckTask.php b/src/Tasks/CVECheckTask.php
@@ -31,9 +31,9 @@ public function getSecurityChecker()
 
     /**
      * @param SecurityChecker $securityChecker
-     * @return CVECheckTask $this
+     * @return $this
      */
-    public function setSecurityChecker($securityChecker)
+    public function setSecurityChecker(SecurityChecker $securityChecker)
     {
         $this->securityChecker = $securityChecker;
         return $this;
@@ -61,7 +61,7 @@ public function run($request)
 
                 // Is this vulnerability known? No, lets add it.
                 if ((int) $vulnerability->count() === 0) {
-                    $vulnerability = new CVE();
+                    $vulnerability = CVE::create();
                     $vulnerability->PackageName  = $package;
                     $vulnerability->Version      = $packageDetails['version'];
                     $vulnerability->Title        = $details['title'];
@@ -87,15 +87,15 @@ public function run($request)
         }
 
         // remove all entries which are resolved (no longer $validEntries)
-        $removeOldCVEs = SQLDelete::create('CVE');
+        $removeOldCVEs = SQLDelete::create('"CVE"');
         if (empty($validEntries)) {
-            // There were no CVEs listed for our installation
+            // There were no CVEs listed for our installation - so flush any old data
             $removeOldCVEs->execute();
         } else {
             $removable = CVE::get()->exclude(array('ID' => $validEntries));
             if ($removable->exists()) {
                 // Be careful not to remove all CVEs on the case that entry is valid
-                $removeOldCVEs = $removeOldCVEs->addWhere('ID', $removable->column('ID'));
+                $removeOldCVEs = $removeOldCVEs->addWhere('"ID"', $removable->column('ID'));
                 $removeOldCVEs->execute();
             }
         }

diff --git a/templates/PackageSecurityAlerts.ss b/templates/PackageSecurityAlerts.ss
@@ -1,13 +1,11 @@
-<% loop $Me %>
-    <% if $SecurityAlerts %>
-    <div class="message bad">
-        <strong>$Name</strong>
-        <dl>
-            <% loop $SecurityAlerts %>
-            <dt><a href="$ExternalLink">$CVE</a></dt>
-            <dd>$Title</dd>
-            <% end_loop %>
-        </dl>
-    </div>
-    <% end_if %>
-<% end_loop %>
+<% if $SecurityAlerts %>
+<div class="package-summary__security-alerts">
+    <strong><span>View</span> security alert info</strong>
+    <dl class="security-alerts__list">
+        <% loop $SecurityAlerts %>
+        <dt><a href="$ExternalLink">$CVE</a></dt>
+        <dd>$Title</dd>
+        <% end_loop %>
+    </dl>
+</div>
+<% end_if %>
diff --git a/templates/SecurityAlertSummary.ss b/templates/SecurityAlertSummary.ss
@@ -0,0 +1,4 @@
+<p>
+    <strong>Security alert</strong><br />
+    <% if Count > 1 %>Notices have<% else %>A notice has<% end_if %> been issued for <strong>$Count</strong> of your modules. Review and updating is recommended.
+</p>
diff --git a/tests/CVECheckTaskTest.php b/tests/CVECheckTaskTest.php
@@ -41,12 +41,12 @@ public function testUpdatesAreSaved()
         $checkTask->setSecurityChecker($securityCheckerMock);
 
         $preCheck = CVE::get();
-        $this->assertEquals(0, $preCheck->count(), 'database is empty to begin with');
+        $this->assertCount(0, $preCheck, 'database is empty to begin with');
 
         $checkTask->run(null);
 
         $postCheck = CVE::get();
-        $this->assertEquals(1, $postCheck->count(), 'CVE has been stored');
+        $this->assertCount(1, $postCheck, 'CVE has been stored');
     }
 
     public function testNoDuplicates()
@@ -56,17 +56,17 @@ public function testNoDuplicates()
         $checkTask->setSecurityChecker($securityCheckerMock);
 
         $preCheck = CVE::get();
-        $this->assertEquals(0, $preCheck->count(), 'database is empty to begin with');
+        $this->assertCount(0, $preCheck, 'database is empty to begin with');
 
         $checkTask->run(null);
 
         $postCheck = CVE::get();
-        $this->assertEquals(1, $postCheck->count(), 'CVE has been stored');
+        $this->assertCount(1, $postCheck, 'CVE has been stored');
 
         $checkTask->run(null);
 
         $postCheck = CVE::get();
-        $this->assertEquals(1, $postCheck->count(), 'The CVE isn\'t stored twice.');
+        $this->assertCount(1, $postCheck, 'The CVE isn\'t stored twice.');
     }
 
     public function testCVERemovals()
@@ -78,14 +78,14 @@ public function testCVERemovals()
         $checkTask->run(null);
 
         $preCheck = CVE::get();
-        $this->assertEquals(1, $preCheck->count(), 'database has stored CVEs');
+        $this->assertCount(1, $preCheck, 'database has stored CVEs');
 
         $securityCheckerMock = $this->getSecurityCheckerMock(true);
         $checkTask->setSecurityChecker($securityCheckerMock);
 
         $checkTask->run(null);
 
         $postCheck = CVE::get();
-        $this->assertEquals(0, $postCheck->count(), 'database is empty to finish with');
+        $this->assertCount(0, $postCheck, 'database is empty to finish with');
     }
 }