API refactor module to be more independent #34

NightJar · 2018-05-25T04:30:50Z

As part of a greater piece of work with site summaries (cf.
bringyourownideas/silverstripe-maintenance#39). This commit tidies class
definitions and makes the import slightly more efficient. Abstractions are
added to relate to the Package object from the aforementioned module.
Unit tests have been added to ensure tidy standards in future maintenance.

resolves #1 (although technically it was already resolved).

As part of a greater piece of work with site summaries (cf. bringyourownideas/silverstripe-maintenance). This commit tidies class definitions and makes the import slightly more efficient. Abstractions are added to relate to the Package object from the aforementioned module. Unit tests have been added to ensure tidy standards in future maintenance.

robbieaverill

Looks good overall! I've left some specific comments, but also there's generally a huge lack of PHPDocs on methods and classes

robbieaverill · 2018-05-28T21:23:40Z

composer.json

    },
    "replace": {
        "spekulatius/silverstripe-composer-security-checker": "self.version"
    },
    "suggest": {
+        "friendsofsilverstripe/silverstripe-maintenance": "Display advisory information via Site Summary Report",


The package name has changed =)

robbieaverill · 2018-05-28T21:24:13Z

composer.json

-        "sensiolabs/security-checker": "^3.0",
-        "symbiote/silverstripe-queuedjobs": "^2.8",
-        "friendsofsilverstripe/silverstripe-maintenance": "^0.3"
+        "silverstripe/framework": "^3",


Let's introduce ^3.2 which matches bringyourownideas/silverstripe-maintenance

robbieaverill · 2018-05-28T21:26:19Z

src/Jobs/CVECheckJob.php

+    /**
+     * @return CVECheckTask
+     */
+    public function getCVECheckTask()


Can you please name the getters/setters the same as the property? I.e. setCheckTask(CheckTask $checkTask)

robbieaverill · 2018-05-28T21:26:33Z

src/Jobs/CVECheckJob.php

+    }
+
+    /**
+     * @param CVECheckTask


Missing variable name - can you please type hint the class in the argument too?

robbieaverill · 2018-05-28T21:27:02Z

src/Tasks/CVECheckTask.php

+     * @param SecurityChecker $securityChecker
+     * @return CVECheckTask $this
+     */
+    public function setSecurityChecker($securityChecker)


Please type hint SecurityChecker

robbieaverill · 2018-05-28T21:28:20Z

src/Tasks/CVECheckTask.php

+
+        // use the security checker of
+        $checker = $this->getSecurityChecker();
+        $alerts = $checker->check(BASE_PATH . DIRECTORY_SEPARATOR . 'composer.lock');


I feel like we should use the ComposerLoader from silverstripe-maintenance for something here, at the least to remove the reliance on the physical path of composer.lock from this code (and leave it to ComposerLoader)

This library has it's own logic for reading and parsing the lock file, and only requires the path in the check method.

ComposerLoader requires a base path, which means either specifying manually still, or relying on BASE_PATH as it is here.

There is no way at current to extract the lock path from ComposerLoader (only the parsed contents).

This modules does not require silverstripe-maintenance, nor is it intended to.

This task is for checking the main installation only, not another at some arbitrary location, nor does it provide the ability to upload and check random lock files.

Conclusion:
ComposerLoader should not be used for this. Although I could add a setter/getter for a lock path, defaulting to what is currently set, I feel it is unnecessary.

robbieaverill · 2018-05-28T21:28:29Z

src/Tasks/CVECheckTask.php

+
+                // Is this vulnerability known? No, lets add it.
+                if ((int) $vulnerability->count() === 0) {
+                    $vulnerability = new CVE();


CVE::create()

robbieaverill · 2018-05-28T21:29:18Z

src/Tasks/CVECheckTask.php

+        }
+
+        // remove all entries which are resolved (no longer $validEntries)
+        $removeOldCVEs = SQLDelete::create('CVE');


Please quote the table name to ensure that this works across DB drivers

robbieaverill · 2018-05-28T21:29:37Z

src/Tasks/CVECheckTask.php

+            $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'));


Quotes for columns

robbieaverill · 2018-05-28T21:31:05Z

tests/CVECheckTaskTest.php

+        $checkTask->setSecurityChecker($securityCheckerMock);
+
+        $preCheck = CVE::get();
+        $this->assertEquals(0, $preCheck->count(), 'database is empty to begin with');


$this->assertCount(0, $preCheck, '...

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)

For screen rendering devices - as per design team feedback. The text denoting an action is present is removed, and the toggle switch for revealing that information is shifted to the badge itself.

NightJar · 2018-05-29T22:16:14Z

Design review is done, this is ready for final review :)

robbieaverill · 2018-05-29T22:45:14Z

.editorconfig

@@ -10,6 +10,13 @@ indent_style = space
 insert_final_newline = true
 trim_trailing_whitespace = true

+[*.md]
+trim_trailing_whitespace = false


robbieaverill

Looks great, nice work! I've left a couple of comments to add translations, but otherwise looks good

robbieaverill · 2018-05-29T22:46:59Z

src/Extensions/PackageSecruityExtension.php

+    public function updateBadges(&$badges)
+    {
+        if ($this->owner->SecurityAlerts()->exists()) {
+            $badges['RISK: Security'] = 'warning security-alerts__toggler';


Can you please make this label translatable?

Oh whoops, yep :>

robbieaverill · 2018-05-29T22:48:28Z

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.


Please add translations

robbieaverill · 2018-05-29T22:49:29Z

Can you add a branch alias (1.x-dev) to composer.json while you're at it?

robbieaverill · 2018-05-30T05:25:04Z

src/Extensions/PackageSecruityExtension.php

+     *
+     * @param array $badges in the format of [title => type]
+     */
+    public function updateBadges(&$badges)


I'm hoping this will change in the other PR so will expect this to change

ooh, good call; derp.

Yep needs updating now to use an ArrayList

PHPDoc still needs updating

robbieaverill · 2018-05-30T20:22:07Z

templates/SecurityAlertSummary.ss

+    <% if Count > 1 %>
+        <%t SecuirtyAlertSummary.NOTICES_HAVE "Notices have" %>
+    <% else %>
+        <%t SecuirtyAlertSummary.NOTICE_HAS "A notice has" %>


On reflection I don’t like how these translations are split up. It provides no context to translators and they might get confused about it. Also typos in the key :-)

robbieaverill · 2018-05-30T20:25:49Z

src/Models/CVE.php

+/**
+ * Describes a known security issue of an installed Composer package
+ */
+class CVE extends DataObject


Last thing- the vulnerabilities are not always assigned CVEs (ours aren’t for example), so maybe the original name or something like PackageVulnerability would be better- what do you think?

Question: Should the SS security ID then be put in place of the CVE ID?
i.e. SS-2015-014 when accessing ->CVE?

No, because this module can work for any PHP packages not just SilverStripe packages. Maybe choose something generic like "Identifier"

Switch away from hard coded English.

Use sensible names, and also fix a few bug discovered by altering the tests with more data and cases (such as the SQLDelete statement). CVE DataObject is now SecurityAlerts, as not all alerts are assigned CVE identifiers. To that end if there is another identifier (such as the one SilverStripe use as a title prefix) then this is set instead. As such the property is renamed 'Identifier' as opposed to 'CVE'.

robbieaverill

Awesome!

robbieaverill · 2018-05-31T05:08:03Z

src/Jobs/SecurityAlertCheckJob.php

+
+    /**
+     * @param SecurityAlertCheckTask $checkTask
+     * @return SecurityAlertCheckJob


Prefer return $this, but this is also true

NightJar mentioned this pull request May 25, 2018

Different output formats #2

Closed

NightJar force-pushed the pulls/1/refactor branch from 0abbe4a to 5701642 Compare May 28, 2018 04:05

robbieaverill suggested changes May 28, 2018

View reviewed changes

NightJar mentioned this pull request May 29, 2018

support security (and other) alerts bringyourownideas/silverstripe-maintenance#72

Merged

NightJar force-pushed the pulls/1/refactor branch 3 times, most recently from 9b283ce to 7746d57 Compare May 29, 2018 05:09

NightJar mentioned this pull request May 29, 2018

Display any available security notifications bringyourownideas/silverstripe-maintenance#39

Closed

NightJar force-pushed the pulls/1/refactor branch 3 times, most recently from 857de7d to 23028c8 Compare May 29, 2018 09:50

NightJar force-pushed the pulls/1/refactor branch from 23028c8 to 079a9d5 Compare May 29, 2018 09:58

Alter package level alert notice functionality

7efa85c

For screen rendering devices - as per design team feedback. The text denoting an action is present is removed, and the toggle switch for revealing that information is shifted to the badge itself.

NightJar mentioned this pull request May 29, 2018

Write tests for the DataExtensions #35

Closed

1 task

robbieaverill reviewed May 29, 2018

View reviewed changes

robbieaverill suggested changes May 29, 2018

View reviewed changes

robbieaverill added affects/v3 type/api-break labels May 29, 2018

robbieaverill changed the base branch from master to 1 May 30, 2018 04:59

robbieaverill reviewed May 30, 2018

View reviewed changes

robbieaverill suggested changes May 30, 2018

View reviewed changes

NightJar force-pushed the pulls/1/refactor branch from af35d4a to dce1f04 Compare May 30, 2018 21:07

Make output strings translatable

a8e9238

Switch away from hard coded English.

NightJar force-pushed the pulls/1/refactor branch from dce1f04 to 0196327 Compare May 31, 2018 04:27

NightJar force-pushed the pulls/1/refactor branch from 0196327 to fd37c29 Compare May 31, 2018 04:40

robbieaverill approved these changes May 31, 2018

View reviewed changes

robbieaverill merged commit b37a6dd into bringyourownideas:1 May 31, 2018

robbieaverill deleted the pulls/1/refactor branch May 31, 2018 05:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API refactor module to be more independent #34

API refactor module to be more independent #34

NightJar commented May 25, 2018 •

edited

Loading

robbieaverill left a comment

robbieaverill May 28, 2018

robbieaverill May 28, 2018

robbieaverill May 28, 2018

robbieaverill May 28, 2018

robbieaverill May 28, 2018

robbieaverill May 28, 2018

NightJar May 29, 2018

robbieaverill May 28, 2018

robbieaverill May 28, 2018

robbieaverill May 28, 2018

robbieaverill May 28, 2018

NightJar commented May 29, 2018

robbieaverill May 29, 2018

NightJar May 30, 2018

robbieaverill left a comment

robbieaverill May 29, 2018

NightJar May 30, 2018

robbieaverill May 29, 2018

robbieaverill commented May 29, 2018

robbieaverill May 30, 2018

NightJar May 30, 2018

robbieaverill May 30, 2018

NightJar May 30, 2018

robbieaverill May 30, 2018

robbieaverill May 30, 2018

robbieaverill May 30, 2018

NightJar May 31, 2018

robbieaverill May 31, 2018

robbieaverill left a comment

robbieaverill May 31, 2018

API refactor module to be more independent #34

API refactor module to be more independent #34

Conversation

NightJar commented May 25, 2018 • edited Loading

robbieaverill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NightJar commented May 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robbieaverill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robbieaverill commented May 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robbieaverill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NightJar commented May 25, 2018 •

edited

Loading